Support override keyword on class methods
rwyborn opened this issue · 223 comments
(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):voidIntelliSense
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:
- type override
- An auto complete drop down appears of all super methods for the class
- A method is selected in the drop down
- The signature for the method is emitted into the class declaration after the override keyword.
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:
- 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).
- 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.setASo 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 decorator syntax will be harder for IDEs to provide code completion (yeah, I guess it could be done but they would need prior knowledge)
- The derived class could change the visibility of a method - which in strict terms shouldn't be allowed
- Difficult for the decorator to check the argument list and types, and compatible return type
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
#2000 (comment)
@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
#2000 (comment)
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
#2000 (comment)
+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:
- It uses an already existing keyword.
- After writing
implements.the IDE has an excelent oportunity to show an autocompletion popup. - 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
}
}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
#2000 (comment)
The problem with override is that once you use the same keyword you have the same expectations:
- it should be mandatory, at least if the method is
abstract. - you miss then some
virtualkeyword 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?
override could potentially be used on fields, if they're being re-declared
in the subclass body. Optional interface methods aren't overrides so are
outside of the scope of what we're talking about here.
On Thu, 14 Apr 2016 at 11:58, Olmo notifications@github.com wrote:
What about setting inherited fields vs redefining new ones? React state
is a good example.Or implementing optional interface methods?
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2000 (comment)
Optional interface methods aren't overrides so are outside of the scope of what we're talking about here.
Optional interface methods are a concept fairly unique to TypeScript as far as I know, and I think a TypeScript implementation of "overrides" should apply to them to.
In the case of React lifecycle methods like componentDidMount, these are optional interface methods that have no implementation in the superclass.
In the case of React lifecycle methods like componentDidMount, these are optional interface methods that have no implementation in the superclass.
Exactly, so they don't override anything. I think we're getting confused about what the override keyword is intended to provide here, because if you're just looking for a way of getting better code hinting/intellisense there are other ways that can be achieved without adding new keywords to the language.
Guys, with all due respect, can we please stay focused. I think that discussing alternative keywords is counterproductive, especially since the Issue was started with a specific request.
Can we all either agree that we need override and ask nicely the Typescript team to add it or drop the request and let the Issue closed?
I think it's always useful to pose the request in the context of what the problem is, there's always different ways to solve the problem. The sticking point with override was around it being mandatory or not (some making the point that it isn't mandatory in C++). The problem a lot of people have referenced is getting the name right for overridable functions (which may or may not be implemented in the super class) - React's lifecycle functions being the main problem.
If override doesn't work, then maybe we should close this issue and open a more general point about this problem. The general problem here is that the interface contract with a superclass is untyped and unchecked, and that's tripping up developers and it would be great if TS or the tooling can help.
@armandn I don't think our lack of closure or the niceness of our suggestion is what's keeping the request rejected, but the differences between C# and TS semantics:
C# base method override:
- Requires
overridekeyword - Creates a new record on VTable
- Checks for mandatory abstract/virtual in base method
- Checks for identical signature
- IDE: Triggers auto-complete after writing override
C# interface implementation:
- No keyword necessary, but explicit interface implementation possible using the interface name.
- Create a new interface record in the VTable.
- Checks for identical signature
- IDE: QuickFix for implement interface / implement interface explicitly.
So the behavior in C# are quite different depending if you ask about classes or interfaces, but in any case you get the main three benefits:
- Checking for identical signature
- Checking that the method can/should be overridden (misspelling in method name)
- IDE support writing the function
With slightly different semantics, we already have 1) in TS, unfortunately 2) and 3) are missing. The reason override was rejected, in my opinion, is that a similar syntax assumes a similar behavior, and this not desirable because:
A class with 5 methods, of which 3 are marked override, wouldn't imply that the other 2 aren't overrides.
It happens that we already have 1) 2) and 3) when writing object literals, but not when writing class members that extend other classes or implement interfaces.
With this in mind I think we can all agree on this semantics:
- The keyword should be ambiguous enough to avoid the 'split world' problem. (i.e.:
checkorsuper., orMyInterface.) - No checks for
abstract/virtualbut checks for member existence in the base class / implemented interfaces. (Benefit 2) - Checks for similar-enough signature compatibility as TS is doing so far. (Benefit 1)
- IDE: Triggers a auto-complete after the keyword. (Benefit 3)
Additionally, I think this two are necessary for completeness of the solution*:
- It should work for Classes and Interfaces, because TS is mainly interface based and classes are a shortcut for prototypical inheritance. Necessary considering optional interface methods.
- It should work for Methods and Fields, because methods are just functions residing in a field.
This two points are useful in the very-real use case of React component implementation:
class MyComponent<MyComponentProps, MyComponentState> {
implements.state = {/*auto-completion for MyComponentState here*/};
implements.componentWillMount(){
//do my stuff
}
}The Annotation-based solution from @RyanCavanaugh is not enough because:
- It won't work with interfaces.
- It won't work with fields.
- Assuming a Roslyn-like infrastructure for helping with the tooling, there is no way to add a QuickFix after writing auto-complete list after writing
@override.
Given the semantics, it's just about choosing the right syntax. Here some alternatives:
override componentWillMount(): Intuitive but misleading.check componentWillMount(): Explicit but keyword-consuming.super componentWillMount():implements componentWillMount():super.componentWillMount():implements.componentWillMount():this.componentWillMount():ReactComponent.componentWillMount():
Opinions?
@olmobrutall good summary. Couple of points:
One keyword option (taken from C#) would be new - indicating a new slot:
class MyComp extends React.Component<IProps,IState> {
...
new componentWillMount() { ... }
componentWillMount() { ...} // would compile, maybe unless strict mode is enabled
new componentwillmount() { ... } <-- errorMy other point is the issue with mandatory nature of using the above. As a contract between the parent super class and the derived class it makes sense to specify what are interface points where the above syntax would be valid. These are really internal extensions points for the super class, so something like:
class Component<P,S> {
extendable componentWillMount() {...}
}The same would apply to the interface too.
Thanks :)
What about just writing this?
class MyComponent<MyComponentProps, MyComponentState> {
this.state = {/*auto-completion for MyComponentState here*/};
this.componentWillMount(){
//do my stuff
}
}About extendable, there's already abstract in TS 1.6, and adding virtual will maybe create again the split world problem?
Right, I thought about abstract but there may be an implementation in the super class, so it doesn't really make sense. Likewise with virtual, as that would imply non-virtual members are not virtual - which is misleading.
this. works, I guess you could even have (as a long form):
this.componentWillMount = () => { }The only issue with this is that it should only be scoped to earmarked extension points, not all members of the base class.
what is going on...
TypeScript isn't and never has been the javascript version of C#. So the reason isn't because the suggested functionality differs from the semantics of C#. The reasons for closure as stated by Ryan and then clarified by Daniel R were
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).
Yet you're still persisting with your problems around autocomplete. Autocomplete doesn't need a new keyword in the language to provide you with improved suggestions.
The reason this thread exists was to get compiler errors when a function is illegally overridden or when a method was declared to be an override but didn't actually override a function in a base class. It's a simple and well defined concept across many languages that also support most if not more language features typescript has to offer. It doesn't need to solve all the worlds problems, it just needs to be the override keyword, for overrides.
More people have since shown interest in the original proposal, so lets stick to the topic the issue was raised for and raise new issues for new ideas.
TypeScript isn't and never has been the javascript version of C#.
I'm comparing it with C# as a reference language, since I assume this is the behavior you guys are assuming.
Autocomplete doesn't need a new keyword in the language to provide you with improved suggestions.
How you suggest it to be triggered then? If you show a autocomplete combobox when writing a random name in a class context will be very annoying when we just want to declare a new field or method.
The reason this thread exists was to get compiler errors when a function is illegally overridden or when a method was declared to be an override but didn't actually override a function in a base class.
I absolutely included this use case, under Benefit 2.
It doesn't need to solve all the worlds problems, it just needs to be the override keyword, for overrides.
So your suggestion is to fix one step at a time instead of moving one step back and look at similar/related problems?. That maybe a good idea for your Scrum Board but not for designing languages.
The reason they are conservative about adding the keyword is that they can not remove features from a language.
Some design mistakes in C# because of lack of completion:
- Unsafe Array co-variance / contra-variance.
varonly working for variable types, not for automatic generic parameters or return types. Maybleautowill be a better keyword like in C++?
Just try to use React for a second and you'll see the other side of the picture.
Overriding a method that's already been implemented in a base class and implementing an interface method are two completely different things. So yes, what's being suggested is to fix one of those scenarios with a dedicated keyword instead of trying to come up with some Swiss-Army keyword.
What good is a keyword that can mean any one of 3 different things to developers reading some code for the first time? It's ambiguous and confusing, especially if you're talking about using a keyword like this which already does other (totally unrelated!) things in the language - it couldn't be more generic, it's almost useless.
If your main concern is auto-complete, editors have enough information now to be able to suggest methods from base classes and implemented interfaces 'as you type'.
Overriding a method that's already been implemented in a base class and implementing an interface method are two completely different things.
In the general case yes, but we're not talking about implementing any interface method. We're talking about an optional interface method where the parent class implements the interface. In this case you can say that 1) interface permits the method to be implemented as undefined, 2) the parent class has an undefined implementation, and 3) the child class is overriding the undefined implementation with a method implementation.
@olmobrutall I think your comment about designing languages and how it is not a scrum board is a little self-serving. I have seen about four updates to TS in the space of under a year.
If the language design was as well considered as you imply it should be, then there would already be a language spec document telling us exactly how overrides should work, and we mightn't even be having this conversation.
I don't make this comment with any denigration of the TS developers/designers, because TS is already excellent and I would dread having to use standard JS instead.
Yes TS is not C# and it's not C++. But a number of languages have chosen the override keyword to meet the objectives discussed here so it seems counterproductive to suggest totally alien syntax.
The chief issue seems to be not wanting to introduce a breaking change. The simple answer is a compiler flag, end of story. For some people like me, an optional override keyword is useless. For others they want to embellish their code incrementally. Compiler flag solves the dilemma.
Signature differences are a different conversation. The new keyword appears unnecessary because JS can't support multiple methods of the same name (unless TS creates signature-derived mangled names a'la C++, which is highly unlikely).
I have seen about four updates to TS in the space of under a year.
I don't mean you can not be fast and iterate quickly. I'm as happy as anybody that ES6 and TS are evolving fast. What I mean is that you have to try to predict the future, to avoid putting the language in a dead end.
I could agree on using override keyword. With proper arguments even keeping fields and interfaces out of the scope, but I can not agree with the 'let's stay focused and solve just this particular problem the way other languages do it without thinking too much' argument.
But a number of languages have chosen the override keyword to meet the objectives discussed here so it seems counterproductive to suggest totally alien syntax.
None of this languages have prototypal inheritance or optional method (methods that are neither abstract or virtual, they just could not exist at runtime), and this are related problems that have to be discussed (and maybe discarded) before making a commitment.
Put another way: let's say we do as you seem to suggest and we implement override without thinking to much. Then me, or anybody else using TSX, adds an issue with why override doesn't work with React components. Whats your plan?
In the general case yes, but we're not talking about implementing any interface method. We're talking about an optional interface method where the parent class implements the interface.
It doesn't matter where the interface was set up, the fact is they are not the same thing and so shouldn't share a keyword because the intent of the program is not clear.
You could, for example, be overriding a method which had been implemented for interface compliance in a base class; If we were to put all our eggs in one keyword for these two different things, how would anyone then know if this was the initial declaration of that function or an override to one previously defined in a base class? You wouldn't, and it wouldn't be possible to know without further inspection of the base class - which may even be in a 3rd party .d.ts file & thus making it an absolute nightmare to find, depending how deep in the inheritance chain the function was originally implemented.
Put another way: let's say we do as you seem to suggest and we implement override without thinking to much. Then me, or anybody else using TSX, adds an issue with why override doesn't work with React components. Whats your plan?
Why does this need to fix React? If React has a different problem to what this is trying to solve then I can't for the life of me fathom why override needs to fix it? Have you tried opening another issue to suggest something be done about interface implementation?
I wouldn't agree that enough thought hasn't been put in to this. We're suggesting a tried and tested technique that's been successful in every other language I can think of that's implemented it.
the fact is they are not the same thing and so shouldn't share a keyword because the intent of the program is not clear.
Are not? Look at this two alternative definitions of BaseClass
class BaseClass {
abstract myMethod();
}interface ISomeInterface {
myMethod?();
}
class BaseClass extends ISomeInterface {
}And then in your code you do:
class ConcreteClass {
override myMethod() {
// Do stuff
}
}You think it should work in just one case and not in the other? The effect is going to be 100% identical in Javascript (creating a new method in ConcreteClass prototype), from the external interface and from the tooling perspective.
Even more, maybe you want to capture this inside of the method, implementing it with a lambda (useful for React event handling). In this case you'll write something like this:
class ConcreteClass {
override myMethod = () => {
// Do stuff
}
}The behavior will be again identical if the method is abstract or comes from the interface: add a field in the class with a lambda implementing it. But it looks a little bit strange to override a field, since you're just assigning a value.
Not let's see it using super. (my favorite syntax right now, but I'm open to alternatives).
class ConcreteClass {
super.myMethod() { //method in prototype
// Do stuff
}
super.myMethod = () => { //method in lambda
// Do stuff
}
}Now the concept is conceptually simpler: My super class says that there is a method or field and the ConcreteClass can define it in the prototype / assign it / read it / call it.
Why does this need to fix React?
Is not just react, look at angular:
Of course most of the interfaces are not meant to be implemented, neither all the classes to be overriden, but one thing is clear: in Typescript interfaces are more important than classes.
Have you tried opening another issue to suggest something be done about interface implementation?
How should I call it? override for interface and fields?
We're suggesting a tried and tested technique that's been successful in every other language I can think of.
The languages you have in mind are quite different. They have inheritance based in a static VTable.
In Typescript a class is just an interface + automatic prototype inheritance of methods. And methods are just fields with a function inside.
In order to adapt the override feature to TS this two fundamental differences have to be considered.
Please @kungfusheep make the effort to think about a solution to my problem. If you want to add different keywords and implement them in a second stage is O.K., but take a second to imagine how it should be.
I can't think of another way to say what I've already said. They are not the same. They are similar, but not the same. Please see this comment by one of the TS devs RE: the readonly keyword - #6532 (comment) - which strengthens what I'm saying.
I agree with the general idea, but let's see in practice:
class MyComponent extends React.Component<{ prop : number }, { value: string; }> {
//assign a field defined in the base class without re-defining it (you want type-checking)
assign state = { value : number};
//optional method defined in an interface implemented by the base class
implement componentDidMount(){
}
//abstract method defined in the base class
override render(){
}
}This looks like VB or Cobol, doesn't it?
It looks like it makes sense, at least.
Consider this example, were there only the override (or just one) keyword.
interface IDo {
do?() : void;
}
class Component implements IDo {
protected commitState() : void {
/// do something
}
override public do() : void {
/// base implements 'do' in this case
}
}
Now lets implement our own component using what we just wrote.
class MyComponent extends Component {
override protected commitState(){
/// do our own thing here
super.commitState();
}
override do() : void {
/// this is ambiguous. Am I implementing this from an interface or overriding a base method? I have no way of knowing.
}
}
One way of knowing would be the type of super:
override do() : void {
super.do(); // this compiles, if it was an interface then super wouldn't support `do`
}exactly! which means the design is wrong. there shouldn't be an investigation involved with skimming code, it should just make sense when you read it.
this is ambiguous. Am I implementing this from an interface or overriding a base method? I have no way of knowing.
What's the difference in practice? The object has just one space of names and you can only place one lambda in the do field. There's no way of explicitly implementing an interface anyway. The implementation is going to be:
MyComponent.prototype.do = function (){
//your stuff
}independently of what you write.
It doesn't matter what the output is. You could be either intentionally or unintentionally overriding some functionality in a base class, but there is no intent in a keyword that is ambiguous.
What error or unexpected behavior will be solved by having two keywords?
Come on now mate. You're obviously a clever guy. I've just said "unintentionally overriding some functionality in a base class"; can we not infer from this any unexpected behaviour that could have potentially just occurred?
To be clear, I'm not proposing turning this issue into a proposal for two keywords. This issue is for the override keyword - anything else will need a new proposal and its own discussion on semantics.
To be clear, I'm not proposing turning this issue into a proposal for two keywords. This issue is for the override keyword - anything else will need a new proposal and its own discussion on semantics.
It doesn't really matter in how many issues it should be discussed, or from whom the idea comes. You propose to split two very related thinks and don't even consider a consistent syntax.
The arguments for whether we need 1, 2 or 3 keywords belong to this thread and is not finished (...but becoming repetitive). Then maybe we can discuss the syntax in another thread (because the semantics are going to be identical anyway :P).
In my example:
class MyComponent extends React.Component<{ prop : number }, { value: string; }> {
//assign a field defined in the base class without re-defining it (you want type-checking)
assign state = { value : number};
//optional method defined in an interface implemented by the base class
implement componentDidMount(){
}
//abstract method defined in the base class
override render(){
}
}Don't assign, implement and override do exactly the same thing: Check that the name exist (in the base class, the implemented interfaces, the interfaces implemented by the base class, etc...).
If there's a name conflict between base classes and some implemented interfaces you'll get an compile-time error whether with 1, 2 or no keyword at all.
Also think about the object literal:
var mc = new MyComponent();
mc.state = null;
mc.componentDidMount =null;
mc.render = null;With exactly the same syntax I can re-assign fields or method independently if they come from the base class, direct interface implementations or interfaces implemented in the base class.
Don't assign, implement and override do exactly the same thing: Check that the name exist (in the base class, the implemented interfaces, the interfaces implemented by the base class, etc...).
You've just described 3 different scenarios there, so obviously they're not the same. I have a feeling I could describe why they're different to you all day and you'd still be sat arguing that they're not, so I'm going to bow out of this particular line of discussion for now. There's nothing to say the TS guys are still considering this at the moment anyway.
With the closure of #6118 I think there's reason to discuss if the problems there and the problems here can be addressed simultaneously.
I was not aware of #6118. The idea looks like a possible alternative to adding override.
If I understood the problem properly, the issue is that you can have more than one base classes / interfaces with compatible but not identical member declarations, and they have to be unified when being initialized in the child class without a type.
Without having a good idea of the consequences, I would be happy if:
- the member will produce a compile-time error that requires explicit type declaration on the child class
- the member will take the intersection of all the possible types.
More important IMO would be to have some way of triggering auto-completion when writing class members (Ctrl+Space). When the cursor is directly inside of a class, you could be defining new members of re-defining inherited ones, so the auto-completion can not be too aggressive, but with manual triggering the behavior should be OK.
Regarding @RyanCavanaugh comments:
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.
Typing a variable as any doesn't imply that some other variable is or isn't also any. But, there is a compiler flat --no-implicit-any to enforce that we explicitly declare it. We could equally have a --no-implicit-override, which I for one would turn on if it were available.
Having an override keyword that is used gives developers an enormous amount of insight when reading code that they are unfamiliar with, and the ability to enforce it would give some additional compile time control.
For all the +1ers -- can you talk about what's not sufficient about the decorator solution shown above?
Is there any way in which a decorator a better solution than an override keyword? There are many reasons why it is worse: 1) It adds runtime overhead however small; 2) It's not a compile time check; 3) I have to include this code to every one of my libraries; 4) There is not way for this to catch functions that are missing the override keyword.
Let's give an example. I have a library with three classes ChildA, ChildB, Base. I've added some method doSomething() to ChildA and ChildB. After some refactoring, I've added some additional logic, optimized, and moved doSomething() to the Base class. Meanwhile, I have another project which depends on my library. There I have ChildC with doSomething(). There is no way when I update my dependencies to find out that ChildC is now implicitly overriding doSomething(), but in an unoptimized way that is also missing some checks. That is why an @overrides decorator will never be sufficient.
What we need is an override keyword, and a --no-implicit-override compiler flag.
An override keyword would help me a lot since I use in my project a simple hierarchy of base classes to create all my components. My problem lies in the fact that these components may need to declare a method to be used somewhere else, and this method may or may not be defined in a parent class and may or may not already do stuff that I need.
For example, let's say that a function validate takes a class with a getValue() method as parameter. To build this class, I can inherit another class that could already define this getValue() method, but I can't really know this unless I look at its source code. What I'd instinctively do is implement this method in my own class, and as long as the signature is correct no one will tell me anything.
But maybe I shouldn't have done that. The following possibilities all suppose that I did an implicit override:
- The base class already defined this method just like I did, so I wrote a function for nothing. Not that much of a problem.
- I incorrectly rewrote the function, most of the time because I forgot to do some non-obvious stuff that was already handled by the base class. What I really needed to do here is call
superin my override, but no one suggested me to do so.
Having a mandatory override keyword would have told me "hey, you're overriding, so maybe you should check what the original method looks like before doing your stuff". That would greatly improve my experience with inheritance.
Of course, as suggested, it should be placed under a --no-implicit-override flag since it would be a breaking change and that most people do not care that much about all this.
I like the comparaison @eggers made with any and --no-implicit-any because it's the same kind of annotation and it would work in the exact same way.
@olmobrutall I was looking at some of your talk about overriding abstract methods & interface methods.
If my opinion, override implies the existence of a super. Neither abstract methods nor methods defined in an interface can be called via a super. call, and so should not be overrideable (there is nothing to override). Instead if we do something more explicit for those cases, it should be an implements keyword. But, that would be a separate feature discussion.
Here are the problems as I see them, ranked most important to least. Did I miss anything?
Failure to override
It's easy to think you're overriding a base class method when you're not:
class Base {
hasFilename(f: string) { return true; }
}
class Derived extends Base {
// oops
hasFileName(f: string) { return false; }
}This is probably the biggest problem.
Failure to implement
This is very closely related, especially when the implemented interface has optional properties:
interface NeatMethods {
hasFilename?(f: string): boolean;
}
class Mine implements NeatMethods {
// oops
hasFileName(f: string) { return false; }
}This is a less important problem than failure to override, but it's still bad.
Accidental override
It's possible to override a base class method without realizing it
class Base {
hasFilename(f: string) { return true; }
}
class Derived extends Base {
// I didn't know there was a base method with this name, so oops?
hasFilename(f: string) { return true; }
}This should be relatively rare just because the space of possible method names is very large, and the odds that you'd also write a compatible signature and not mean to have been doing this in the first place are low.
Accidental anys
It's easy to think that override methods will get the parameter types of their base:
class Base {
hasFilename(f: string) { return true; }
}
class Derived extends Base {
// oops
hasFilename(f) { return f.lentgh > 0; }
}We tried to type these parameters automatically but ran into problems. If hasFilename were explicitly marked override we might be able to solve those problems more easily.
Readability
It's not immediately clear when a method is overriding a base class method and when it's not. This seems like a smaller problem because there are reasonable workarounds available today (comments, decorators).
Editor experience
You have to manually type out base method names when writing derived class override methods, which is annoying. We could easily fix this by always providing those names in the completion list (I think we actually have a bug on this already), so we don't really need a language feature to fix this problem.
Non-problems
Listing these out as things that either belong in a separate issue or simply aren't going to happen:
- Exact signature matching -- this isn't something that should be mixed in with
override, and it really does have undesirable semantics in many good scenarios - Insufficiently exotic syntax (e.g.
implements.foo = ...). No need to bikeshed here
@RyanCavanaugh I agree with all of the above in about that order as well. Just as a comment, I don't think that the override keyword should be solving the "failure to implement" problem. As I said above, I think that problem should be solved by a different keyword (e.g. implements) as part of a different ticket. (override should imply a super. method)
@RyanCavanaugh I agree with all the points but I don't see any reference to the also very related problem of initialized fields declared in a parent type without overriding the type (and then loosing type checking). I think the feature should not be confined to method declarations in a language where methods are just functions in object fields.
@eggers even if at the end the end two keywords are needed, the semantics will be very similar and I think the features should be discussed together.
override should imply a super. method
In C# override can (and must) be used also for abstract methods (with no .super). In Java @OverRide is an attribute. Of course they are different language but with similar keywords you expect similar behaviors.
I agree with @RyanCavanaugh 's points, though I'd say the "accidental override" problem might be a little more common than he believes (especially when dealing with extending a class that already extends something else, like a React component that would already define a componentWillMount method in the first extension).
I believe though, like @eggers said, that the "failure to implement" issue is something pretty different, and it would feel weird to use an override keyword for a method that doesn't exist in a parent class. I know these are different languages with different problematics, but in C# override isn't used for interfaces. I would suggest, if we could get a --no-implicit-override flag, that interface methods would have to be prefixed with the interface name (which would look a lot like C# and thus feel more natural).
Something like
interface IBase {
method1?(): void
}
class Base {
method2() { return true; }
}
class Test extends Base implements IBase {
IBase.method1() { }
override method2() { return true; }
}I think @RyanCavanaugh lists the fundamental problems. I would complement that with "What are the disagreements":
- Is declaring a method to match an interface considered an
override? For example:
interface IDrawable
{
draw?( centerPoint: Point ): void;
}
class Square implements IDrawable
{
draw( centerPoint: Point ): void; // is this an override?
}
- is declaring a property to match an interface considered an
override?
interface IPoint2
{
x?: number;
y?: number;
}
class Circle implements IPoint2
{
x: number; // is this an override?
y: number; // is this an override?
radius: number;
}
- Is there a need for some kind of
implementskeyword, to handle the above cases instead of anoverridekeyword, and the form it would take.
@kevmeister68 thanks for explicitly stating the disagreements.
One thing: you have to make the interface members optional to really show the problem, like this the typescript compiler will complain when a field or method is not implemented, solving "Failure to implement".
@olmobrutall Thanks for that. I've never declared a method optional - can it be (I come from a C# background and there is no such concept)? I've updated the examples I listed above to change the member variables to optional - is that better?
@kevmeister68 also the draw method in IDrawable :)
Insufficiently exotic syntax (e.g. implements.foo = ...). No need to bikeshed here
Thanks for teaching me a new expression. I don't see a lot of problems in the semantics of the feature:
- We all want compile-time errors if the method does not exist
- We all want compile-time errors the type does not match.
- We also want that the parameters are implicitly typed to the parent one, like lambda expressions do.
- We want some IDE help to write the method properly.
Most of the problems rely on the syntax and the behaviours that they impliy:
- Optional members in Interface vs members in Base classes
- Methods vs fields (don't know where lambdas will be)
Let's compare the tree more promising syntaxes:
override / implements
class Person{
dateOfBirth: Date;
abstract talk();
walk(){ //...}
}
interface ICanFly{
fly?();
altitude?: number;
}
class SuperMan extends Person implements ICanFly {
dateOfBirth = new Date(); //what goes here?
override talk(){/*...*/}
walk = () => {/* force 'this' to be captured*/} //what goes here
implements fly() {/*...*/}
altitude = 1000; //what goes here?
}Advantages:
- It's natural for programmers with OO background.
- Feels right when comparing with
extends/implements.
Disadvantages:
- Doesn't provide a solution to the field problem, none
overrideorimplementsfeels right. Maybesuper., but then what we do with interfaces? - If the methods are overridden using a lambda (useful to capture this) of a
function()the same problems happens.
override / InterfaceName.
class Person{
dateOfBirth: Date;
abstract talk();
walk(){ //...}
}
interface ICanFly{
fly?();
altitude?: number;
}
class SuperMan extends Person implements ICanFly {
dateOfBirth = new Date(); //what goes here?
override talk(){/*...*/}
walk = () => {/* force 'this' to be captured*/} //what goes here
ICanFly.fly() {/*...*/}
ICanFly.altitude = 1000; //what goes here?
}Advantages:
- Is also natural for programmers with OO background.
- Solves the field problem for interfaces, but not for classes, but we could use
super.for them.
Disadvantages:
- Interface names can be long, specially with generics, and showing the auto-competition list will also be problematic, requiring some Alt+Space to make it explicit.
- Makes it looks like you can implement two members with the same name from different interfaces independently. This works in C# but won't work in Javascript.
this.
class Person{
dateOfBirth: Date;
abstract talk();
walk(){ //...}
}
interface ICanFly{
fly?();
altitude?: number;
}
class SuperMan extends Person implements ICanFly {
this.dateOfBirth = new Date();
this.talk(){/*...*/}
this.walk = () => {/* force 'this' to be captured*/}
this.fly() {/*...*/}
this.altitude = 1000;
}Advantages:
- Solves classes, interfaces, methods and fields.
- Uses (abuses?) just one pre-existing keyword.
- Using
this.to trigger auto-completion feels natural. - Makes clear that the object is just one namespace, and you can only have one thing for each name.
Disadvantages:
- Looks like an expression, not a declaration, making it hard to parse for non-trained eyes.
Whilst failure to implement a non-optional interface method will cause a compiler error, if the interface changes at some point in the future (lets say a method is removed) there will be no warning for all classes that implemented that method. This might not be as big an issue as optional's but I think it's fair to say the scope of this goes beyond them.
I do however, still believe override is a distinctly different thing and potentially having two keywords rather than one would better convey the intent of the program.
For example, consider a scenario where a developer believes they are implementing an interface method when in-fact they're overriding the method which has already been declared in a base class. With two keywords the compiler can prevent this kind of mistake and throw an error to the tone of method already implemented in a base class.
interface IDelegate {
execute?() : void;
}
class Base implements IDelegate {
implement public execute() : void { /// fine, this is correctly implementing execute
}
}
class Derived extends Base {
implement public execute() : void {
/// ERROR: `method "execute():void" already implemented in a base class`
}
}I don't know if this is relevant in JS, but class fields are supposed to be private in OO, so I don't think we need to be overriding fields explicitly since the fact that they're private already prevent us to override altogether.
Instance methods are fields though, and from what I gather a lot of people use them instead of prototype methods. About that,
class Person{
walk(){ //...}
}
class SuperMan extends Person {
walk = () => {/* force 'this' to be captured*/}
}is a compiler error, because walk is a prototype method in Person and an instance method in SuperMan.
Anyway, i'm not sure override would be a fit here, because well, we don't override fields. Again, it would look like C#, but I'd rather use the new keyword instead of override here. Because it's not the same thing as a method override (I can call super.myMethod in an override, not here).
My preferred solution would then be something like (assuming we're in strict override mode):
class Person{
dateOfBirth: Date;
talk() { }
walk = () => { }
}
interface ICanFly {
fly?();
altitude?: number;
}
class SuperMan extends Person implements ICanFly {
new dateOfBirth = new Date();
override talk() { }
new walk = () => { }
implements fly() {/*...*/}
implements altitude = 1000;
}My main concern is about interfaces though. We shouldn't have to write implements for non optional interface members, because we'll be already caught by the compiler. Then, if we do so, there'll be a confusion between what is from an interface and what isn't, since not all interfaces members would be prefixed by implements. And that word is a mouthful. I'm not ready to write something like this:
class C extends React.Component {
implements componentWillMount() { }
implements componentDidMount() { }
implements componentWillReceiveProps(props) { }
/// ... and the list goes on
}I proposed to prefix with the interface name earlier but @olmobrutall showed me that it was a worse idea.
Anyway, I'm pretty convinced that we'll need 3 different new keywords to properly tackle this.
I also don't think that it's necessary to implicitly type the overrides since the compiler will already prevent us to write something that's not compatible, especially because it is apparently difficult to do right.
Isn't new, override and implement too much keyword overhead for essentially the same JS semantics? Even if in C# they are different things there is virtual no difference in JS/TS.
It's also quite ambiguous:
- Why fields are
newfor classes butimplementsfor interfaces? - If you're changing a method from a base class that was implemented with an interface what should you use? I can see four possibilities:
override,implements, any of the two or both at the same time?
Also think about this:
class Animal {
}
class Human extends Animal {
}
class Habitat {
owner: Animal;
}
class House extends Habitat {
owner = new Human();
}
var house = new House();
house.owner = new Dog(); //Should this be allowed?? The question is:
Does owner = new Human(); re-define the field with a new (but compatible) type, or just assigns a value.
I think in general you re-defined the field except if you using the magic keyword. My preference is for this. right now.
class House extends Habitat {
this.owner = new Human(); //just setting a value, the type is still Animal
}@olmobrutall That might indeed be a bit much keyword overhead, but all 3 are indeed different things.
overridewould apply to methods (defined on the prototype), meaning that it does not erase the original method, which is still accessible behindsuper.newwould apply to fields, and mean that the original field has been erased by the new one.implementswould apply to anything from an interface, and mean that it does not erase or replace anything that already existed in a parent class, because an interface "doesn't exist".
If you're changing a method from a base class that was implemented with an interface what should you use? I can see four possibilities: override , implements , any of the two or both at the same time?
It seems pretty logical to me that it would be override since that what you're effectively doing here. implements would mean that you're implementing something from the interface that doesn't exist otherwise. In a sense, override and new would have priority over implements.
And about your field override exemple, nothing should change about how it works today. I'm not sure what happens here, but whatever it is it shouldn't change (or maybe it should, but that's not what we're discussing here).
I feel that override would be suitable for both base methods and properties, including abstract ones (explained below).
new is an interesting idea in concept, but this particular keyword may be confused with class instantiation, so it may give the false impression it would only work for classes, despite the fact that properties can have primitive, interface, union or even function types. Perhaps a different keyword like reassign could work better there, but it has the problem that it may give a false idea in the case the value is only re-declared but not actually assigned with anything in the derived class (new may have that problem as well). I think redefine is also interesting, but could lead to the mistaken expectation that the 'redefined' property could also have a different type than the base one, so I'm not sure.. (edit: actually I checked and it could have a different type as long as the new one is a subtype of the base one, so this may not be that bad).
implement (I prefer this particular verb form for consistency with override) seems to work for interfaces. I believe it could also technically work for abstract base methods as well, but using override could feel more consistent, despite the slightly different semantics. Another reason is that it would make it a bit inconvenient to change a method from abstract to non-abstract, as one would need and go to all the derived classes and change override to implement.
There could be better ideas, but that's all I have at this point..
I proposed the keyword new because it's the one C# uses for this exact feature, but I agree this isn't the best one. implement is indeed a better choice over implements. I don't think we should use it for abstract methods since they're part of a base class and not an interface. override + new -> base class, implement -> interface. That seems clearer.
About overriding prototype with instance field
I've checked and you're right:
class Person{
walk(){ //...}
}
class SuperMan extends Person {
walk = () => {/* force 'this' to be captured*/}
}Fails with Compiler error: Class 'Person' defines instance member function 'walk', but extended class 'SuperMan' defines it as instance member property.
I don't really see the reason to fail in this case, since the parent type contract is fulfilled and you can write this anyway:
var p = new SuperMan();
p.walk = () => { };Or even
class SuperMan extends Person {
constructor() {
super();
this.walk = () => { };
}
}About override
override would apply to methods (defined on the prototype), meaning that it does not erase the original method, which is still accessible behind super.
That's actually an argument. There is least some observable difference between override and implements... but not quite.
Both override and implements are implemented in the prototype, being able to use super is independent, because you call super.walk() not just super(), and is available in every method ( overrides, implements, news and normal definitions).
class SuperMan extends Person implements ICanFly {
new dateOfBirth = new Date();
override talk() { } //goes to prototype
new walk = () => { }
implements fly() {/*...*/} //also goes to the prototype
implements altitude = 1000;
}And being able to use super.walk() also works if you assign to the instance
class SuperMan extends Person {
constructor() {
super();
this.walk = () => { super.walk(); };
}
//or with the this. syntax
this.walk = () => { super.walk(); };
}There's already a clear way to differentiate prototype fields from instance fields and is the = token.
class SuperMan extends Person implements ICanFly {
this.dateOfBirth = new Date(); //instance
this.talk() { } //prototype
this.walk = () => { } //instance
this.fly() {/*...*/} //prototype
this.altitude = 1000; //instance
}With the this. syntax the problem is solved more elegantly:
this.means check my type (base classes and implemented interfaces).=means assign to the instance instead of the prototype.
About New
newwould apply to fields, and mean that the original field has been erased by the new one.
Take into account that you can not change the field or method with a different type because you will break the type system. In C# or Java when you do new the new method will be used only on statically dispatched calls to the new type. In Javascript all the calls will be dynamic and if you change the type the code will likely break at run-time (Coercing the type could be allowed for practical purposes however, but not widening).
class Person {
walk() { }
run() {
this.walk();
this.walk();
this.walk();
}
}
class SuperMan extends Person {
new walk(destination: string) { } //Even if you write `new` this code will break
}So more than new the keyword should be assign, because this is the only type-safe thing you can do:
class Person{
dateOfBirth: Date;
}
class SuperMan extends Person implements ICanFly {
assign dateOfBirth = new Date();
}Note that reassign doesn't make sense, because Person only declares the field but doesn't set any value on it.
Writing assign seems redundant however, it's clear that we're assigning because we have a = at the other side.
Again the this. syntax solves the problem gracefully:
class Person{
dateOfBirth: Date;
}
class SuperMan extends Person implements ICanFly {
this.dateOfBirth = new Date();
}