Quick fix: "Implement inherited abstract class" should add override keyword to methods

[KilianB]

Suggestion

🔍 Search Terms

quick fix, code action, override

✅ Viability Checklist

My suggestion meets these guidelines:

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

⭐ Suggestion

When selecting the quick fix action Implement inherited abstract class the generated method signatures should include the override keyword which has been present since version 4.3.

class AdyenPaymentProvider extends LoggingPaymentProvider {
  protected override claimBalance(paymentReference: string, userId?: number | undefined): boolean {
    throw new Error('Method not implemented.');
  }
}

instead of

class AdyenPaymentProvider extends LoggingPaymentProvider {
  protected claimBalance(paymentReference: string, userId?: number | undefined): boolean {
    throw new Error('Method not implemented.');
  }
}

📃 Motivating Example

The autogenerated @Override annotation in Eclipse/java has helped me catch many subtle bug which would have otherwise taken a long time to track down. No harm is done adding the keyword automatically as it will promote the usage of said keyword to a wider audience which will result in better quality code down the line.

In conjunction with changes like #8306 typescript's OOP would simply be less error prone due to stricter enforcements down the line.

[a-tarasyuk]

@RyanCavanaugh Should override be used with inherited abstract methods (Playground)? Should we add override when noImplicitOverride is enabled?

/cc @DanielRosenwasser

[RyanCavanaugh]

There's only one answer that results in no errors, right? I think I'm missing something

[DanielRosenwasser]

We don't error on an abstract implementation member that omits override, the design meeting notes make it seem like it was brushed aside.

[JoshuaKGoldberg]

@a-tarasyuk did you mean to ask whether we should only add override when noImplicitOverride is enabled?

[a-tarasyuk]

@JoshuaKGoldberg I meant that override is not required when noImplicitOverride is disabled/enabled for abstract classes.

// @noImplicitOverride: true
abstract class A {
    abstract m(): void;
}

class B extends A {
    // ok
    m(): void {
        throw new Error("Method not implemented.");
    }
}

class A1 {
    m() {}
}

class B1 extends A1 {
    // requires override
    m(): void {
        throw new Error("Method not implemented.");
    }
}

[gabritto]

We don't error on an abstract implementation member that omits override, the design meeting notes make it seem like it was brushed aside.

@DanielRosenwasser can you link to the design notes or maybe explain why override isn't needed for abstract classes?

[RyanCavanaugh]

https://github.com/microsoft/TypeScript/issues/44457#issuecomment-856170640

[gabritto]

#44457 (comment)

Thanks, that makes a lot of sense. So why would we want to put override in the case brought up by this issue? Is it just to convey intention more explicitly? I guess what I want to understand is if we've committed to accepting a fix to this issue, and maybe why.

[RyanCavanaugh]

Well, I think we kind of messed up with noImplicitOverride under abstract and need to figure out what to do next. The intent of the flag, at least in my formulation, was that if you turned on that flag, everywhere you can write override, you must, and that override was legal if-and-only-iff you could call super.thisMethod();. It seems like for abstract we're not enforcing that correctly on either axis, but it's probably unpalatable to force people to write an override in places where super. isn't legal, or conversely to tell people they can't write override when there "is" a base class method.

Maybe the simplest argument, having put ourselves in this position, is that it's easier to remove the extra overrides than it is to figure out which methods "should" have it, so the quick fix should do the thing that results in the least amount of work in the case where it doesn't match the user's preference.