From 166f4de4fb4c6ba9879b0c021dbc6cf4c4c7a98a Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Wed, 20 Mar 2019 11:01:56 -0700 Subject: [PATCH 1/9] recursively look for inherited constructor references --- src/services/findAllReferences.ts | 104 +++++++++++++++++++----------- 1 file changed, 68 insertions(+), 36 deletions(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index ef6a344c99218..b5a06ec960c7a 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: @@ -1293,10 +1296,26 @@ namespace ts.FindAllReferences.Core { const classExtending = tryGetClassByExtendingIdentifier(referenceLocation); if (classExtending) { findSuperConstructorAccesses(classExtending, pusher()); + findInheritedConstructorReferences(classExtending, state); + // TODO: check if class doesn't override constructor + // TODO: get class symbol + // TODO: construct seach with `state.createSearch` + // TODO: figure out what getReferences function to call } } } + function hasOwnConstructor(classDeclaration: ClassLikeDeclaration): boolean { + return !!getClassConstructorSymbol(classDeclaration.symbol); + } + + function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void { // TODO: move this + if (hasOwnConstructor(classDeclaration)) return undefined; + const classSymbol = classDeclaration.symbol; + const search = state.createSearch(/*location*/ undefined, classSymbol, /*comingFrom*/ undefined); + getReferencesInContainerOrFiles(classSymbol, state, search); + } + function addClassStaticThisReferences(referenceLocation: Node, search: Search, state: State): void { addReference(referenceLocation, search.symbol, state); const classLike = referenceLocation.parent; @@ -1325,35 +1344,48 @@ 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 { + const classMembers = classSymbol.members; + if (classMembers) { + return classMembers.get(InternalSymbolName.Constructor); + } + return undefined; } /** 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) { From 129276de0fb6f8b35117630f0fb01003439619c3 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Wed, 20 Mar 2019 11:14:32 -0700 Subject: [PATCH 2/9] add test --- tests/cases/fourslash/findAllRefsOfConstructor.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 tests/cases/fourslash/findAllRefsOfConstructor.ts diff --git a/tests/cases/fourslash/findAllRefsOfConstructor.ts b/tests/cases/fourslash/findAllRefsOfConstructor.ts new file mode 100644 index 0000000000000..f262a84c14e09 --- /dev/null +++ b/tests/cases/fourslash/findAllRefsOfConstructor.ts @@ -0,0 +1,11 @@ +/// + +////class A { +//// [|constructor|]() {} +////} +////class B extends A { } +////var b = new [|B|](); +////var x = new [|A|](); + +const [aCtr, bNew, aNew] = test.ranges(); +verify.referenceGroups(aCtr, [{ definition: "class A", ranges: [aCtr, aNew] }, { definition: "class B", ranges: [bNew]}]); From 768af25f84b7ccbd2ad27bc8469cf4fc928f7db5 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Wed, 20 Mar 2019 13:54:09 -0700 Subject: [PATCH 3/9] remove outdated comment --- src/services/findAllReferences.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index b5a06ec960c7a..27693cfe4fd14 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -710,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, From a482f66220352bf36aa2678137f50d9fe7d5a800 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Wed, 20 Mar 2019 15:53:34 -0700 Subject: [PATCH 4/9] add tests --- .../fourslash/findAllRefsOfConstructor.ts | 28 +++++++++++---- .../findAllRefsOfConstructor_multipleFiles.ts | 35 +++++++++++++++++++ 2 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 tests/cases/fourslash/findAllRefsOfConstructor_multipleFiles.ts diff --git a/tests/cases/fourslash/findAllRefsOfConstructor.ts b/tests/cases/fourslash/findAllRefsOfConstructor.ts index f262a84c14e09..73cf4873156c8 100644 --- a/tests/cases/fourslash/findAllRefsOfConstructor.ts +++ b/tests/cases/fourslash/findAllRefsOfConstructor.ts @@ -1,11 +1,27 @@ /// +// @Filename: f.ts + ////class A { -//// [|constructor|]() {} +//// [|constructor|](s: string) {} +////} +////class B extends A { } +////class C extends B { +//// constructor() { +//// [|super|](""); +//// } ////} -////class B extends A { } -////var b = new [|B|](); -////var x = new [|A|](); +////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(); -const [aCtr, bNew, aNew] = test.ranges(); -verify.referenceGroups(aCtr, [{ definition: "class A", ranges: [aCtr, aNew] }, { definition: "class B", ranges: [bNew]}]); +verify.noErrors(); +const [aCtr, cSuper, aNew, bNew, dNew] = test.ranges(); +verify.referenceGroups(aCtr, [ + { definition: "class A", ranges: [aCtr, aNew] }, + { definition: "class B", ranges: [cSuper, bNew]}, + { definition: "class D", ranges: [dNew]}]); 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]}]); From 17616accd743364d7a0a809a292d464d2f8af35c Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Wed, 20 Mar 2019 16:34:24 -0700 Subject: [PATCH 5/9] move function --- src/services/findAllReferences.ts | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 27693cfe4fd14..0e85f47c8adec 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -1296,25 +1296,10 @@ namespace ts.FindAllReferences.Core { if (classExtending) { findSuperConstructorAccesses(classExtending, pusher()); findInheritedConstructorReferences(classExtending, state); - // TODO: check if class doesn't override constructor - // TODO: get class symbol - // TODO: construct seach with `state.createSearch` - // TODO: figure out what getReferences function to call } } } - function hasOwnConstructor(classDeclaration: ClassLikeDeclaration): boolean { - return !!getClassConstructorSymbol(classDeclaration.symbol); - } - - function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void { // TODO: move this - if (hasOwnConstructor(classDeclaration)) return undefined; - const classSymbol = classDeclaration.symbol; - const search = state.createSearch(/*location*/ undefined, classSymbol, /*comingFrom*/ undefined); - getReferencesInContainerOrFiles(classSymbol, state, search); - } - function addClassStaticThisReferences(referenceLocation: Node, search: Search, state: State): void { addReference(referenceLocation, search.symbol, state); const classLike = referenceLocation.parent; @@ -1397,6 +1382,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 undefined; + 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)) { From 6c7de67d2de86e5c29e231cdbaf42898aba96fb8 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 21 Mar 2019 14:47:18 -0700 Subject: [PATCH 6/9] improve tests --- .../fourslash/findAllRefsOfConstructor.ts | 8 +++--- .../fourslash/findAllRefsOfConstructor2.ts | 25 +++++++++++++++++++ ...DestructuredObject_inheritedConstructor.ts | 5 +--- 3 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 tests/cases/fourslash/findAllRefsOfConstructor2.ts diff --git a/tests/cases/fourslash/findAllRefsOfConstructor.ts b/tests/cases/fourslash/findAllRefsOfConstructor.ts index 73cf4873156c8..08846380749f0 100644 --- a/tests/cases/fourslash/findAllRefsOfConstructor.ts +++ b/tests/cases/fourslash/findAllRefsOfConstructor.ts @@ -1,13 +1,12 @@ /// -// @Filename: f.ts ////class A { //// [|constructor|](s: string) {} ////} ////class B extends A { } ////class C extends B { -//// constructor() { +//// [|constructor|]() { //// [|super|](""); //// } ////} @@ -15,13 +14,14 @@ ////class E implements A { } ////const a = new [|A|]("a"); ////const b = new [|B|]("b"); -////const c = new C(); +////const c = new [|C|](); ////const d = new [|D|]("d"); ////const e = new E(); verify.noErrors(); -const [aCtr, cSuper, aNew, bNew, dNew] = test.ranges(); +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/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 From b1ba80d0575fc6a80bd9a808c85b43f7d5555b68 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 21 Mar 2019 14:50:42 -0700 Subject: [PATCH 7/9] minor refactor --- src/services/findAllReferences.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 0e85f47c8adec..147fee53393c1 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -1355,11 +1355,7 @@ namespace ts.FindAllReferences.Core { } function getClassConstructorSymbol(classSymbol: Symbol): Symbol | undefined { - const classMembers = classSymbol.members; - if (classMembers) { - return classMembers.get(InternalSymbolName.Constructor); - } - return undefined; + return classSymbol.members && classSymbol.members.get(InternalSymbolName.Constructor); } /** Find references to `super` in the constructor of an extending class. */ @@ -1387,7 +1383,7 @@ namespace ts.FindAllReferences.Core { } function findInheritedConstructorReferences(classDeclaration: ClassLikeDeclaration, state: State): void { - if (hasOwnConstructor(classDeclaration)) return undefined; + if (hasOwnConstructor(classDeclaration)) return; const classSymbol = classDeclaration.symbol; const search = state.createSearch(/*location*/ undefined, classSymbol, /*comingFrom*/ undefined); getReferencesInContainerOrFiles(classSymbol, state, search); From 5e20db964ade547b856ceae5b4e26f2afced63b0 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 21 Mar 2019 15:22:31 -0700 Subject: [PATCH 8/9] fix convert params refactoring to deal with inherited constructor calls --- .../refactors/convertParamsToDestructuredObject.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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); From 7e636e796f2c34d82c2ed2845a9f3fe8012b7392 Mon Sep 17 00:00:00 2001 From: Gabriela Araujo Britto Date: Thu, 21 Mar 2019 15:22:51 -0700 Subject: [PATCH 9/9] simplify refactor test --- ...onvertParamsToDestructuredObject_methodCallUnion.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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