Analysis server should ignore `.dart_tool`

[matanlurey]

Otherwise, here is an example of using AngularDart and IntelliJ:

Note that thanks to @MichaelRFairhurst's work, generated files are now resolved! But unfortunately the side-effect is that the generated files are also treated as source files, and produce additional analysis errors.

Additionally, analysis_options.yaml doesn't seem to work:

analyzer:
  exclude:
    - ".dart_tool"

[matanlurey]

Here is an example of what produces this: https://github.com/matanlurey/angular-ide-example.

[MichaelRFairhurst]

Regarding excluding not working, I did some work on this. There's a bug filed somewhere about this, basically the cli analyzer was designed to more or less take a single file as an argument and give you a transitive closure of errors, and errors from imports (and imports of imports). It still does this, so if you import a generated file, you get those errors. Then there are subtle incompatibilities that I don't remember exactly about how it handles the analysis roots and their branches vs the exclusion semantics.

Like I said I put some work into this. It was not a particularly low effort or high reward task, and involved breaking the command line analyzer API in a way that was not easy to trace what effects could later come. The only example I remember right now is that there's a flag --disable-package-warnings which basically stops the transitive closure at package barriers, and it interacts with the little known or documented, but integral, build mode.

There's also a usability issue (you could call it a bug) where opening an excluded file in intelliJ causes errors to appear even though it's excluded. This is intentional, because we want all ide features available when inspecting files in other packages and, yes, excluded files too. However the IDE doesn't know the file is excluded or otherwise special, and the analysis server doesn't know when the file is closed by the user. So you could call it an API bug. Fix here is doable but also not low effort or particularly high reward.

In the meantime, probably best to use // ignore_for_file: unused import in the code produced by these generators, unfortunately. It's simply much more likely to get done soon.

I'm on my phone so I didn't link to other tickets and could be mistaken on the apis I mentioned.

[matanlurey]

Thanks for the response. Not an immediate priority issue, just thought I'd file for posterity.

Regarding excluding not working, I did some work on this. There's a bug filed somewhere about this, basically the cli analyzer was designed to more or less take a single file as an argument and give you a transitive closure of errors, and errors from imports (and imports of imports).

Right, I remember hearing about this. Thanks for clarifying.

The only example I remember right now is that there's a flag --disable-package-warnings which basically stops the transitive closure at package barriers, and it interacts with the little known or documented, but integral, build mode.

There's also a usability issue (you could call it a bug) where opening an excluded file in IntelliJ causes errors to appear even though it's excluded.

Interesting. I was curious how this worked in non-IntelliJ, so I opened it up in VSCode:

... so it looks like this is not IntelliJ specific.

This is intentional, because we want all IDE features available when inspecting files in other packages and, yes, excluded files too.

Maybe excluded: ... isn't fine-grained enough? (Or you need another category?) In IntelliJ, marking a folder as "excluded" means it isn't even indexed AFAIK. There are also some other categories (like "generated"), I'm not sure the differences though - @jwren might!

However the IDE doesn't know the file is excluded or otherwise special, and the analysis server doesn't know when the file is closed by the user. So you could call it an API bug. Fix here is doable but also not low effort or particularly high reward. In the meantime, probably best to use // ignore_for_file: unused import in the code produced by these generators, unfortunately. It's simply much more likely to get done soon.

Interesting idea, unfortunately I'd basically need a // ignore_for_file: undefined_identifier and probably a bunch of other errors that are side-effects of imports not being found. The other option, I suppose, is the generated code knowing the import path back to the "real" package. That sounds hard though.

Another idea (?), make it impossible to control-click into the generated code. Not sure if possible.

[matanlurey]

At one point the analyzer/IDEs had special behavior for some folders (like build).

Could we mark .dart_tool as a folder, similar to build, if that is still a thing?

[MichaelRFairhurst]

Yeah I think the Java behavior is it asks you when you open it, "do you want to index this file?"

So essentially an opt-in, permanent setting. What we've talked about adding was an automatic, temporary change in behavior. These are both dimensions to a possible solution, in that it could be requested and temporary, or automatic and permanent. We should be careful to choose the right one. There's also an interesting third dimension, which is what we want IDEs to do by default if they don't fully implement every feature exposed by the API. For instance, merely adding new data that intelliJ has to send means vscode will work the same until Danny and Nate implement the new data transfer in vscode and vimlsc. Or we could switch it so that clients have to code up the data path in order to get any resolution in this case, and "default" to no analysis just from opening a file.

