Add override keyword #13217

pcj · 2016-12-29T17:41:05Z

Fixes #2000

This initial commit simply introduces the keyword and performs some initial modifier flags checks. The actual checks against the inheritance chain are not implemented yet.

@RyanCavanaugh or other: Can you suggest when/where the appropriate time to perform these checks are?

No formal review required at this time (obviously), but would appreciate any strategy recommendations or other suggestions.

RyanCavanaugh · 2016-12-29T17:51:38Z

src/compiler/commandLineParser.ts

@@ -145,6 +145,11 @@ namespace ts {
            description: Diagnostics.Raise_error_on_expressions_and_declarations_with_an_implied_any_type,
        },
        {
+            name: "noImplicitOverride",
+            type: "boolean",
+            description: Diagnostics.Member_function_0_was_marked_override_but_no_matching_definition_was_found_in_the_superclass_chain_1,


Description here is what you would see on the commandline under -?, i.e. this should be a string like "Raise an error when overriding class methods are not marked with 'overide'"

RyanCavanaugh · 2016-12-29T17:54:50Z

src/compiler/checker.ts

+        }
+
+        function checkOverrideMethodDeclaration(node: MethodDeclaration) {
+          forEachEnclosingClass(node, enclosingClass => {


You'll want to only checking the containing class. The function forEachEnclosingClass is for dealing with nesting of classes, not inheritance chains

RyanCavanaugh · 2016-12-29T17:57:13Z

src/compiler/checker.ts

@@ -15712,6 +15712,21 @@ namespace ts {
            if (getModifierFlags(node) & ModifierFlags.Abstract && node.body) {
                error(node, Diagnostics.Method_0_cannot_have_an_implementation_because_it_is_marked_abstract, declarationNameToString(node.name));
            }
+
+            // Is this the correct time to make assertions against the inheritance chain?


Around line 18164 (look for checkKindsOfPropertyMemberOverrides(type, baseType);) / 18261 might be better; here the base type members are already in scope and we are iterating through the class properties.

mhegazy · 2016-12-29T17:55:22Z

src/compiler/types.ts

@@ -155,6 +155,7 @@ namespace ts {
        PrivateKeyword,
        ProtectedKeyword,
        PublicKeyword,
+        OverrideKeyword,


override is not an ES reserved word. move it to the contextual keyword section.

mhegazy · 2016-12-29T18:03:42Z

src/compiler/checker.ts

@@ -20964,6 +20985,25 @@ namespace ts {
                        flags |= ModifierFlags.Async;
                        lastAsync = modifier;
                        break;
+
+                case SyntaxKind.OverrideKeyword:


you also need to add a check in checkFlagAgreementBetweenOverloads to make sure all overloads are marked as override.

Also add a check in checkAccessorDeclaration for accessors matching in override

Also a check if declarations are consecutive, see checkFunctionOrConstructorSymbol

Done for (1), (2). As for (3), I still don't really understand what consecutive declaration checks mean. Please propose a test class that illustrates it if possible.

mhegazy · 2016-12-29T18:04:58Z

src/compiler/checker.ts

+                  else if (flags & ModifierFlags.Abstract) {
+                    return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "abstract", "override");
+                  }
+                  else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {


this should follow the abstract modifier checks. override is only applicable on a method declaration in a class or a class expression.

mhegazy · 2016-12-29T18:06:31Z

src/compiler/checker.ts

+
+        function checkOverrideMethodDeclaration(node: MethodDeclaration) {
+          forEachEnclosingClass(node, enclosingClass => {
+            // TODO: save the methodDeclaration node here in a cache and


also check that an override method can not be marked as optional.

mhegazy · 2016-12-29T18:17:35Z

src/compiler/checker.ts

+            // Is this the correct time to make assertions against the inheritance chain?
+            // Have all other methods been resolved?  Probably need to record that an override exists
+            // and perform the actual resolution later.
+            if (getModifierFlags(node) & ModifierFlags.Override) {


i would move this to checkKindsOfPropertyMemberOverrides

mhegazy · 2016-12-29T18:19:16Z

src/compiler/emitter.ts

@@ -324,6 +324,7 @@ namespace ts {
                case SyntaxKind.PrivateKeyword:
                case SyntaxKind.ProtectedKeyword:
                case SyntaxKind.PublicKeyword:
+                case SyntaxKind.OverrideKeyword:


you also need to handle this in the declarationEmitter.ts, and please add declaration emit tests.

This has not been done yet. I was not able to identify an appropriate declaration emitter test to mimic. Can you suggest one to investigate?

pcj · 2016-12-29T19:00:05Z

Thank you for all the excellent feedback, will begin addressing your comments and submit a new iteration over the next day or so.

pcj · 2017-01-04T08:12:12Z

See comments above. In addition, consider the following issues:

Declaration emit test still need to be implemented. Is there a
similar test case for study here? Please advise.
Regarding handling of an @override jsdoc tag. Where should I
look for the presence of this jsDoc tag and merge it into
ModifierFlags.Override? Alternatively, should a separate
ModifierFlags.JSDocOverride flag exist and perform dual checks for
these (perhaps as a utility function)?
Regarding the semantics of static methods: I have assumed that override
should not be used with static. Is this true?
Regarding the semantics of an abstract method: should it be
permissible to apply override to a repeated abstract method in an
derived abstract class? For example:

abstract class Base {
    abstract getMeaningOfLife(): number;
}

abstract class Derived extends Base {
    // Here, override keyword is not adding any utility (compiler
    // already reports inappropriate usage with abstract keyword alone),
    // but it may be semantically correct.  I see pro/con of each but defer to
    // language designers for decision.  My bias is to not use override in 
    // conjunction with non-concrete members.  

    // Current implementation rejects
    // simultaneous use of both override and abstract.
    override abstract getMeaningOfLife(): number;
}

As an extension to this line of thought, the --noImplicitOverride flag does not complain about the absence of override on interfaces. AFAICT, it does not add anything in terms of correctness and adds unnecessary verbosity to the language. Only when a derived member exists in a concrete class will those assertions kick in.

Regarding implicit extension of Object. I had naively imagined that all
classes implicitly extend from Object (and that
getBaseTypes(classType) would include Object in the result
list). For example, all instantiated typescript objects know that
toLocaleString() is a valid method that returns a string. This
is defined in lib.d.ts (as you know). However,
checkKindsofPropertyMemberOverrides never considers those
implicity pseudo-inherited properties unless extends Object is
explicitly specified.

For example, I want to generate an error if --noImplicitOverride
is true and 'override' is not present on the following class:

class Foo {
      toLocaleString(): string { 'foo' };
}

I have addressed this with the new checkImplicitPropertyMemberOverrides() function that looks for matching members on the Object type. An alternative implementation might add in a heritageClause for Object early in checkKindsOfPropertyMemberOverrides. Other suggestions welcome.

pcj · 2017-01-04T08:17:18Z

In addition, the transpile test for the compiler option is failing as follows. This does not get resolved with jake baseline-accept. Any recommendations for addressing this?

  1) Transpile Supports setting 'noImplicitOverride' Correct output for undefined:
       Error: The baseline file transpile/Supports setting noImplicitOverride.js has changed.
        at writeComparison (built/local/run.js:90502:23)

mhegazy · 2017-01-04T23:45:56Z

See comments above. In addition, consider the following issues:

Declaration emit test still need to be implemented. Is there a
similar test case for study here? Please advise.

what you need is // @declaration: true at the top of your test case.

Regarding handling of an @override jsdoc tag. Where should I
look for the presence of this jsDoc tag and merge it into
ModifierFlags.Override? Alternatively, should a separate
ModifierFlags.JSDocOverride flag exist and perform dual checks for
these (perhaps as a utility function)?

This is only applicable in JS files, i would get this done in a separate PR.

Regarding the semantics of static methods: I have assumed that override
should not be used with static. Is this true?

Do not see why you can not have a static override

Regarding the semantics of an abstract method: should it be
permissible to apply override to a repeated abstract method in an
derived abstract class? For example:

I would say no. you can just drop the abstract modifier. need to think more though. @RyanCavanaugh thoughts?

As an extension to this line of thought, the --noImplicitOverride flag does not complain about the absence of override on interfaces. AFAICT, it does not add anything in terms of correctness and adds unnecessary verbosity to the language. Only when a derived member exists in a concrete class will those assertions kick in.

I do not think it makes much sense. the only use case, is the same as the abstract one above, which is override with more specific types, not sure if this is that important.

Regarding implicit extension of Object. I had naively imagined that all
classes implicitly extend from Object (and that
getBaseTypes(classType) would include Object in the result
list).

the properties from Object are "augmentations" that are applied to every object. not sure
i would want to go there, but will need to discuss this in a design meeting.

pcj · 2017-01-05T19:03:17Z

the properties from Object are "augmentations" that are applied to every object. not sure
i would want to go there, but will need to discuss this in a design meeting.

Excluding augmented methods from override would be a glaring omission IMHO. Unless I'm missing something, the implementation does not seem particularly onerous. Look forward to hearing your thoughts about this following the design meeting.

mhegazy · 2017-01-06T00:25:19Z

src/compiler/declarationEmitter.ts

@@ -717,6 +717,9 @@ namespace ts {
        }

        function emitClassMemberDeclarationFlags(flags: ModifierFlags) {
+            if (flags & ModifierFlags.Override) {


this should be after protected. also please add a test for that.

and after static.

mhegazy · 2017-01-06T00:26:43Z

src/compiler/checker.ts

+                       else if (flags & ModifierFlags.Abstract) {
+                           return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "abstract", "override");
+                       }
+                       else if (node.parent.kind === SyntaxKind.ModuleBlock || node.parent.kind === SyntaxKind.SourceFile) {


still check too wide. you want to follow the abstract check above. basically override is only allowed on class property/method/accessor declarations.

here is the order we should enforce/check for:

[public | protected | private] [abstract | override] [static] [readonly | async] [get | set] identifier

class C { public override static readonly p: number; protected overide static async method() {} }

mhegazy · 2017-01-06T00:47:41Z

src/compiler/checker.ts

+                       if (flags & ModifierFlags.Override) {
+                           return grammarErrorOnNode(modifier, Diagnostics._0_modifier_already_seen, "override");
+                       }
+                       else if (flags & ModifierFlags.Static) {


should be allowed

mhegazy · 2017-01-06T00:50:44Z

src/compiler/checker.ts

+            if (!objectType) {
+                return;
+            }
+            for (const name in propertiesToCheck) {


we should disable this check in ambient contexts.

mhegazy · 2017-01-06T00:51:14Z

src/compiler/checker.ts

+
+                            // Before accepting the correct case, ensure 'override' is marked if --noImplicitOverride is true.
+                            // Abstract members are an exception as override checks are suspended until the implementation solidifies.
+                            if (compilerOptions.noImplicitOverride


disable this in ambient contexts.

…ler option. * Moves `OverrideKeyword` to the more appropriate contextual keyword section (types.ts) * Introduces the `CompilerOptions.noImplicitOverride` boolean option (types.ts). * Moves the location for performing override assertions to `checkKindsOfPropertyMemberOverrides` as suggested (checker.ts). * Adds several other diagnostic messages (diagnosticMessages.json). * Improves the breadth of the `overrideKeyword.ts` test case. * Adds a new `noImplicitOverride.ts` test case.

* Rename check{Implicit -> Augmented}PropertyMemberOverrides. * Disable augment checks in ambients contexts. * Allow override on static methods. * Add override keyword to spec.md. * Use chained diagnostics when possible. * Remove hardcoded diagnotic-related strings in checker.ts. * Split tests into separate files overrideKeyword{es5 + es6}.ts.

pcj · 2017-01-15T06:53:59Z

Thanks for your patience... Comments addressed. Force-pushed 3143818 after rebase to 2711303 (Jan 12). A few more issues, but I think we are getting close. Just want to say again how much I appreciate the excellent reviews.

You mentioned earlier about consecutive declaration checks. I don't understand this. I put this test in, but I'm sure it's missing the mark. Can you clarify what the intent is?
I'm still not able to resolve the following error, does not go away after baseline-accept. Any pointers?

 [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (1m)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (1m)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (58s)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6547 passing (1m)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (1m)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (59s)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✓ 6548 passing (1m)
  [▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬▬] ✖ 7264/7265 passing (1m)


  53099 passing (9m)
  1 failing

  1) Transpile Supports setting 'noImplicitOverride' Correct output for undefined:
       Error: The baseline file transpile/Supports setting noImplicitOverride.js has changed.
        at writeComparison (built/local/run.js:91196:23)

I added override to the updated the spec under "The following keywords have special meaning in certain contexts, but are valid identifiers". After looking through the spec, I was not convinced much extra explanation was necessary. Any suggestions for additional documentation welcome.

pcj · 2017-01-19T17:36:19Z

Friendly ping.

RyanCavanaugh · 2017-01-19T17:44:05Z

@pcj looks like the new baseline tasks don't handle the transpile folder correctly yet. Try moving the file from the tests/baselines/local/transpile/ folder to tests/baselines/reference/transpile and commiting it

pcj · 2017-01-20T03:37:10Z

Thanks @RyanCavanaugh that did it.

pcj · 2017-01-24T15:46:23Z

@mhegazy @RyanCavanaugh, do we have a roadmap on moving this forward?

mhegazy · 2017-01-25T00:30:19Z

@mhegazy @RyanCavanaugh, do we have a roadmap on moving this forward?

Some one needs to review it, and we need to discuss it in the design meeting. I will put it on the agenda for this week.

pcj · 2017-02-08T15:43:22Z

Appreciate posting the design meeting notes at #13729.

Two questions, (one big, one small):

From a high level, is there agreement that override should exist in typescript? (If true, then we're just working out the semantics at this point). Implication is that we are planning to move forward with this PR. In that case, can we assign a milestone?
What is the next actionable step? Don't want the code to drift too much relative to HEAD.

Thanks!

ghost

Sorry for the long delay!

Please add this test (seems to work without any modification):

// @noImplicitOverride: true

const mixin = <BC extends new (...args: any[]) => {}>(Base: BC) => class extends Base {
    mixedIn() {}
};

class A {
    normal() {}
}

class B extends mixin(A) {
    normal() {} // Error: needs 'override'
    mixedIn() {} // Error: needs 'override'
}

Also, merge from master before reading my comments because they may only apply to the latest version of the compiler. In a few places it looks like you were using old-style maps and now we use es6 maps, so my comments could help you update.

ghost · 2017-05-05T19:53:28Z

src/compiler/checker.ts

+                for (const prop of getPropertiesOfObjectType(type)) {
+                    properties[prop.name] = prop;
+                }
+                checkAugmentedPropertyMemberOverrides(type, properties);


checkAugmentedPropertyMemberOverrides(type, resolveStructuredTypeMembers(type).members);

Got it, done

ghost · 2017-05-05T19:53:58Z

src/compiler/checker.ts

+                return;
+            }
+
+            const objectType = getSymbol(globals, "Object", SymbolFlags.Type);


const objectType = resolveStructuredTypeMembers(globalObjectType);. No need to test for non-null.

ghost · 2017-05-05T19:54:22Z

src/compiler/checker.ts

+                return;
+            }
+
+            for (const name in propertiesToCheck) {


propertiesToCheck.forEach(propertyToCheck => {

ghost · 2017-05-05T19:54:32Z

src/compiler/checker.ts

+            for (const name in propertiesToCheck) {
+                const derived = getTargetSymbol(propertiesToCheck[name]);
+                const derivedDeclarationFlags = getDeclarationModifierFlagsFromSymbol(derived);
+                const found = objectType.members[name];


objectType.members.get(propertyToCheck.name)

ghost · 2017-05-05T19:55:05Z

src/compiler/checker.ts

@@ -18372,9 +18456,19 @@ namespace ts {
            // derived class instance member variables and accessors, but not by other kinds of members.

            // NOTE: assignability is checked in checkClassDeclaration
+
+            // Track which symbols in the derived class have not been seen.
+            const onlyInDerived = createMap<Symbol>();


const onlyInDerived = cloneMap(resolveStructuredTypeMembers(type).members); (no need for for loop)

ghost · 2017-05-05T20:07:08Z

src/compiler/checker.ts

@@ -16104,6 +16127,9 @@ namespace ts {
                        else if (deviation & ModifierFlags.Abstract) {
                            error(o.name, Diagnostics.Overload_signatures_must_all_be_abstract_or_non_abstract);
                        }
+                        else if (deviation & ModifierFlags.Override) {


Remember to modify const flagsToCheck below to include override. And make a test for this.

ghost · 2017-05-05T20:08:39Z

src/compiler/checker.ts

+                            symbolToString(derived),
+                            symbolToString(found)
+                        );
+                        if (!isPropertyIdenticalTo(derived, foundSymbol)) {


This checks for identical properties, but the error message says "assignable to".

Removed check (was unnecessary)

ghost · 2017-05-05T20:09:42Z

tests/baselines/reference/noImplicitOverride.errors.txt

@@ -0,0 +1,86 @@
+tests/cases/compiler/noImplicitOverride.ts(11,5): error TS90034: Class member 'toString' must be marked 'override' when noImplicitOverride is enabled (augmented from Object.Object)


Something's wrong with the way this is being rendered. We shouldn't have Object. in front of all of these. (Except e.g. Object.toString which is actually a property of Object.)

ghost · 2017-05-05T20:10:50Z

src/compiler/checker.ts

+                           return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "private", "override");
+                       }
+                       else if (node.kind === SyntaxKind.Parameter) {
+                           return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_appear_on_a_parameter, "override");


Actually, I think we should allow override as a marker of a parameter property. E.g.:

abstract class A { abstract x: number; } class B extends A { constructor(override x: number) }

ghost · 2017-05-05T20:15:14Z

tests/baselines/reference/noImplicitOverride.errors.txt

+        }
+    }
+
+    class RejectWhenOverrideMissingOnAugmentedProperty {


Include a positive version of this test. class A { override toString() { return ""; } } should succeed with no errors.

pcj · 2017-05-05T21:05:32Z

Thanks @andy-ms for your review. I'll merge from master and begin addressing your comments over the weekend.

ghost · 2017-05-08T21:15:52Z

src/compiler/checker.ts

                                return grammarErrorOnNode(modifier, Diagnostics.Abstract_methods_can_only_appear_within_an_abstract_class);
                            }
-                            if (flags & ModifierFlags.Static) {
+                            else if (flags & ModifierFlags.Override) {
+                                return grammarErrorOnNode(modifier, Diagnostics._0_modifier_cannot_be_used_with_1_modifier, "override", "abstract");


In this situation, I think it would make sense to put an override modifier on the subclass' method:

abstract class A { abstract m(): void; } abstract class B extends A { abstract m(): void; }

ghost · 2017-07-27T22:01:39Z

@pcj bump?

ghost · 2017-09-20T15:42:20Z

@pcj bump

pcj · 2017-09-28T02:41:33Z

@andy-ms Haven't forgot, sorry for the delay! Still working on this.

ChiriVulpes · 2017-11-29T12:28:42Z

Isn't this PR close to finished? Was it abandoned? I would really like this functionality... so what are the community's next steps here? Do you need people to step in and help out?

pcj · 2017-12-01T04:32:41Z

Sorry guys, this got de-prioritized on my end when other stuff started rolling. If I can't get his going again in the next two weeks, someone else should take over.

pcj · 2017-12-01T15:42:19Z

OK, this PR is now sync'd back up to master. Will allocate some time this weekend to address some of @andy-ms review comments.

Test output looking reasonable, no errors seen.

pcj · 2017-12-03T20:09:28Z

OK, I have this mostly up to speed. Still need to address some of @andy-ms comments. Issue I am still working out is thse cases:

class A {
    protected static type: string;
}

class B extends A {
    protected static type: string = 'B';
}

In this case, the compiler should (maybe?) detect that B.type is masking A.type, and insist that B.type declaration includes the override keyword. However, in contrast to instance member variables, detecting this case is not already part of the compiler's logic. This gets back to the issue if override should be mandated in conjuction with static. There may be differing opinions on this.

The second one is somewhat esoteric that I got from @ahejlsberg in #5989:

class A {
    "constructor": typeof A;  // Explicitly declare constructor property
    protected static type: string;

    public log() {
        console.log(this.constructor.type);  // Access actual constructor object
    }
}

class B extends A {
    protected static type: string = 'B';
}

class C extends A {
    protected static type: string = 'C';
}

    class A {
        "constructor": typeof A;  // Explicitly declare constructor property
        ~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2322: Type 'typeof A' is not assignable to type 'Function'.
!!! error TS2322:   Class member '"constructor"' must be marked 'override' when noImplicitOverride is enabled (inherited from Object)
        protected static type: string;
    
        public log() {
            console.log(this.constructor.type);  // Access actual constructor object
            ~~~~~~~
!!! error TS2304: Cannot find name 'console'.
        }
    }
    
    class B extends A {
        protected static type: string = 'B';
    }
    
    class C extends A {
        protected static type: string = 'C';
    }

In this case, I am not certain this is valid syntax anymore. Also, putting override "constructor" = typeof A does not resolve the override error here, and it probably should.

pcj · 2017-12-05T00:05:31Z

Sorry to loop back on this after long delay, but I'm personally not seeing the point of using override with static members. @mhegazy you mentioned it should be possible (yes), but what is the use case for it?

Aren't users always specifying the base type constructor (like FooFactory.buildFoo(args) to access static members? I always do. Maybe there's something I'm missing.

ghost · 2017-12-06T15:41:17Z

@pcj A static member on a subclass overrides a static member on a superclass with the same name.

class C {
    static a(): number {
        // 'this' = the class from the LHS of '.', like with an instance member.
        // So 'this.b' doesn't always mean 'C.b'.
        return this.b();
    }

    static b(): number {
        return 0;
    }
}

class D extends C {
    static b() {
        return 1;
    }
}

console.log(D.a()); // 1

In the example you gave above the compiler can currently detect an error if you write protected static type = 0; in the subclass, because it's supposed to be a string.

As for the other error, you shouldn't be getting Type 'typeof A' is not assignable to type 'Function'. -- const f: Function = A; works...

pcj · 2017-12-07T03:03:12Z

Whoa. Okay, I didn't know that! (static members accessible through the this reference). You convinced me.

DanielRosenwasser · 2017-12-15T20:15:52Z

Hi @pcj. I'm sorry that we've let this PR linger without more direct team feedback for as long as it has. We discussed the issue and PR in our last design meeting (#20724), and after having discussed it more broadly among members of the team, we didn't feel entirely confident as to whether override is a great fit for the language at this time. One thing we'd like to experiment with is whether the need can be served by design-time decorators instead (#2900).

Again, I apologize that we didn't address this PR earlier. We reiterated the importance of transparently communicating our own positions outside of the core team. The work you've done here is great, so I hope this experience doesn't dissuade you from continuing to participate in the community and on our issue tracker.

pcj · 2017-12-16T01:20:56Z

Hi @DanielRosenwasser I understand your concerns. The feature as-is is largely complete with the exception of static / constructor inheritance that I may push up as a WIP.

If there is future support for it, it can be resurrected from here.

armandn · 2018-05-16T06:39:57Z

For me personally, as a developer with experience in Java, C# and others, override feels like a much better solution than design-time decorators. I really hope this is not a final decision of the Typescript team.

sam-s4s · 2018-05-18T05:12:50Z

@DanielRosenwasser I know what you mean about the nature of Javascript - but the same could be argued about typing - typing is a bad fit for Javascript. But Typescript fixed that (for the most part) in the same way that it could fix the lack of override (#2000). I feel like it is a valid feature request.

fedorl · 2018-05-25T16:27:12Z

It is sad to see that such must-to-have feature has sunk in multiple year discussions about theoretical corner cases and unimportant implementation details, especially given that many mature languages have already figured all details out and there is no need to reinvent the wheel. It was much better to make a breaking change and a flag which turns it off by default.

I guess many people will be happy to turn such flag on and spend few days refactoring the entire codebase and refactor again when (and if..) the compiler 'override' implementation changes, instead of not having it at all for many years and foreseeable future.

msftclas added the cla-signed label Dec 29, 2016

pcj mentioned this pull request Dec 29, 2016

Support override keyword on class methods #2000

Closed

RyanCavanaugh reviewed Dec 29, 2016

View reviewed changes

mhegazy suggested changes Dec 29, 2016

View reviewed changes

mhegazy suggested changes Jan 6, 2017

View reviewed changes

pcj added 2 commits January 13, 2017 06:08

Adds override keyword and initial modifier flags checks.

65f98b7

andy-hanson mentioned this pull request Jan 14, 2017

Add prefer-function-over-method rule palantir/tslint#2037

Merged

5 tasks

pcj changed the title ~~Adds override keyword~~ Add override keyword Jan 18, 2017

Include transpile reference test

a1cdd8c

pcj mentioned this pull request Jan 20, 2017

Include selected local subdirs in baseline-accept #13587

Closed

DanielRosenwasser mentioned this pull request Jan 27, 2017

Design Meeting Notes, 1/27/2017 #13729

Closed

ghost reviewed May 5, 2017

View reviewed changes

ghost reviewed May 8, 2017

View reviewed changes

msftgits removed the cla-signed label Sep 26, 2017

microsoft deleted a comment from msftclas Sep 27, 2017

Merge remote-tracking branch 'upstream/master' into over-new

a186758

pcj added 4 commits December 1, 2017 20:29

Resolve compiler errors following merge

3626403

Restore master spec.md; Stop nodejs mem crash

c791e7c

Restore baseline test functions

7dbf75d

Test output looking reasonable, no errors seen.

Accept new baselines

5fe3b4d

Include override as valid parameter property

489d09c

DanielRosenwasser mentioned this pull request Dec 15, 2017

Design Meeting Notes, 12/8/2017 #20724

Closed

pcj closed this Dec 16, 2017

microsoft locked and limited conversation to collaborators Jul 31, 2018

		@@ -0,0 +1,86 @@
		tests/cases/compiler/noImplicitOverride.ts(11,5): error TS90034: Class member 'toString' must be marked 'override' when noImplicitOverride is enabled (augmented from Object.Object)

Add override keyword #13217

Add override keyword #13217

Conversation

pcj commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented Dec 29, 2016 • edited

pcj commented Jan 4, 2017

pcj commented Jan 4, 2017

mhegazy commented Jan 4, 2017

pcj commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented Jan 15, 2017

pcj commented Jan 19, 2017

RyanCavanaugh commented Jan 19, 2017

pcj commented Jan 20, 2017

pcj commented Jan 24, 2017

mhegazy commented Jan 25, 2017

pcj commented Feb 8, 2017

ghost left a comment • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented May 5, 2017

Choose a reason for hiding this comment

ghost commented Jul 27, 2017

ghost commented Sep 20, 2017

pcj commented Sep 28, 2017

ChiriVulpes commented Nov 29, 2017

pcj commented Dec 1, 2017

pcj commented Dec 1, 2017

pcj commented Dec 3, 2017

pcj commented Dec 5, 2017

ghost commented Dec 6, 2017 • edited by ghost

pcj commented Dec 29, 2016 •

edited

ghost left a comment •

edited by ghost

ghost commented Dec 6, 2017 •

edited by ghost

pcj commented Dec 7, 2017 •

edited