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

Support override keyword on class methods #2000

Closed
rwyborn opened this issue Feb 10, 2015 · 223 comments · Fixed by #39669
Closed

Support override keyword on class methods #2000

rwyborn opened this issue Feb 10, 2015 · 223 comments · Fixed by #39669
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@rwyborn
Copy link

rwyborn commented Feb 10, 2015

(Update by @RyanCavanuagh)

Please see this comment before asking for "any updates", "please add this now", etc.. Comments not meaningfully adding to the discussion will be removed to keep the thread length somewhat reasonable.


(NOTE, this is not a duplicate of Issue #1524. The proposal here is more along the lines of the C++ override specifier, which makes much more sense for typescript)

An override keyword would be immensely useful in typescript. It would be an optional keyword on any method that overrides a super class method, and similar to the override specifier in C++ would indicate an intent that "the name+signature of this method should always match the name+signature of a super class method". This catches a whole range of issues in larger code bases that can otherwise be easily missed.

Again similar to C++, it is not an error to omit the override keyword from an overridden method. In this case the compiler just acts exactly as it currently does, and skips the extra compile time checks associated with the override keyword. This allows for the more complex untyped javascript scenarios where the derived class override does not exactly match the signature of the base class.

Example use

class Animal {
    move(meters:number):void {
    }
}

class Snake extends Animal {
    override move(meters:number):void {
    }
}

Example compile error conditions

// Add an additional param to move, unaware that the intent was 
// to override a specific signature in the base class
class Snake extends Animal {
    override move(meters:number, height:number):void {
    }
}
// COMPILE ERROR: Snake super does not define move(meters:number,height:number):void

// Rename the function in the base class, unaware that a derived class
// existed that was overriding the same method and hence it needs renaming as well
class Animal {
    megamove(meters:number):void {
    }
}
// COMPILE ERROR: Snake super does not define move(meters:number):void

// Require the function to now return a bool, unaware that a derived class
// existed that was still using void, and hence it needs updating
class Animal {
    move(meters:number):bool {
    }
}
// COMPILE ERROR: Snake super does not define move(meters:number):void

IntelliSense

As well as additional compile time validation, the override keyword provides a mechanism for typescript intellisense to easily display and select available super methods, where the intent is to specifically override one of them in a derived class. Currently this is very clunky and involves browsing through the super class chain, finding the method you want to override, and then copy pasting it in to the derived class to guarantee the signatures match.

Proposed IntelliSense mechanism

Within a class declaration:

  1. type override
  2. An auto complete drop down appears of all super methods for the class
  3. A method is selected in the drop down
  4. The signature for the method is emitted into the class declaration after the override keyword.
@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Feb 10, 2015
@stephanedr
Copy link

Indeed a good proposal.

However what about the following examples, which are valid overrides to me:

class Snake extends Animal {
    override move(meters:number, height=-1):void {
    }
}
class A {...}
class Animal {
    setA(a: A): void {...}
    getA(): A {...}
}

class B extends A {...}
class Snake extends Animal {
    override setA(a: B): void {...}
    override getA(): B {...}
}

Additionally I would add a compiler flag to force the override keyword to be present (or reported as a warning).
The reason is to catch when renaming a method in a base class that inherited classes already implement (but not supposed to be an override).

@rwyborn
Copy link
Author

rwyborn commented Feb 10, 2015

Ah nice examples. Generally speaking I would expect the use of the override keyword to enforce exact matching of signatures, as the goal of using it is to maintain a strict typed class hierarchy. So to address your examples:

  1. Adding an additional default param. This would generate a compile error: Snake super does not define move(meters:number):void. While the derived method is functionally consistent, client code calling Animal.move may not expect derived classes to also be factoring in height (as the base API does not expose it).
  2. This would (and should always) generate a compile error, as it is not functionally consistent. Consider the following addition to the example:
class C extends A {...}
var animal : Animal = new Snake();
animal.setA(new C());
// This will have undefined run-time behavior, as C will be interpreted as type B in Snake.setA

So example (2.) is actually a great demo of how an override keyword can catch subtle edge cases at compile time that would otherwise be missed! :)

