Sdk: Analyzer doesn't flush errors for files removed from priorityFiles that aren't otherwise analyzed

[DanTup]

As mentioned here:

https://github.com/dart-lang/sdk/issues/25551#issuecomment-418437599

If you send a priority file that is not otherwise analyzed, the server analyzes it, but doesn't flush its errors when it's removed from priority files. This causes the errors to hang around (I would expect until the IDE restarts, however due to caching maybe it's forever unless you clear ~./dartServer based on https://github.com/Dart-Code/Dart-Code/issues/1480#issuecomment-468023869?).

[jascodes]

@DanTup Any updates for this? this is quiet annoying.

[DanTup]

I actually would've expected this to be resolved by https://github.com/dart-lang/sdk/issues/33778#issuecomment-490833276. The issue was that files were being implicitly analyzed when requests like Hovers were sent, which resulted in their errors being sent - however there was no appropriate time to flush them (since they were never added to the analyzed set).

Can you give a specific example of what you're seeing and what SDK version you're on?

[szotp]

You just need to have exclude settings in your analysis_options that would prevent analyzing some files, and then open such file. Errors from excluded directories will appear and stay until IDE gets restarted.

The true problem is showing errors for files that were excluded, but it would not be too annoying if errors disappeared after closing the file.

[DanTup]

@bwilkerson I was going to take a look at this (it also came up in https://github.com/Dart-Code/Dart-Code/issues/1695#issuecomment-638891720) to see if I could flush files when they're removed from priority files if they are excluded by analysis_options.yaml.

However, I re-discovered that LSP won't tell us when a user closed a file (the editor is allowed to keep files "open" over LSP even if they're closed in the editor, and may call textDocment/didClose at any time in future that it deems suitable). In the case of VS Code, we can see up to 3 minute delays between closing a file and it deciding to tell extensions/servers.

This means the proposed fix would be bad over LSP

• User opens ignored file
• Server sends errors
• User closes ignored file
• (3 minutes pass)
• Errors disappear from the Problems lis

Therefore I think the only reasonable fix (at least until LSP and VS Code can properly tell us when files are closed - https://github.com/microsoft/vscode/issues/15178) is to never send errors for excluded files (even if they're opened).

WDYT?

[bwilkerson]

You might be right, but that solution also leads to a poor UX in which users can open a file and not see any diagnostics and assume that it's because there aren't any. If we had a way to indicate that the file was excluded, that would be better, but I'm fairly sure we don't. It sounds like we're stuck choosing the better of two bad choices.

I have to wonder whether there's any prior art we can draw on. Is Dart the only language that allows files to be excluded? Does VSCode have any mechanism for ignoring/excluding files? Would it be worthwhile to ask the LSP community for advice in this situation?

[DanTup]

You might be right, but that solution also leads to a poor UX in which users can open a file and not see any diagnostics and assume that it's because there aren't any. If we had a way to indicate that the file was excluded, that would be better, but I'm fairly sure we don't.

Not really. We could show a notification with window/showMessage saying errors aren't being shown because the file is ignored when the user opens it - but that also means assuming that when we get textDocument/open it was because the user opened the document (but unfortunately this is not always the case - for example in VS Code when an extension opens a file via the API (not visibly, but - for example - to convert a line/col to an offset) it would also trigger this).

Would it be worthwhile to ask the LSP community for advice in this situation?

I think they're all waiting for https://github.com/microsoft/vscode/issues/15178 too. There are 275 👍 's and a lot of comments on it. Even one of the developers on the Code/LSP team is waiting for it:

https://github.com/microsoft/vscode/issues/15178#issuecomment-625143454

It has been scheduled for the last few months, but pushed back. It's possible it will turn up soon, so we could hold off doing anything a little longer. It's been around a long time though, so it's hard to guess when we might get it.

I might file an issue in LSP for discussion. Even if VS Code does nothing, there's no reason LSP couldn't additionally support telling us the visible text editors (VS Code has an API for that, just not open editors - eg. including those in open in "other tabs") which we could switch to instead of open/close for priority files. Then we could have the errors show/hide as the editor is shown/hidden (rather than when the files are open in other tabs).

[DanTup]

Another thought I had - a custom message in the LSP server for setting priority files. If a client says they support sending these, we don't set priority files based on LSP open/close events (since LSP's open/close is intended only for determining who is managing the file, not what's visible) but instead let the client tell us. Then in the VS Code extension, we send the visible editors as priority files whenever they change.

This would fix the issue for any clients with custom code to pass the visible editors over until LSP has some mechanism for it (if it ever gets it).

I've opened https://github.com/microsoft/language-server-protocol/issues/1031 for some discussion in LSP anyway. I think I might wait a little before doing anything, as the "Open editors API" issue in VS Code is still currently assigned to the June milestone.

[bwilkerson]

Sounds good to me.

[DanTup]

With https://github.com/microsoft/vscode/issues/15178 being locked and https://github.com/microsoft/language-server-protocol/issues/1031#issuecomment-648706536 saying they're reluctant to pass UI state to the server in LSP, I don't think there are many remaining fixes here.

The best I can think of:

• LSP stops using tying priority files to what LSP considers "open files" (eg. for the current implementation, we just removed LSP's calls to setPriorityFiles)
• We implement a custom LSP notification (client->server) for "update visible documents" which must be implemented by the client plugin, that just tells the server whenever the visible documents change, and the server sets them as priority files

This would allow the server to be sure that any priority file is open (or at least visible) on the client (for both non-LSP and LSP) - though it couldn't be sure the opposite is true (it can't rely on open files being priority, since an LSP client might not inform it of the visible files).

With that done, we could use adding/removing priority files as the trigger to show/not the errors in ignored files and have it work for non-LSP clients and for VS Code in LSP mode.

WDYT?

[bwilkerson]

I like the idea of being able to use the same solution for both protocols, and I agree that LSP's notion of "open files" doesn't correspond to server's notion of priority files, so we shouldn't treat them as if they were the same.

But I think we have to assume that most clients won't implement the "update visible documents" notification. The only implication I know of for that is that opening an excluded file won't generate any diagnostic notifications from server to client. (Let me know if there's something else that I'm missing.) Given that, is it worthwhile implementing the new notification? Maybe we should just stop sending diagnostics for excluded files and see whether any users are actually surprised by that behavior.

I might have asked this before, but are there other languages that you know of that use LSP and have a notion of "excluded" files? Just wondering if there's prior art we should be thinking about.

[DanTup]

I agree that LSP's notion of "open files" doesn't correspond to server's notion of priority files, so we shouldn't treat them as if they were the same.

Yep, that's my feeling too, so I'll remove these from the LSP server.

But I think we have to assume that most clients won't implement the "update visible documents" notification. The only implication I know of for that is that opening an excluded file won't generate any diagnostic notifications from server to client. (Let me know if there's something else that I'm missing.) Given that, is it worthwhile implementing the new notification? Maybe we should just stop sending diagnostics for excluded files and see whether any users are actually surprised by that behavior.

I believe you're correct that the only implication would be missing the diagnostics for excluded files when opened. I agree other editors probably won't implement it (some might not even be able to, as they may just be generic LSP clients for multiple languages), though we could implement this in VS Code at least.

I think leaving it to see what feedback there is sounds reasonable though. This hasn't worked properly ever, and all the complaints have been about ignored things showing up, so this might be what people would prefer. If not, we can consider the extra notification.

[DanTup]

I might have asked this before, but are there other languages that you know of that use LSP and have a notion of "excluded" files? Just wondering if there's prior art we should be thinking about.

So interestingly, I just looked into TypeScript's extension. It actually has this (annoying IMO) behaviour where errors only ever show for open files - that also means they disappear when you close them. I looked through the source and it seems like they're using onDidCloseTextDocument to trigger the removal of the diagnostics:

https://github.com/microsoft/vscode/blob/e41c19505142614f706ad853bd5c4c2ad0f74e89/extensions/typescript-language-features/src/features/bufferSyncSupport.ts#L500-L502

https://github.com/microsoft/vscode/blob/3841f78377fc02ab5ef514cbd0fa760ed4ac6f0d/extensions/typescript-language-features/src/typescriptServiceClient.ts#L159

This means TypeScript seems to be making the assumptions we've been told we shouldn't make.

So some other possible options are:

1. Make the same assumption. It works today in VS Code (onDidCloseTextDocument now fires immediately, instead of after 3 minutes as it did in the past). If it breaks, we can remove it.
2. Add an initializationOptions flag to control this (default to not), and then editors that do work this way (immediately close documents) can opt-in (and we can opt VS Code in while we know it's working this way).

[bwilkerson]

That is interesting. It sounds like they're using the single-file approach from the spec (https://microsoft.github.io/language-server-protocol/specification#textDocument_publishDiagnostics):

The following rule is used for VS Code servers that generate diagnostics:

• if a language is single file only (for example HTML) then diagnostics are cleared by the server when the file is closed.
• if a language has a project system (for example C#) diagnostics are not cleared when a file closes. When a project is opened all diagnostics for all files are recomputed (or read from a cache).

but I don't believe that TypeScript is a single-file language.

I still think we should just never send diagnostics for excluded files and see what the experience is like for users. It seems closer to the spirit of the spec. If it turns out that there really is a dichotomy of user styles, then we can introduce an option to support both sets.

[DanTup]

Ugh, I'd not noticed that text before. It seems to conflict with the description of the onDidClose event:

As with the open notification the close notification is about managing the document's content. Receiving a close notification doesn't mean that the document was open in an editor before. A close notification requires a previous open notification to be sent. Note that a server's ability to fulfill requests is independent of whether a text document is open or closed.

I'll raise an issue asking for some clarification, because this seems to contradict itself. If an editor is not required to notify the server immediately when a file closes, this would break this experience, since you'd see errors hanging around for some unknown amount of time after closing a file.

I still think we should just never send diagnostics for excluded files and see what the experience is like for users.

Yep, I agree - just adding some tests for this and I'll file a CL. Depending on the response to the issue I'll open, we can revisit (though based on the reluctance to give real open/close events, I expect the above will probably be updated/removed).

[DanTup]

I've raised https://dart-review.googlesource.com/c/sdk/+/155130/ that prevents errors in ignored files.

I was going to remove priority files entirely from LSP based on the above, however we do actually use it for a few other things (for example ensuring we only send things like Outline notifications for "open files"). I think it makes sense to keep for these (at least until questions about the spec quotes above are answered) as these uses are just to avoid sending outlines for files that are "definitely not open" (to save traffic) and won't cause bad behaviour in the case an editor opens/closes files out of sync with the user.

[bwilkerson]

Controlling notifications, like outline notifications, was/is the primary purpose of the concept of priority files, so it makes sense to use it for that under LSP as well.

[DanTup]

This was fixed by 0aed132f4d73503dad79c6f491a993ffe58fd20c. Errors are no longer sent for ignored files (whether open or not).

[maks]

@DanTup I just switched over to using the experimental LSP mode in Dartcode v3.15.1 in VSC 1.50.1 and unfort I'm still seeing this.
I am using Built_value so I have in analysis_options.yaml:

  exclude:
    # Ignore generated code
    - lib/*.g.dart
    - lib/**/*.g.dart

and running flutter analyze in VSC terminal shows no issues, but if I open one of the generated files (eg. lib\foo\bar\dtos.g.dart) which does have lint issues, then even when I close it again I still see issues in the problems tab:

[DanTup]

@maks what SDK version are you using? I tested with v2.10.0 of Dart though it seems to be working ok:

If you can repro in a small project and share it with the exact SDK version, I'll take a look. Thanks!