From 698965f4acc7440075c901cf9d11307c97c2bcd4 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 28 Sep 2020 10:27:59 -0700 Subject: [PATCH 1/5] Add test --- src/harness/fourslashImpl.ts | 15 +++++++++++++-- .../goToDefinitionDestructuredRequire.ts | 13 +++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) create mode 100644 tests/cases/fourslash/goToDefinitionDestructuredRequire.ts diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts index a87d0a9e1daa6..41048899e0f0b 100644 --- a/src/harness/fourslashImpl.ts +++ b/src/harness/fourslashImpl.ts @@ -759,7 +759,18 @@ namespace FourSlash { ts.zipWith(endMarkers, definitions, (endMarker, definition, i) => { const marker = this.getMarkerByName(endMarker); if (ts.comparePaths(marker.fileName, definition.fileName, /*ignoreCase*/ true) !== ts.Comparison.EqualTo || marker.position !== definition.textSpan.start) { - this.raiseError(`${testName} failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}`); + const filesToDisplay = ts.deduplicate([marker.fileName, definition.fileName], ts.equateValues); + const markers = [{ text: "EXPECTED", fileName: marker.fileName, position: marker.position }, { text: "ACTUAL", fileName: definition.fileName, position: definition.textSpan.start }]; + const text = filesToDisplay.map(fileName => { + const markersToRender = markers.filter(m => m.fileName === fileName).sort((a, b) => b.position - a.position); + let fileContent = this.getFileContent(fileName); + for (const marker of markersToRender) { + fileContent = fileContent.slice(0, marker.position) + `\x1b[1;4m/*${marker.text}*/\x1b[0;31m` + fileContent.slice(marker.position); + } + return `// @Filename: ${fileName}\n${fileContent}`; + }).join("\n\n"); + + this.raiseError(`${testName} failed for definition ${endMarker} (${i}): expected ${marker.fileName} at ${marker.position}, got ${definition.fileName} at ${definition.textSpan.start}\n\n${text}\n`); } }); } @@ -777,7 +788,7 @@ namespace FourSlash { const fileContent = this.getFileContent(startFile); const spanContent = fileContent.slice(defs.textSpan.start, ts.textSpanEnd(defs.textSpan)); const spanContentWithMarker = spanContent.slice(0, marker.position - defs.textSpan.start) + `/*${startMarkerName}*/` + spanContent.slice(marker.position - defs.textSpan.start); - const suggestedFileContent = (fileContent.slice(0, defs.textSpan.start) + `\x1b[1;4m[|${spanContentWithMarker}|]\x1b[31m` + fileContent.slice(ts.textSpanEnd(defs.textSpan))) + const suggestedFileContent = (fileContent.slice(0, defs.textSpan.start) + `\x1b[1;4m[|${spanContentWithMarker}|]\x1b[0;31m` + fileContent.slice(ts.textSpanEnd(defs.textSpan))) .split(/\r?\n/).map(line => " ".repeat(6) + line).join(ts.sys.newLine); this.raiseError(`goToDefinitionsAndBoundSpan failed. Found a starting TextSpan around '${spanContent}' in '${startFile}' (at position ${defs.textSpan.start}). ` + `If this is the correct input span, put a fourslash range around it: \n\n${suggestedFileContent}\n`); diff --git a/tests/cases/fourslash/goToDefinitionDestructuredRequire.ts b/tests/cases/fourslash/goToDefinitionDestructuredRequire.ts new file mode 100644 index 0000000000000..68c6cb35ca2b3 --- /dev/null +++ b/tests/cases/fourslash/goToDefinitionDestructuredRequire.ts @@ -0,0 +1,13 @@ +/// + +// @allowJs: true + +// @Filename: util.js +//// class /*2*/Util {} +//// module.exports = { Util }; + +// @Filename: index.js +//// const { Util } = require('./util'); +//// new [|Util/*1*/|]() + +verify.goToDefinition("1", "2"); From 5b986f879d4265c17b9b147b5c53e44a22ca5611 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 28 Sep 2020 13:47:14 -0700 Subject: [PATCH 2/5] Skip shorthand property assignments of module.exports in go-to-definition --- src/services/goToDefinition.ts | 14 ++++++++++++++ .../fourslash/goToDefinitionImportedNames11.ts | 4 ++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index f0ec33c88842b..f30a4a7a1c24f 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -92,9 +92,23 @@ namespace ts.GoToDefinition { getDefinitionFromSymbol(typeChecker, propertySymbol, node)); } } + + if (isShorthandPropertyAssignmentOfModuleExports(symbol)) { + const shorthandTarget = typeChecker.resolveName(symbol.name, symbol.valueDeclaration, SymbolFlags.Value, /*excludeGlobals*/ false); + if (shorthandTarget) { + return getDefinitionFromSymbol(typeChecker, shorthandTarget, node); + } + } + return getDefinitionFromSymbol(typeChecker, symbol, node); } + function isShorthandPropertyAssignmentOfModuleExports(symbol: Symbol): boolean { + const shorthandProperty = tryCast(symbol.valueDeclaration, isShorthandPropertyAssignment); + const binaryExpression = tryCast(shorthandProperty?.parent.parent, isAssignmentExpression); + return !!binaryExpression && getAssignmentDeclarationKind(binaryExpression) === AssignmentDeclarationKind.ModuleExports; + } + /** * True if we should not add definitions for both the signature symbol and the definition symbol. * True for `const |f = |() => 0`, false for `function |f() {} const |g = f;`. diff --git a/tests/cases/fourslash/goToDefinitionImportedNames11.ts b/tests/cases/fourslash/goToDefinitionImportedNames11.ts index f34b3543881a6..1eb3494c3a74e 100644 --- a/tests/cases/fourslash/goToDefinitionImportedNames11.ts +++ b/tests/cases/fourslash/goToDefinitionImportedNames11.ts @@ -2,10 +2,10 @@ // @allowjs: true // @Filename: a.js -//// class Class { +//// class /*classDefinition*/Class { //// f; //// } -//// module.exports = { /*classDefinition*/Class }; +//// module.exports = { Class }; // @Filename: b.js ////const { Class } = require("./a"); From 2e25d90eee43fa4448c12094f9a53306838bd5e7 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Tue, 29 Sep 2020 11:29:17 -0700 Subject: [PATCH 3/5] Skip past shorthand property assignments in module.exports in go-to-definition --- src/compiler/checker.ts | 3 +- src/services/goToDefinition.ts | 31 ++++++++++++------- ... => goToDefinitionDestructuredRequire1.ts} | 0 .../goToDefinitionDestructuredRequire2.ts | 17 ++++++++++ 4 files changed, 38 insertions(+), 13 deletions(-) rename tests/cases/fourslash/{goToDefinitionDestructuredRequire.ts => goToDefinitionDestructuredRequire1.ts} (100%) create mode 100644 tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 5658109668c2c..e81b6ad18ba49 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -37284,9 +37284,10 @@ namespace ts { return getSymbolOfNode(node.parent); case SyntaxKind.ImportType: return isLiteralImportTypeNode(node) ? getSymbolAtLocation(node.argument.literal, ignoreErrors) : undefined; - case SyntaxKind.ExportKeyword: return isExportAssignment(node.parent) ? Debug.checkDefined(node.parent.symbol) : undefined; + // case SyntaxKind.ShorthandPropertyAssignment: + // return getSymbolOfNode(node); default: return undefined; diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index f30a4a7a1c24f..c7ba4862ce59b 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -93,13 +93,6 @@ namespace ts.GoToDefinition { } } - if (isShorthandPropertyAssignmentOfModuleExports(symbol)) { - const shorthandTarget = typeChecker.resolveName(symbol.name, symbol.valueDeclaration, SymbolFlags.Value, /*excludeGlobals*/ false); - if (shorthandTarget) { - return getDefinitionFromSymbol(typeChecker, shorthandTarget, node); - } - } - return getDefinitionFromSymbol(typeChecker, symbol, node); } @@ -211,15 +204,29 @@ namespace ts.GoToDefinition { } function getSymbol(node: Node, checker: TypeChecker): Symbol | undefined { - const symbol = checker.getSymbolAtLocation(node); + let symbol = checker.getSymbolAtLocation(node); // If this is an alias, and the request came at the declaration location // get the aliased symbol instead. This allows for goto def on an import e.g. // import {A, B} from "mod"; // to jump to the implementation directly. - if (symbol && symbol.flags & SymbolFlags.Alias && shouldSkipAlias(node, symbol.declarations[0])) { - const aliased = checker.getAliasedSymbol(symbol); - if (aliased.declarations) { - return aliased; + while (symbol) { + if (symbol.flags & SymbolFlags.Alias && shouldSkipAlias(node, symbol.declarations[0])) { + const aliased = checker.getAliasedSymbol(symbol); + if (!aliased.declarations) { + break; + } + symbol = aliased; + } + else if (isShorthandPropertyAssignmentOfModuleExports(symbol)) { + // Skip past `module.export = { Foo }` even though 'Foo' is not a real alias + const shorthandTarget = checker.resolveName(symbol.name, symbol.valueDeclaration, SymbolFlags.Value, /*excludeGlobals*/ false); + if (!some(shorthandTarget?.declarations)) { + break; + } + symbol = shorthandTarget; + } + else { + break; } } return symbol; diff --git a/tests/cases/fourslash/goToDefinitionDestructuredRequire.ts b/tests/cases/fourslash/goToDefinitionDestructuredRequire1.ts similarity index 100% rename from tests/cases/fourslash/goToDefinitionDestructuredRequire.ts rename to tests/cases/fourslash/goToDefinitionDestructuredRequire1.ts diff --git a/tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts b/tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts new file mode 100644 index 0000000000000..2fa27564ff9bb --- /dev/null +++ b/tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts @@ -0,0 +1,17 @@ +/// + +// @allowJs: true + +// @Filename: util.js +//// class /*2*/Util {} +//// module.exports = { Util }; + +// @Filename: reexport.js +//// const { Util } = require('./util'); +//// module.exports = { Util }; + +// @Filename: index.js +//// const { Util } = require('./reexport'); +//// new [|Util/*1*/|]() + +verify.goToDefinition("1", "2"); From 687a662e2c8284ba8efee783f2768ccc70f30575 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Tue, 29 Sep 2020 11:33:50 -0700 Subject: [PATCH 4/5] Revert WIP change --- src/compiler/checker.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e81b6ad18ba49..5658109668c2c 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -37284,10 +37284,9 @@ namespace ts { return getSymbolOfNode(node.parent); case SyntaxKind.ImportType: return isLiteralImportTypeNode(node) ? getSymbolAtLocation(node.argument.literal, ignoreErrors) : undefined; + case SyntaxKind.ExportKeyword: return isExportAssignment(node.parent) ? Debug.checkDefined(node.parent.symbol) : undefined; - // case SyntaxKind.ShorthandPropertyAssignment: - // return getSymbolOfNode(node); default: return undefined; From f07771c7625ec9ba7d4a150a554598fd23540697 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 5 Oct 2020 09:56:43 -0700 Subject: [PATCH 5/5] Fix comment typo Co-authored-by: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> --- src/services/goToDefinition.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index c7ba4862ce59b..de473a79c97e8 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -218,7 +218,7 @@ namespace ts.GoToDefinition { symbol = aliased; } else if (isShorthandPropertyAssignmentOfModuleExports(symbol)) { - // Skip past `module.export = { Foo }` even though 'Foo' is not a real alias + // Skip past `module.exports = { Foo }` even though 'Foo' is not a real alias const shorthandTarget = checker.resolveName(symbol.name, symbol.valueDeclaration, SymbolFlags.Value, /*excludeGlobals*/ false); if (!some(shorthandTarget?.declarations)) { break;