And I would again stress that both examples may be valid in specific controlled/advanced javascript scenarios that may be required... in this case users can just choose to omit the override keyword.

@NoelAbrahams
Copy link

This will be useful. We currently work around this by including a dummy reference to the super method:

class Snake extends Animal {
    move(meters:number, height?:number):void {
         super.move; // override fix
    }
}

But this only guards against the second case: super methods being renamed. Changes in signature do not trigger a compilation error. Furthermore, this is clearly a hack.

I also don't think that default and optional parameters in the derived class method's signature should trigger a compilation error. That may be correct but goes against the inherent flexibility of JavaScript.

@stephanedr
Copy link

@rwyborn
It seems we don't expect the same behaviour.
You would use this override keyword to ensure a same signature, whereas I would use it more as a readibiliy option (so my request to add a compiler option to force its usage).
In fact what I would really expect is that TS detects invalid overriding methods (even without the use of override).
Typically:

class Snake extends Animal {
    move(meters:number, height:number):void {}
}

should raise an error, because it's really an override of Animal.move() (JS behaviour), but an incompatible one (because height is not supposed to be optional, whereas it will be undefined if called from an Animal "reference").
In fact using override would only confirm (by the compiler) that this method really exists in the base class (and so with a compliant signature, but due to the previous point, not due to the override keyword).

@rwyborn
Copy link
Author

rwyborn commented Feb 11, 2015

