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

New flag --noImplicitAbstractOverride which mandates the use of override when implementing an abstract method #47250

Open
5 tasks done
SCLeoX opened this issue Dec 26, 2021 · 22 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@SCLeoX
Copy link

SCLeoX commented Dec 26, 2021

Suggestion

πŸ” Search Terms

flag override implicit noImplicitOverride abstract

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Currently, when --noImplicitOverride is enabled, overriding (or implementing) an abstract method does not require the use of keyword override.

I suggest adding a new flag --noImplicitAbstractOverride, which, when enabled in conjunction with --noImplicitOverride, will also require override when implementing an abstract method.

πŸ“ƒ Motivating Example

In issue #44457, @RyanCavanaugh explained the rationale behind --noImplicitOverride not covering implementing abstract methods is that --noImplicitOverride is designed to prevent typos. Since not implementing an abstract method will already result in an error, it is not necessary to let --noImplicitOverride also cover that case.

However, that is only one use case for the override keyword. In fact, I don't think --noImplicitOverride has ever helped me catch any typo bugs that way, thanks to the modern IDE I use, which automatically fills in method names when I am about to override a method. (I am not saying it is not helpful, though.)

In my opinion, one of the most valuable functionality that override provides is explicitly telling the programmers which methods are from the the super class and which methods are new. This is especially valuable when reading code of a class in a complicated inheritance tree.

Ideally, I think such behavior should be a part of --noImplicitOverride. However, since that ship has already sailed, it seems a new flag is required.


Side note: I do realize that implementing an abstract method is technically not really overriding anything. However, by this line of reasoning, when implementing abstract methods override should be completely disallowed instead of optional.


(Hey, the person giving me thumbs down, it will be better if you can come out and explain why you don't agree with me.)

πŸ‘‡

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Jan 3, 2022
@diesal11
Copy link

diesal11 commented Jan 5, 2022

I found myself here after having enabled --noImplicitOverride thinking this would be the default behaviour. Would love to see this implemented in future.

@jonlepage
Copy link

jonlepage commented Jan 12, 2022

I am rather ok with this !, sugar view i always welcome for productivity and ts is a powerful productivity tool !
And i would like even a new flag to distinguish a implementation made from an abstraction.
A modifier flag like contrat so we can know it came from a Abstract class.

ts example

@soyliquid
Copy link

I see the problem as the lack of identifiability of the inherited abstract methods.
The --noImplicitOverride did not work as expected.
I hope this issue will be resolved.

@fhaag
Copy link

fhaag commented Jun 5, 2022

I would wish for this feature, as well, and I would like to add the following scenario that applies especially for larger teams/projects:


Developer A creates abstract class X with an abstract method doSomething().

Other developers derive various subclasses from X, implementing doSomething() without the override keyword (which works, even though --noImplicitOverride is set).

Now, as it turns out, several of these classes share the same implementation of doSomething(), and considering the use cases that came up so far, it becomes obvious that this implementation can indeed be regarded as some sort of a default. Therefore, as a courtesy to developers who derive subclasses of X in the future, this default implementation of doSomething() is added to the base class by making doSomething() non-abstract.

Result: Suddenly, all of the existing subclasses of X break, now being in violation of --noImplicitOverride.

Undesirable consequences: Just two options, both of which should not be necessary, and could have been prevented with the proposed --noImplicitAbstractOverride option:

  • A modifies all the subclasses to include the override keyword, even though those classes are not within the scope of code that A is supposed to touch.
  • All the developers of subclasses have to update their code, even though conceptually, nothing has changed on their side (they are overriding a method from the base class).

@frank-weindel
Copy link

frank-weindel commented Sep 2, 2022

I agree with this. I think the override keyword more than anything else helps inform developers that a method and its signature are specified in the base class and hence its implementation may have some influence on the class that is not readily apparent by just looking at the sub-class implementation. While override may not be actually what you're doing in an abstract class, I still think its relatively intuitive.

@theacodes
Copy link

Just ran into this issue myself, I'd love to see it added. What needs to be done to get it accepted?

@fhaag
Copy link

fhaag commented Feb 19, 2023

@djmisterjon

I am rather ok with this !, sugar view i always welcome for productivity and ts is a powerful productivity tool ! And i would like even a new flag to distinguish a implementation made from an abstraction. A modifier flag like contrat so we can know it came from a Abstract class.

ts example

Would you mind having a look at my use case and share your thoughts?

My motivation for supporting this feature request is specifically that I do not want to have to change my subclass code depending on whether I am overriding an abstract (i.e. non-implemented, must-override) or a virtual (i.e. implemented, but overrideable) method of the parent class.

Same as in C#, if you will, where override just indicates you are overriding something inherited from the base class, and it doesn't matter whether the inherited method was virtual or abstract (or it may even change after you have finished your subclass implementation).

@wscales10
Copy link

I'd really appreciate this - requiring the use of the override keyword makes it easier to see which methods are inherited.

@movedoa
Copy link

movedoa commented Aug 9, 2023

Just found this issue after wondering why noImplicitOverride doesnt work. If typescript has a way to force override, there should be a way to enable it for everything.

@theacodes
Copy link

Bumping this question:

What needs to be done to get it accepted?

I'd be happy to take on whatever additional work needs to be done if given direction. Does the proposal need more details or motivating examples? Do we just need to be patient? There hasn't been any activity by any of the contributors since the initial filing well over a year ago.

@jemunk
Copy link

jemunk commented Aug 28, 2023

I would also vote for the addition of this flag.
I miss a warning whenever I forget specifying "override" on methods implementing an abstract method. ... Or simply make the "noImplicitOverride" flag also do this check

@fudom
Copy link

fudom commented Aug 31, 2023

Interesting... I came here to figure out how to flag use of unnecessary override on abstract methods. During a code review. Because it's no override at all. It's a declaration of required method like using an interface. I wondered why TypeScript does not flag if override is added on a declaration of an abstract method.

export abstract class Foo {
  public abstract bar(): boolean;

  public qux(): void {
    console.log('LOL');
  }
}

export class Baz extends Foo {
  // Expected warning/error for override keyword here. It's no override. It's an initial implementation.
  public override bar(): boolean {
    return true;
  }

  // This is correct. Because it's an override.
  public override qux(): void {
    console.log('ROFL');
  }
}

Here I would remove override from logical aspect. The declaration from the abstract method is required and is not an override.

Currently you can add or omit the override without any issues. I understand the idea behind require override on abstract method to directly see what methods comes from extern / inherited classes. But I'm not sure if this is correct. There is nothing overridden. Maybe the IDE could highlight such extern methods? I'm just thinking aloud. You can't remove or rename a declaration of abstract method without causing an error. And is it important to know what methods are from abstract class? The override keyword for methods you override is helpful. But I also understand the request of a mark on abstract methods. But it's no implicit override in case of abstract methods. Don't h8 me. ^^ Maybe this is a good idea. Idk. I wonder what a member thinks about this... And how other languages do it... And what are the use cases... You can see which methods come from the abstract definition. But it's still no override. It might also not be a good idea to introduce a new keyword for this. Idk. What does a member of TS say?

@SCLeoX
Copy link
Author

SCLeoX commented Sep 1, 2023

Interesting... I came here to figure out how to flag use of unnecessary override on abstract methods. During a code review. Because it's no override at all. It's a declaration of required method like using an interface. I wondered why TypeScript does not flag if override is added on a declaration of an abstract method.

export abstract class Foo {
  public abstract bar(): boolean;

  public qux(): void {
    console.log('LOL');
  }
}

export class Baz extends Foo {
  // Expected warning/error for override keyword here. It's no override. It's an initial implementation.
  public override bar(): boolean {
    return true;
  }

  // This is correct. Because it's an override.
  public override qux(): void {
    console.log('ROFL');
  }
}

Here I would remove override from logical aspect. The declaration from the abstract method is required and is not an override.

Currently you can add or omit the override without any issues. I understand the idea behind require override on abstract method to directly see what methods comes from extern / inherited classes. But I'm not sure if this is correct. There is nothing overridden. Maybe the IDE could highlight such extern methods? I'm just thinking aloud. You can't remove or rename a declaration of abstract method without causing an error. And is it important to know what methods are from abstract class? The override keyword for methods you override is helpful. But I also understand the request of a mark on abstract methods. But it's no implicit override in case of abstract methods. Don't h8 me. ^^ Maybe this is a good idea. Idk. I wonder what a member thinks about this... And how other languages do it... And what are the use cases... You can see which methods come from the abstract definition. But it's still no override. It might also not be a good idea to introduce a new keyword for this. Idk. What does a member of TS say?

In the context of your example, recognizing that bar originates from the parent class provides a crucial insight. The inclusion of the override modifier serves as a clear indicator that this method is inherent to the parent class rather than introducing new functionalities.

Therefore, I think it is better to force override, as it makes more logical sense, even if it is technically not overriding anything. It is also how java handles the @Override modifier.

@SCLeoX
Copy link
Author

SCLeoX commented Sep 1, 2023

For example, if you have:

export abstract class Animal {
  public abstract makeNoise();
  public makeLotsOfNoise() {
    setInterval(() => this.makeNoise(), 100);
  }
}

And in another file, you have:

class Dog extends Animal {
  public override makeNoise() {
    console.info("Woof");
  }
  public catchBall() {
    // idk
  }
}

For the makeNoise method, from the override modifier alone, you can immediately tell, the intention of the developer is to complete or modify the from the base class. Whatever you put into makeNoise, it is a part of Animal.

However, for non-override methods, it is very clear that those are completely new methods. They are specific to Dogs.

@fhaag
Copy link

fhaag commented Sep 6, 2023

Because it's no override at all.

Is it not? It changes the behaviour of something declared (and thus callable) on the base class. In this case, from "having undefined behaviour" (even though in practice, you won't get there, because it doesn't compile) to "doing whatever is implemented in the overridden method.

I understand the idea behind require override on abstract method to directly see what methods comes from extern / inherited classes. But I'm not sure if this is correct. There is nothing overridden.

At least in my view, a part of this is that it should not matter to the subclass whether whatever method it overrides was abstract or virtual in the base class. The base class should be able to e.g. replace an abstract method with a virtual method at a later time without breaking any of the already existing subclasses. It's no conceptual change from the subclass's point of view - the subclass author just needs to know they are supplying behaviour that might be invoked via the base class.

Maybe you can argue override is not the right term to express the concept, but whatever term is the right one, I'm strongly in favour of using the same term no matter whether the base method was abstract or virtual.

Maybe the IDE could highlight such extern methods? I'm just thinking aloud. You can't remove or rename a declaration of abstract method without causing an error.

If you have no marker on the method that implements an abstract method, how can the compiler know the abstract method implemented by a given method X has been removed, rather than the method X simply being a newly introduced method in the class at hand?

And how other languages do it...

Well, in C#, it's the override modifier, which is mandatory whenever you override an inherited virtual or abstract member. Granted, this might bias my impression of what is the "correct" thing to enforce ...

@Jethril
Copy link

Jethril commented Sep 12, 2023

Because it's no override at all.

In most languages, (e.g Kotlin, C#, Java), "override" not only means we meant to override some method but it also warns the user (if he's not prevented from compiling) if the method doesn't override nor implement anything.
It helps a lot while refactoring abstract classes, and could even be useful when dealing with interfaces.

I think the discussion should be about the meaning of the "override" word which may not be appropriate. Maybe we could use an "implements" keyword, but adding further keywords has to be balanced by the language complexity and learning curve.

@theacodes
Copy link

it also warns the user (if he's not prevented from compiling) if the method doesn't override nor implement anything.

This is the critical thing that's missing in TypeScript without --noImplicitAbstractOverride. From Dart:

The @override annotation expresses the intent that a declaration should override an interface method, something which is not visible from the declaration itself. This extra information allows the analyzer to provide a warning when that intent is not satisfied, where a member is intended to override a superclass member or implement an interface member, but fails to do so. Such a situation can arise if a member name is mistyped, or if the superclass renames the member.

Python similarly has an override annotation for static type checking with the same intent:

This PEP proposes adding an @override decorator to the Python type system. This will allow type checkers to prevent a class of bugs that occur when a base class changes methods that are inherited by derived classes.

And explicitly mentions a strict mode that requires @override:

... a strict mode where explicit @OverRide is required

I think the discussion should be about the meaning of the "override" word which may not be appropriate

I don't see a need to endlessly work on defining what "override" means. C#, Kotlin, Scala, and Swift always require explicit overrides. Override has clear semantics and precedence across other languages that already match TypeScript. The only part that's missing is a warning or error if the user implicitly overrides a base class/interface method. Like Python, we can make it opt-in and possibly add it to --strict, which is exactly what this feature request asks for.

@Jethril
Copy link

Jethril commented Sep 13, 2023

I don't see a need to endlessly work on defining what "override" means. C#, Kotlin, Scala, and Swift always require explicit overrides. Override has clear semantics and precedence across other languages that already match TypeScript. The only part that's missing is a warning or error if the user implicitly overrides a base class/interface method. Like Python, we can make it opt-in and possibly add it to --strict, which is exactly what this feature request asks for.

I completely agree with you, I just provided another point of view that fhaag may find acceptable.

@fhaag
Copy link

fhaag commented Sep 13, 2023

I just provided another point of view that fhaag may find acceptable.

For the use case described above, I do not really care what the keyword is, as long as it is the same keyword no matter whether the inherited method was virtual or abstract.

@zepumph
Copy link

zepumph commented Nov 27, 2023

My team just ran into this, and I wish we could accept this proposal. It looks like there was already a PR the didn't go anywhere in #51357. What will it take to get momentum on this issue? 50 upvotes and a fix already proposed isn't enough?

@dgreensp
Copy link

I would love this. I hope it gets accepted; seems like an easy win to me.

@adamk33n3r
Copy link

Wow, I honestly can't believe it has been just about 3 years since this issue was opened and no movement from maintainers yet...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.