Repository navigation
VSCode error/info hovers left in inconsistent state. #3928
Description
Describe the bug
In a file with errors or warnings, if I use f8 to navigate to an error which is on a field in a class, I get a hover with the error message.
For some errors, fixing the error has no effect whatsoever on the hover (it continues to display the stale error). I'm not sure if this is WAI or not, it doesn't seem ideal, but I could see it going either way.
For errors specifically associated with a field in a class, the error text in the hover does update as I type, but stops updating in a transient error state with a stale transient error message.
To Reproduce
Create a file named example.dart, containing:
class Foo {
final bar;
Foo(this.bar);
}
and an analysis_options.yaml containing:
analyzer:
language:
strict-inference: true
There will be a diagnostic indicating that the type of bar cannot be inferred. Pressing f8 will pop this up in a hover, and take you to the location.
Now place the cursor immediately before the letter b, and type dynamic to fix the error. Note that the hover text continuously updates with transient errors (this doesn't seem to happen with local variables), but when the typing is completed, the last transient error is left in the hover (even though all errors in the diagnostics window are now gone). The text of the hover will say "The type of dynamicbar can't be inferred without either a type or initializer.
Try specifying the type of the variable.dart(inference_failure_on_uninitialized_variable)" (note that it has captured the state immediately before the space was added to separate dynamic and bar).
Expected behavior
Ideally, I think I would expect the hover to reflect the fact that the error has been fixed. The alternative behavior of retaining the original error doesn't seem ideal to me, but is at least consistent. The current behavior of capturing transitory errors is very confusing.
Screenshots
Please complete the following information:
-
Operating System and version: Darwin leafp-macbookpro2.roam.internal 21.4.0 Darwin Kernel Version 21.4.0: Fri Mar 18 00:46:32 PDT 2022; root:xnu-8020.101.4~15/RELEASE_ARM64_T6000 arm64
-
VS Code version:
Version: 1.66.2 (Universal)
Commit: dfd34e8260c270da74b5c2d86d61aee4b6d56977
Date: 2022-04-11T07:49:20.994Z (4 days ago)
Electron: 17.2.0
Chromium: 98.0.4758.109
Node.js: 16.13.0
V8: 9.8.177.11-electron.0
OS: Darwin arm64 21.4.0 -
Dart extension version: v3.38.2
-
Dart/Flutter SDK version: Dart SDK version: 2.18.0-edge.5910da4f021fd6c52359d43dc76f93bc11f492d3
-
Target device (if the issue relates to Flutter debugging): N/A
Activity
DanTup commentedon Apr 19, 2022
I think what's happening here is that the Peek window is kept open so you can use it to navigate to the next problem (rather than just closing it). It shows the last state of the error, which in this case unfortunately happens to include some text from the source which is not updated when the diagnostic is removed (because VS Code doesn't know it's code from the source, and the analysis server doesn't produce new diagnostic text because the issue is resolved).
I think the greyed/italic text is intended to show that the issue is resolved/stale, but I don't think that's very clear.
I've opened microsoft/vscode#147683 suggesting some possible improvements (such as striking the text out, or adding more explicit text stating that the problem is resolved).
I don't know if there's much we can do from Dart's side. We probably could send an additional diagnostic with different text like "This problem was resolved" immediately before removing it, but I don't know if that's worthwhile (at least not without knowing if VS Code might make some changes here). (@bwilkerson FYI)
leafpetersen commentedon Apr 19, 2022
So there's something else going on here I think, because I do not see this behavior if I do exactly the same thing on a local variable. There, the hover text never updates at all - it continues to show the original error (which I think is a better UX than what I see on class fields, even if not what I would ideally have expected).
DanTup commentedon Apr 19, 2022
Oh sorry, I missed that. I can repro a difference, though it seems to be related to the errors changing order (which changes the one that's visible) rather than a real difference in behaviour by VS Code. For some reason, when I add the
cto dynamic, the diagnostics change order (see the Problems list at the bottom) which means it's a different error being shown by VS Code (I guess it just uses the index). Is this what you see:I'll see if I can figure out why the errors are ordered differently only after typing the
c, although I suspect fixing that will cause the same (unwanted) behaviour here 😞DanTup commentedon Apr 19, 2022
@bwilkerson it's not the reported issue here, but I think diagnostics re-ordering during typing is something to fix (or at least minimise). In the GIF above I'm just adding/removing a character that isn't changing the meaning of anything but the diagnostics re-order.
I believe the reason is that we don't apply any unique sorting to them, and during analysis they're stored in a
HashSet(https://github.com/dart-lang/sdk/blob/139385c6154d73236a94e2171190904ffbb32845/pkg/analyzer/lib/error/listener.dart#L260) where the ordering (when they're later converted to a list) is affected by the hashcode.I could sort them in the LSP server to reduce this (for example by severity, error code, error message) although I wonder if having a more-stable ordering for the other protocol/CLI. might be a good thing too. Changing the set in
RecordingErrorListenerto aLinkedHashSetprevents the re-ordering (since the errors are reported in the same order as you type), although I don't know if that's a cost to this (and if so, how that compares to sorting once at the end).Any opinions/preferences?
Edit: Split this to #3934.
leafpetersen commentedon Apr 19, 2022
There's something else going on - sorry, I think my field vs local variable was a red herring.
If I start with the following code:
and I navigate with f8 to the error on bar, and then I move the cursor to immediately before the
b, and delete one space, the hover text does not change even though the errors reported in the problems window do change. I can complete the fix (by deleting "int" and replacing it with "String") and the error text in the hover doesn't change at all.On the other hand, if I navigate to the error on
baz(which comes from thestrict-inferenceoption), and I navigate to immediately before thebinbaz, and type ad, the error in the hover text does change, to match the error reported in the problems window:I don't think this actually has anything to do with local variable vs field (that was my mistake), I think it might have to do with error vs other diagnostic? Not sure. But it's definitely inconsistent behavior.
bwilkerson commentedon Apr 19, 2022
If the ordering is actually causing a problem, then I don't have any problem with sorting them before returning them. Clients of the legacy protocol sort diagnostics after server returns them, so it shouldn't impact those clients to keep them sorted as we build the list.
That's probably just really old code. If you just use a set literal you'll get a
LinkedHashSet.DanTup commentedon Apr 20, 2022
@leafpetersen I don't know if I'm seeing the same - perhaps a recording of screenshots will help. I think in the first case, you're starting with this:
And you delete the space so you then see this:
And in the second case you start with this:
But after adding a character, I think I see something slightly different:
Despite the difference in error, perhaps it still illustrates what you're describing though. In the first example the message became greyed and italic (and is stale/does not updated) but in the second case the error is updated (and neither greyed nor italic).
I believe the reason for this difference is that in the first case the original diagnostic is gone (although a new diagnostic is produced, it's at a different location/error code). VS Code indicates this by making it grey/italic to show it's no longer applicable (perhaps they felt hiding the box or jumping to another error might be jarring?). In the second example, although the original diagnostic was removed, a new one was added with the same range/index/error code so I think this causes VS Code to show it in its place, rather than the grey/italic stale version.
Does this seem to match what you're seeing? Unfortunately we have no control over the UI here - we only provide the diagnostics to VS Code and this UI/feature ("Problem Peek") is handled by VS Code without any input from us. If there are things that could be improved, we can file issues against VS Code though (I've opened one about the grey/italics not being particularly clear with some suggestions like striking the text out, or adding a note that the diagnostic has been resolved).
DanTup commentedon Apr 28, 2022
@leafpetersen the VS Code issue I raised about making it clearer the greyed items were no longer issues was closed:
Any issues caused by the order changing should be resolved with dart-lang/sdk@147dca3 though. If you're able to use an SDK that includes that, do you still see behaviour that seems odd? Are you able to record exactly what you see?
leafpetersen commentedon May 6, 2022
I see the behavior you describe, yes, and it sounds like from the VSCode perspective it's WAI. I continue to find it an odd experience as a user but it sounds like it's not in our control so....