@stephanedr , speaking as a single user I actually agree with you that the compiler should just always confirm the signature, as I personally like to enforce strict typing within my class hierarchies (even if javascript doesn't!!).

However in proposing that this behavior is optional via the override keyword I am trying to keep in mind that ultimately javascript is untyped, and hence enforcing the strict signature matching by default would result in some javascript design patterns no longer being expressible in Typescript.

@kungfusheep
Copy link

@rwyborn I'm glad you mentioned the C++ implementation because that's exactly how I imagined it should work before I got here - optionally. Although, a compiler flag that forced the use of the override keyword would go down well in my book.

The keyword would allow compile time errors for a developers clumsy typing, which is what worries me most about overrides in their current form.

class Base {
    protected commitState() : void {

    }
}


class Implementation extends Base {
    override protected comitState() : void {   /// error - 'comitState' doesn't exist on base type

    }
}

Currently (as of 1.4) the Implementation class above would just declare a new method and the developer would be none the wiser until they notice their code isn't working.

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed In Discussion Not yet reached consensus labels May 4, 2015
@RyanCavanaugh
Copy link
Member

Discussed at suggestion review.

We definitely understand the use cases here. The problem is that adding it at this stage in the language adds more confusion than it removes. A class with 5 methods, of which 3 are marked override, wouldn't imply that the other 2 aren't overrides. To justify its existence, the modifier would really need to divide the world more cleanly than that.

@hdachev
Copy link

hdachev commented May 5, 2015

Please excuse the whining, but honestly, while your argument does apply to the public keyword in a language where everything is public by default, quite world dividing indeed, having things like abstract and an optional override keyword will simply help developers feel safer, make less mistakes and waste less time.

Overriding is one of the few remaining highly-typo-sensitive aspects of the language, because a mistyped overriding method name is not an obvious compile time problem. The benefit of override is obvious, as it you allows you to state your intention to override - if the base method doesn't exist its a compile time error. All hail the type system. Why would anyone not want this?

@rwyborn
Copy link
Author

rwyborn commented May 5, 2015

I concur 100% with @hdachev , the small inconsistency referred too by @RyanCavanaugh is easily out weighed by the benefits of the keyword in bringing compile time checks to method overrides. I would again point out that C++ uses an optional override keyword successfully in exactly the same way as suggested for typescript.

I can not emphasize enough how much of a difference override checking makes in a large scale code base with complex OO trees.

Finally I would add that if the inconsistency of an optional keyword really is a concern, then the C# approach could be used, that is the mandatory use of either the "new" or "override" keywords:

class Dervied extends Base {

    new FuncA(newParam) {} // "new" says that I am implementing a new version of FuncA() with a different signature to the base class version

    override FuncB() {} // "override" says that I am implementing exactly the same signature as the base class version

    FuncC() {} // If FuncC exists in the base class then this is a compile error. I must either use the override keyword (I am matching the signature) or the new keyword (I am changing the signature)
}

@RyanCavanaugh
Copy link
Member

This isn't analogous to public because a property without an access modifier is known to be public; a method without override is not known to be non-override.

Here's a runtime-checked solution using decorators (coming in TS1.5) that produces good error messages with very little overhead:

/* Put this in a helper library somewhere */
function override(container, key, other1) {
    var baseType = Object.getPrototypeOf(container);
    if(typeof baseType[key] !== 'function') {
        throw new Error('Method ' + key + ' of ' + container.constructor.name + ' does not override any base class method');
    }
}

/* User code */
class Base {
    public baseMethod() {
        console.log('Base says hello');
    }
}

class Derived extends Base {
    // Works
    @override
    public baseMethod() {
        console.log('Derived says hello');
    }

    // Causes exception
    @override
    public notAnOverride() {
        console.log('hello world');
    }
}

Running this code produces an error:

Error: Method notAnOverride of Derived does not override any base class method

Since this code runs at class initialization time, you don't even need unit tests specific to the methods in question; the error will happen as soon as your code loads. You can also sub in a "fast" version of override that doesn't do any checking for production deployments.

@rwyborn
Copy link
Author

rwyborn commented Oct 1, 2015

@RyanCavanaugh So we are at Typescript 1.6 and decorators are still an experimental feature, not something I want to deploy in a large scale production codebase as a hack to get override working.

To try yet another angle, every popular typed language out there supports the "override" keyword; Swift, ActionScript, C#, C++ and F# to name a few. All these languages share the minor issues you have expressed in this thread about override, but clearly there is a large group out there who see that the benefits of override far out weigh these minor issues.

Are your objections purely based on cost/benefit? If I was to actually go ahead and implement this in a PR would it be accepted?

@DanielRosenwasser
Copy link
Member

It's not just a cost/benefit issue. As Ryan explained, the issue is that marking a method as an override doesn't imply that another method isn't an override. The only way that it would make sense is if all overrides needed to be marked with an override keyword (which if we mandated, would be a breaking change).

@rwyborn
Copy link
Author

rwyborn commented Oct 1, 2015

@DanielRosenwasser As outlined above, in C++ the override keyword is optional (exactly as proposed for Typescript) yet everyone uses it no problem and it is hugely useful on large code bases. Furthermore in Typescript is actually makes a lot of sense for it to be optional because of javascript function overloading.

class Base {
    method(param: number): void { }
}

class DerivedA extends Base {
    // I want to *exactly* implement method with the same signature
    override method(param: number): void {}
}

class DerivedB extends Base {
    // I want to implement method, but support an extended signature
    method(param: number, extraParam: any): void {}
}

As for the whole "doesn't imply that another method isn't an override" argument, It is exactly analogous to "private". You can write an entire codebase without ever using the private keyword. Some of the variables in that codebase will only ever be accessed privately and everything will compile and work fine. However "private" is some extra syntactic sugar you can use to tell the compiler "No really, compile error if someone tries to access this". In the same way "overload" is extra syntactic sugar to tell the compiler "I want this to exactly match the base class declaration. If it doesn't compile error".

@rwyborn
Copy link
Author

rwyborn commented Oct 1, 2015

You know what, I think you guys are fixated on the literal interpretation of "override". Really what it is marking up is "exactly_match_signature_of_superclass_method", but thats not quite as readable :)

