Improve reference finding for imported variables

[jkillian]

We ran into a TSLint issue where used, imported, variables were being marked as unused because the TS language services weren't finding all references correctly. In the example below, b is marked as used but not c or d.

let a;

import {b} from "mod1";
a = {b};

import c from "mod2";
a = {c};

import * as d from "mod3";
a = {d};

I spent a little bit debugging the issue a little, and it seemed to be that for some ES6 style imports, the symbol the import aliases is added to the search set and thus a match is found. But for every other kind of import, it is not and this sometimes causes problems.

This PR seems to fix the issue for ES6-style imports. I think this is all related to the work @mhegazy did at #2126. I don't have full confidence that this PR is doing things the right way, but wanted to put it out here for discussion.

A similar issue still persists for import e = require("mod4"); type imports, but when I expanded my changes to include them, it broke the referencesForAmbients and referencesForImports tests.

[DanielRosenwasser]

Since you only use kind, can you just declare a variable named kind or destructure it from the parameter?

[jkillian]

👍 Fixed and amended to the last commit

[DanielRosenwasser]

@mhegazy, since you wrote the original code, do you feel this change is sufficient?

[mhegazy]

I do not think this is the right approch.. it is a bit subtle.. what we are doing here is trying to associate one reference location to another using symbols. since a single entity can have multiple symbols (aliases, imports, exports, and shorthand properties, through contextual types... etc...), we build a list of search symbols that then we consult to know if the reference we are looking at relates to any of them.

The change above will add the target symbol of all import/export specifier to the search set. (i.e. the symbol of the actual declaration in the source module). This will solve the reported issue, as shown in the test, but it include additional symbols that might be surprising.. e.g.:

// file1.d.ts
declare namespace N {
     export var x: number;
}

declare module "M" {
    export = N;
}

// file2.ts

import * as N from "M"; 

N; // Renaming N here would rename the namespace "N" above, 
   // which would not be expected, and is not correct as it is a public API, 
   // what you rely want to rename is the local import N, not all references to the namespace N.

The core of the issue here, i think, is in the shorthand property declaration handling. when we are relating symbols, we look for the symbol of the shorthand property points to by searching for the name at that location, see https://github.com/Microsoft/TypeScript/blob/master/src/compiler/checker.ts#L15010

this will follow aliases, and will return us the symbol of the actual declaration, and not the symbol of the local import, and this is why it is missed, and i think this can be fixed by adding SymbolFlags.Alias to the resolveEntity call to stop it from resolving aliases.

the subtle part is why import and export specifiers are handled specially, i.e. including the target symbol. the idea was if you rename x in export {x} from "a" you rely want to rename this x, the declaration of x in module "a" and any other export {x} from "a" references in other files. so for these we include the target symbol. that does not apply to for default or namespace imports as again they only represent local name bindings and should not change their target.

I think i can put out a PR with a change to the shorthandpropety lookup shortelly

[mhegazy]

so ran into another issue with exports as well (see #6376), similar to shorthand properties, i believe they should be handled specially.

[jkillian]

Thanks for the explanation @mhegazy - very helpful! I'll close this as I don't think it's needed now with #6376 (nor is this one correct 😉 )