diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index ef6a344c99218..147fee53393c1 100644 --- a/src/services/findAllReferences.ts +++ b/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, readonly sourceFilesSet: ReadonlyMap, - /** 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 = (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 = (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 = (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)) { diff --git a/src/services/refactors/convertParamsToDestructuredObject.ts b/src/services/refactors/convertParamsToDestructuredObject.ts index 91836f298217b..3453adba5b304 100644 --- a/src/services/refactors/convertParamsToDestructuredObject.ts +++ b/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); diff --git a/tests/cases/fourslash/findAllRefsOfConstructor.ts b/tests/cases/fourslash/findAllRefsOfConstructor.ts new file mode 100644 index 0000000000000..08846380749f0 --- /dev/null +++ b/tests/cases/fourslash/findAllRefsOfConstructor.ts @@ -0,0 +1,27 @@ +/// + + +////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]}]); \ No newline at end of file diff --git a/tests/cases/fourslash/findAllRefsOfConstructor2.ts b/tests/cases/fourslash/findAllRefsOfConstructor2.ts new file mode 100644 index 0000000000000..17991cb555c81 --- /dev/null +++ b/tests/cases/fourslash/findAllRefsOfConstructor2.ts @@ -0,0 +1,25 @@ +/// + + +////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]}]); \ No newline at end of file diff --git a/tests/cases/fourslash/findAllRefsOfConstructor_multipleFiles.ts b/tests/cases/fourslash/findAllRefsOfConstructor_multipleFiles.ts new file mode 100644 index 0000000000000..a20d9fc1f29ec --- /dev/null +++ b/tests/cases/fourslash/findAllRefsOfConstructor_multipleFiles.ts @@ -0,0 +1,35 @@ +/// + +// @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]}]); diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_inheritedConstructor.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_inheritedConstructor.ts index 6e27c33078a14..70a5d2022b5d9 100644 --- a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_inheritedConstructor.ts +++ b/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" });` }); \ No newline at end of file diff --git a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_methodCallUnion.ts b/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_methodCallUnion.ts index 031b50dc45e58..fca08439b6d95 100644 --- a/tests/cases/fourslash/refactorConvertParamsToDestructuredObject_methodCallUnion.ts +++ b/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);` }); \ No newline at end of file