class DerivedA extends Base {
    exactly_match_signature_of_superclass_method method(param: number): void {}
}

@jkristia
Copy link

I too would like to have the override keyword available, and have the compiler generate an error if the method marked for override doesn't exist, or has a different signature, in the base class. It would help readability and for refactoring

@olmobrutall
Copy link

+1, also the tooling will get much better. My use case is using react. I've to check the definiton every time I'm using the ComponentLifecycle methods:

    interface ComponentLifecycle<P, S> {
        componentWillMount?(): void;
        componentDidMount?(): void;
        componentWillReceiveProps?(nextProps: P, nextContext: any): void;
        shouldComponentUpdate?(nextProps: P, nextState: S, nextContext: any): boolean;
        componentWillUpdate?(nextProps: P, nextState: S, nextContext: any): void;
        componentDidUpdate?(prevProps: P, prevState: S, prevContext: any): void;
        componentWillUnmount?(): void;
    }

With override, or other equivalent solution,you'll get a nice auto-completion.

One problem however is that I will need to override interface methods...

export default class MyControlextends React.Component<{},[}> {
    override /*I want intellisense here*/ componentWillUpdate(nextProps, nextState, nextContext): void {

    }
}

@RyanCavanaugh
Copy link
Member

@olmobrutall it seems like your use case is better solved by the language service offering refactorings like "implement interface" or offering better completion, not by adding a new keyword to the language.

@rwyborn
Copy link
Author

rwyborn commented Dec 14, 2015

Lets not get distracted thou :) Language service features are only a small part of maintaining interface hierarchies in a large code base. By far and away the biggest win is actually getting compile time errors when a class way down in your hierarchy somewhere does not conform. This is why C++ added an optional override keyword (non-breaking change). This is why Typescript should do the same.

Microsoft's documentation for C++ override sums things up nicely, https://msdn.microsoft.com/en-us/library/jj678987.aspx

Use override to help prevent inadvertent inheritance behavior in your code. The following example shows where, without using override, the member function behavior of the derived class may not have been intended. The compiler doesn't emit any errors for this code.
...
When you use override, the compiler generates errors instead of silently creating new member functions.

@kungfusheep
Copy link

By far and away the biggest win is actually getting compile time errors when a class way down in your hierarchy somewhere does not conform.

I have to agree. One pitfall that crops up in our teams is people thinking they've overridden methods when in fact they've slightly mis-typed or extended the wrong class.

Interface changes in a base library when working across many projects is harder than it should be in a language with an agenda like TypeScript.

We have so much great stuff, but then there are oddities like this and no class-level const properties.

@olmobrutall
Copy link

@RyanCavanaugh true, but the language service could be triggered after writing override, as many languages do, without the keyword is much harder to figure out when is the right moment.

About implement interface, note that most of the methods in the interface are optional and you're meant to override only the few ones you need, not the whole package. You could open a dialog with check boxes but still...

And while my current pain point is to find the name of the method, in the future will be great to get notified with compile-time errors if someone renames or changes the signature of a base method.

Couldn't the inconsitency be solve just by adding a warning when you don't write override? I think typescript is doing the right thing adding small reasonable breaking changes instead of preserving bad decisions for the end of time.

Also abstract is already there, they will make an awesome couple :)

@armandn
Copy link

armandn commented Mar 4, 2016

I felt the need for an 'override' specifier, too. In medium to large projects, this feature becomes essential and with all due respect I hope the Typescript team will reconsider the decision to decline this suggestion.

@kungfusheep
Copy link

For anyone interested we've written a custom tslint rule that provides the same functionality as the override keyword by using a decorator, similar to Ryan's suggestion above but checked at compile-time. We'll be open-sourcing it soon, I'll post back when it's available.

@kyasbal
Copy link

kyasbal commented Mar 11, 2016

I strongly felt necessity of 'override' keyword, too.
In my case, I changed some of method name in a base class and I forgot to rename some of names of overriding methods. Of course, this leads some of bugs.