I misread the errors. I thought it was unused imports not missing main function. I have no idea what the underlying issue is with the imports in this case then. That it isn't an easy fix is unfortunate! And yeah, probably not worth adding that type of list of ignores. Though I do wonder, somehow it finds the right file when it builds, so, not knowing anything about it, I'm tempted to say it must be solvable. I won't say that, though, since I don't know the use case, and feel like I must be making an assumption that's wrong that I don't realize :)

On Jul 8, 2018 12:12 AM, "Matan Lurey" notifications@github.com wrote:

At one point the analyzer/IDEs had special behavior for some folders (like build).

Could we mark .dart_tool as a folder, similar to build, if that is still a thing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dart-lang/sdk/issues/33778#issuecomment-403260947, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjWe8zFUbbWb4jS5hhDEmZvkpf1fwUVks5uEYbEgaJpZM4VGkfO .

[bwilkerson]

Yes, the command-line analyzer is long overdue for some attention. The plan of record is to make it take a list of files and directories and to treat them the same way that analysis server would treat them if a user opened them in an IDE. Unfortunately, Mike is right; the code has become badly structured and straightening out the mess is not a small task.

And yes, we need to fix the analysis server. I believe that server does know when excluded files are closed, so that leaves us with at least two possibilities: either never report errors for excluded files, or report the errors only for as long as the file is open (and remove the errors when the file is closed). My personal preference would be a middle ground in which the IDEs display those errors in the editor but not outside the editor (such as in the view containing all reported analysis errors). The third option would take more work (on both server and client sides), but I think it would provide a better UX.

And again, yes, I believe that there is still a list of known folders that should be ignored and that we can and should add .dart_tool to that list. There is also a list of file patterns for generated files, and we should make sure that that list has been kept up to date. Analyzer already knows to ignore "follow on" errors if an import is invalid because a generated file has not been generated, and it sounds like that mechanism might have broken down in this case.

[MichaelRFairhurst]

Brian and I just talked -- I kind of threw us off the mark a little bit in that we of course have ways of analyzing other files (for the sake of navigation, autocomplete etc) without showing their errors in the analysis panel. We do this for imported libs for instance. So I read just a bit too much into this.

There is some behavior related to not analyzing code in dot files/directories which is not working, maybe due to my implementation of the generated files analysis specifically.

And I'm not positive that the exclude issue I talked about exists for the server at all. If not, then there seems to maybe be a bug related to excluding files found by my workspace?

So those are two leads to investigate here that aren't a fix-an-underlying-problem level task, which we should take on once we get some breathing room.

[matanlurey]

No worries! I realize some of this is an archeology lesson.

[passsy]

This problem drives me crazy in my daily work because it basically stops Hot Reload from working.

I have Flutter as a git submodule in .flutter and after a few minutes Dart Analysis starts to complain about errors in the flutter source and prevents me from triggering Hot Reload.

I then have to manually click "Restart Dart Analysis Server" to be able to hot reload, for 2-3 minutes before I have to click it again.

I'm not able to add any exclusion rules or prevents Analysis to run at all. I'd be fine to have this run on CI only.

[bergwerf]

Today I also encountered this in VS code while using angular. The generated .template.dart files import the 'parent' library which is not copied to the build folder by the builder it seems. Excluding .dart_tool or */.template.dart does not work.

EDIT: Any recommended workaround? All I can think of now is to copy the entire /lib into the build folder manually (although that is not very durable), or ask for the Dart-Code plugin to implement a simple filter on top of analyzer warnings (given that this issue seems persistent).

EDIT 2: VS code user just type !.dart_tool in the problems filter :D

[DanTup]

or ask for the Dart-Code plugin to implement a simple filter on top of analyzer warnings

Does anyone see any negative side-effects if I filter these out in the VS Code plugin as a short-term fix? It's not a great solution (it only fixes VS Code, and it'll undo itself if we moved to LSP without adding a similar filter there) but if there are no good reasons to show errors in this folder (I don't know if that's true - though I get the impression it is from this issue) it would at least avoid some frustration for users in the meantime.

[bwilkerson]

I'd much rather see it fixed in a way that would solve the problem in all of the IDEs. Would you be willing to look at fixing it in analysis server?

If not, then I suppose I can't see any harm filtering it on the client side.

[DanTup]

Would you be willing to look at fixing it in analysis server?

If it's within my ability, of course! I got the impression from a comment above that it was going to be a fairly complicated job, but if someone can point me in the right direction (both how we'd expect it to work - I'm not sure whether it's ever valid that we'd want to surface these errors - and where we think the fix belongs) I'd happily take a look :-)

[bwilkerson]

I wasn't sure how complex this would be to track down, so I started digging. Ignoring some of the other issues that were raised, focusing only on why server is reporting errors for files in .dart_tool, I came up with a couple of observations.

