implement code fix for override of js files (#45780)

* feat: code fix for override in js files

Co-Authored-By: Wenlu Wang <kingwenlu@gmail.com>

* fix comments

Co-Authored-By: Wenlu Wang <kingwenlu@gmail.com>

* remove tryMergeJsdocTags

* fix: bring the two methods back as functions

* revert emitter changes

* fix comments

* fix: test failures

Co-authored-by: Wenlu Wang <kingwenlu@gmail.com>
Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com>

src/compiler/emitter.ts

@@ -1604,6 +1604,7 @@ namespace ts {
                         return emitJSDocSignature(node as JSDocSignature);
                     case SyntaxKind.JSDocTag:
                     case SyntaxKind.JSDocClassTag:
+                    case SyntaxKind.JSDocOverrideTag:
                         return emitJSDocSimpleTag(node as JSDocTag);
                     case SyntaxKind.JSDocAugmentsTag:
                     case SyntaxKind.JSDocImplementsTag:
@@ -1616,7 +1617,6 @@ namespace ts {
                     case SyntaxKind.JSDocPrivateTag:
                     case SyntaxKind.JSDocProtectedTag:
                     case SyntaxKind.JSDocReadonlyTag:
-                    case SyntaxKind.JSDocOverrideTag:
                         return;
                     case SyntaxKind.JSDocCallbackTag:
                         return emitJSDocCallbackTag(node as JSDocCallbackTag);

src/services/codefixes/fixOverrideModifier.ts

@@ -17,37 +17,81 @@ namespace ts.codefix {
         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_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code
+        Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code,
+        Diagnostics.This_member_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0.code,
+        Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_its_containing_class_0_does_not_extend_another_class.code,
+        Diagnostics.This_parameter_property_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0.code,
+        Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0.code,
     ];
 
-    const errorCodeFixIdMap: Record<number, [DiagnosticMessage, string | undefined, DiagnosticMessage | undefined]> = {
-        [Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0.code]: [
-            Diagnostics.Add_override_modifier, fixAddOverrideId, Diagnostics.Add_all_missing_override_modifiers,
-        ],
-        [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
-        ],
-        [Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0.code]: [
-            Diagnostics.Remove_override_modifier, fixRemoveOverrideId, Diagnostics.Remove_all_unnecessary_override_modifiers
-        ]
+    interface ErrorCodeFixInfo {
+        descriptions: DiagnosticMessage;
+        fixId?: string | undefined;
+        fixAllDescriptions?: DiagnosticMessage | undefined;
+    }
+
+    const errorCodeFixIdMap: Record<number, ErrorCodeFixInfo> = {
+        // case #1:
+        [Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_a_member_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Add_override_modifier,
+            fixId: fixAddOverrideId,
+            fixAllDescriptions: Diagnostics.Add_all_missing_override_modifiers,
+        },
+        [Diagnostics.This_member_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Add_override_modifier,
+            fixId: fixAddOverrideId,
+            fixAllDescriptions: Diagnostics.Add_all_missing_override_modifiers
+        },
+        // case #2:
+        [Diagnostics.This_member_cannot_have_an_override_modifier_because_its_containing_class_0_does_not_extend_another_class.code]: {
+            descriptions: Diagnostics.Remove_override_modifier,
+            fixId: fixRemoveOverrideId,
+            fixAllDescriptions: Diagnostics.Remove_all_unnecessary_override_modifiers,
+        },
+        [Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_its_containing_class_0_does_not_extend_another_class.code]: {
+            descriptions: Diagnostics.Remove_override_modifier,
+            fixId: fixRemoveOverrideId,
+            fixAllDescriptions: Diagnostics.Remove_override_modifier
+        },
+        // case #3:
+        [Diagnostics.This_parameter_property_must_have_an_override_modifier_because_it_overrides_a_member_in_base_class_0.code]: {
+            descriptions: Diagnostics.Add_override_modifier,
+            fixId: fixAddOverrideId,
+            fixAllDescriptions: Diagnostics.Add_all_missing_override_modifiers,
+        },
+        [Diagnostics.This_parameter_property_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Add_override_modifier,
+            fixId: fixAddOverrideId,
+            fixAllDescriptions: Diagnostics.Add_all_missing_override_modifiers,
+        },
+        // case #4:
+        [Diagnostics.This_member_must_have_an_override_modifier_because_it_overrides_an_abstract_method_that_is_declared_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Add_override_modifier,
+            fixId: fixAddOverrideId,
+            fixAllDescriptions: Diagnostics.Remove_all_unnecessary_override_modifiers,
+        },
+        // case #5:
+        [Diagnostics.This_member_cannot_have_an_override_modifier_because_it_is_not_declared_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Remove_override_modifier,
+            fixId: fixRemoveOverrideId,
+            fixAllDescriptions: Diagnostics.Remove_all_unnecessary_override_modifiers,
+        },
+        [Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_it_is_not_declared_in_the_base_class_0.code]: {
+            descriptions: Diagnostics.Remove_override_modifier,
+            fixId: fixRemoveOverrideId,
+            fixAllDescriptions: Diagnostics.Remove_all_unnecessary_override_modifiers,
+        }
     };
 
     registerCodeFix({
         errorCodes,
         getCodeActions: context => {
-            const { errorCode, span, sourceFile } = context;
+            const { errorCode, span } = context;
 
             const info = errorCodeFixIdMap[errorCode];
             if (!info) return emptyArray;
 
-            const [ descriptions, fixId, fixAllDescriptions ] = info;
-            if (isSourceFileJS(sourceFile)) return emptyArray;
+            const { descriptions, fixId, fixAllDescriptions } = info;
             const changes = textChanges.ChangeTracker.with(context, changes => dispatchChanges(changes, context, errorCode, span.start));
 
             return [
@@ -57,9 +101,9 @@ namespace ts.codefix {
         fixIds: [fixName, fixAddOverrideId, fixRemoveOverrideId],
         getAllCodeActions: context =>
             codeFixAll(context, errorCodes, (changes, diag) => {
-                const { code, start, file } = diag;
+                const { code, start } = diag;
                 const info = errorCodeFixIdMap[code];
-                if (!info || info[1] !== context.fixId || isSourceFileJS(file)) {
+                if (!info || info.fixId !== context.fixId) {
                     return;
                 }
 
@@ -74,11 +118,15 @@ namespace ts.codefix {
         pos: number) {
         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_a_JSDoc_comment_with_an_override_tag_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:
+            case Diagnostics.This_parameter_property_must_have_a_JSDoc_comment_with_an_override_tag_because_it_overrides_a_member_in_the_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_a_JSDoc_comment_with_an_override_tag_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:
+            case Diagnostics.This_member_cannot_have_a_JSDoc_comment_with_an_override_tag_because_its_containing_class_0_does_not_extend_another_class.code:
                 return doRemoveOverrideModifierChange(changeTracker, context.sourceFile, pos);
             default:
                 Debug.fail("Unexpected error code: " + errorCode);
@@ -87,6 +135,10 @@ namespace ts.codefix {
 
     function doAddOverrideModifierChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
         const classElement = findContainerClassElementLike(sourceFile, pos);
+        if (isSourceFileJS(sourceFile)) {
+            changeTracker.addJSDocTags(sourceFile, classElement, [factory.createJSDocOverrideTag(factory.createIdentifier("override"))]);
+            return;
+        }
         const modifiers = classElement.modifiers || emptyArray;
         const staticModifier = find(modifiers, isStaticModifier);
         const abstractModifier = find(modifiers, isAbstractModifier);
@@ -101,6 +153,10 @@ namespace ts.codefix {
 
     function doRemoveOverrideModifierChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
         const classElement = findContainerClassElementLike(sourceFile, pos);
+        if (isSourceFileJS(sourceFile)) {
+            changeTracker.filterJSDocTags(sourceFile, classElement, not(isJSDocOverrideTag));
+            return;
+        }
         const overrideModifier = classElement.modifiers && find(classElement.modifiers, modifier => modifier.kind === SyntaxKind.OverrideKeyword);
         Debug.assertIsDefined(overrideModifier);
 

src/services/codefixes/inferFromUsage.ts

@@ -131,7 +131,7 @@ namespace ts.codefix {
                     if (typeNode) {
                         // Note that the codefix will never fire with an existing `@type` tag, so there is no need to merge tags
                         const typeTag = factory.createJSDocTypeTag(/*tagName*/ undefined, factory.createJSDocTypeExpression(typeNode), /*comment*/ undefined);
-                        addJSDocTags(changes, sourceFile, cast(parent.parent.parent, isExpressionStatement), [typeTag]);
+                        changes.addJSDocTags(sourceFile, cast(parent.parent.parent, isExpressionStatement), [typeTag]);
                     }
                     importAdder.writeFixes(changes);
                     return parent;
@@ -271,7 +271,7 @@ namespace ts.codefix {
     }
 
     function annotateJSDocThis(changes: textChanges.ChangeTracker, sourceFile: SourceFile, containingFunction: SignatureDeclaration, typeNode: TypeNode) {
-        addJSDocTags(changes, sourceFile, containingFunction, [
+        changes.addJSDocTags(sourceFile, containingFunction, [
             factory.createJSDocThisTag(/*tagName*/ undefined, factory.createJSDocTypeExpression(typeNode)),
         ]);
     }
@@ -311,7 +311,7 @@ namespace ts.codefix {
                 }
                 const typeExpression = factory.createJSDocTypeExpression(typeNode);
                 const typeTag = isGetAccessorDeclaration(declaration) ? factory.createJSDocReturnTag(/*tagName*/ undefined, typeExpression, /*comment*/ undefined) : factory.createJSDocTypeTag(/*tagName*/ undefined, typeExpression, /*comment*/ undefined);
-                addJSDocTags(changes, sourceFile, parent, [typeTag]);
+                changes.addJSDocTags(sourceFile, parent, [typeTag]);
             }
             else if (!tryReplaceImportTypeNodeWithAutoImport(typeNode, declaration, sourceFile, changes, importAdder, getEmitScriptTarget(program.getCompilerOptions()))) {
                 changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode);
@@ -378,46 +378,7 @@ namespace ts.codefix {
         else {
             const paramTags = map(inferences, ({ name, typeNode, isOptional }) =>
                 factory.createJSDocParameterTag(/*tagName*/ undefined, name, /*isBracketed*/ !!isOptional, factory.createJSDocTypeExpression(typeNode), /* isNameFirst */ false, /*comment*/ undefined));
-            addJSDocTags(changes, sourceFile, signature, paramTags);
-        }
-    }
-
-    export function addJSDocTags(changes: textChanges.ChangeTracker, sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
-        const comments = flatMap(parent.jsDoc, j => typeof j.comment === "string" ? factory.createJSDocText(j.comment) : j.comment) as JSDocComment[];
-        const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
-        const unmergedNewTags = newTags.filter(newTag => !oldTags || !oldTags.some((tag, i) => {
-            const merged = tryMergeJsdocTags(tag, newTag);
-            if (merged) oldTags[i] = merged;
-            return !!merged;
-        }));
-        const tag = factory.createJSDocComment(factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n"))), factory.createNodeArray([...(oldTags || emptyArray), ...unmergedNewTags]));
-        const jsDocNode = parent.kind === SyntaxKind.ArrowFunction ? getJsDocNodeForArrowFunction(parent) : parent;
-        jsDocNode.jsDoc = parent.jsDoc;
-        jsDocNode.jsDocCache = parent.jsDocCache;
-        changes.insertJsdocCommentBefore(sourceFile, jsDocNode, tag);
-    }
-
-    function getJsDocNodeForArrowFunction(signature: ArrowFunction): HasJSDoc {
-        if (signature.parent.kind === SyntaxKind.PropertyDeclaration) {
-            return signature.parent as HasJSDoc;
-        }
-        return signature.parent.parent as HasJSDoc;
-    }
-
-    function tryMergeJsdocTags(oldTag: JSDocTag, newTag: JSDocTag): JSDocTag | undefined {
-        if (oldTag.kind !== newTag.kind) {
-            return undefined;
-        }
-        switch (oldTag.kind) {
-            case SyntaxKind.JSDocParameterTag: {
-                const oldParam = oldTag as JSDocParameterTag;
-                const newParam = newTag as JSDocParameterTag;
-                return isIdentifier(oldParam.name) && isIdentifier(newParam.name) && oldParam.name.escapedText === newParam.name.escapedText
-                    ? factory.createJSDocParameterTag(/*tagName*/ undefined, newParam.name, /*isBracketed*/ false, newParam.typeExpression, newParam.isNameFirst, oldParam.comment)
-                    : undefined;
-            }
-            case SyntaxKind.JSDocReturnTag:
-                return factory.createJSDocReturnTag(/*tagName*/ undefined, (newTag as JSDocReturnTag).typeExpression, oldTag.comment);
+            changes.addJSDocTags(sourceFile, signature, paramTags);
         }
     }
 

src/services/textChanges.ts

@@ -494,6 +494,27 @@ namespace ts.textChanges {
             this.insertNodeAt(sourceFile, fnStart, tag, { preserveLeadingWhitespace: false, suffix: this.newLineCharacter + indent });
         }
 
+        public addJSDocTags(sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
+            const comments = flatMap(parent.jsDoc, j => typeof j.comment === "string" ? factory.createJSDocText(j.comment) : j.comment) as JSDocComment[];
+            const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
+            const unmergedNewTags = newTags.filter(newTag => !oldTags.some((tag, i) => {
+                const merged = tryMergeJsdocTags(tag, newTag);
+                if (merged) oldTags[i] = merged;
+                return !!merged;
+            }));
+            const tag = factory.createJSDocComment(factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n"))), factory.createNodeArray([...oldTags, ...unmergedNewTags]));
+            const host = updateJSDocHost(parent);
+            this.insertJsdocCommentBefore(sourceFile, host, tag);
+        }
+
+        public filterJSDocTags(sourceFile: SourceFile, parent: HasJSDoc, predicate: (tag: JSDocTag) => boolean): void {
+            const comments = flatMap(parent.jsDoc, j => typeof j.comment === "string" ? factory.createJSDocText(j.comment) : j.comment) as JSDocComment[];
+            const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
+            const tag = factory.createJSDocComment(factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n"))), factory.createNodeArray([...(filter(oldTags, predicate) || emptyArray)]));
+            const host = updateJSDocHost(parent);
+            this.insertJsdocCommentBefore(sourceFile, host, tag);
+        }
+
         public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string): void {
             this.changes.push({ kind: ChangeKind.Text, sourceFile, range, text });
         }
@@ -920,6 +941,35 @@ namespace ts.textChanges {
         }
     }
 
+    function updateJSDocHost(parent: HasJSDoc): HasJSDoc {
+        if (parent.kind !== SyntaxKind.ArrowFunction) {
+            return parent;
+        }
+        const jsDocNode = parent.parent.kind === SyntaxKind.PropertyDeclaration ?
+            parent.parent as HasJSDoc :
+            parent.parent.parent as HasJSDoc;
+        jsDocNode.jsDoc = parent.jsDoc;
+        jsDocNode.jsDocCache = parent.jsDocCache;
+        return jsDocNode;
+    }
+
+    function tryMergeJsdocTags(oldTag: JSDocTag, newTag: JSDocTag): JSDocTag | undefined {
+        if (oldTag.kind !== newTag.kind) {
+            return undefined;
+        }
+        switch (oldTag.kind) {
+            case SyntaxKind.JSDocParameterTag: {
+                const oldParam = oldTag as JSDocParameterTag;
+                const newParam = newTag as JSDocParameterTag;
+                return isIdentifier(oldParam.name) && isIdentifier(newParam.name) && oldParam.name.escapedText === newParam.name.escapedText
+                    ? factory.createJSDocParameterTag(/*tagName*/ undefined, newParam.name, /*isBracketed*/ false, newParam.typeExpression, newParam.isNameFirst, oldParam.comment)
+                    : undefined;
+            }
+            case SyntaxKind.JSDocReturnTag:
+                return factory.createJSDocReturnTag(/*tagName*/ undefined, (newTag as JSDocReturnTag).typeExpression, oldTag.comment);
+        }
+    }
+
     // find first non-whitespace position in the leading trivia of the node
     function startPositionToDeleteNodeInList(sourceFile: SourceFile, node: Node): number {
         return skipTrivia(sourceFile.text, getAdjustedStartPosition(sourceFile, node, { leadingTriviaOption: LeadingTriviaOption.IncludeAll }), /*stopAfterLineBreak*/ false, /*stopAtComments*/ true);

tests/cases/fourslash/codeFixOverrideModifier_js1.ts

@@ -1,5 +1,4 @@
 /// <reference path='fourslash.ts' />
-
 // @allowJs: true
 // @checkJs: true
 // @noEmit: true
@@ -9,14 +8,22 @@
 ////     foo (v) {}
 //// }
 //// class D extends B {
+////     /** @public */
 ////     foo (v) {}
-////     /**@override*/
-////     bar (v) {}
-//// }
-//// class C {
-////     /**@override*/
-////     foo () {}
 //// }
 
-verify.not.codeFixAvailable("fixAddOverrideModifier");
-verify.not.codeFixAvailable("fixRemoveOverrideModifier");
+verify.codeFix({
+    description: "Add 'override' modifier",
+    index: 0,
+    newFileContent:
+`class B {
+    foo (v) {}
+}
+class D extends B {
+    /**
+     * 
+     * @override
+     */
+    foo (v) {}
+}`,
+})

tests/cases/fourslash/codeFixOverrideModifier_js2.ts

@@ -0,0 +1,28 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+// @checkJs: true
+// @noEmit: true
+// @noImplicitOverride: true
+// @filename: a.js
+//// class B { }
+//// class D extends B {
+////     /**
+////      * @public
+////      * @override
+////      */
+////     foo (v) {}
+//// }
+
+verify.codeFix({
+    description: "Remove 'override' modifier",
+    index: 0,
+    newFileContent:
+`class B { }
+class D extends B {
+    /**
+     * 
+     */
+    foo (v) {}
+}`,
+})