Fix find all references of inherited constructor (#30514)

* recursively look for inherited constructor references

* add test

* remove outdated comment

* add tests

* move function

* improve tests

* minor refactor

* fix convert params refactoring to deal with inherited constructor calls

* simplify refactor test

Author: gabritto

src/services/findAllReferences.ts

@@ -586,25 +586,28 @@ namespace ts.FindAllReferences.Core {
         }
         else {
             const search = state.createSearch(node, symbol, /*comingFrom*/ undefined, { allSearchSymbols: node ? populateSearchSymbolSet(symbol, node, checker, !!options.isForRename, !!options.providePrefixAndSuffixTextForRename, !!options.implementations) : [symbol] });
-
-            // Try to get the smallest valid scope that we can limit our search to;
-            // otherwise we'll need to search globally (i.e. include each file).
-            const scope = getSymbolScope(symbol);
-            if (scope) {
-                getReferencesInContainer(scope, scope.getSourceFile(), search, state, /*addReferencesHere*/ !(isSourceFile(scope) && !contains(sourceFiles, scope)));
-            }
-            else {
-                // Global search
-                for (const sourceFile of state.sourceFiles) {
-                    state.cancellationToken.throwIfCancellationRequested();
-                    searchForName(sourceFile, search, state);
-                }
-            }
+            getReferencesInContainerOrFiles(symbol, state, search);
         }
 
         return result;
     }
 
+    function getReferencesInContainerOrFiles(symbol: Symbol, state: State, search: Search): void {
+        // Try to get the smallest valid scope that we can limit our search to;
+        // otherwise we'll need to search globally (i.e. include each file).
+        const scope = getSymbolScope(symbol);
+        if (scope) {
+            getReferencesInContainer(scope, scope.getSourceFile(), search, state, /*addReferencesHere*/ !(isSourceFile(scope) && !contains(state.sourceFiles, scope)));
+        }
+        else {
+            // Global search
+            for (const sourceFile of state.sourceFiles) {
+                state.cancellationToken.throwIfCancellationRequested();
+                searchForName(sourceFile, search, state);
+            }
+        }
+    }
+
     function getSpecialSearchKind(node: Node): SpecialSearchKind {
         switch (node.kind) {
             case SyntaxKind.ConstructorKeyword:
@@ -707,7 +710,6 @@ namespace ts.FindAllReferences.Core {
         constructor(
             readonly sourceFiles: ReadonlyArray<SourceFile>,
             readonly sourceFilesSet: ReadonlyMap<true>,
-            /** True if we're searching for constructor references. */
             readonly specialSearchKind: SpecialSearchKind,
             readonly checker: TypeChecker,
             readonly cancellationToken: CancellationToken,
@@ -1293,6 +1295,7 @@ namespace ts.FindAllReferences.Core {
             const classExtending = tryGetClassByExtendingIdentifier(referenceLocation);
             if (classExtending) {
                 findSuperConstructorAccesses(classExtending, pusher());
+                findInheritedConstructorReferences(classExtending, state);
             }
         }
     }
@@ -1325,35 +1328,44 @@ namespace ts.FindAllReferences.Core {
      * Reference the constructor and all calls to `new this()`.
      */
     function findOwnConstructorReferences(classSymbol: Symbol, sourceFile: SourceFile, addNode: (node: Node) => void): void {
-        for (const decl of classSymbol.members!.get(InternalSymbolName.Constructor)!.declarations) {
-            const ctrKeyword = findChildOfKind(decl, SyntaxKind.ConstructorKeyword, sourceFile)!;
-            Debug.assert(decl.kind === SyntaxKind.Constructor && !!ctrKeyword);
-            addNode(ctrKeyword);
-        }
-
-        classSymbol.exports!.forEach(member => {
-            const decl = member.valueDeclaration;
-            if (decl && decl.kind === SyntaxKind.MethodDeclaration) {
-                const body = (<MethodDeclaration>decl).body;
-                if (body) {
-                    forEachDescendantOfKind(body, SyntaxKind.ThisKeyword, thisKeyword => {
-                        if (isNewExpressionTarget(thisKeyword)) {
-                            addNode(thisKeyword);
-                        }
-                    });
-                }
+        const constructorSymbol = getClassConstructorSymbol(classSymbol);
+        if (constructorSymbol) {
+            for (const decl of constructorSymbol.declarations) {
+                const ctrKeyword = findChildOfKind(decl, SyntaxKind.ConstructorKeyword, sourceFile)!;
+                Debug.assert(decl.kind === SyntaxKind.Constructor && !!ctrKeyword);
+                addNode(ctrKeyword);
             }
-        });
+        }
+
+        if (classSymbol.exports) {
+            classSymbol.exports.forEach(member => {
+                const decl = member.valueDeclaration;
+                if (decl && decl.kind === SyntaxKind.MethodDeclaration) {
+                    const body = (<MethodDeclaration>decl).body;
+                    if (body) {
+                        forEachDescendantOfKind(body, SyntaxKind.ThisKeyword, thisKeyword => {
+                            if (isNewExpressionTarget(thisKeyword)) {
+                                addNode(thisKeyword);
+                            }
+                        });
+                    }
+                }
+            });
+        }
+    }
+
+    function getClassConstructorSymbol(classSymbol: Symbol): Symbol | undefined {
+        return classSymbol.members && classSymbol.members.get(InternalSymbolName.Constructor);
     }
 
     /** Find references to `super` in the constructor of an extending class.  */
-    function findSuperConstructorAccesses(cls: ClassLikeDeclaration, addNode: (node: Node) => void): void {
-        const ctr = cls.symbol.members!.get(InternalSymbolName.Constructor);
-        if (!ctr) {
+    function findSuperConstructorAccesses(classDeclaration: ClassLikeDeclaration, addNode: (node: Node) => void): void {
+        const constructor = getClassConstructorSymbol(classDeclaration.symbol);
+        if (!constructor) {
             return;
         }
 
-        for (const decl of ctr.declarations) {
+        for (const decl of constructor.declarations) {
             Debug.assert(decl.kind === SyntaxKind.Constructor);
             const body = (<ConstructorDeclaration>decl).body;
             if (body) {
@@ -1366,6 +1378,17 @@ namespace ts.FindAllReferences.Core {
         }
     }
 
+    function hasOwnConstructor(classDeclaration: ClassLikeDeclaration): boolean {
+        return !!getClassConstructorSymbol(classDeclaration.symbol);
+    }
+
+    function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void {
+        if (hasOwnConstructor(classDeclaration)) return;
+        const classSymbol = classDeclaration.symbol;
+        const search = state.createSearch(/*location*/ undefined, classSymbol, /*comingFrom*/ undefined);
+        getReferencesInContainerOrFiles(classSymbol, state, search);
+    }
+
     function addImplementationReferences(refNode: Node, addReference: (node: Node) => void, state: State): void {
         // Check if we found a function/propertyAssignment/method with an implementation or initializer
         if (isDeclarationName(refNode) && isImplementation(refNode.parent)) {

src/services/refactors/convertParamsToDestructuredObject.ts

@@ -99,7 +99,19 @@ namespace ts.refactor.convertParamsToDestructuredObject {
                     groupedReferences.valid = false;
                     continue;
                 }
-                if (contains(functionSymbols, checker.getSymbolAtLocation(entry.node), symbolComparer)) {
+
+                /* We compare symbols because in some cases find all references wil return a reference that may or may not be to the refactored function.
+                Example from the refactorConvertParamsToDestructuredObject_methodCallUnion.ts test:
+                    class A { foo(a: number, b: number) { return a + b; } }
+                    class B { foo(c: number, d: number) { return c + d; } }
+                    declare const ab: A | B;
+                    ab.foo(1, 2);
+                Find all references will return `ab.foo(1, 2)` as a reference to A's `foo` but we could be calling B's `foo`.
+                When looking for constructor calls, however, the symbol on the constructor call reference is going to be the corresponding class symbol.
+                So we need to add a special case for this because when calling a constructor of a class through one of its subclasses,
+                the symbols are going to be different.
+                */
+                if (contains(functionSymbols, checker.getSymbolAtLocation(entry.node), symbolComparer) || isNewExpressionTarget(entry.node)) {
                     const decl = entryToDeclaration(entry);
                     if (decl) {
                         groupedReferences.declarations.push(decl);

tests/cases/fourslash/findAllRefsOfConstructor.ts

@@ -0,0 +1,27 @@
+/// <reference path="fourslash.ts" />
+
+
+////class A {
+////    [|constructor|](s: string) {}
+////}
+////class B extends A { }
+////class C extends B {
+////    [|constructor|]() {
+////        [|super|]("");
+////    }
+////}
+////class D extends B { }
+////class E implements A { }
+////const a = new [|A|]("a");
+////const b = new [|B|]("b");
+////const c = new [|C|]();
+////const d = new [|D|]("d");
+////const e = new E();
+
+verify.noErrors();
+const [aCtr, cCtr, cSuper, aNew, bNew, cNew, dNew] = test.ranges();
+verify.referenceGroups(aCtr, [
+    { definition: "class A", ranges: [aCtr, aNew] },
+    { definition: "class B", ranges: [cSuper, bNew]},
+    { definition: "class D", ranges: [dNew]}]);
+verify.referenceGroups(cCtr, [{ definition: "class C", ranges: [cCtr, cNew]}]);

tests/cases/fourslash/findAllRefsOfConstructor2.ts

@@ -0,0 +1,25 @@
+/// <reference path="fourslash.ts" />
+
+
+////class A {
+////    [|constructor|](s: string) {}
+////}
+////class B extends A {
+////    [|constructor|]() { [|super|](""); }
+////}
+////class C extends B {
+////    [|constructor|]() {
+////        [|super|]();
+////    }
+////}
+////class D extends B { }
+////const a = new [|A|]("a");
+////const b = new [|B|]();
+////const c = new [|C|]();
+////const d = new [|D|]();
+
+verify.noErrors();
+const [aCtr, bCtr, bSuper, cCtr, cSuper, aNew, bNew, cNew, dNew] = test.ranges();
+verify.referenceGroups(aCtr, [{ definition: "class A", ranges: [aCtr, bSuper, aNew] }]);
+verify.referenceGroups(bCtr, [{ definition: "class B", ranges: [bCtr, cSuper, bNew]}, { definition: "class D", ranges: [dNew]}]);
+verify.referenceGroups(cCtr, [{ definition: "class C", ranges: [cCtr, cNew]}]);

tests/cases/fourslash/findAllRefsOfConstructor_multipleFiles.ts

@@ -0,0 +1,35 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: f.ts
+
+////class A {
+////    [|constructor|](s: string) {}
+////}
+////class B extends A { }
+////export { [|{| "isWriteAccess": true, "isDefinition": true |}A|], [|{| "isWriteAccess": true, "isDefinition": true |}B|] };
+
+// @Filename: a.ts
+
+////import { [|A|] as A1 } from "./f";
+////const a1 = new [|A1|]("a1");
+////export default class extends A1 { }
+////export { [|B|] as [|{| "isWriteAccess": true, "isDefinition": true |}B1|] } from "./f";
+
+// @Filename: b.ts
+
+////import [|B|], { B1 } from "./a";
+////const d = new [|B|]("b");
+////const d1 = new [|B1|]("b1");
+
+verify.noErrors();
+const [aCtr, aExport, bExport, aImport, a1New, bReExport, b1Export, bDefault, bNew, b1New ] = test.ranges();
+verify.referenceGroups(aCtr, [
+    { definition: "class A", ranges: [aCtr, aExport] },
+    { definition: "class B", ranges: [bExport]},
+    { definition: "(alias) class B\nexport B", ranges: [bReExport]},
+    { definition: "(alias) class B1\nexport B1", ranges: [b1Export]},
+    { definition: "(alias) class B1\nimport B1", ranges: [b1New]},
+    { definition: "(alias) class A\nexport A", ranges: [aImport]},
+    { definition: "(alias) class A1\nimport A1", ranges: [a1New]},
+    { definition: "class default", ranges: []},
+    { definition: { text: "import B", range: bDefault }, ranges: [bNew]}]);

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_inheritedConstructor.ts

@@ -8,9 +8,6 @@
 ////var foo = new Foo("c", "d");
 
 goTo.select("a", "b");
-/*  The expected new content is currently wrong.
-    `new Bar("a", "b")` should be modified by the refactor to be `new Bar({ t: "a", s: "b" })`
-*/
 edit.applyRefactor({
     refactorName: "Convert parameters to destructured object",
     actionName: "Convert parameters to destructured object",
@@ -19,6 +16,6 @@ edit.applyRefactor({
     constructor({ t, s }: { t: string; s: string; }) { }
 }
 class Bar extends Foo { }
-var bar = new Bar("a", "b");
+var bar = new Bar({ t: "a", s: "b" });
 var foo = new Foo({ t: "c", s: "d" });`
 });

tests/cases/fourslash/refactorConvertParamsToDestructuredObject_methodCallUnion.ts

@@ -6,9 +6,8 @@
 ////class B {
 ////    foo(c: number, d: number) { return c + d; }
 ////}
-////function foo(ab: A | B) {
-////    return ab.foo(1, 2);
-////}
+////declare var ab: A | B;
+////ab.foo(1, 2);
 
 
 goTo.select("a", "b");
@@ -23,7 +22,6 @@ edit.applyRefactor({
 class B {
     foo(c: number, d: number) { return c + d; }
 }
-function foo(ab: A | B) {
-    return ab.foo(1, 2);
-}`
+declare var ab: A | B;
+ab.foo(1, 2);`
 });