Files and directories that start with a period are suppose to be automatically excluded from being reported, and that doesn't appear to be the case.

Every time a file in an analysis root is analyzed by the driver, server gets a callback, via a stream, to process the results. The callback is in-line in ServerContextManagerCallbacks.addAnalysisDriver (in sdk/pkg/analysis_server/lib/src/analysis_server.dart) starting on line 797. The call to record errors (which is what causes them to be sent to the client) is on line 803.

The documentation for the stream indicates that it should only contain results for files that were explicitly added to the driver, but contains an exception that states "Analysis results for other files are produced only if the changes affect analysis results of other files." I'm not quite sure how to interpret this exception. Perhaps @scheglov could shed some light (possibly by updating the docs).

I think that leaves us with three possible root causes.

It's possible that the files in .dart_tool are being added to the driver when they shouldn't be. That would indicate a bug in server at the point at which the driver is being configured.

It's possible that the files are not being added but that their results are being added to the stream when they shouldn't be. (I'd need to understand the exception mentioned above to know whether that's the case.) In which case, the bug is in the driver code that adds results to the stream.

It's possible that the results are suppose to be getting added to the stream and that server needs to ignore them, in which case the bug is in the callback mentioned above.

So, without more information I'm not sure where the bug is or how to fix it.

[DanTup]

@scheglov do you know which of the three Brian lists above is correct? I don't mind having a go at fixing it, but I'm also not sure the where the correct place is. Thanks!

[scheglov]

The statement in AnalysisDriver.results, that says Analysis results for other files are produced only if the changes affect analysis results of other files. means that if there are two libraries a.dart and b.dart, and a.dart imports b.dart, then when b.dart changes in a way that affects a.dart (for example a signature of a used function was changed), then a.dart will also be reanalyzed, and the new result produced into the results stream.

I suspect that the errors are reported because the used opened main.template.dart, so it was analyzed, and errors send to the IDE. It does not matter if the file was not added to AnalysisDriver - you asked for the results, you got them :-) The issue is that when the user closes the file, we need to remove errors for this file. It is not obvious however if DAS has a clear signal when to send this empty errors list for the client. Maybe when a file stopped being a priority file, and it is not added to any AnalysisDriver?

[passsy]

If the driver reports errors for all open files in the IDE then we should definitely merge https://github.com/flutter/flutter-intellij/pull/3181 because the IntelliJ plugin shouldn't prevent users from doing a Hot Reload because they jumped into excluded dart source or dart files of dependencies which may contain errors.

[DanTup]

The issue is that when the user closes the file, we need to remove errors for this file.

Ok, so maybe this is just the same as #36064?

It is not obvious however if DAS has a clear signal when to send this empty errors list for the client. Maybe when a file stopped being a priority file, and it is not added to any AnalysisDriver?