But, If there was such a feature, we can find these class methods easily.

As @RyanCavanaugh mentioned, if this keyword is just a optional keyword, this feature makes confusion. So, how about to make something flag in tsc to enable this feature?

Please reconsider this feature again....

@bioball
Copy link

bioball commented Nov 11, 2020

@nin-jin In your model, this means not using the override keyword is still legal, right? For instance:

class Base {
  myMethod () { ... }
}

class Overridden extends Base {
  // this would not fail because this is interpreted as define | override.
  myMethod () { ... }
}

Ideally, the above example produces an error unless you use the override keyword. Albeit, I imagine that there would be a compiler flag to opt-out of override checking, where opting out is the same thing as override | define for all methods

@nin-jin
Copy link

nin-jin commented Nov 11, 2020

@bioball yes, it's required for compatibility with a lot of existed code.

@sam-s4s
Copy link

sam-s4s commented Nov 12, 2020

@sam-s4s Entity extends Base, of course. :-) I have fixed my message. virtual is about something else.

I'm still not sure what you mean by define... this is the definition of virtual in C#:

The virtual keyword is used to modify a method, property, indexer, or event declaration and allow for it to be overridden in a derived class.

Maybe you mean the opposite, where, instead of needing to mark the function as virtual in order to override, you can mark the function as define to then mean you have to use the override keyword to override it?

(why define? I have not seen that keyword in another language)

@fresheneesz
Copy link

I skimmed through this thead in a couple minutes, but I haven't seen anyone propose using override as a way to avoid duplicate specification of parameters and return values. For example:

class Animal {
    move(meters:number):void {
    }
}

class Snake extends Animal {
    override move(meters) {
    }
}

In the above example, move would have the same required return type (void) and meters would also have the same type, but specifying it wouldn't be necessary. The large company I work for is trying to migrate all our javascript to typescript, away from Closure compiler typing. However, while in Closure we can just use @override and all the types will be inferred from the superclass. This is quite useful in reducing the possibility for mismatch and reducing duplication. One could even imagine implementing this such that extension of the method with additional parameters is still possible, even while not specifying types for the parameters that are specified in the superclass. For example:

class Animal {
    move(meters:number):number {
    }
}

class Snake extends Animal {
    override move(meters, time:number) {
    }
}

The motivation for me coming here and writing a comment is that our company is now requiring that we specify all the types even for overridden methods, because typescript doesn't have this functionality (and we're in a long migration). Its quite annoying.

@shicks
Copy link
Contributor

shicks commented Dec 3, 2020

FWIW, while it's easier to write, omitting parameter types (and other instances of cross-file type inference) hinders readability, since it requires someone unfamiliar with the code to dig around to find where the superclass is defined in order to know what type meters is (assuming you're reading it outside an IDE, which is quite common for unfamiliar projects where you don't have an IDE set up yet). And code is read much more often than it's written.

@fresheneesz
Copy link

fresheneesz commented Dec 3, 2020

@shicks You could say that about literally any imported variable or class. Duplicating all the information you might need onto a single file defeats the purpose of abstraction and modularity. Forcing types to be duplicated violates the DRY principle.

@distante
Copy link

distante commented Jan 18, 2021

If I can share my 2 cents.

Since override is somehow contraintuitive for javascript developers (where all start as public and "overridable" for default). Why don't we think about add a final ?

abstract class MyBase {
   myRegularMethod(): {}
   final myFinalMethod(): {}
}


class MyImplementation extends MyBase {
   myRegularMethod(): { 
     /// Everything works here! 👍 
   }

// Compiler error!!!  myFinalMethod is final💀  
   myFinalMethod(): { 
    
   }
}

That would be backwards compatible.

@tmtron
Copy link

tmtron commented Jan 18, 2021

@distante as pointed out here, override and final solve 2 different problems

@distante
Copy link

@distante as pointed out here, override and final solve 2 different problems

Oh really sorry, I missed it.

