Make isDefinition aware of declaring symbol

[Nathan Shively-Sanders (sandersn)]

After this PR, isDefinition is only true when the declaring symbol is found, and the declaration for the reference's node is one of the symbol's declarations.

This improves the References Codelens, fixing the cases in #42889.
Edit: Previously, I changed isDefinition for commonjs assignment declarations. The current PR now has a simple workaround for this case. I put the old discussion at the bottom.

Remaining work

I still need to update a lot of baselines. However, the change itself is ready to review.

Fixes #42889

Old

One change is with CommonJS aliases:

// @filename: a.js
function f() { }
module.exports.f = f

// @filename: b.js
const { f } = require('./a')
f/**/

Previously module.exports.f = f has isDefinition: true and didn't contribute to f's reference count. Now it has isDefinition: false, and it does.
This is arguable, since it's not a particularly interesting reference, even though it's technically correct. But fixing will involve re-creating the ad-hoc commonjs alias resolution code from the checker. I don't think it's worth it for an edge case like this.

[Nathan Shively-Sanders (sandersn)]

Matt Bierner (@mjbvz) You're probably interested in this too.

Andrew Casey (@amcasey) I tagged you on the review because this changes isDefinition for VS too. However, I think it's a good change -- basically a fix to an incomplete implementation. The only question is whether the old behaviour is critical to VS in some way. (I doubt it.)

[Matt Bierner (mjbvz)]

LGTM

Here are the two places in the VS Code codebase where we use this property:

https://github.com/microsoft/vscode/blob/a4b011697892ab656e1071b42c8af4b192078f28/extensions/typescript-language-features/src/languageFeatures/references.ts#L35

https://github.com/microsoft/vscode/blob/73ae56b82b0fd268cab08e5903666025c1f6c335/extensions/typescript-language-features/src/languageFeatures/codeLens/referencesCodeLens.ts#L44

I think the new behavior makes sense in both instances

[Nathan Shively-Sanders (sandersn)]

Looking over the baselines, I can see that the changes are pretty much universally improvements.

[Rafał Chłodnicki (rchl)]

I've noticed regressions in my tests that tests references returned from the typescript API.

For documents:

1. foo.ts

export function doStuff(): boolean {
    return true;
}

2. bar.ts

import { doStuff } from './foo';
doStuff();

getting references for doStuff(); in bar.ts, one of the returned references is:

    {
      "file": "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/../bar.ts",
      "start": {
        "line": 1,
        "offset": 10
      },
      "end": {
        "line": 1,
        "offset": 17
      },
      "contextStart": {
        "line": 1,
        "offset": 1
      },
      "contextEnd": {
        "line": 1,
        "offset": 33
      },
      "lineText": "import { doStuff } from './foo';",
      "isWriteAccess": true,
      "isDefinition": true
    },

where isDefinition is true while before 4.5.2 it was false.

Is that an intended change?

It seems to be problematic for the feature that I have that allows findings all callers of given symbol. With that change the import statement is included as a caller which doesn't seem right.

[Rafał Chłodnicki (rchl)]

No comments about whether this is intended changed or not?

[Andrew Casey (amcasey)]

ping Nathan Shively-Sanders (@sandersn)

[Nathan Shively-Sanders (sandersn)]

Yes, that change is intended. An import is not one of the declarations of doStuff in your example. Find-all-callers should be a subset of find-all-references -- [doStuff] is another example of a non-call reference that would have been included even before this change.

[Rafał Chłodnicki (rchl)]

OK, thanks for clarifying.