That sounds right to me - if adding a priority file caused it to be analyzed (because it wasn't already being analyzed), then removing it from priority files should flush the errors (I'm assuming that once it's removed, we would never get updates for errors in it, so those that have already been sent to the editor are just going stale?).

[bwilkerson]

Yes, the old errors are just going stale.

I started to fix this once, but got pulled into other things and never returned to it. It really seems like it should be fairly easy to fix. Maybe I can find time to brush off the old changes and finish the work (unless someone beats me to it).

[DanTup]

I was going to have a go at beating you to it, but I've been failing at trying to write a failing test (or at least, failing for the right reason). At first I tried in test/analysis/notification_errors_test.dart, but it seems like it doesn't have enough of the server to handle this (as far as I could tell, setPriorityFiles([brokenFile]) doesn't actually send anything to a server).

So I tried in test/integration/analysis/set_priority_files_test.dart like this:

@soloTest
test_ignoredFiles() async {
  String validFile = sourcePath('foo.dart');
  String ignoredFile = sourcePath('.dart_tool/broken.dart');
  writeFile(validFile, 'class Foo { void baz() {} }');
  writeFile(ignoredFile, 'notDart');

  standardAnalysisSetup();
  await analysisFinished;

  // There should be no errors to start with, as .dart_tool is excluded because
  // it starts with a dot.
  expect(currentAnalysisErrors[ignoredFile], isNull);

  // Specifically adding the file to priority files should result in it being
  // analyzed and generated errors.
  await sendAnalysisSetPriorityFiles([ignoredFile]);
  await analysisFinished;
  expect(currentAnalysisErrors[ignoredFile], hasLength(greaterThan(0)));

  // Removing from priority files should result in the errors going away.
  await sendAnalysisSetPriorityFiles([]);
  await analysisFinished;
  expect(currentAnalysisErrors[ignoredFile], isNull);
}

However, this hangs on the second await analysisFinished; as if it never completes. It's hard to debug because I can't add breakpoints since it's in another process. I tried changing it to await pumpEventQueue(times: 5000); however then the next line (asserting there are errors) fails (because there are not).

@bwilkerson any ideas?

[DanTup]

Aha, I found debugStdio().

00:00 +0: SetPriorityFilesTest | test_ignoredFiles
0.324386366: <== {"event":"server.connected","params":{"version":"1.25.0","pid":38668,"sessionId":""}}
0.350353755: ==> {"id":"0","method":"server.setSubscriptions","params":{"subscriptions":["STATUS"]}}
0.354237044: ==> {"id":"1","method":"analysis.setAnalysisRoots","params":{"included":["/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerUlOxt8"],"excluded":[]}}
0.362024554: <== {"id":"0"}
0.470678146: <== {"id":"1"}
0.478425535: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
0.890763255: <== {"event":"analysis.errors","params":{"file":"/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerUlOxt8/foo.dart","errors":[]}}
0.908993707: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}
0.912680105: ==> {"id":"2","method":"analysis.setPriorityFiles","params":{"files":["/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerUlOxt8/.dart_tool/broken.dart"]}}
0.915938127: <== {"id":"2"}

For some reason, opening the broken file doesn't seem to trigger analysis (though it does in my "real" repro of this). I'll try doing some more digging after lunch - maybe I can set the Observatory port and then attach the VS Code debugger to it to step through.

[bwilkerson]

However, this hangs on the second await analysisFinished; as if it never completes. ... For some reason, opening the broken file doesn't seem to trigger analysis ...

I don't know why that would be, but that does explain the hang. Server won't indicate that analysis is done unless it first indicates that analysis has begun, so not triggering analysis looks the same as never completing analysis.

[DanTup]

Yeah, I'm trying to track down why it's not starting. The same log generated by me in VS Code does show analysis immediately after:

[13:17:13 GMT+0000 (GMT)] [Analyzer] [Info] ==> {"id":"8","method":"analysis.setPriorityFiles","params":{"files":["/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart"]},"clientRequestTime":1553606233203}
[13:17:13 GMT+0000 (GMT)] [Analyzer] [Info] <== {"id":"3","result":{}}
[13:17:13 GMT+0000 (GMT)] [Analyzer] [Info] <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[13:17:13 GMT+0000 (GMT)] [Analyzer] [Info] <== {"id":"4"}
[13:17:13 GMT+0000 (GMT)] [Analyzer] [Info] <== {"id":"8"}

Is there a good way to debug the (server inside the) integration tests? I was able to attach to it by setting the observatory port, disabling snapshots and hard-coding the path the to the server, but breakpoints still don't seem to hit. I'd settle for print debugging, but print doesn't seem to get to stdout. I've gotten part way by sending fake notifications:

channel.sendNotification(
        new Notification("LOG", {"msg": "Setting priority files"}));

but now I need to debug somewhere that doesn't have easy access to channel either! Is there an easy way to debug or write logs or something? (I think I can easily enable the instrumentation log if there's an easy way to write to it?).

[DanTup]

Actually, I fixed my breakpoints (though I think I found a Dart Code bug in the process :))

[DanTup]

Right - I figured that bit out. It's not priorityFiles causing the issue at all, it's the content overlay. In my real testing in VS Code, the file is added as priority and we send an AddContentOverlay. Adding the same to the test results in analysis. I'll see if I can make a fix.

This actually makes me think some other changes made in Dart Code to try and reduce this were misguided (reducing priority files to only visible files), so I'll review them once this one is all figured out.

[DanTup]

Ok, this seems to be more complicated than I thought :)

So, the errors seemed to be a result of adding the overlay, rather than priority files. I have a failing test like this (test/integration/analysis/set_priority_files_test.dart):

test_ignoredFiles() async {
  debugStdio();
  String validFile = sourcePath('foo.dart');
  String ignoredFile = sourcePath('.dart_tool/broken.dart');
  writeFile(validFile, 'class Foo { void baz() {} }');
  writeFile(ignoredFile, 'notDart');

  standardAnalysisSetup();
  await analysisFinished;

  // There should be no errors to start with, as .dart_tool is excluded because
  // it starts with a dot.
  expect(currentAnalysisErrors[ignoredFile], isNull);

  // Specifically adding the file to priority files should result in it being
  // analyzed and generated errors.
  await sendAnalysisSetPriorityFiles([ignoredFile]);
  await sendAnalysisUpdateContent(
      {ignoredFile: AddContentOverlay('notDart')});
  await analysisFinished;
  expect(currentAnalysisErrors[ignoredFile], hasLength(greaterThan(0)));

  // Removing from priority files should result in the errors going away.
  await sendAnalysisSetPriorityFiles([]);
  await sendAnalysisUpdateContent({ignoredFile: new RemoveContentOverlay()});
  await analysisFinished;
  expect(currentAnalysisErrors[ignoredFile], isNull);
}

For testing, I tried flushing the errors when we remove the overlay in AnalysisServer.updateContent:

} else {
  resourceProvider.removeOverlay(file);
  sendAnalysisNotificationFlushResults(this, [file]);  // new
}

However, the errors came back:

6.981725986: ==> {"id":"5","method":"analysis.updateContent","params":{"files":{"/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerDGsVlP/.dart_tool/broken.dart":{"type":"remove"}}}}
6.984448666: <== {"event":"analysis.flushResults","params":{"files":["/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerDGsVlP/.dart_tool/broken.dart"]}}
6.986642018: <== {"id":"5","result":{}}
6.990752108: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
6.992123991: <== {"event":"analysis.errors","params":{"file":"/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerDGsVlP/.dart_tool/broken.dart","errors":[{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerDGsVlP/.dart_tool/broken.dart","offset":0,"length":7,"startLine":1,"startColumn":1},"message":"Variables must be declared using the keywords 'const', 'final', 'var' or a type name.","correction":"Try adding the name of the type of the variable or the keyword 'var'.","code":"missing_const_final_var_or_type","hasFix":false},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/private/var/folders/8r/5r4vmgn94h9fy343vxdvr4_r00hbgw/T/analysisServerDGsVlP/.dart_tool/broken.dart","offset":0,"length":7,"startLine":1,"startColumn":1},"message":"Expected to find ';'.","code":"expected_token","hasFix":true}]}}
6.992828923: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

So I think adding the overlay is doing more than just causing an analysis - I think it's causing the file to "no longer be ignored" in future too. My guess is that it might be related to this code (also in updateContent):

driverMap.values.forEach((driver) {
  driver.changeFile(file);
});

// If the file did not exist, and is "overlay only", it still should be
// analyzed. Add it to driver to which it should have been added.
contextManager.getDriverFor(file)?.addFile(file);

I think maybe this is result in files that are ignored during initial scanning of the FS to be registered and then analyzed from disk after the overlay is removed? I don't think it's as simple as skipping that code for RemoveOverlay calls, because in many cases we do want the analysis to then fall back to the disk version.

@bwilkerson @scheglov LMK if you have any thoughts on how we should handle this (or if you can just fix it faster than explain, feel free! :-))

[DanTup]

Ok I did some more digging into this since it didn't add up. It turns out that the issue isn't specific to overlays, or even priority files :(

For example, here I disabled everything except hovers, and the errors came through when I sent a request for a hover:

[10:16:57] ==> {"id":"3","method":"analysis.getHover","params":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":19},"clientRequestTime":1553768217623}
[10:16:57] <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[10:16:57] <== {"id":"3","result":{"hovers":[{"offset":18,"length":4,"containingLibraryPath":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","containingLibraryName":"","elementDescription":"dynamic dart","elementKind":"top level variable","isDeprecated":false,"staticType":"dynamic"}]}}
[10:16:57] <== {"event":"analysis.errors","params":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","errors":[{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":5,"length":2,"startLine":1,"startColumn":6},"message":"Expected to find ';'.","code":"expected_token","hasFix":true},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":5,"length":2,"startLine":1,"startColumn":6},"message":"Variables must be declared using the keywords 'const', 'final', 'var' or a type name.","correction":"Try adding the name of the type of the variable or the keyword 'var'.","code":"missing_const_final_var_or_type","hasFix":false},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":0,"length":4,"startLine":1,"startColumn":1},"message":"Variables must be declared using the keywords 'const', 'final', 'var' or a type name.","correction":"Try adding the name of the type of the variable or the keyword 'var'.","code":"missing_const_final_var_or_type","hasFix":false},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":0,"length":4,"startLine":1,"startColumn":1},"message":"Expected to find ';'.","code":"expected_token","hasFix":true},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":18,"length":4,"startLine":1,"startColumn":19},"message":"Variables must be declared using the keywords 'const', 'final', 'var' or a type name.","correction":"Try adding the name of the type of the variable or the keyword 'var'.","code":"missing_const_final_var_or_type","hasFix":false},{"severity":"ERROR","type":"STATIC_WARNING","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":8,"length":3,"startLine":1,"startColumn":9},"message":"Undefined class 'not'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'not'.","code":"undefined_class","hasFix":true},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":18,"length":4,"startLine":1,"startColumn":19},"message":"Expected to find ';'.","code":"expected_token","hasFix":true},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":0,"length":4,"startLine":1,"startColumn":1},"message":"Expected an identifier.","code":"missing_identifier","hasFix":false},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":12,"length":5,"startLine":1,"startColumn":13},"message":"Expected to find ';'.","code":"expected_token","hasFix":true},{"severity":"ERROR","type":"SYNTACTIC_ERROR","location":{"file":"/Users/dantup/Desktop/Dart Mini Sample 1/.dart_tool/broken.dart","offset":5,"length":2,"startLine":1,"startColumn":6},"message":"Expected an identifier.","code":"missing_identifier","hasFix":false}]}}
[10:16:57] <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

So it's less clear now where the fix should be. This file was originally excluded from analysis (presumably because it's in a dot-folder) and was not made priority nor given an overlay, but a hover request triggered analysis for it and produced errors. There's no obvious event that should cause us to flush the errors here since we've never added the file to anything that (obviously) opts it in to analysis.

[scheglov]

OK, maybe then we should be more radical and just don't send and errors for files that are not included into analysis. Semantic highlighting and navigation are fine, because they are harmless and clients know when to discard them.

[bwilkerson]

My recollection is that we've had users ask to have full analysis, including errors, for files that they open, whether they're being analyzed or not, but maybe I'm misremembering. I suppose we could do that and see whether anyone requests errors. If they do we can re-add it, but in such a way that when the file is no longer a priority file the errors are removed (or overwritten with an empty list).

[DanTup]

So should the fix be a condition in here?

https://github.com/dart-lang/sdk/blob/3124125008f1e3e68f269fd529b8ae4bfbc79eeb/pkg/analysis_server/lib/src/analysis_server.dart#L590-L594

In if so, is it as simple as just "does any segment of the path start with a dot"?

[bwilkerson]

Maybe. We have that check in several other places (search for startsWith('.'), especially ContextManager._isContainedInDotFolder), and I had expected that they would cover this case, but maybe it needs to be here too.

My personal preference would be to send error notifications for priority files, even if they start with a period or are in a folder that does, but then clear the errors when the file is closed (no longer a priority file).

If we do just shortcut it here in order to solve the problem more quickly, I'd like to see a TODO to re-examine showing errors in that case.

[DanTup]

My personal preference would be to send error notifications for priority files, even if they start with a period or are in a folder that does, but then clear the errors when the file is closed (no longer a priority file).

This sounds sensible, but I think there's more to this. It's not adding the file as priority that's triggering the errors - it's the subsequent requests like hover/fixes/etc.. You can even get the errors by not setting the file as priority, but just sending a getFixes request. This means that flushing errors when a file is removed from priority doesn't cover all the cases.

So I think maybe the changes should be something like:

1. Adding files to priority should immediately trigger analysis of them (which would send the errors)
2. shouldSendErrors should not return true for any file that is not either in analysis roots or priority files (this will stop us sending errors for non-analyzed files just because we got a getFixes or something)
3. Removing a file from priority that is not in analysis roots should flush its errors

I think 2 is important here, because otherwise the client can end up with errors for a file that has no obvious trigger to remove them. While it's not expected that Dart Code would send requests for files that aren't priority, I can imagine many things that could trigger it (for ex. if hovers worked in peek windows that weren't considered "open").

WDYT?

[bwilkerson]

That sounds reasonable to me.

[DanTup]

I'm hoping to look at this soon - though I just hit a nasty issue in VS Code that I think has the same cause (but would not be fixed by the proposal above).

https://github.com/Dart-Code/Dart-Code/issues/1678

When a user hovers over errors in VS Code's Problems view, we're asked for Code Actions (which translates to fixes+assists+availableRefactorings). If the file is not a priority file, these requests to the server appear to result in the file being analyzed and its errors sent back (this is despite the file already being in the analysis roots). The errors coming back results in VS Code refreshing the errors list, which results in it re-requesting the data (because the mouse is over a problem again - it doesn't really know that it's the same problem).

This basically gets stuck in a loop, so hovering your mouse over an error in the Problems view results in high CPU, the error list flickering, and "Analyzing..." appearing/disappearing on the status bar until you move the mouse away.

I'm not sure if this is expected - if a file is in analysis roots, but not a priority file - should requesting fixes/assists result in analysis + errors? The client already has the errors for these files (since they're being analyzed) so it seems strange - but it's possible only priority files analysis data is held in the servers memory? If this isn't intended, I guess it's a different bug. If this is the intended behaviour then I think we should change the proposal above to never send errors except when a while is explicitly being analyzed (either by adding to analysis roots, or priority files), never when analysis is triggered as a result of requesting other data (hovers, fixes, etc.).

WDYT?

For now I'll need to figure out a workaround for VS Code (possibly by just caching the last set of errors and not giving them to VS Code if they're the same as the previous set) because there will be some time before any server fix would make it to a stable flutter release and this seems pretty bad.

[bwilkerson]

As part of an unrelated experiment, I recently wrote some code that would cause server to not send back errors if the list was the same as the last time they were sent. Sounds like we might want to extract that piece and land it.

[DanTup]

That should definitely fix the new issue (or at least, reduce it to only two sets of requests for assists/etc. instead of infinite!), though I think the behaviour of sending errors in response to something like getAssists is still a little unexpected.

Just to confirm - is it expected that only priority files (and not everything analyzed) will be cached, such that asking for assists (etc.) won't result in analysis? For some reason I previously thought that all files in the analysis roots would have their results held on to. If it is only priority files, I might want to revert the change where we only send visible files as priority (we used to send all "open files", but VS Code has a very fuzzy definition of what "open" means).

[bwilkerson]

... though I think the behaviour of sending errors in response to something like getAssists is still a little unexpected.

Looking at it differently, server is just sending errors in response to analyzing a file, without any knowledge of why the file was analyzed. Server doesn't make any guarantees about when a file will be analyzed, so it doesn't make any guarantees about when errors will be sent.

... is it expected that only priority files (and not everything analyzed) will be cached ...

Server doesn't make any guarantees about which files, if any, will be cached. We used to cache everything, but it turned out that it hurt our memory usage and didn't really help with latency, so we changed it. It currently caches priority files and nothing else. This seems to be working fairly well, but if we think of a better way that improves performance, then we're free to change server.

... I might want to revert the change where we only send visible files as priority ...

I think that the set of visible files more closely matches the intent for priority files: files that should be prioritized in order to provide user visible feedback more quickly.

Setting all "open" files as priority files will increase server's memory usage, which might well hurt the UX.

[DanTup]

Looking at it differently, server is just sending errors in response to analyzing a file, without any knowledge of why the file was analyzed.

