Find All Reference from requiring file to module file works only if not default CommonJS exports

[mjbvz]

From @Hoishin on February 28, 2018 4:58

By "default CommonJS exports" I mean module.exports = something, rather than module.exports.foo = something or module.exports = {foo: something}

Might belong to microsoft/vscode#21507, but I thought it is a bit different.

• VSCode Version: 1.20.1
• OS Version: macOS High Sierra Version 10.13.3

Steps to Reproduce:

1. module.exports something

const f = 1234;
module.exports = f;

2. require it

const f = require('./above-file');
console.log(f);

3. The f in console.log doesn't show references across files in Find All References. You can find references across files if you Find All References from the module file.

However,

1. module.exports.foo something

const f = 1234;
module.exports.foo = f;

2. require it

const {foo} = require('./above-file');
console.log(foo);

3. The foo in console.log DOES show reference across files in Find All References. You can find references across files if you Find All References from the module file.

Does this issue occur when all extensions are disabled?: Yes

Copied from original issue: microsoft/vscode#44700

[mhegazy]

The f in console.log doesn't show references across files in Find All References. You can find references across files if you Find All References from the module file.

The bug here is that find all references shows the f in ./above-file. the f there is a different name, and should not be in the reference set.

The foo in console.log DOES show reference across files in Find All References. You can find references across files if you Find All References from the module file.

this makes sense, since you are referencing a property on the module.

In importTracker I added this to isNodeImport:

case SyntaxKind.VariableDeclaration:
    return { isNamedImport: false };

But then it fails at checker.getImmediateAliasedSymbol(symbol); in getImport.
The problem is that const x = require("./x") doesn't give us an alias to the export. It just gives us a variable with the same type as the export from x.
Going forward from the export to the import works because detecting an import is based on the file being imported. But going backwards doesn't work unless we have a way of getting the original symbol.
@rbuckton @sandersn Is there a reason we don't create alias symbols at const x = require("./x") but do at import x = require("./x")?

I tested with:

/// <reference path='fourslash.ts'/>

// @allowJs: true

// @Filename: /a.ts
////function [|{| "isWriteAccess": true, "isDefinition": true |}x|]() {};
////export = [|x|];

// @Filename: /b.js
////const [|{| "isWriteAccess": true, "isDefinition": true |}x|] = require("./a");
////[|x|];

const [r0, r1, r2, r3] = test.ranges();
verify.referenceGroups(r2, [/*todo*/]);

The following test cases crashes currently (with no modifications) because of the failure to find a symbol:

/// <reference path='fourslash.ts'/>

// @allowJs: true

// @Filename: /a.js
////function [|{| "isWriteAccess": true, "isDefinition": true |}x|]() {};
////module.exports = [|x|];

// @Filename: /b.ts
////import [|{| "isWriteAccess": true, "isDefinition": true |}x|] = require("./a");
////[|x|];

const [r0, r1, r2, r3] = test.ranges();
verify.referenceGroups(r2, [/*todo*/]);

[sandersn]

Does #23570 fix this? It makes module.exports= an alias, just like export= is.

@sandersn That fixes the second example, but it looks like const x = require("./a"); is still not an alias. So it will still crash in the first example (given the modification to isNodeImport).