Closed rwyborn closed 3 years ago
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).
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:
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.
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.
@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).
@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.
@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.
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.
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?
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)
}
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.
@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?
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).
@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".
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 {}
}
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
+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 {
}
}
@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.
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.
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.
@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 :)
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.
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.
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....
To me, if the override keyword is to be useful, it needs to be enforced, like it is in C#. If you specify a method in C# that overrides the signature of a base method, you must label it either override or new.
C++ is annoying and inferior compared to C# in some places because it makes too many keywords optional, and therefore precludes consistency. For example, if you overload a virtual method, the overloaded method can be marked virtual or not - either way it will be virtual. I prefer the latter because it aids other developers who read the code, but I can't get the compiler to enforce it being put in, which means our code-base will undoubtedly have missing virtual keywords where they should really be. The override keyword is similarly optional. Both are crap in my opinion. What is overlooked here is that code can serve as documentation and improve maintainability by enforcing the need for keywords rather than a "take it or leave it" approach. The "throw" keyword in C++ is similar.
To achieve the above objective in TypeScript, the compiler needs a flag to "enable" this stringent behaviour or not.
Insofar as the function signatures of the base and the override are concerned, they should be identical. But it would be desirable to allow covariance in the specification of the return type to enforce a compile-time check.
I'm coming to TS from AS3 so of course, I'm going to throw my vote here for an override
keyword as well. To a developer who's new to any given codebase, seeing an override
is a huge clue as to what might be going on in a (child) class. I think such a keyword hugely adds value. I would opt to make it mandatory, but I can see how that would be a breaking change and so should be optional. I really don't see a problem with the ambiguity that imposes, though I am sensitive to the difference between an optional override
and the default public
keyword.
For all the +1ers -- can you talk about what's not sufficient about the decorator solution shown above?
To me it feels like an artificial construct, not a property of the language itself. And I guess that's by design, because that's what it is. Which I guess sends developer (well, me anyway) the message that it's transient, not a best practice.
obviously every language has its own paradigm and, being new to TypeScript I'm slow with the paradigm shift. I have to say though that override
DOES feel like a best practice to me for a number of reasons. I'm making the switch to TypeScript because I have fully swallowed the strongly-typed koolaid, and believe that the cost (in terms of keystrokes and learning curve) is heavily outweighed by the benefits both to error-free code and code comprehension. override
is a very important piece of that puzzle, and communicates some very important information about the role of the overriding method.
For me it's less about IDE convenience, though that is undeniably awesome when properly supported, and more about fulfilling what I believe to be the principles upon which you've built this language.
@RyanCavanaugh some issues that I see:
The compiler is already checking the argument list and return type, though. And I think even if override
did exist as a first-class keyword, we wouldn't enforce some new rule about signature identicalness
And I think even if override did exist as a first-class keyword, we wouldn't enforce some new rule about signature identicalness.
@RyanCavanaugh then I think you might be on a different page about the intent of the keyword. The whole point of it is for cases where you want to enforce signature identicalness. This is for the design pattern where you have a deep complex hierarchy sitting on a base class that defines a contract of interface methods, and all classes in the hierarchy must exactly match those signatures. By adding the override keyword on these methods in all the derived classes, you are alerted to any cases where the signature diverges from the contract set out in the base class.
As I keep saying this is not some esoteric use case. When working on a large code bases (with say hundreds or thousands of classes) this is an every day occurrence, ie someone needs to change the signature of the base class (modify the contract), and you want the compiler to alert you to any cases throughout the hierarchy that no longer match.
The compiler will already warn of illegal overrides. The issue with the current implementation is the lack of intent when declaring an overridden method.
The decorator mentioned before just seems to go against what the language is trying to achieve. There shouldn't be a runtime cost incurred for something that can be dealt with at compile time, however small the cost. We want to catch as much stuff going wrong as possible, without needing to run the code to find out.
It's possible to achieve using the decorator and custom a custom tslint rule, but would be better for the tooling ecosystem and the community for the language to have an official keyword.
I think being optional is one approach, but as with with some C++ compilers there are flags you can set which enforce its use (e.g. suggest-override, inconsistent-missing-override). This would seem the best approach to avoid breaking changes and seems consistent with other new features being added recently such as nullable types and decorators.
On Wed, 23 Mar 2016 at 21:31, Rowan Wyborn notifications@github.com wrote:
And I think even if override did exist as a first-class keyword, we wouldn't enforce some new rule about signature identicalness.
@RyanCavanaugh https://github.com/RyanCavanaugh then I think you might be on a different page about the intent of the keyword. The whole point of it is for cases where you want to enforce signature identicalness. This is for the design pattern where you have a deep complex hierarchy sitting on a base class that defines a contract of interface methods, and all classes in the hierarchy must exactly match those signatures. By adding the override keyword on these methods in all the derived classes, you are alerted to any cases where the signature diverges from the contract set out in the base class.
As I keep saying this is not some esoteric use case. When working on a large code bases (with say hundreds or thousands of classes) this is an every day occurrence, ie someone needs to change the signature of the base class (modify the contract), and you want the compiler to alert you to any cases throughout the hierarchy that no longer match.
— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/2000#issuecomment-200551774
@kungfusheep fwiw the compiler only catches a certain class of illegal overrides, that is where there is a direct clash between declared params. It does not catch the addition or removal of params, nor does it catch return type changes. These extra checks are what any override keyword would switch on.
The compiler correctly warns you if you add a parameter to an overriding function.
Removing a parameter, though, is totally valid:
class BaseEventHandler {
handleEvent(e: EventArgs, timestamp: number) { }
}
class DerivedEventHandler extends BaseEventHandler {
handleEvent(e: EventArgs) {
// I don't need timestamp, it's OK
}
}
Changing the return type is also totally valid:
class Base {
specialClone(): Base { ... }
}
class Derived extends Base {
specialClone(): Derived { ... }
}
@RyanCavanaugh yes they are valid from a strict language perspective, but this is why I started using the term "contract" above. If a base class lays out a specific signature, when I use override I am stating that I want my derived class to strictly follow that signature. If the base class adds additional params or changes return type, I want to know every point in my code base that no longer matches, as the contract they were originally written against has now changed in some way.
The idea of having a big class hierarchy each with its own slightly different permutations of the base methods (even if they are valid from a language perspective) is nightmare inducing and takes me back to the bad old days of javascript before typescript came along :)
If the optionality is the main problem with the keyword, then why not make it mandatory when the base class method is defined as abstract
? This way, if you wanted to enforce the pattern strictly you could simply add an abstract base class. To avoid breaking code a compiler switch could disable the check.
We seem to be talking about two different sets of expectations now. There's the missing-override/illegal override situation and then there's the explicit signature one. Can we all agree the former is the absolute bare-minimum we'd expect from this keyword?
I only say this because there are other ways of enforcing explicit method signatures currently, such as interfaces, but there is currently no way of explicitly stating the intent to override.
Ok, there is no way of enforcing explicit signatures of overridden methods but given that the compiler enforces that any signature changes are at least 'safe' then it seems like there is a separate conversation to be had around the solution to that problem.
Yeah agreed. If I had to choose, the missing-override/illegal override situation is the more important problem to solve.
I'm a bit late to the party... I think the whole point of Typescript is to enforce rules at compile-time, not at runtime, otherwise we'd all be using plain Javascript. Also, it's a bit weird to use hacks/kludges to do things that are standard in so many languages.
Should there be an override
keyword in Typescript? I certainly believe so. Should it be mandatory? For compatibility reasons I'd say its behavior could be specified with a compiler argument. Should it enforce the exact signature? I think this should be a separate discussion, but I haven't had any problem with the current behavior so far.
The original reason for closing this seems to be that we cannot introduce the breaking change that all overridden methods needs the override
specifier, which would make it confusing since the methods not marked with override
could also in fact be overrides.
Why not enforce it, either with a compiler option, and/or if there is at least one method marked override
in the class, then all methods that are overrides must be marked as such, otherwise it is an error?
Is it worth reopening this whilst it's up in the air?
On Fri, 8 Apr 2016 at 14:38, Peter Palotas notifications@github.com wrote:
The original reason for closing this seems to be that we cannot introduce the breaking change that all overridden methods needs the override specifier, which would make it confusing since the methods not marked with 'override' could also in fact be overrides.
Why not enforce it, either with a compiler option, and/or if there is at least 'one' method marked override in the class, then all methods that are overrides must be marked as such, otherwise it is an error?
— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/2000#issuecomment-207434898
I say please reopen! I'd be happy with a compiler option.
yes - please reopen
Reopen this!
Doesnt ES7 require this overriding/overloading multiple methods having same name? On Apr 8, 2016 10:56 AM, "Aram Taieb" notifications@github.com wrote:
Yes, please reopen this!
— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/2000#issuecomment-207466464
+1 - for me this is the largest gap in type safety that I have in day-to-day TypeScript development.
Every time I implement a React lifecycle method like "componentDidMount" I search for the relevant react documentation page and copy/paste the method name, to ensure that I don't have a typo. I do this because it takes 20 seconds whereas the bugs from a typo are indirect and subtle, and can take much longer to track down.
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.
If this is the major concern, call the keyword check_override
to make clear that you're opting in to override checking, and by implication the others methods are not checked rather than not overridden.
What about using implements.
? Being something like this:
class MyComponent extends React.Component<MyComponentProps, void>{
implements.componentWillMount(){
//do my stuff
}
}
This syntax has some advantages:
implements.
the IDE has an excelent oportunity to show an autocompletion popup. class MyComponent<MyComponentProps, MyComponentState> {
implements.state = {/*auto-completion for MyComponentState here*/};
implements.componentWillMount(){
//do my stuff
}
}
Note: Alternatively we could use base.
, it's sorter and more intuitive, but maybe more confusing (defining or calling?) and the meaning is not so compatible with interface implementation. Example:
class MyComponent<MyComponentProps, MyComponentState> {
base.state = {/*auto-completion for MyComponentState here*/};
base.componentWillMount(){ //DEFINING
//do my stuff
base.componentWillMount(); //CALLING
//do other stuff
}
}
I don't think implements
sufficiently covers all of the use cases
mentioned previously & it's a little ambiguous. It doesn't give any more
information to an IDE that override
couldn't either, so it would seem to
make sense to stick with the terminology many other languages have used to
achieve the same thing.
On Wed, 13 Apr 2016 at 19:06, Olmo notifications@github.com wrote:
What about using implements.? Being something like this:
class MyComponent extends React.Component<MyComponentProps, void>{ implements.componentWillMount(){ //do my stuff } }
This syntax has some advantages:
- It uses an already existing keyword.
- After writing implements. the IDE has an excelent oportunity to show an autocompletion method.
- the keyword clarifies that can be used to force checking on abstract methods of base classes but also implemented interfaces.
- the syntax is ambiguous enough to be usable also for fields, like this:
class MyComponent<MyComponentProps, MyComponentState> { implements.state = {/auto-completion for MyComponentState here/};
implements.componentWillMount(){ //do my stuff }
}
— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/Microsoft/TypeScript/issues/2000#issuecomment-209571753
The problem with override is that once you use the same keyword you have the same expectations:
abstract
. virtual
keyword to annotate methods that are meant to be overridden but have default behavior. I think the TS team doesn't want to add so much OO baggage to the language, and I think is a good idea.
By using implements.
you have a lightweight syntax for getting the main benefits: auto-complete and compile-time checked name, without inventing new keywords or incrementing the concept count.
Also has the benefit of working for classes and interfaces, and for methods (in the prototype) or direct fields.
Ways of making override
mandatory have already been discussed in the thread and the solutions aren't different to other features implemented by the compiler.
The virtual
keyword doesn't really make sense in the context of the language and neither is it named in a way that is intuitive for people who haven't used languages like C++ before. Perhaps a better solution to provide this type of guard, if needed, would be the final
keyword.
I agree language features should not be added hastily that could create 'baggage', but override
plugs a legitimate hole in the inheritance system that many other languages have deemed necessary. It's functionality is best achieved with a new keyword, certainly when alternative approaches are to suggest fundamental syntax changes.
What about setting inherited fields vs redefining new ones? React state
is a good example.
Or implementing optional interface methods?
(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
Example compile error conditions
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: