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.

[ghost]

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*/]);

[ghost]

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.

[ghost]

@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).

[ghost]

I think fixing #25533 would fix this.

[sandersn]

Typescript behaviour on this front is so broken that it needs to be fixed before fixing Javascript:

// @Filename: welove.ts
const g = 4567;
export = g;
// @Filename: app.tsx
import good = require('./welove')
console.log(good/*1*/)

Find-all-refs at /*1*/ finds all four references, which is arguable, since g and good have different names. Rename at /*1*/ renames all four references, which seems bad since g and good are not the same name, and welove.ts is likely a d.ts where renaming is going to cause a break.

// @Filename: welove.ts
const g = 4567;
export const good = g;
// @Filename: app.tsx
import { good } from './welove'
console.log(good/*1*/)

Here, find-all-refs at (1) finds the last 3 lines, which seems fine. But it does even with the import is import { good as go }, which seems wrong in the same way as the export= example. Rename is just broken. Renaming at (1) renames the last line, renames the import to import { good as go } and then incorrectly renames export const go = g.

This may be fixed in some version of VS but it's broken in VS Code and emacs. I'll see if there's an existing bug.