Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

noImplicitOverride does not complain on abstract methods #44457

Closed
Lusito opened this issue Jun 6, 2021 · 8 comments
Closed

noImplicitOverride does not complain on abstract methods #44457

Lusito opened this issue Jun 6, 2021 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Lusito
Copy link

Lusito commented Jun 6, 2021

Bug Report

When using noImplicitOverride, I would expect a missing override to complain for abstract methods as well.

🔎 Search Terms

noImplicitOverride abstract

Found #13729, which states "Let's say no for now.", but I would argue against that.

🕗 Version & Regression Information

TypeScript 4.3.2 and Nightly

  • I was unable to test this on prior versions because this is a new feature

⏯ Playground Link

Playground link with relevant code

You'll need to manually set the tsconfig option in the Playground, as the share link doesn't seem to contain that option.

💻 Code

abstract class Hello {
    abstract doesNotComplain(): void;
}

class World extends Hello {
    // should complain here for missing override keyword
    doesNotComplain() {}
}

🙁 Actual behavior

With noImplicitOverride=true, tsc is fine with the above snippet

🙂 Expected behavior

With noImplicitOverride=true, it should complain about the missing override keyword.

Reasoning

I can add an override keyword for implementations of abstract methods. So if I can define it as override, there must be an implicit override if I don't write it manually and the option noImplicitOverride should catch that.

Why do I want this to happen aside from the confusing/incorrect wording of the option?

If I use the override keyword manually, I get an error if the base class removes the abstract method declaration and I can properly update my code. If I didn't have the override keyword, I would have dead code, which needs to be handled differently in newer versions of the base class (for example by registering an event listener instead of implementing an abstract method).

@MartinJohns
Copy link
Contributor

Related: #44439

@Kingwl
Copy link
Contributor

Kingwl commented Jun 6, 2021

It's working as expected.

You can see this thread to learn more.

@Lusito
Copy link
Author

Lusito commented Jun 6, 2021

@MartinJohns that's a different issue and has nothing to do with this one.

@Kingwl Thanks for the info. Looking at the PR description, it was at least not very clear and the comment you highlighted seems more about semantics than about usefulness. I would expect noImplicitOverride to help me find possible errors.
I guess if this is not wanted in noImplicitOverride, It might be possible to create an eslint rule to add that extra safety.

@RyanCavanaugh
Copy link
Member

(Crossposting this comment) This is intentional. The mental model to have in place here is, "If I typo this method name, what will happen?"

If you typo the name of an abstract member (assuming you're in a concrete class), you'll get an error, because you failed to implement all abstract members. override isn't necessary to prevent you from making this mistake, so you don't need to write override in this case.

If you typo the name of an optional member, you'll now have calculateHeight and cacluateHeight, and the latter will just go silently uncalled forever. override is necessary to prevent you from making this mistake, so when overriding an optional member, you should be writing override.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 7, 2021
@Lusito
Copy link
Author

Lusito commented Jun 7, 2021

@RyanCavanaugh
So your scenario is:

  1. Write a new class, implement abstract method, make a typo
  2. Wanting to get informed about typo.

While I would say, that this is a perfectly valid scenario, I would say the more important scenario is:

  1. Write new class, implement abstract method, test => all good
  2. Update dependency, abstract method has been renamed or removed
  3. Wanting to get informed about this change.

When implementing an abstract method for the first time, you usually make sure it works correctly, so you're less likely to make a mistake here. But if you update a dependency and the abstract method has been removed from the base class (not just renamed), you will at best be left with dead code, at worst with a broken feature.

@RyanCavanaugh
Copy link
Member

I don't disagree.

It can also be argued that you should only be able to write override foo() in places where super.foo() is legal to write, and that doesn't apply for abstract members.

People will have varying expectations and at the end of the day we have to pick one for clarity and folks who had different expectations will have to adjust a bit.

@Lusito
Copy link
Author

Lusito commented Jun 7, 2021

That is true, though at least Java uses @Override for interfaces as well.

So we either need to introduce a new keyword "implement" (not fixed on the naming), which can be used for methods, which implement abstract methods or optional properties, or we should handle both cases in exactly the same way under one keyword "override". I'd be fine with implement foo() {} for all places, where super.foo() is not legal.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

devversion added a commit to devversion/angular that referenced this issue Jul 7, 2021
…act members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
devversion added a commit to devversion/angular that referenced this issue Jul 7, 2021
…act members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
devversion added a commit to devversion/angular that referenced this issue Jul 7, 2021
…act members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
devversion added a commit to devversion/angular that referenced this issue Jul 8, 2021
…act members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
devversion added a commit to devversion/angular that referenced this issue Jul 8, 2021
…act members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
devversion added a commit to devversion/angular that referenced this issue Jul 9, 2021
…ct members

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.
AndrewKushnir pushed a commit to angular/angular that referenced this issue Jul 12, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
AndrewKushnir pushed a commit to angular/angular that referenced this issue Jul 12, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to josephperrott/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to josephperrott/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to josephperrott/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to josephperrott/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to josephperrott/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
josephperrott pushed a commit to angular/dev-infra that referenced this issue Jul 26, 2021
…ct members (#42512)

TypeScript introduced a new flag called `noImplicitOverride` as part
of TypeScript v4.3. This flag introduces a new keyword called `override`
that can be applied to members which override declarations from a base
class. This helps with code health as TS will report an error if e.g.
the base class changes the method name but the override would still
have the old method name. Similarly, if the base class removes the method
completely, TS would complain that the memeber with `override` no longer
overrides any method.

A similar concept applies to abstract methods, with the exception that
TypeScript's builtin `noImplicitOverride` option does not flag members
which are implemented as part of an abstract class. We want to enforce
this as a best-practice in the repository as adding `override` to such
implemented members will cause TS to complain if an abstract member is
removed, but still implemented by derived classes.

More details: microsoft/TypeScript#44457.

PR Close #42512
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants