Improve type hints behavior #1652

SomeoneToIgnore · 2019-08-04T21:43:00Z

This PR fixed the following type hints issues:

Restructures the InlayKind enum contents based on the discussion here: Add if let, while let and match arm inlay hints #1606 (comment)
Races described in Type hints are sometimes weird #1639
Caches the latest decorations received for each file to show them the next time the file is opened (instead of a new server request)

bjorn3 · 2019-08-05T06:37:28Z

editors/code/src/commands/inlay_hints.ts

+                        editor!
+                    );
+                } else {
+                    await editor!.setDecorations(


What if the file changed outside of the control of vscode?

On my machine (macOS) vscode is fine with tracking external file changes in rust-analyzer project: it emits vscode.TextDocumentChangeEvent which we handle via the updateHints method.

I've also found some old issue that hints that vscode devs try to support the file watch as better as possible: microsoft/vscode#31600

If vscode fails to track the external file change at some point, we mess up code coloring and current inlay hints from master branch anyway, so my PR does not make it worse :)

Yeah, I think VS Code should work strictly in terms of in-memory representation of the file. If in memory and on-disk contents are different and an extension observes it, than that's the bug in VS Code.

so my PR does not make it worse

That's actually incorrect: master branch would have made a server request and updated the hints, while this version displays incorrect old hints from drawnDecorations.

Luckily, the new hints will be queried after the document is changed in the vscode, so it is easy to override.

I think this kind of situations should not be frequent in real life and a fair price for not querying the server for every .rs file opened in the editor (as we do it now)

I think this kind of situations should not be frequent in real life and a fair price for not querying the server for every .rs file opened in the editor (as we do it now)

We actually should query the server for each of .rs files: change in one file can affect type-hints in another file. If we want to save computation here, we should add an ability to filter type-hints by the viewport (so that we query only visible things).

Indeed, how could I miss it?

What if we still keep the cache and clean it when the next edit is made?
Decorations disappear every time the editor is closed, but we might reuse the ones already queried if no changes are made?

matklad · 2019-08-05T07:22:03Z

editors/code/src/commands/inlay_hints.ts

+                        editor!
+                    );
+                } else {
+                    await editor!.setDecorations(


Yeah, I think VS Code should work strictly in terms of in-memory representation of the file. If in memory and on-disk contents are different and an extension observes it, than that's the bug in VS Code.

matklad · 2019-08-05T07:27:02Z

editors/code/src/commands/inlay_hints.ts

@@ -21,32 +21,44 @@ const typeHintDecorationType = vscode.window.createTextEditorDecorationType({

 export class HintsUpdater {
    private displayHints = true;
+    private drawnDecorations = new Map<string, vscode.DecorationOptions[]>();


So, we never clear this map unless we turn the hints of? This is (a small) memory leak than. I don't think by itself it is a huge problem, but it hints that the general lifetime cycle is not ideal here. I wonder if we can do better?

I think we can do one of the two things:

VsCode seems to use subscriptions/disposable pattern to clean up various observers automatically. I wonder if there's a disposable array, associated with the vscode.TextEditor, which we can use to react on editor opend/closed.

alternatively, a pure JS solution would be to use a WeakMap, keyed by an actual editor here. Not sure if WeakMap is available in VsCode

Also, I've seen this problem quite a few times: if you have an observer, that observes an object X, it's usually a hard problem to decide when you should stop observing (because there's a reference cycle between observer and an observee). I wonder if there's a blog post about this somewhere...

Nice catch, had forgotten to tell my concerns about it.

There's a onDidCloseTextDocument that is triggered whenever a tab is closed, but that does not help in the current situation much: the whole idea is to quickly redraw the decorations when it's reopened, so we cannot remove them from the drawnDecorations yet.

WeakMap sounds like a nice idea, I will try it out.

Looks like both Map and WeakMap compare vscode.Uri instances by reference and those change every time we close and open the editor tab for the file.
The uri string stays the same, but WeakMap does not accept string types as keys, so I had to use the Map again.

But now, since we clear the map every time an edit is made, the leak gets even smaller, doesn't it?

matklad · 2019-08-05T13:50:27Z

Hm, I am still not quite satisfied with the way we manage state here. I wonder if we can avoid it? Would the following work?:

we connect to window.onDidChangeVisibleTextEditors event and issue update on the event
we connect to workspace.onDidChangeTextDocument event and issue update on any .rs file event
In the update, we walk through all ~~window.activeTextEditor~~ window.visibleTextEditors editors which show .rs documents, issue a server request for them (ideally, in parallel), and set decorations
On disable event, we walk through all editors and set decorations to an empty set

It seems like we don't need to store any state at all with these approach?

EDIT: I think we even don't need a separate disable method, update can just look at the current value of setting.

SomeoneToIgnore · 2019-08-05T14:45:36Z

Hm, I am still not quite satisfied with the way we manage state here.

Would be great to know what concerns you, just out of curiosity :)

we connect to window.onDidChangeVisibleTextEditors event and issue update on the event

In the update, we walk through all window.activeTextEditor window.visibleTextEditors editors which show .rs documents, issue a server request for them (ideally, in parallel), and set decorations

If I understood you correctly, you propose to issue a server request for every visible text editor whenever the visible editors change (some get closed or open).

I agree with the overall idea, but I wanted to optimize one corner case: currently vscode "forgets" the previous decorations that were set to the editor after some other editor is opened.
This means that we need to set the decorations every time a file editor is shown again, even if we close and open the same file.

If we follow your proposal (or the way it's done in master), we will be querying the server to set those decorations, in the current PR we reuse the already queried decorations if there were no changes to the *.rs files in the project.

My reasoning for caching those results was the idea that it's completely useless to make another server request in such cases: since the project did not change, we know that the same file reopened needs the same hints.
Since the hints we store are cleared on any document edit, I thought that it's fine if we don't care about the cache size that much.

SomeoneToIgnore · 2019-08-05T14:46:40Z

Nice idea with the window.visibleTextEditors, currently only an active editor is updated, I will adjust this.

matklad · 2019-08-05T15:39:10Z

Would be great to know what concerns you, just out of curiosity :)

Mutable state :) Specifically, when observer tries to maintain current state by observing changes, it's very easy to arrive at an inconsistent state.

If I understood you correctly, you propose to issue a server request for every visible text editor whenever the visible editors change (some get closed or open).

Yeah, exactly.

This means that we need to set the decorations every time a file editor is shown again, even if we close and open the same file.

I think it's fine, this stuff is pretty fast, and the situation is pretty rare. If we do optimizations here, I think we should focus more on debouncing sevral changes (ie, don't send requests faster than once in 100ms). For that to work, we'll need a per-editor state which we could store in a weak map, keyed by the editor itself.

SomeoneToIgnore · 2019-08-05T20:13:00Z

Thanks for the explanations.
Had applied the changes you've suggested and let's consider the optimizations in some other PR.

matklad · 2019-08-06T16:50:34Z

bors r+

Thanks!

1652: Improve type hints behavior r=matklad a=SomeoneToIgnore This PR fixed the following type hints issues: * Restructures the `InlayKind` enum contents based on the discussion here: #1606 (comment) * Races described in #1639 * Caches the latest decorations received for each file to show them the next time the file is opened (instead of a new server request) Co-authored-by: Kirill Bulatov <[email protected]>

bors · 2019-08-06T17:01:14Z

Build succeeded

continuous-integration/travis-ci/push

SomeoneToIgnore added 2 commits August 5, 2019 00:23

Query less hints on file open

2c02aeb

Use proper inlay kinds

15411d4

SomeoneToIgnore force-pushed the avoid-hints-races branch from 01be2d4 to e02a00a Compare August 4, 2019 21:50

Style and test fixes

3fb6462

SomeoneToIgnore force-pushed the avoid-hints-races branch from e02a00a to 3fb6462 Compare August 4, 2019 22:03

bjorn3 reviewed Aug 5, 2019

View reviewed changes

matklad reviewed Aug 5, 2019

View reviewed changes

SomeoneToIgnore force-pushed the avoid-hints-races branch from 15140b3 to ef376cb Compare August 5, 2019 08:11

Use WeakMap to avoid memory leaks

f358b4c

SomeoneToIgnore force-pushed the avoid-hints-races branch from ef376cb to f358b4c Compare August 5, 2019 08:14

Cache decorations before the first change only

777552b

Avoid shared mutable state

c5598d9

bors bot merged commit c5598d9 into rust-lang:master Aug 6, 2019

SomeoneToIgnore deleted the avoid-hints-races branch August 6, 2019 17:20

SomeoneToIgnore mentioned this pull request Aug 6, 2019

Type hints are sometimes weird #1639

Closed

Improve type hints behavior #1652

Improve type hints behavior #1652

Uh oh!

Conversation

SomeoneToIgnore commented Aug 4, 2019

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!

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!

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!

matklad commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomeoneToIgnore commented Aug 5, 2019

Uh oh!

SomeoneToIgnore commented Aug 5, 2019

Uh oh!

matklad commented Aug 5, 2019

Uh oh!

SomeoneToIgnore commented Aug 5, 2019

Uh oh!

matklad commented Aug 6, 2019

Uh oh!

bors bot commented Aug 6, 2019

Build succeeded

Uh oh!

Uh oh!

matklad commented Aug 5, 2019 •

edited

Loading