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

Inlay hints should be requested on all open files (similarly to diagnostics) #130430

Closed
DanielRosenwasser opened this issue Aug 9, 2021 · 11 comments
Assignees
Labels
api feature-request Request for new features or functionality inlay-hints typescript Typescript support issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 9, 2021

Issue Type: Bug

Today, VS Code seems to request inlay hints for the most-recently edited (or currently focused?) file; however, inlay hints can be affected by edits in other files. That means you can end up in situations where 2 files can be edited side-by-side where one ends up with stale inlay hints.

image

VS Code version: Code - Insiders 1.60.0-insider (49af1cb, 2021-08-06T06:41:50.275Z)
OS version: Windows_NT x64 10.0.19043
Restricted Mode: No

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 10, 2021

@jrieken To support this on the TypeScript side, I believe we either need:

  • Make VS Code call provideInlayHints for all visible files when one file changes

  • Have onDidChangeInlayHints take a uri so that TypeScript can fire this event for all visible files

@mjbvz mjbvz added inlay-hints typescript Typescript support issues labels Aug 10, 2021
@DanielRosenwasser
Copy link
Member Author

I should mention that semantic diagnostics probably has the same issue.

@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Sep 1, 2021
@jrieken
Copy link
Member

jrieken commented Sep 1, 2021

Have onDidChangeInlayHints take a uri so that TypeScript can fire this event for all visible files

Yeah, I believe that's the most reasonable thing to do. Unsure about requesting inlay hint for all files tho

@jrieken
Copy link
Member

jrieken commented Sep 24, 2021

There already is InlayHintsProvider#onDidChangeInlayHints. No plans to refresh all editors when the any model changes

@jrieken jrieken closed this as completed Sep 24, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 27, 2021

@jrieken Can this event take an optional resource uri? Otherwise I have to add extra code to avoid calculating the hints twice

If I have file A and file B open for example:

  1. File A changes -> hints requested by VS Code
  2. TS extension fires onDidChangeInlayHints because multiple files are visible
  3. VS Code requests hints for all visible files
  4. But we only really want to compute the hints from file B here since we already handled file A in step 1

@mjbvz mjbvz added this to the October 2021 milestone Sep 27, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 27, 2021

Reopening to track fix for JS/TS specifically

@mjbvz mjbvz reopened this Sep 27, 2021
@mjbvz mjbvz added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Sep 27, 2021
@jrieken
Copy link
Member

jrieken commented Sep 28, 2021

Yeah, adding an optional uri event type should be easy.

@jrieken
Copy link
Member

jrieken commented Oct 8, 2021

Re-opening this because for semantic tokens we have discusses second thought on allowing extensions to tell out one or more uris. Allowing to send uris add extra burden to extensions and/or language servers with little win. The implementation in the typescript extension is almost the same as sending the global event, except for omitting the current file. For that we have come up with the following ideas:

  • Inlay hints (also code lens, semantic token etc) will ignore events for models for which a request is pending. Instead, providers must always respond with the most up-to-date data.
  • With that there is no more need for spelling out uris. In fact we avoid the invitation for computational complexity, e.g to truly know what inlay hints depend on what changes is too expensive to pay off, esp if we are going to ignore the event anyways.
  • (optionally) we can consider to expose EmitterOption#onFirstListener. That would allow extensions to optimize this further by use letting them know if there is any listener or not

@jrieken jrieken added feature-request Request for new features or functionality api and removed bug Issue identified by VS Code Team member as probable bug labels Oct 8, 2021
@jrieken jrieken closed this as completed Oct 11, 2021
@jrieken
Copy link
Member

jrieken commented Oct 11, 2021

fixed with 5dcc080

@jrieken jrieken added the verification-needed Verification of issue is requested label Oct 25, 2021
@jrieken
Copy link
Member

jrieken commented Oct 25, 2021

Verify that there is the option for InlayHintsProvider#onDidChangeInlayHints

@rchiodo rchiodo added the verified Verification succeeded label Oct 26, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Oct 26, 2021

/verified

image

@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api feature-request Request for new features or functionality inlay-hints typescript Typescript support issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@DanielRosenwasser @jrieken @mjbvz @rchiodo and others