Rule Proposal: Override checking for interfaces

[dominic-simplan]

The new override keyword in TS4.3 works for classes but not for interfaces.
The ts-lint rule (https://github.com/hmil/tslint-override) supported both. We currently still have to use the tslint rule because of that.
Would be great to have a rule which supports override for interfaces.

See issue #293.

[bradzacher]

Could you explain the usecase for override on interfaces?

I can think of a few cases that you might want to but they're exceedingly rare. Rare enough that I don't think such a rule would belong in this project.

[dominic-simplan]

Main usecase for us are optional properties / methods in the interface:

interface ISomething {
  doSomething?: () => void;
}

class Something implements ISomething {
  doSomething: () => {console.log("Something")}
}

In this case it is possible to rename or remove the doSomething method in ISomething without getting a warning/error. Which can lead to bugs in the application as the expected code is not executed anymore.

[bradzacher]

I was expecting you to show a usecase like

interface I { method(): string }
interface II extends I { method(): 'a' }

Your terminology is wrong - your usecase is not for overrides.
In that instance you are not "overriding" that method, you are "implementing" it.
This is why the override keyword does not work for the usecase.

The two are very different levels. One is a code safety and correctness issue.
The other is a potential deadcode issue.

Overriding a base class's method means you want to change how it works on your subclass.
So for example if the base class renames the method and your subclass doesn't - then you very likely have a bug in your code that otherwise would be uncaught without override checking.

OTOH, implementing an interface just ensures your code adheres to some contract.
If the contract changes (renamed or added to) but your implementation doesn't change, then that's a hard TS error already.
If a method is removed from the interface, then leaving it on your implementation doesn't actually matter. It could lead to dead code for sure, but there's no code correctness issue with leaving the method behind.

---

I'll be honest when I say that the tslint plugin conflating the two is not great. I understand the intention, but the effect is just confusion.

[dominic-simplan]

Thank you for the detailed explanation. I hadn't thought about this distinction but it makes sense. I was probably used to add @Override annotations when implementing methods from an interface in Java (if I remember correctly, some IDEs even add it automatically). So it's not only the tslint plugin conflating these two concepts :)

Still, I think bugs could be introduced in my above example. If you rename doSomething() in the interface to doSomethingElse(), this would not throw a hard TS error, since it is optional and therefore doesn't have to be implemented. The implementing class would still have a doSomething() method which never gets executed.
This is what I currently use the tslint @override for. It would show a warning/error for doSomething() stating that "Member with @OverRide keyword does not override any base class member"

[bradzacher]

Have you considered raising this with the TS folks?
Considering they have already implemented the override keyword, I would have thought the topic of either extending the override functionality to also check interface implementations or perhaps adding a new keyword to specify the implementation could be something they're interested in.

[dominic-simplan]

Only found this comment so far on the PR which added the override keyword for classes:

No Plan yet

However if it is only me being interested in this feature I can just continue to use the tslint rule in eslint (if I can get it working again) and I'll probably also create a ticket in TypeScript. Not sure though if it'll be accepted and even if it does, probably not coming soon (see above).

[bradzacher]

Revisiting this issue.
In order to support this from the linting side - we would need to add some form of a comment syntax to flag a method as implementing an interface method.
We don't have any support for JSDoc in this package (intentionally so) - so we don't have a mechanism for supporting such a requirement.

Whilst I do see value in this lint rule, due to the requirement for additional comment syntax and associated infra - I don't think it will have broad enough applicability to exist within this project specifically.

I think it's a great idea though! You're very free to build on top of our tooling within another eslint-plugin which is better suited, or within a new plugin entirely.

Thanks for the issue!

[dominic-simplan]

I understand. Thank you for your consideration and explanation!