That's true, but it's the servers decision that reading a file (for any request) should send errors. I think it's what led to both of these issues - the original issue here because if the server does this, there's no appropriate event to flush the errors (you can't "unrequest" a hover for example) and the new issue is because the request was spawned from the list of errors itself which we're (accidentally) replacing.

I think ideally we would only send errors if there was some explicit or implicit request for them (adding to priority files or analysis roots is an implicit request, and there's a clear idea of when that is revoked - unlike if we send errors just because someone asks for a hover or assist).

I think that the set of visible files more closely matches the intent for priority files: files that should be prioritized in order to provide user visible feedback more quickly.

That does make sense - it just occurred to me that some of the things I that we had tied to priority files are not (outline, closing labels, etc.) - they have their own subscriptions, and for those we use "open files". So maybe we're getting the best of both worlds here (and minimising the events when switching between files).

Looking at the logs, we have some hack to "force" subscriptions to be refreshed when you open a file (we send an updateContent with no real change). I need to figure out if we still need that, since that might also be resulting in some events we don't really need...

[DanTup]

Actually, setting a file as priority does still seem to result in notifications for all those other events without my hack. If it's easy to extend your Errors fix to them, it might be worthwhile - though I think for most of these we only update a local cache when we get them anyway (and then they're fetched from the cache on-demand) so it might not make any performance difference for VS Code.

[DanTup]

I found another quirk. VS Code asks us for things like code actions before it fires the visible editors event, which means we ask the server for them before setting priority files, which means the file is opened/analyzed/closed multiple times as a result of one file change. That's why the above screenshot shows all the events fire twice after a single files (just off the top of the log is requests for codeactions/etc.).

I don't think there's a good way to fix that besides hard-coding a short delay in all of the code action providers.

@bwilkerson would it be easy/possible to have the server keep the last file it analyzed in the cache? I think that would solve a few of these issues... It would prevent the errors being sent multiple times when interacting with the errors list, and also prevent multiple analysis passes of the same file when given multiple requests like the above and/or out of order (priority files not coming first).

[bwilkerson]

I think ideally we would only send errors if there was some explicit or implicit request for them ...

Technically, there's always an implicit request for errors for all files within the analysis roots.

... it just occurred to me that some of the things I that we had tied to priority files are not (outline, closing labels, etc.) - they have their own subscriptions, and for those we use "open files".

I'm not sure why you'd want to subscribe to such things for files that are not visible (priority).

... we have some hack to "force" subscriptions to be refreshed when you open a file ...

I wouldn't be surprised if we unintentionally built in an expectation that only priority files would have subscriptions. If we did, that would be a bug.

... would it be easy/possible to have the server keep the last file it analyzed in the cache?

It seems like it ought to be fairly easy (although I think what you're asking for is to have the last file for which an explicit request was made kept in the cache). @scheglov would know better than I.

[bwilkerson]

Unfortunately, I was misremembering. The code I wrote causes server to not return errors if there are no errors to return and there were no errors to return the last time we computed errors. I considered doing the full comparison, but I didn't need to do the extra work for my purposes, so I didn't.

[DanTup]

Technically, there's always an implicit request for errors for all files within the analysis roots.

Heh, yeah, I guess I described that badly. I guess I meant that a request like getHover probably never has any need to send errors. For files outside of analysis roots/priority files, they shouldn't be sent (that's what results in this problem - since there's no "event" to flush them) and for any files inside analysis roots/priority files, the client should already have up-to-date errors from when the file was originally analyzed.

I guess what I was getting at, was that if the code that does analysis had a flag that said whether the reason for analysis was "analysis" versus "to service some other request" maybe it could skip sending errors for any non-analysis paths.

I can't make a good case for the server not being allowed to send errors to the client at any time at all, but it does causes this unfortunate bug and it's similarly hard to argue VS Code is doing something bad here.

I've added a workaround for now that will just drop any error notifications that are identical (in JSON) to the previously received one (and clear the "old" string if errors are explicitly flushed).

I'm not sure why you'd want to subscribe to such things for files that are not visible (priority).

We currently cache this data while a file is open (and update it when we receive notifications) because there are many things driven by this data (closing labels, folding regions, outline view, breadcrumb) that VS Code wants as soon as we switch to a file. If we had to delay the response to wait for the server there would be many places in the UI that are missing their data for a short period every time you toggled between files (and because VS code asks for the data rather than us pushing it, we can't just provide old data initially and then refresh it when new data arrives).

Of course, opening a new file will still show a delay, but I think that's more understandable than if you're just Ctrl+Tabbing between two files (eg. test + implementation) while working. In some cases, this data results in the code visible shifting on the screen (for example outline data is used to inject the test Code Lens links) so the greater the delay between opening the file and us providing that data, the more jarring that change is.

(It could be argued that we don't need to have subscriptions the whole time just to cache for the duration of the file being open though, I'm not sure if there are implications of that change.. the chances of outline data etc. changing while a file is not visible is probably slim, though could happen with things like refactors).

I wouldn't be surprised if we unintentionally built in an expectation that only priority files would have subscriptions. If we did, that would be a bug.

I'll make a note to do some more testing of this this week. The only remaining code sending these is Closing Labels, and the implementation references https://github.com/dart-lang/sdk/issues/30238 which is closed. I think this code may also be from before we started caching data like this for open files, so it may be invalid now.

It seems like it ought to be fairly easy (although I think what you're asking for is to have the last file for which an explicit request was made kept in the cache). @scheglov would know better than I.

Yep, that's what I was asking for. VS Code often asks us for data that results in multiple requests to the server (for example CodeActions results in getFixes, getAssists, getAvailableRefactors) and both VS Code and LSP assume it's valid to ask for this data when a file is not open - currently it results in analysis of the same file repeatedly. I think the chances of multiple requests coming in for the same file sequentially are good - though whether we'd get the full benefit given that in many cases these requests might arrive at the same time, I'm not sure (if the work is async, they could all kick off the same analysis work?).

Unfortunately, I was misremembering. The code I wrote causes server to not return errors if there are no errors to return and there were no errors to return the last time we computed errors.

No worries, I've put in a workaround for now, and I'll thing more about this (to try and address the original reported issue here) sometime very soon.

[DanTup]

A slight change from the plan above - it turns out that adding files to the priority list is not intended to affect whether they'll be analyzed, only prioritise them. This means the only actual fix was to prevent errors being sent for files that would normally be excluded, during responding to requests like hovers.

https://dart-review.googlesource.com/c/sdk/+/101822 https://dart-review.googlesource.com/c/sdk/+/101980/

The consequence of this fix is that you won't see errors for files inside .dart_tool at all (even if you open them). The reason they were coming through before seems accidental (or at least, not logically sound).

If we do wish to provide errors for opened files that are normally not analyzed (for ex. in .dart_tool) then we need to tie it to something like priority files so there's an obvious event to remove them. If that's desired, we should open another issue and handle it separately (I don't know if anyone wants this or not?).

I'll close this when my CLs above land. For the other issue, I'll follow up in another issue later (it's not urgent, I have a workaround and it turns out it's not quite as bad as I thought - VS Code only sends those requests if files are open - though it's not obvious why :-))