Dart-code: Don't show issues from excluded directories in analysis_options

[xster]

A bunch of warnings from bin/cache even though analysis_options.yaml excludes that directory.

[DanTup]

I believe this is a server issue:

• https://github.com/dart-lang/sdk/issues/28754
• https://github.com/dart-lang/sdk/issues/25551

[DanTup]

Tidying up the issues list here; going to close this in favour of the SDK issues above since I don't think there's anything VS Code-specific.

[xster]

Seems like issue isn't occurring in the intellij plugin however. Wonder if they did something different.

[xster]

Strange, I just rebooted vscode and this issue isn't present anymore. Maybe the analyzer cached something since I last updated the dart sdk and it was fixed upstream already.

Nope, I lied. It's still there. Seems like it only happens after running an app or a test from vscode.

[DanTup]

Running an app or test shouldn't affect the analysis in any way. Could you give exact steps and maybe a small repro project? An analysis instrumentation log covering the whole session may help too.

[xster]

Hope the attached file helps. I somewhat have a repro.

It's one of the few ways I think I stumbled on in the past but, basically open the Flutter repo, then bring up the go to symbol prompt. Type in something that belongs to something in the excluded paths. It seems like as long as you highlight a symbol in a file that's excluded, analysis problems from that file will appear in the problems panel.

Dart-Code-Log-2018-10-03 16-43-18.txt

[DanTup]

Ok, I think there are a few issues working together here.

1. Errors for Excluded, but Priority Files

When you open a file (or VS Code does, for a preview) we send it as a "priority file" to the server. This results in us getting back errors for that file even if it's excluded. I don't know if this is expected. The comment at https://github.com/dart-lang/sdk/issues/25551#issuecomment-418437599 suggests it is, but this doesn't seem to match the description of "priority" if it's changing behaviour beyond priorities.

@bwilkerson @scheglov - what are your thoughts? I don't think it's ever good to show errors for an excluded file and the client shouldn't need to parse analysis_options and resolve which to exclude. However, it does mean other things (like outline, colors, etc.) wouldn't work if they were completely ignored, and I'm not sure if that would be bad too.

2. Files removed from Priority Files that are excluded do not clear the error lists

This is described by @scheglov at https://github.com/dart-lang/sdk/issues/25551#issuecomment-418437599 and seems to match what I see in logs here. This one is definitely a bug (and if 1 is working as intended, a pretty nasty one, as you end up with a growing list of excluded errors in your error list that you need to restart the server/reload the window to clear).

3. VS Code does not tell us when the user closes a file

