internal: Do not send inlay hint refresh requests on file edits

[SomeoneToIgnore]

See #13369 (comment)

Editor itself is able to invalidate hints after edits, and /refresh was sent after editor reports changes to the language server. This forces the editor to either query & invalidate the hints twice after every edit, or wait for /refresh to come before querying the hints.

Both options are rather useless, so instead, send a request on server startup only: client editors do not know when the server actually starts up, this will help to query the initial hints after editor was open and the server was still starting up.

[lnicola]

Does this work if you disable cache priming? It feels a bit weird to have them linked.

[Veykril]

Hmm, actually looking at the code here. The previous logic does makse sense. If proc-macros were rebuilt, build scripts reran or the workspace has changed, anything could've changed in the open buffers so we should ask the client to refresh the hints.

I misunderstood and read it as we ask for a refresh on any file change, but that's not what this code path does (if it does hit on every rust source file change thats a bug)

[SomeoneToIgnore]

Thank you for the pointers, I have a better thought at the code, looks like I've got it better now: the server will send a /refresh hint request when it's quiescent (as before), and any of the previous tasks had either reported a procmacro change, or a cargo metadata load in BuildDataProgress::End.

The latter is the best event that matches the "load everything and be ready to respond with inlay hints" state I've found after cache priming: this seem to work for editors that query hints after this /refresh.
Unfortunately, VSCode is still behaving odd: it does receive the /refresh request, but does not query any hints after it.

Otherwise, we now do not react on file edits with /refresh, which was my main goal.

I misunderstood and read it as we ask for a refresh on any file change, but that's not what this code path does (if it does hit on every rust source file change thats a bug)

Yes, that was the case before my changes.

[Veykril]

So, I think I retract my statement that that's a bug actually. I think I know why we might be doing things the way we are doing right now. Edits in one file can affect the inlay hints and similar in another file if we change a struct name for example. If we no longer send out refreshes I imagine those of the other files will become possibly outdated?

[SomeoneToIgnore]

And what if not an editor is there first to register the edit fact, grab all open buffers' hints caches and invalidate them? 🙂
It is also somehow aware which of these file buffers belong to which language server(s), since it has to communicate file changes towards one of them.
I could agree that some silent, background job like macro expansion could trigger inlay hint values and worth re-querying, totally agree that language server startup should issue a /refresh request, but not sure why edits have to issue that.

Let's look from the editor's perspective: it opts into /refresh request with the client capability, and then waits for language servers to respond.
It does not know whether the server will send those /refresh requests at all and when exactly, so when somebody edits text in the editor, what should it do?

• wait N ms for the langserver to send back /refresh after each edit? That means introducing latency on the spot which we can avoid. Also, there's no guarantee that N ms wait time is enough, langserver could be busy and take more time to send another /refresh?
• do not wait and risk re-querying the same thing again? Extra 10_000 json characters per edit (or per series of edits, if we debounce fast typing) is not fun.
• do something even more heuristical like "wait once for /refresh to come and understand whether it's always sent after the edit" but that's odd and might break badly for other langservers?

Neither option seems good or bringing any benefits even if implemented, since simply "do nothing and send no /refresh requests" brings exactly the same result with less effort?
Editors have to query hints nonetheless, since they cannot be sure in /refresh ever coming, so they just have to do it right.
Also, it looks to me that /refresh support is not very widespread in language servers, so editors are already doing that with inlay hints now.

But I might miss something, hence was asking for more details in the original issue.

---

Edits in one file can affect the inlay hints and similar in another file if we change a struct name for example.

Also to note, currently any change, comments included, trigger another /refresh query from r-a.
So even if we follow that path, seems a lot of work to do to make it robust?

[Veykril]

Oh don't get me wrong, this is fine if the editors re-request inlay hints for other files after a change in one particular. I was just wondering, given that the refresh request is so not well documented 😅

I am fine with merging this for now it might just work exactly how we want it to 🤞 (and I definitely agree that us sending so much json data unnecessarily is suboptimal). If it causes issues we can reconsider.

@bors r+

[bors]

📌 Commit 62d1897 has been approved by Veykril

It is now in the queue for this repository.

[bors]

⌛ Testing commit 62d1897 with merge b67606c...

[Veykril]

On a different note, wouldn't the same apply to semantic highlighting? 🤔

[SomeoneToIgnore]

Alas, I'm not aware of anything related to highlighting.
Albeit I miss the unresolvedReference semantic token type which I've used as a cargo-less diagnostic, so one day I hopefully will learn how to query this info from r-a in other editors.

[Veykril]

One day we'll hopefully lint for unresolved paths (unfortunately it is non trivial as path resolution is split across multiple contexts in r-a opposed to rustc (for good reason) 😩)

[bors]

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b67606c to master...