microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.29k stars 12.39k forks source link

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

Open SCLeoX opened 2 years ago

SCLeoX commented 2 years ago

Suggestion

🔍 Search Terms

flag override implicit noImplicitOverride abstract

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ 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.)

👇

diesal11 commented 2 years ago

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

jonlepage commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

frank-weindel commented 2 years ago

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 commented 1 year ago

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

fhaag commented 1 year ago

@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 commented 1 year ago

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

movedoa commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 span>@</spanoverride 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 span>@</spanoverride 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 span>@</spanoverride:

... a strict mode where explicit span>@override</span 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 commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

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 https://github.com/microsoft/TypeScript/pull/51357. What will it take to get momentum on this issue? 50 upvotes and a fix already proposed isn't enough?

dgreensp commented 4 months ago

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