As crazy as this sounds, there is no notification when a user closes a file (please 👍 https://github.com/Microsoft/vscode/issues/15178 for this!). This goes for LSP too. For performance reasons, closing an editor doesn't actually close "the file", that may happen any time later. Whether the file is "open" or "closed" refers to whether the client is managing the state of the file (versus the disk) but is not guaranteed to line up with the file being visible (VS Code runs cleanup that will close any files that aren't open in an editor "after 3mins or when 80MB storage for documents are exceeded").

This unfortunately means that if 2 is fixed but 1 is working as intended, then the errors may still persist for up to 3 minutes after you close the file/preview.

[DanTup]

@bwilkerson @scheglov What would be the impact of changing priority files a lot (eg. if I only sent the current visible files, rather than all "opened" files)? If it's just prioritising work in a queue, presumably it wouldn't add any extra work to do (eg. changing priority files when there is no work to do won't trigger any work)?

This might solve 3 above, so if 2 is fixed, at least the errors wouldn't persist when the user closes (at the expense of files that are open. but not visible, not being considered priority).

[DanTup]

Actually, switching to visible docs is probably bad - it'd mean we're unsubscribing/re-subscribing for all subscriptions, so switching between docs would result in things like outline/folding/etc. being re-sent all the time (in this case, the VS Code behaviour maybe isn't so bad - you close a file and re-open it, and we'd be using the outline/etc. we still have in memory).

Sooo, I guess we really want some way of dealing with 1. A way to specifically not get errors for excluded files (I don't think anything else needs to be excluded, they all make sense to get).

[bwilkerson]

1. Errors for Excluded, but Priority Files

Our experience is that users want to see full analysis for any file they open, whether it's in an analysis root or not (for example, files opened via navigation). I'm not positive that this also applies to excluded files, but my intuition is that it does. Even if we don't produce errors for excluded files, the bug you reference in (2) still impacts files that are outside of analysis roots and needs to be fixed.

What would be the impact of changing priority files ...

I think it makes perfect sense to make visible files be priority files and to not make open but not visible files be priority files. I think that's how the IntelliJ plugin works.

... in this case, the VS Code behaviour maybe isn't so bad - you close a file and re-open it, and we'd be using the outline/etc. we still have in memory ...

Server would still end up sending the data in response to the new subscriptions, but I don't expect it to be too bad. If we're only processing visible files there probably are only one or two of them to analyze. Even when there are two, I'd expect that only one file would change at a time, so we're really only looking at adding a subscription for the file most recently made visible. That better not be too taxing for server to handle.

[DanTup]

I'm not positive that this also applies to excluded files, but my intuition is that it does

I think it definitely applies to most things, but I'm not so sure about errors. Isn't the purpose of excluding a file because you want to opt-out of seeing its errors listed in the editor? I think it's good for everything else to work though.

I think it makes perfect sense to make visible files be priority files and to not make open but not visible files be priority files. I think that's how the IntelliJ plugin works.

Even when there are two, I'd expect that only one file would change at a time, so we're really only looking at adding a subscription for the file most recently made visible.

I'm concerned that if we do that (and I'm talking about both priority files and subscriptions since we track the same files) then a user switching between two files with Ctrl+Tab would result in a bunch of pointless work (telling the server to drop and re-create the subscriptions, which would in turn invalidate and re-cache the results in VS Code).

Hmm, or maybe - is there a way for me to opt-out of error notifications for specific files? So I could keep subscriptions for everything else for "open" files, but only get errors for the visible ones?

[bwilkerson]

... is there a way for me to opt-out of error notifications for specific files?

Not at the moment, no. You can opt out of _all_ error notifications and explicitly request them, but I don't think that's a good model for general use.

[DanTup]

Yeah, I agree. I also think it wouldn't help me, because I don't know which files to ignore - the user would expect errors for all files that are closed (if they're not excluded).

I think a possible fix (once 2 is solved) is to only send priority files for visible editors, but keep subscriptions for "open" files (I'm using VS Code's definition for open). I think that'll mean the errors disappear when the file is closed, but all other subscriptions will persist longer.

That said, we may need to revisit this for LSP. In LSP we will not have access to "visible files" only "open files" (where open means "was probably open, but may have been closed up to three minutes ago") so we'll end up with the errors hanging around again (and since LSP doesn't dictate when a client must close the file, potentially other clients will hold the file "open" even longer).

[DanTup]

vNext will set only the visible files as priority, so they shouldn't remain priority for up to 3 minutes after you close them. We still keep the subscriptions for things like Outlines active for the original period (where VS code says the file is open) so tabbing between files shouldn't result in resetting all of those.

It doesn't fix this issue yet, but if the server starts flushing the errors from ignored files when they become not-priority, they'll at least disappear when you close the file. It'd be good to get some testing to ensure these changes don't break anything, so if possible please install this early build of v2.22.0 to use and let me know if you think the changes have introduced any issues.

https://github.com/Dart-Code/Dart-Code/releases/tag/v2.22.0-alpha.1

[xster]

It seems like this issue is blocked on https://github.com/dart-lang/sdk/issues/25551 right?

Is there a way to flush the existing list from VSCode or toggle a mode to filter against open files only?

[xster]

Actually there's a filter input field in the analysis pane and just having !bin/cache cuts down massively on the noise already.

I think it's good enough of a work around currently.

[DanTup]

It seems like this issue is blocked on dart-lang/sdk#25551 right?

Yeah, that one or https://github.com/dart-lang/sdk/issues/36064 (fixing this one would at least make them go away when you close the file).

I don't currently know a great workaround besides the filter you've found.

[Nazacheres]

Any progress on this so far? I wanted to have it as part of the CI, but this generated files and warnings in them are blockers.

[DanTup]

@Nazacheres I don't think excluded files are handled, but some cases of this (for ex. .dart_tool) are fixed by https://github.com/dart-lang/sdk/issues/33778. Where are the errors you're seeing?

[Nazacheres]

I have generated with yarn gql-gen APIs and correspondent JSON implementations with flutter packages pub run build_runner build. Correspondingly I have:
analyzer: exclude: - lib/generated/** - lib/network/api/spinpark_api.dart - lib/network/api/spinpark_api.g.dart - lib/network/data/token.g.dart
In the end of my analysis_options.yaml.
Still both IntelliJ's lint and dartanalyzer --options analysis_options.yaml . are catching and counting and showing these warnings. @DanTup

[Nazacheres]

So you are proposing me to move all generated code to .dart_tool? @DanTup

[DanTup]

So you are proposing me to move all generated code to .dart_tool?

No, that's just where some code ends up that generates errors that people also hit this issue with. It wasn't clear from your message whether your errors might be in there (in which case it might be solved).

That said, you mentioned setting this up on CI. Fixing it in the analysis server (as was done for .dart_tool) will not affect results from the command line tools so I think this needs fixing further up (I think https://github.com/dart-lang/sdk/issues/25551 is the most relevant issue).

[Nazacheres]

@DanTup Aha, I see now. It is such an old issue. And such an important thing :(

[narcodico]

Aaaaand...still not working.

[slavap]

Still not working.
The only workaround I've found so far - close excluded files in editor and then restart VSCode :-(

[saturov]

Still not working... So annoying bug.

[DanTup]

I recently fixed this as part of https://github.com/dart-lang/sdk/issues/36064. It won't be in any stable branches yet, but should be available in nightlies and will turn up in a future stable release.