vscode: redesign inlay hints to be capable of handling multiple editors for one source file #3378

Veetaha · 2020-02-29T18:26:45Z

Fixes: #3008 (inlay corruption with multiple editors for one file).
Fixes: #3319 (unnecessary requests for inlay hints when switching unrelated source files or output/log/console windows)
Also, I don't know how, but the problem described in #3057 doesn't appear for me anymore (maybe it was some fix on the server-side, idk), the inlay hints are displaying right away. Though the last time I checked this it was caused by the server returning an empty array of hints and responding with a very big latency, I am not sure that this redesign actually fixed #3057....

We didn't handle the case when one rust source file is open in multiple editors in vscode (e.g. by manually adding another editor for the same file or by opening an inline git diff view or just any kind of preview within the same file).

The git diff preview is actually quite special because it causes memory leaks in vscode (microsoft/vscode#91782). It is not removed from visibleEditors once it is closed. However, this bug doesn't affect the inlay hints anymore, because we don't issue a request and set inlay hints for each editor in isolation. Editors are grouped by their respective files and we issue requests only for files and then update all duplicate editors using the results (so we just update the decorations for already closed git diff preview read-only editors).

Also, note on a hack I had to use. vscode.TextEdtior identity is not actually public, its id field is not exposed to us. I created a dedicated upstream issue for this (microsoft/vscode#91788).

Regarding #3319: the newly designed hints client doesn't issue requests for type hints when switching the visible editors if it has them already cached (though it does rerender things anyway, but this could be optimized in future if so wanted).

Before

After

Veetaha · 2020-02-29T18:27:06Z

@matklad , @lnicola I still cannot add reviewers 😢

Veetaha · 2020-02-29T18:49:47Z

cc @SomeoneToIgnore

matklad · 2020-02-29T20:35:10Z

editors/code/src/inlay_hints.ts

+            if (contentChanges.length === 0) return;
+            if (!isRustTextDocument(document)) return;
+
+            hintsUpdater.refreshRustDocument(document);


We should refresh all visible editors: change to one file might affect hints in another file.

Done! 995ad75

matklad · 2020-02-29T20:36:48Z

editors/code/src/inlay_hints.ts

+            // FIXME: ctx.config may have not been refreshed at this point of time, i.e.
+            // it's on onDidChangeConfiguration() handler may've not executed yet
+            // (order of invokation is unspecified)
+            // To fix this we should expose an event emitter from our `Config` itself.


I guess a better approach is to just make ctx.config always reflect the most recent config? Ie, we should rid of config.onDidChange handler.

Yeah, we will get rid of cache

Though, we won't get rid of config.onDidChange, since it triggerses (option requires reload) message and updates logging setEnabled(). Switching to event emitter for config is actually very simple, vscode exposes vscode.EventEmitter for this.

matklad · 2020-02-29T20:45:12Z

editors/code/src/inlay_hints.ts

-        this.setParameterDecorations(editor, newParameterDecorations);
+/**
+ * This class encapsulates a map of file uris to respective inlay hints
+ * request cancellation token source (cts) and an array of editors.


Haven't read into this yet, but I think it should be possible to solve this without storing editors. I would think that the current queryHints logic is enough here, it should handle per-document in-flight requests correctly (modulo bugs).

It seems it should be enought to tweak refresh function logic such that it first groups editors by the document they are displaying, and then refreshes each group, without maintaing any additional state or datastructures.

The state keeps track of editors with already rendered inlay hints (some cache to prevent requests when opening duplicate editors) and prevents interaction with disposed editors (see
this line) (vscode issues a warning for this otherwise).

And it also prevents requests for inlay hints when the user browses unrelated files (i.e. #3319), windows (output/console/preview windows etc.)

Would the following work as a way to get rid of the extra state here?

class RustSourceFile { constructor( public readonly document: vscode.TextDocument, public inlaysRequest: null | vscode.CancellationTokenSource = null, public decorations: null | { type: vscode.DecorationOptions[]; param: vscode.DecorationOptions[]; } = null ) { } get editors(): vscode.TextEditor[] { return vscode.window.visibleTextEditors.filter((it) => it.document.uri.toString == this.document.uri.toString) } }

This will remove the extra state, though we will looze the info about which editors have decorations applied and which don't. So removing the state like this we will still rerender the decorations while the user is browsing editors/outputs/terminal widnows, but we will at least have decorations cache in order not to issue requests for inlay hints to the lsp server.

I can think of dirty hack where we add boolean properties to editors to put the state in them instead of our custom map.

I can think of dirty hack where we add boolean property areDecorationsApplied to editors to put the state in them instead of our custom map.

Or better use a symbol. This will do work and maybe would be more efficient in terms of performance. However, the stability of a reference to the editor (i.e. pointer to it) is not documented by vscode, though in practice we can rely on this.

I think the following example will make this more clear:

const IS_DECORATED = Symbol("inlay hints decorations"); get editorsToRender() { return vscode.window.visibleTextEditrs.fitler( suspect => !suspect[IS_DECORATED] && <uris equal>... ); } rerender(editor) { editor.setDecorations(...); editor[IS_DECORATED] = true; }

matklad · 2020-03-02T19:54:45Z

Why is **just** applying the decorations bad?

…

On Monday, 2 March 2020, Veetaha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In editors/code/src/inlay_hints.ts <#3378 (comment)> : > - this.setTypeDecorations(editor, newTypeDecorations); - - const newParameterDecorations = newHints - .filter(hint => hint.kind === ra.InlayKind.ParameterHint) - .map(hint => ({ - range: this.ctx.client.protocol2CodeConverter.asRange(hint.range), - renderOptions: { - before: { - contentText: `${hint.label}: `, - }, - }, - })); - this.setParameterDecorations(editor, newParameterDecorations); +/** + * This class encapsulates a map of file uris to respective inlay hints + * request cancellation token source (cts) and an array of editors. I think the following example will make this more clear: const DEC = Symbol("decorations"); get editorToRender() { return vscode.window.visibleTextEditrs.fitler(suspect => suspect[DEC] && <uris equal>...) } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3378>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M23ZJTGJR2GLNYEYW3RFQBHPANCNFSM4K64VUWA> .

Veetaha · 2020-03-02T20:07:45Z

It's "bad" in terms of rerendering inlays decorations when not modifying the code, but it's "good" in terms of not storing the state and, anyway, inlay hints are updated on each character typed, so rerendering them on browsing unrelated editors would be shadowed by this fact.
I wish vscode guys took over the implementation themselves for us here, so that they could access their private APIs and this state could already be accessible by them...

matklad · 2020-03-02T20:22:43Z

I would expect setting decorations to be a very cheap operation, especially in comparisons to IPC we do to get this infor in the first place, and to actually type inference we do to compute it

…

On Mon, 2 Mar 2020 at 21:07, Veetaha ***@***.***> wrote: It's *"bad"* in terms of rerendering inlays decorations when not modifying the code, but it's *"good"* in terms of not storing the state and, anyway, inlay hints are updated on each character typed, so rerendering them on browsing unrelated editors would be shadowed by this fact. I wish vscode guys took over the implementation themselves for us here, so that they could access their private APIs and this state could already be accessible by them... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3378>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M63BBRDK34SUSKX45DRFQGZFANCNFSM4K64VUWA> .

Veetaha · 2020-03-02T20:49:30Z

Don't forget that the extension communicates with the editor via IPC too, anyway. Let me try to simplify this with a boolean-per-editor and if you don't like it we will remove the state at all...

matklad · 2020-03-03T13:44:49Z

Let me try to simplify this with a boolean-per-editor and if you don't like it we will remove the state at all...

I pretty strongly feel that any state management is premature optimization, and a non-insignificant amount of complexity.

3388: Remove inlay hint in diff views r=matklad a=vbfox If the left side of a diff view that contain the old version of the file apply inlays they are misplaced and produce a weird display: ![image](https://user-images.githubusercontent.com/131878/75628802-b1ac1900-5bdc-11ea-8c26-6722d8e38371.png) After the change: ![image](https://user-images.githubusercontent.com/131878/75628831-e91ac580-5bdc-11ea-9039-c6b4ffbdb2be.png) The detection is done by blacklisting the url schemes used by git and subversion scm extensions, whitelisting `file` is also possible but neither is perfect as VSCode now support both pluggable scm extensions and pluggable remote filesystems. But I suspect that the list of scm extensions is more easily manageable. **Note**: I can rebase on #3378 if needed as it touches the same lines of code Co-authored-by: Julien Roncaglia <julien@roncaglia.fr>

kjeremy · 2020-03-03T20:04:08Z

@Veetaha this may be off topic (I haven't really looked at how this feature works) but have you looked at CodeLensProvider and friends?

My motivation is something "easily digestible" for #2797

Veetaha · 2020-03-03T20:51:31Z

@kjeremy, haven't looked into it. I am not sure I caught your intent, do you mean we should make changes to the LSP server side first before merging this PR?

kjeremy · 2020-03-03T21:16:51Z

@Veetaha what I mean is that the code lense feature is similar to our inlay hints feature and already supported by vscode. We should see how the client side typescript solves the cases that we also need to solve.

caveat: I'm not terribly familiar with the ts side of inlay hints so could be completely off the mark :)

…(needed further)

matklad · 2020-03-07T12:21:25Z

r=me

matklad · 2020-03-07T12:21:28Z

bors d+

bors · 2020-03-07T12:21:29Z

✌️ Veetaha can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Veetaha · 2020-03-07T12:21:42Z

@kjeremy I've looked at CodeLens implementation and it uses codelens LRU cache along with registering the CodelensController (in our analogy this is HintsUpdater) using the private api. So that each instance of the codelens controller is attached to a single editor and it has access to its full lifecycle (i.e. it is disposed along with closing the editor), whilst we have to bookeep the opened editors.

Veetaha · 2020-03-07T12:25:20Z

Also, @matklad, please note that the first request seems to get an invalid (maybe partial) inlay hits response. Is this the expected behaviour? (I also find myself re-setting up those debug logs in the following demo a lot, but I guess we should remove them anyway)

…call)

Veetaha · 2020-03-07T12:44:02Z

I'll merge this because partial inlay hints results at bootstrap are definitely another issue that is server-side.
bors r+

bors · 2020-03-07T12:50:22Z

Build succeeded

kjeremy · 2020-03-07T13:55:17Z

@Veetaha I really appreciate you looking at how codelens works and why we can't go that route. I wonder if we can file an issue to open up the relevant API. It may be useful to others and when we upstream this. Would that have made this work easier?

Veetaha · 2020-03-07T14:21:58Z

It's actually hard to tell, there are some more topics I'd need to go through like viewport optimization and the universal shape of inlays data.
I have an idea of fetching inlays for a whole array of visible files in one request (since we currently issue a request per source file), but it does have its own disadvantages (like we can't update inlays asynchronously independent on other visible source files, though I think this won't be perceivable due to small amount of visible files on average anyway).

However, inlays do resemble codelens to some degree (except there is no need to actually resolve an inlay hint), so some of their design may be used for them. I don't fill confident enough in LSP, but I might try to think of an api proposal once I get to it!

matklad reviewed Feb 29, 2020

View reviewed changes

vbfox mentioned this pull request Mar 1, 2020

Remove inlay hint in diff views #3388

Merged

Veetaha added 7 commits March 7, 2020 14:08

vscode: extract Type and Param hint cases of InlayHint at type level …

0e6d066

…(needed further)

vscode: redesign inlay hints to be capable of handling multiple editors

6441988

vscode: simpify

fd709c0

vscode: add dat semicolon

057cd95

vscode: refresh all editors on text changes, simplify inlays api

3d93e21

vscode: prerefactor util.ts and ctx.ts

a63446f

vscode: remove logic for caching editors as per @matklad

ef52fd5

Veetaha force-pushed the fix/inlay-change-revert-no-updated-2 branch from 995ad75 to ef52fd5 Compare March 7, 2020 12:08

Veetaha requested a review from matklad March 7, 2020 12:09

Veetaha added 3 commits March 7, 2020 14:34

vscode: remove logging from inlays, run fix lint issues

2734ffa

vscode: more privacy for HintsUpdater

61a4ea2

vscode: post refactor HintsUpdater (simplify create() -> constructor …

65cecff

…call)

bors bot merged commit aff82cf into rust-lang:master Mar 7, 2020

Veetaha deleted the fix/inlay-change-revert-no-updated-2 branch March 7, 2020 12:57

vscode: redesign inlay hints to be capable of handling multiple editors for one source file #3378

vscode: redesign inlay hints to be capable of handling multiple editors for one source file #3378

Uh oh!

Conversation

Veetaha commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha commented Feb 29, 2020

Uh oh!

Veetaha commented Feb 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Mar 2, 2020 via email

Uh oh!

Veetaha commented Mar 2, 2020

Uh oh!

matklad commented Mar 2, 2020 via email

Uh oh!

Veetaha commented Mar 2, 2020

Uh oh!

matklad commented Mar 3, 2020

Uh oh!

kjeremy commented Mar 3, 2020

Uh oh!

Veetaha commented Mar 3, 2020

Uh oh!

kjeremy commented Mar 3, 2020

Uh oh!

matklad commented Mar 7, 2020

Uh oh!

matklad commented Mar 7, 2020

Uh oh!

bors bot commented Mar 7, 2020

Uh oh!

Veetaha commented Mar 7, 2020

Uh oh!

Veetaha commented Mar 7, 2020

Uh oh!

Veetaha commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors bot commented Mar 7, 2020

Build succeeded

Uh oh!

kjeremy commented Mar 7, 2020

Veetaha commented Feb 29, 2020 •

edited

Loading

Veetaha Feb 29, 2020 •

edited

Loading

Veetaha Feb 29, 2020 •

edited

Loading

Veetaha Mar 2, 2020 •

edited

Loading

Veetaha Mar 2, 2020 •

edited

Loading

Veetaha commented Mar 7, 2020 •

edited

Loading