Skip to content

Skip past module.exports = { Foo } in go-to-defintion on 'Foo' #40835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
}
});
}
Expand All @@ -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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look to see colour markup in error messages

.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`);
Expand Down
31 changes: 26 additions & 5 deletions src/services/goToDefinition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,16 @@ namespace ts.GoToDefinition {
getDefinitionFromSymbol(typeChecker, propertySymbol, 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;`.
Expand Down Expand Up @@ -197,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does this happen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just the converse of the previous code, but I think it could happen in certain error cases where the checker falls back to unknownSymbol. Not sure if that’s actually possible in getAliasedSymbol, but I feel like our assumption that symbols always have declarations is a frequent source of crashes, particularly in error-ridden code.

break;
}
symbol = aliased;
}
else if (isShorthandPropertyAssignmentOfModuleExports(symbol)) {
// 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;
}
symbol = shorthandTarget;
}
else {
break;
}
}
return symbol;
Expand Down
13 changes: 13 additions & 0 deletions tests/cases/fourslash/goToDefinitionDestructuredRequire1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/// <reference path="fourslash.ts" />

// @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");
17 changes: 17 additions & 0 deletions tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />

// @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");
4 changes: 2 additions & 2 deletions tests/cases/fourslash/goToDefinitionImportedNames11.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down