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

* Add test

* Skip shorthand property assignments of module.exports in go-to-definition

* Skip past shorthand property assignments in module.exports in go-to-definition

* Revert WIP change

* Fix comment typo

Co-authored-by: Nathan Shively-Sanders <[email protected]>

Co-authored-by: Nathan Shively-Sanders <[email protected]>

Author: andrewbranch

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`);

src/services/goToDefinition.ts

@@ -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;`.
@@ -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) {
+                    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;

tests/cases/fourslash/goToDefinitionDestructuredRequire1.ts

@@ -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");

tests/cases/fourslash/goToDefinitionDestructuredRequire2.ts

@@ -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");

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");