If this helps somebody, there is a tslint rule to force a @override comment here.

@aipirvu
Copy link

aipirvu commented Jan 18, 2021

If I can share my 2 cents.

Since override is somehow contraintuitive for javascript developers (where all start as public and "overridable" for default). Why don't we think about add a final ?

abstract class MyBase {
   myRegularMethod(): {}
   final myFinalMethod(): {}
}


class MyImplementation extends MyBase {
   myRegularMethod(): { 
     /// Everything works here! 👍 
   }

// Compiler error!!!  myFinalMethod is final💀  
   myFinalMethod(): { 
    
   }
}

That would be backwards compatible.

Because with a final you prevent the use of that method in the extended class as with override that is permitted but you need to explicitly mention your intent and the writer of the super/base method will know that whoever overrides it will make a conscious choice to call the super method or not.

As for the familiarity of javascript developer with override, that is not necessarily a valid argument. So is the case with other features that TypeScript has over JavaScript and that is the exact reason why it is used. The functionality can however be an option "explcitOverride" that is turned off by default

@aprilmintacpineda
Copy link

What's the latest update? seems like it's still unclear and it's been almost 6 years...

@Kingwl
Copy link
Contributor

Kingwl commented Jan 19, 2021

What's the latest update? seems like it's still unclear and it's been almost 6 years...

Actually there's a PR: #39669

@felixhirt
Copy link

seems its also still planned to be included in the next release: #41601 (at least at the time of writing this)

@DanielRosenwasser
Copy link
Member

Thank you @Kingwl, and thank you @pcj for the groundwork in the initial proof-of-concept!

@DanielRosenwasser DanielRosenwasser added Committed The team has roadmapped this issue Fixed A PR has been merged for this issue and removed Revisit An issue worth coming back to labels Mar 26, 2021
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.3.0 milestone Mar 26, 2021
@seiyria
Copy link

seiyria commented Apr 5, 2021

I'm not sure where to ask this, but it seems like tsc is compiling and leaving the override keyword in the compiled result. Is this something other folks are seeing? It seems like it needs the signature to match exactly, or it will show up in the output, see here:

https://www.typescriptlang.org/play?ts=4.3.0-beta#code/LAKAxgNghgzjAEBlADgUwheBvUp72QFcAjCASzHgHNUAXAYQAsoAnGmACgEoAueAO0IBbYqhbY8+eCzqEW-eAAYA3JIC+uEPiKkK1OgBE5UWmQD2-bn0EixErVJm05Cles3aS5SmFi0r8ABuZmQAJtgaIJGgkLAIABJmEACeAILG8KgAHrSo-KEIKOiYOCCSZoFiLGGo+gzMbKicXPZS0rLy8ACMbg7RDhVVNXVGLCbmlrwCwqLipW1OLvAAzL34-fiDLNWhtb4w-lPBYa3w-WpAA

@abdulkareemnalband
Copy link

Also .d.ts file is missing override.
Is it by design? aka for back compat with old ts compilers

@jakebailey
Copy link
Member

Also .d.ts file is missing override.
Is it by design? aka for back compat with old ts compilers

I don't see a reason why .d.ts files would need to contain that keyword; the override check is only useful for each individual class (as it's saying "this class you are writing has methods that don't declare they are overrides"). You can't really do anything with the knowledge that some other class has overriden a method from one of its own parent classes.

Leaving "override" in the JS source definitely sounds like a bug though. 🙂

@DanielRosenwasser
Copy link
Member

Jake is right. I've opened #43535 to track the JS output bug.

@legistek
Copy link

legistek commented Sep 9, 2021

This is a fantastic feature but what about taking it a step farther and requiring use of the virtual keyword on the base class so you can actually control when you want to allow sub-classes to override or not? That could also be an opt-in switch, noImplicitVirtual.
There are often situations where overriding a function without calling super could break the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add a Flag Any problem can be solved by flags, except for the problem of having too many flags Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.