Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add way to differentiate references for definitions at requesting location #42889

Closed
mjbvz opened this issue Feb 19, 2021 · 6 comments · Fixed by #45920
Closed

Add way to differentiate references for definitions at requesting location #42889

mjbvz opened this issue Feb 19, 2021 · 6 comments · Fixed by #45920
Assignees
Labels
Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Feb 19, 2021

Bug Report

🔎 Search Terms

  • references
  • find all references

Problem

We use the references request to power VS Code's references code lens feature. References returned by TS have an optional isDefinition property that tells if the reference is to a definition or not. We use this to avoid showing multiple references for a case such as:

function foo(a: number): number;
function foo(a: string): string;
function foo(a: string | number): string | number;

The references code lens basically just filtered out any references that are marked isDefinition

However this also prevents us from showing the correct reference count in cases such as:

interface IFoo {
    foo(): void;
}

class Foo implements IFoo {
    foo(): void { }
}

In this case, the two references to foo should have 1 reference, but we show 0 references because both references are definitions.

microsoft/vscode#117021 shows another case with object literal shortcut

Proposal

We'd like a new property on the references response that tells us if the reference belongs to definition from the requesting location (not yet sure what the name of this should be).

For a case with overloads such as:

function foo(a: number): number;
function foo(a: string): string;
function foo(a: string | number): string | number;

If we request references on any of the calls to foo, this new property would be marked true. However it would not be true in cases such as:

export const foo = 123;

console.log({ foo });

or:

interface IFoo {
    foo(): void;
}

class Foo implements IFoo {
    foo(): void { }
}
@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 19, 2021

I'm also open for alternative proposals on how to handle this. The main issue is that can't handle these cases reliably on the VS Code side today for the references code lens, and would like to improve this

@DanielRosenwasser DanielRosenwasser added Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Feb 19, 2021
@DanielRosenwasser
Copy link
Member

Let's chat about this on Wednesday at the next editor sync.

const foo = 123;
console.log({ foo });

This is a great test case, but it also maybe speaks to how we name things here.

@DanielRosenwasser
Copy link
Member

One of the ideas here was to instead add an argument to the find-all-references request, and the language server could do the filtering on its own side. Naming will still be weird/hard here.

@mjbvz
Copy link
Contributor Author

mjbvz commented Mar 30, 2021

I'm going to mark this as a VS Code priority since it is a pretty annoying papercut. We still need to finalize the names in the api but I think agreed that the general concept of this api makes sense

This would be for the 4.4 timeframe, not for the current 4.3 release

@mjbvz mjbvz added the VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone label Mar 30, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.0 milestone Aug 26, 2021
@sandersn
Copy link
Member

Is there any case where the count would be wrong except for overloads? Maybe the solution is to have isDefinition: false for overloads, or all but the last overload for signatures?

@sandersn
Copy link
Member

sandersn commented Sep 1, 2021

From talking about this at the editor sync meeting:

  1. It is difficult (or at least error-prone) for VS Code to identify the self-declaration in the list of references returned. The best solution for Code is
    for Typescript to have a new property isSelfDeclaration which takes a declaration node, and only marks isSelfDeclaration: true for declarations of the symbol (and any overloads).
  2. Nobody thought of any cases besides overloads.
  3. Nobody was sure if/how VS uses isDefinition, or something else. There may be related, or even duplicate code here.
  4. This was discussed previously and concluded

If we just had both isDefinition and isReference, VS Code could accomplish what it wants and no one else would be broken.

I'll think about how hard it is to calculate isReference versus isSelfDeclaration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Symbol Navigation Relates to go-to-definition, find-all-references, highlighting/occurrences. Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript VS Code Priority Critical issues that VS Code needs fixed in the current TypeScript milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants