Update diagnostic message and quickfix for parameter property (#44010)

microsoft · May 9, 2021 · 5b1e873 · 5b1e873
1 parent 3125398
commit 5b1e873
Show file tree

Hide file tree

Showing 13 changed files with 110 additions and 27 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -37214,7 +37214,7 @@ namespace ts {
 
                         if (!baseHasAbstract) {
                             const diag = memberIsParameterProperty ?
-                                Diagnostics.This_parameter_property_must_be_rewritten_as_a_property_declaration_with_an_override_modifier_because_it_overrides_a_member_in_base_class_0 :
+                                Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0 :
                                 Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0;
                             error(member, diag, baseClassName);
                         }

diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -3717,7 +3717,7 @@
         "category": "Error",
         "code": 4114
     },
-    "This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class '{0}'.": {
+    "This parameter property must have an 'override' modifier because it overrides a member in base class '{0}'.": {
         "category": "Error",
         "code": 4115
     },

diff --git a/src/services/codefixes/fixOverrideModifier.ts b/src/services/codefixes/fixOverrideModifier.ts
@@ -4,18 +4,20 @@ namespace ts.codefix {
     const fixAddOverrideId = "fixAddOverrideModifier";
     const fixRemoveOverrideId = "fixRemoveOverrideModifier";
 
-    type ClassElementHasJSDoc =
+    type ClassElementLikeHasJSDoc =
         | ConstructorDeclaration
         | PropertyDeclaration
         | MethodDeclaration
         | GetAccessorDeclaration
-        | SetAccessorDeclaration;
+        | SetAccessorDeclaration
+        | ParameterPropertyDeclaration;
 
     const errorCodes = [
         Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0.code,
         Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class.code,
         Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0.code,
-        Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0.code
+        Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0.code,
+        Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code
     ];
 
     const errorCodeFixIdMap: Record<number, [DiagnosticMessage, string | undefined, DiagnosticMessage | undefined]> = {
@@ -25,6 +27,9 @@ namespace ts.codefix {
         [Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class.code]: [
             Diagnostics.Remove_override_modifier, fixRemoveOverrideId, Diagnostics.Remove_all_unnecessary_override_modifiers
         ],
+        [Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code]: [
+            Diagnostics.Add_override_modifier, fixAddOverrideId, Diagnostics.Add_all_missing_override_modifiers,
+        ],
         [Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0.code]: [
             Diagnostics.Add_override_modifier, fixAddOverrideId, Diagnostics.Remove_all_unnecessary_override_modifiers
         ],
@@ -70,6 +75,7 @@ namespace ts.codefix {
         switch (errorCode) {
             case Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0.code:
             case Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0.code:
+            case Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code:
                 return doAddOverrideModifierChange(changeTracker, context.sourceFile, pos);
             case Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0.code:
             case Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class.code:
@@ -80,7 +86,7 @@ namespace ts.codefix {
     }
 
     function doAddOverrideModifierChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
-        const classElement = findContainerClassElement(sourceFile, pos);
+        const classElement = findContainerClassElementLike(sourceFile, pos);
         const modifiers = classElement.modifiers || emptyArray;
         const staticModifier = find(modifiers, isStaticModifier);
         const accessibilityModifier = find(modifiers, m => isAccessibilityModifier(m.kind));
@@ -92,34 +98,36 @@ namespace ts.codefix {
     }
 
     function doRemoveOverrideModifierChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
-        const classElement = findContainerClassElement(sourceFile, pos);
+        const classElement = findContainerClassElementLike(sourceFile, pos);
         const overrideModifier = classElement.modifiers && find(classElement.modifiers, modifier => modifier.kind === SyntaxKind.OverrideKeyword);
         Debug.assertIsDefined(overrideModifier);
 
         changeTracker.deleteModifier(sourceFile, overrideModifier);
     }
 
-    function isClassElementHasJSDoc(node: Node): node is ClassElementHasJSDoc {
+    function isClassElementLikeHasJSDoc(node: Node): node is ClassElementLikeHasJSDoc {
         switch (node.kind) {
             case SyntaxKind.Constructor:
             case SyntaxKind.PropertyDeclaration:
             case SyntaxKind.MethodDeclaration:
             case SyntaxKind.GetAccessor:
             case SyntaxKind.SetAccessor:
                 return true;
+            case SyntaxKind.Parameter:
+                return isParameterPropertyDeclaration(node, node.parent);
             default:
                 return false;
         }
     }
 
-    function findContainerClassElement(sourceFile: SourceFile, pos: number) {
+    function findContainerClassElementLike(sourceFile: SourceFile, pos: number) {
         const token = getTokenAtPosition(sourceFile, pos);
         const classElement = findAncestor(token, node => {
             if (isClassLike(node)) return "quit";
-            return isClassElementHasJSDoc(node);
+            return isClassElementLikeHasJSDoc(node);
         });
 
-        Debug.assert(classElement && isClassElementHasJSDoc(classElement));
+        Debug.assert(classElement && isClassElementLikeHasJSDoc(classElement));
         return classElement;
     }
 }

diff --git a/tests/baselines/reference/override6.errors.txt b/tests/baselines/reference/override6.errors.txt
@@ -1,6 +1,6 @@
 tests/cases/conformance/override/override6.ts(9,12): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'B'.
-tests/cases/conformance/override/override6.ts(10,17): error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
-tests/cases/conformance/override/override6.ts(10,37): error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
+tests/cases/conformance/override/override6.ts(10,17): error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
+tests/cases/conformance/override/override6.ts(10,37): error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
 
 
 ==== tests/cases/conformance/override/override6.ts (3 errors) ====
@@ -17,9 +17,9 @@ tests/cases/conformance/override/override6.ts(10,37): error TS4115: This paramet
 !!! error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'B'.
         constructor(public foo: string, public baz: number) {
                     ~~~~~~~~~~~~~~~~~~
-!!! error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
+!!! error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
                                         ~~~~~~~~~~~~~~~~~~
-!!! error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
+!!! error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
             super(foo, 42)
         }
     }

diff --git a/tests/baselines/reference/override8.errors.txt b/tests/baselines/reference/override8.errors.txt
@@ -1,5 +1,5 @@
-tests/cases/conformance/override/override8.ts(6,17): error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
-tests/cases/conformance/override/override8.ts(18,17): error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'BB'.
+tests/cases/conformance/override/override8.ts(6,17): error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
+tests/cases/conformance/override/override8.ts(18,17): error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'BB'.
 tests/cases/conformance/override/override8.ts(24,12): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'BB'.
 
 
@@ -11,7 +11,7 @@ tests/cases/conformance/override/override8.ts(24,12): error TS4114: This member
     class D extends B {
         constructor(public a: string, public b: string) {
                     ~~~~~~~~~~~~~~~~
-!!! error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'B'.
+!!! error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'B'.
             super();
         }
     }
@@ -25,7 +25,7 @@ tests/cases/conformance/override/override8.ts(24,12): error TS4114: This member
     class DD extends BB {
         constructor(public a: string) {
                     ~~~~~~~~~~~~~~~~
-!!! error TS4115: This parameter property must be rewritten as a property declaration with an 'override' modifier because it overrides a member in base class 'BB'.
+!!! error TS4115: This parameter property must have an 'override' modifier because it overrides a member in base class 'BB'.
             super(a)
         }
     }

diff --git a/tests/baselines/reference/overrideParameterProperty.errors.txt b/tests/baselines/reference/overrideParameterProperty.errors.txt
@@ -1,8 +1,9 @@
 tests/cases/conformance/override/overrideParameterProperty.ts(20,24): error TS1029: 'public' modifier must precede 'override' modifier.
 tests/cases/conformance/override/overrideParameterProperty.ts(25,5): error TS2369: A parameter property is only allowed in a constructor implementation.
+tests/cases/conformance/override/overrideParameterProperty.ts(29,15): error TS4113: This member cannot have an 'override' modifier because it is not declared in the base class 'Base'.
 
 
-==== tests/cases/conformance/override/overrideParameterProperty.ts (2 errors) ====
+==== tests/cases/conformance/override/overrideParameterProperty.ts (3 errors) ====
     class Base {
       p1!: string;
     }
@@ -33,4 +34,11 @@ tests/cases/conformance/override/overrideParameterProperty.ts(25,5): error TS236
         ~~~~~~~~~~~~~~~~~~~~
 !!! error TS2369: A parameter property is only allowed in a constructor implementation.
     }
-
+
+    class C4 extends Base {
+      constructor(public override p2: string) {
+                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! error TS4113: This member cannot have an 'override' modifier because it is not declared in the base class 'Base'.
+        super();
+      }
+    }
diff --git a/tests/baselines/reference/overrideParameterProperty.js b/tests/baselines/reference/overrideParameterProperty.js
@@ -25,7 +25,12 @@ class C3 extends Base {
 
   m(override p1: "hello") {}
 }
-
+
+class C4 extends Base {
+  constructor(public override p2: string) {
+    super();
+  }
+}
 
 //// [overrideParameterProperty.js]
 var __extends = (this && this.__extends) || (function () {
@@ -79,3 +84,12 @@ var C3 = /** @class */ (function (_super) {
     C3.prototype.m = function (p1) { };
     return C3;
 }(Base));
+var C4 = /** @class */ (function (_super) {
+    __extends(C4, _super);
+    function C4(p2) {
+        var _this = _super.call(this) || this;
+        _this.p2 = p2;
+        return _this;
+    }
+    return C4;
+}(Base));
diff --git a/tests/baselines/reference/overrideParameterProperty.symbols b/tests/baselines/reference/overrideParameterProperty.symbols
@@ -61,3 +61,14 @@ class C3 extends Base {
 >p1 : Symbol(p1, Decl(overrideParameterProperty.ts, 24, 4))
 }
 
+class C4 extends Base {
+>C4 : Symbol(C4, Decl(overrideParameterProperty.ts, 25, 1))
+>Base : Symbol(Base, Decl(overrideParameterProperty.ts, 0, 0))
+
+  constructor(public override p2: string) {
+>p2 : Symbol(C4.p2, Decl(overrideParameterProperty.ts, 28, 14))
+
+    super();
+>super : Symbol(Base, Decl(overrideParameterProperty.ts, 0, 0))
+  }
+}
diff --git a/tests/baselines/reference/overrideParameterProperty.types b/tests/baselines/reference/overrideParameterProperty.types
@@ -64,3 +64,15 @@ class C3 extends Base {
 >p1 : "hello"
 }
 
+class C4 extends Base {
+>C4 : C4
+>Base : Base
+
+  constructor(public override p2: string) {
+>p2 : string
+
+    super();
+>super() : void
+>super : typeof Base
+  }
+}
diff --git a/tests/cases/conformance/override/overrideParameterProperty.ts b/tests/cases/conformance/override/overrideParameterProperty.ts
@@ -26,3 +26,9 @@ class C3 extends Base {
 
   m(override p1: "hello") {}
 }
+
+class C4 extends Base {
+  constructor(public override p2: string) {
+    super();
+  }
+}
diff --git a/tests/cases/fourslash/codeFixOverrideModifier10.ts b/tests/cases/fourslash/codeFixOverrideModifier10.ts
@@ -7,9 +7,13 @@
 //// }
 //// class D extends B {
 ////     c = 10;
-////     constructor(public a: string, public readonly b: string) {
+////     constructor(public a: string, [|public readonly b: string|]) {
 ////         super();
 ////     }
 //// }
 
-verify.not.codeFixAvailable();
+verify.codeFix({
+    description: "Add 'override' modifier",
+    newRangeContent: "public override readonly b: string",
+    index: 0
+})
diff --git a/tests/cases/fourslash/codeFixOverrideModifier21.ts b/tests/cases/fourslash/codeFixOverrideModifier21.ts
@@ -0,0 +1,17 @@
+/// <reference path='fourslash.ts' />
+
+// @noImplicitOverride: true
+
+//// class B { }
+//// class D extends B {
+////     c = 10;
+////     constructor([|public override a: string|]) {
+////         super();
+////     }
+//// }
+
+verify.codeFix({
+    description: "Remove 'override' modifier",
+    newRangeContent: "public a: string",
+    index: 0
+})
diff --git a/tests/cases/fourslash/codeFixOverrideModifier9.ts b/tests/cases/fourslash/codeFixOverrideModifier9.ts
@@ -6,10 +6,13 @@
 ////     a: string
 //// }
 //// class D extends B {
-////     constructor(public a: string, public b: string) {
+////     constructor([|public a: string|], public b: string) {
 ////         super();
 ////     }
 //// }
 
-verify.not.codeFixAvailable();
-
+verify.codeFix({
+    description: "Add 'override' modifier",
+    newRangeContent: "public override a: string",
+    index: 0
+})