From c96b472e0b3bf6ed6e3d76c3e8f019a1f43e207c Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Fri, 30 Apr 2021 13:17:59 -0700 Subject: [PATCH] Handle localness in special cases by checking exported variable assignment (#43851) * Handle localness in special cases by checking exported variable assignment Fixes #42976 * Fix existing tests where arrow now behaves similar to function expression * Update src/services/goToDefinition.ts --- src/compiler/utilitiesPublic.ts | 2 +- src/services/goToDefinition.ts | 33 +- .../unittests/tsserver/projectReferences.ts | 299 ++++++++++++++++++ ...mplicitAnyFromCircularInference.errors.txt | 6 +- .../goToDefinitionJsModuleExports.ts | 5 +- .../fourslash/goToDefinitionSignatureAlias.ts | 10 +- .../goToTypeDefinition_returnType.ts | 2 +- 7 files changed, 348 insertions(+), 9 deletions(-) diff --git a/src/compiler/utilitiesPublic.ts b/src/compiler/utilitiesPublic.ts index 6bec2e5f07e..a6ec7bd7617 100644 --- a/src/compiler/utilitiesPublic.ts +++ b/src/compiler/utilitiesPublic.ts @@ -619,7 +619,7 @@ namespace ts { export function getNameOfDeclaration(declaration: Declaration | Expression | undefined): DeclarationName | undefined { if (declaration === undefined) return undefined; return getNonAssignedNameOfDeclaration(declaration) || - (isFunctionExpression(declaration) || isClassExpression(declaration) ? getAssignedName(declaration) : undefined); + (isFunctionExpression(declaration) || isArrowFunction(declaration) || isClassExpression(declaration) ? getAssignedName(declaration) : undefined); } /*@internal*/ diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index 76006e0d53f..d4f317d7543 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -326,10 +326,41 @@ namespace ts.GoToDefinition { sourceFile, FindAllReferences.getContextNode(declaration) ), - isLocal: !checker.isDeclarationVisible(declaration) + isLocal: !isDefinitionVisible(checker, declaration) }; } + function isDefinitionVisible(checker: TypeChecker, declaration: Declaration): boolean { + if (checker.isDeclarationVisible(declaration)) return true; + if (!declaration.parent) return false; + + // Variable initializers are visible if variable is visible + if (hasInitializer(declaration.parent) && declaration.parent.initializer === declaration) return isDefinitionVisible(checker, declaration.parent as Declaration); + + // Handle some exceptions here like arrow function, members of class and object literal expression which are technically not visible but we want the definition to be determined by its parent + switch (declaration.kind) { + case SyntaxKind.PropertyDeclaration: + case SyntaxKind.GetAccessor: + case SyntaxKind.SetAccessor: + case SyntaxKind.MethodDeclaration: + // Private/protected properties/methods are not visible + if (hasEffectiveModifier(declaration, ModifierFlags.Private)) return false; + // Public properties/methods are visible if its parents are visible, so: + // falls through + + case SyntaxKind.Constructor: + case SyntaxKind.PropertyAssignment: + case SyntaxKind.ShorthandPropertyAssignment: + case SyntaxKind.ObjectLiteralExpression: + case SyntaxKind.ClassExpression: + case SyntaxKind.ArrowFunction: + case SyntaxKind.FunctionExpression: + return isDefinitionVisible(checker, declaration.parent as Declaration); + default: + return false; + } + } + function createDefinitionFromSignatureDeclaration(typeChecker: TypeChecker, decl: SignatureDeclaration): DefinitionInfo { return createDefinitionInfo(decl, typeChecker, decl.symbol, decl); } diff --git a/src/testRunner/unittests/tsserver/projectReferences.ts b/src/testRunner/unittests/tsserver/projectReferences.ts index b2ef4d5bff9..c8a86ab44b8 100644 --- a/src/testRunner/unittests/tsserver/projectReferences.ts +++ b/src/testRunner/unittests/tsserver/projectReferences.ts @@ -621,6 +621,305 @@ bar(); checkProjectActualFiles(service.configuredProjects.get(servicesConfig.path)!, [servicesFile.path, servicesConfig.path, libFile.path, typesFile.path, programFile.path]); }); + describe("special handling of localness of the definitions for findAllRefs", () => { + function setup(definition: string, usage: string) { + const solutionLocation = "/user/username/projects/solution"; + const solution: File = { + path: `${solutionLocation}/tsconfig.json`, + content: JSON.stringify({ + files: [], + references: [ + { path: "./api" }, + { path: "./app" }, + ] + }) + }; + const apiConfig: File = { + path: `${solutionLocation}/api/tsconfig.json`, + content: JSON.stringify({ + compilerOptions: { + composite: true, + outDir: "dist", + rootDir: "src", + }, + include: ["src"], + references: [{ path: "../shared" }] + }) + }; + const apiFile: File = { + path: `${solutionLocation}/api/src/server.ts`, + content: `import * as shared from "../../shared/dist"; +${usage}` + }; + const appConfig: File = { + path: `${solutionLocation}/app/tsconfig.json`, + content: apiConfig.content + }; + const appFile: File = { + path: `${solutionLocation}/app/src/app.ts`, + content: apiFile.content + }; + const sharedConfig: File = { + path: `${solutionLocation}/shared/tsconfig.json`, + content: JSON.stringify({ + compilerOptions: { + composite: true, + outDir: "dist", + rootDir: "src", + }, + include: ["src"] + }) + }; + const sharedFile: File = { + path: `${solutionLocation}/shared/src/index.ts`, + content: definition + }; + const host = createServerHost([libFile, solution, libFile, apiConfig, apiFile, appConfig, appFile, sharedConfig, sharedFile]); + const session = createSession(host); + const service = session.getProjectService(); + service.openClientFile(apiFile.path); + verifyApiProjectLoadAndSolutionPending(); + return { session, verifySolutionTreeLoaded, verifyApiAndSharedProjectLoadAndSolutionPending, apiFile, appFile, sharedFile }; + + function checkApiProject() { + const apiProject = service.configuredProjects.get(apiConfig.path)!; + checkProjectActualFiles(apiProject, [libFile.path, apiConfig.path, apiFile.path, sharedFile.path]); + } + function checkAppProject() { + const appProject = service.configuredProjects.get(appConfig.path)!; + checkProjectActualFiles(appProject, [libFile.path, appConfig.path, appFile.path, sharedFile.path]); + } + function checkSharedProject() { + const sharedProject = service.configuredProjects.get(sharedConfig.path)!; + checkProjectActualFiles(sharedProject, [libFile.path, sharedConfig.path, sharedFile.path]); + } + function checkSolutionLoadPending() { + const solutionProject = service.configuredProjects.get(solution.path)!; + assert.isFalse(solutionProject.isInitialLoadPending()); + } + function checkSolutionLoadComplete() { + const solutionProject = service.configuredProjects.get(solution.path)!; + assert.isTrue(solutionProject.isInitialLoadPending()); + } + function verifySolutionTreeLoaded() { + checkNumberOfProjects(service, { configuredProjects: 4 }); + checkApiProject(); + checkAppProject(); + checkSharedProject(); + checkSolutionLoadPending(); + } + + function verifyApiProjectLoadAndSolutionPending() { + checkNumberOfProjects(service, { configuredProjects: 2 }); + checkApiProject(); + checkSolutionLoadComplete(); + } + + function verifyApiAndSharedProjectLoadAndSolutionPending() { + checkNumberOfProjects(service, { configuredProjects: 3 }); + checkApiProject(); + checkSharedProject(); + checkSolutionLoadComplete(); + } + } + + it("when using arrow function assignment", () => { + const { session, apiFile, appFile, sharedFile, verifySolutionTreeLoaded } = setup( + `export const dog = () => { };`, + `shared.dog();` + ); + + // Find all references + const response = session.executeCommandSeq({ + command: protocol.CommandTypes.References, + arguments: protocolFileLocationFromSubstring(apiFile, "dog") + }).response as protocol.ReferencesResponseBody; + assert.deepEqual(response, { + refs: [ + makeReferenceItem({ + file: sharedFile, + text: "dog", + contextText: sharedFile.content, + isDefinition: true, + lineText: sharedFile.content, + }), + makeReferenceItem({ + file: apiFile, + text: "dog", + isDefinition: false, + lineText: "shared.dog();", + }), + makeReferenceItem({ + file: appFile, + text: "dog", + isDefinition: false, + lineText: "shared.dog();", + }) + ], + symbolName: "dog", + symbolStartOffset: protocolLocationFromSubstring(apiFile.content, "dog").offset, + symbolDisplayString: "const dog: () => void" + }); + verifySolutionTreeLoaded(); + }); + + it("when using arrow function as object literal property", () => { + const { session, apiFile, appFile, sharedFile, verifySolutionTreeLoaded } = setup( + `export const foo = { bar: () => { } };`, + `shared.foo.bar();` + ); + + // Find all references + const response = session.executeCommandSeq({ + command: protocol.CommandTypes.References, + arguments: protocolFileLocationFromSubstring(apiFile, "bar") + }).response as protocol.ReferencesResponseBody; + assert.deepEqual(response, { + refs: [ + makeReferenceItem({ + file: sharedFile, + text: "bar", + contextText: `bar: () => { }`, + isDefinition: true, + lineText: sharedFile.content, + }), + makeReferenceItem({ + file: apiFile, + text: "bar", + isDefinition: false, + lineText: "shared.foo.bar();", + }), + makeReferenceItem({ + file: appFile, + text: "bar", + isDefinition: false, + lineText: "shared.foo.bar();", + }) + ], + symbolName: "bar", + symbolStartOffset: protocolLocationFromSubstring(apiFile.content, "bar").offset, + symbolDisplayString: "(property) bar: () => void" + }); + verifySolutionTreeLoaded(); + }); + + it("when using object literal property", () => { + const { session, apiFile, appFile, sharedFile, verifySolutionTreeLoaded } = setup( + `export const foo = { baz: "BAZ" };`, + `shared.foo.baz;` + ); + + // Find all references + const response = session.executeCommandSeq({ + command: protocol.CommandTypes.References, + arguments: protocolFileLocationFromSubstring(apiFile, "baz") + }).response as protocol.ReferencesResponseBody; + assert.deepEqual(response, { + refs: [ + makeReferenceItem({ + file: sharedFile, + text: "baz", + contextText: `baz: "BAZ"`, + isDefinition: true, + lineText: sharedFile.content, + }), + makeReferenceItem({ + file: apiFile, + text: "baz", + isDefinition: false, + lineText: "shared.foo.baz;", + }), + makeReferenceItem({ + file: appFile, + text: "baz", + isDefinition: false, + lineText: "shared.foo.baz;", + }) + ], + symbolName: "baz", + symbolStartOffset: protocolLocationFromSubstring(apiFile.content, "baz").offset, + symbolDisplayString: `(property) baz: string` + }); + verifySolutionTreeLoaded(); + }); + + it("when using method of class expression", () => { + const { session, apiFile, appFile, sharedFile, verifySolutionTreeLoaded } = setup( + `export const foo = class { fly() {} };`, + `const instance = new shared.foo(); +instance.fly();` + ); + + // Find all references + const response = session.executeCommandSeq({ + command: protocol.CommandTypes.References, + arguments: protocolFileLocationFromSubstring(apiFile, "fly") + }).response as protocol.ReferencesResponseBody; + assert.deepEqual(response, { + refs: [ + makeReferenceItem({ + file: sharedFile, + text: "fly", + contextText: `fly() {}`, + isDefinition: true, + lineText: sharedFile.content, + }), + makeReferenceItem({ + file: apiFile, + text: "fly", + isDefinition: false, + lineText: "instance.fly();", + }), + makeReferenceItem({ + file: appFile, + text: "fly", + isDefinition: false, + lineText: "instance.fly();", + }) + ], + symbolName: "fly", + symbolStartOffset: protocolLocationFromSubstring(apiFile.content, "fly").offset, + symbolDisplayString: `(method) foo.fly(): void` + }); + verifySolutionTreeLoaded(); + }); + + it("when using arrow function as object literal property is loaded through indirect assignment with original declaration local to project is treated as local", () => { + const { session, apiFile, sharedFile, verifyApiAndSharedProjectLoadAndSolutionPending } = setup( + `const local = { bar: () => { } }; +export const foo = local;`, + `shared.foo.bar();` + ); + + // Find all references + const response = session.executeCommandSeq({ + command: protocol.CommandTypes.References, + arguments: protocolFileLocationFromSubstring(apiFile, "bar") + }).response as protocol.ReferencesResponseBody; + assert.deepEqual(response, { + refs: [ + makeReferenceItem({ + file: sharedFile, + text: "bar", + contextText: `bar: () => { }`, + isDefinition: true, + lineText: `const local = { bar: () => { } };`, + }), + makeReferenceItem({ + file: apiFile, + text: "bar", + isDefinition: false, + lineText: "shared.foo.bar();", + }), + ], + symbolName: "bar", + symbolStartOffset: protocolLocationFromSubstring(apiFile.content, "bar").offset, + symbolDisplayString: "(property) bar: () => void" + }); + verifyApiAndSharedProjectLoadAndSolutionPending(); + }); + }); + it("when disableSolutionSearching is true, solution and siblings are not loaded", () => { const solutionLocation = "/user/username/projects/solution"; const solution: File = { diff --git a/tests/baselines/reference/implicitAnyFromCircularInference.errors.txt b/tests/baselines/reference/implicitAnyFromCircularInference.errors.txt index d41b3860b83..73665d0bfe5 100644 --- a/tests/baselines/reference/implicitAnyFromCircularInference.errors.txt +++ b/tests/baselines/reference/implicitAnyFromCircularInference.errors.txt @@ -4,7 +4,7 @@ tests/cases/compiler/implicitAnyFromCircularInference.ts(6,5): error TS2502: 'c' tests/cases/compiler/implicitAnyFromCircularInference.ts(9,5): error TS2502: 'd' is referenced directly or indirectly in its own type annotation. tests/cases/compiler/implicitAnyFromCircularInference.ts(14,10): error TS7023: 'g' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. tests/cases/compiler/implicitAnyFromCircularInference.ts(17,5): error TS7023: 'f1' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. -tests/cases/compiler/implicitAnyFromCircularInference.ts(22,10): error TS7024: Function implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. +tests/cases/compiler/implicitAnyFromCircularInference.ts(22,5): error TS7023: 'f2' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. tests/cases/compiler/implicitAnyFromCircularInference.ts(25,10): error TS7023: 'h' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. tests/cases/compiler/implicitAnyFromCircularInference.ts(27,14): error TS7023: 'foo' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. tests/cases/compiler/implicitAnyFromCircularInference.ts(45,9): error TS7023: 'x' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. @@ -45,8 +45,8 @@ tests/cases/compiler/implicitAnyFromCircularInference.ts(45,9): error TS7023: 'x // Error expected var f2 = () => f2(); - ~~~~~~~~~~ -!!! error TS7024: Function implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. + ~~ +!!! error TS7023: 'f2' implicitly has return type 'any' because it does not have a return type annotation and is referenced directly or indirectly in one of its return expressions. // Error expected function h() { diff --git a/tests/cases/fourslash/goToDefinitionJsModuleExports.ts b/tests/cases/fourslash/goToDefinitionJsModuleExports.ts index 43d81085a47..cd367986d10 100644 --- a/tests/cases/fourslash/goToDefinitionJsModuleExports.ts +++ b/tests/cases/fourslash/goToDefinitionJsModuleExports.ts @@ -4,7 +4,10 @@ // @allowJs: true // @Filename: foo.js -////x.test = /*def*/() => { } +////x./*def*/test = () => { } ////x.[|/*ref*/test|](); +////x./*defFn*/test3 = function () { } +////x.[|/*refFn*/test3|](); verify.goToDefinition("ref", "def"); +verify.goToDefinition("refFn", "defFn"); diff --git a/tests/cases/fourslash/goToDefinitionSignatureAlias.ts b/tests/cases/fourslash/goToDefinitionSignatureAlias.ts index 4ace3635cc8..925be86b052 100644 --- a/tests/cases/fourslash/goToDefinitionSignatureAlias.ts +++ b/tests/cases/fourslash/goToDefinitionSignatureAlias.ts @@ -11,14 +11,18 @@ ////[|/*useG*/g|](); ////[|/*useH*/h|](); -////const i = /*i*/() => 0; +////const /*i*/i = () => 0; +////const /*iFn*/iFn = function () { return 0; }; ////const /*j*/j = i; ////[|/*useI*/i|](); +////[|/*useIFn*/iFn|](); ////[|/*useJ*/j|](); -////const o = { m: /*m*/() => 0 }; +////const o = { /*m*/m: () => 0 }; ////o.[|/*useM*/m|](); +////const oFn = { /*mFn*/mFn: function () { return 0; } }; +////oFn.[|/*useMFn*/mFn|](); ////class Component { /*componentCtr*/constructor(props: {}) {} } ////type ComponentClass = /*ComponentClass*/new () => Component; @@ -44,8 +48,10 @@ verify.goToDefinition({ useH: ["h", "f"], useI: "i", + useIFn: "iFn", useJ: ["j", "i"], useM: "m", + useMFn: "mFn", jsxMyComponent: "MyComponent", newMyComponent: ["MyComponent", "componentCtr"], diff --git a/tests/cases/fourslash/goToTypeDefinition_returnType.ts b/tests/cases/fourslash/goToTypeDefinition_returnType.ts index b48dd16b84b..c968f6b6263 100644 --- a/tests/cases/fourslash/goToTypeDefinition_returnType.ts +++ b/tests/cases/fourslash/goToTypeDefinition_returnType.ts @@ -18,7 +18,7 @@ //// ////const f6 = (i: I, j: J, b: boolean) => b ? i : j; //// -////const f7 = /*f7Def*/(i: I) => {}; +////const /*f7Def*/f7 = (i: I) => {}; //// ////function f8(i: I): I; ////function f8(j: J): J;