Sdk: Errors reported in SDK files after Go-to-definition (1.23.0-dev.2.0)

[DanTup]

@bwilkerson I'm not sure whether I should be raising issues in pre-release SDKs like this; on one hand it might help you find bugs before release, but often you already know about/have fixed them. If you'd rather I waited for release candidates, let me know!

In 1.23.0-dev.2.0 if I hi Go-to-definition on an SDK class (eg. List), when that file gets opened in my IDE I get a bunch of errors reported in that file:

The log for this is huge (it seems to have those errors reported a huge number of times) so I won't attach it unless you think there's value in it (in which case I can try and trim it down).

[bwilkerson]

@DanTup Thanks! Actually, we prefer to get reports sooner rather than later; they're generally easier to track down.

@scheglov It looks like we're not getting the driver configured correctly when analyzing code in the SDK.

[scheglov]

I don't see errors reported from DAS on Mac.
Might be a Windows specific issue in the driver.

[DanTup]

@scheglov If a log of the service traffic or an instrumentation file will help at all, let me know.

[scheglov]

I can reproduce the problem on Windows with VSCode.

Also by some reason Analyzing... blinks long time when I navigate to a SDK file.
This is not a good symptom :-)

I need help with two things.

1. How to specify to run Analysis Server from sources instead of snapshot? This way I will able to make changes and see results quickly.
2. How to specify additional arguments for Analysis Server? I'd like to turn on additional logging.

[DanTup]

1. This isn't currently possible, though it's a great idea. If you can tell me how you would invoke it (normally we do {sdkPath}/bin/dart {sdkPath}/bin/snapshots/analysis_server.dart.snapshot) I can add an option to support this.

2. If you Ctrl+Shift+P to get command window and type settings to find Preferences: Open User Settings that will let you edit settings as a JSON object. If you add an option called dart.analyzerAdditionalArgs as an array you can provide additional args (note: it won't appear in intellisense and you will get a warning that it's not valid; this is just because it's a "hidden" option).

Here's some of the options that might be useful. Changing some of them (which require restarting the Analyzer) should prompt you with a Reload button.

"dart.analyzerDiagnosticsPort": 8081,
"dart.analyzerObservatoryPort": 8082,
"dart.analyzerLogFile": "M:\\Coding\\Applications\\Dart-Code\\analyzer.log",
"dart.analyzerInstrumentationLogFile": "M:\\Coding\\Applications\\Dart-Code\\analyzer.instrum.log",
"dart.sdkPath": "M:\\Apps\\Dart-1.23.0-dev.2.0",
"dart.analyzerAdditionalArgs": "--enable-new-analysis-driver"

[DanTup]

That crazy flickering might be caused by getFixes triggering analysis? It seems that Dart Code gets the error, sends a getFixes and then as well as getting the response it gets sent the error again, which results in it starting over (and loops forever):

[17:05:18]: ==> {"id":"36","method":"edit.getFixes","params":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2459}}
[17:05:18]: <== {"id":"35","result":{"fixes":[{"error":{"severity":"WARNING","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Undefined class 'EfficientLengthIterable'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'EfficientLengthIterable'.","code":"undefined_class","hasFix":true},"fixes":[{"message":"Create class 'EfficientLengthIterable'","edits":[{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","fileStamp":0,"edits":[{"offset":18652,"length":0,"replacement":"\r\n\r\nclass EfficientLengthIterable {\r\n}"}]}],"linkedEditGroups":[{"positions":[{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":18662},{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478}],"length":23,"suggestions":[]}]}]}]}}
     // I don't think this should have happened?
[17:05:18]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[17:05:18]: <== {"event":"analysis.errors","params":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","errors":[{"severity":"ERROR","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Undefined class 'EfficientLengthIterable'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'EfficientLengthIterable'.","code":"undefined_class","hasFix":true},{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Classes can only implement other classes.","correction":"Try specifying a class, or remove the name from the list.","code":"implements_non_class","hasFix":false},{"severity":"ERROR","type":"STATIC_TYPE_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":5322,"length":22,"startLine":137,"startColumn":16},"message":"A value of type 'List<E> (M:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)' can't be assigned to a variable of type 'List<E> (m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)'.","correction":"Try changing the type of the variable, or casting the right-hand type to 'List<E> (m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)'.","code":"invalid_assignment","hasFix":false},{"severity":"ERROR","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":8420,"length":6,"startLine":239,"startColumn":17},"message":"Undefined class 'Random'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'Random'.","code":"undefined_class","hasFix":true}]}}
[17:05:18]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}
[17:05:18]: ==> {"id":"37","method":"edit.getFixes","params":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2459}}
[17:05:18]: <== {"id":"36","result":{"fixes":[{"error":{"severity":"WARNING","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Undefined class 'EfficientLengthIterable'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'EfficientLengthIterable'.","code":"undefined_class","hasFix":true},"fixes":[{"message":"Create class 'EfficientLengthIterable'","edits":[{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","fileStamp":0,"edits":[{"offset":18652,"length":0,"replacement":"\r\n\r\nclass EfficientLengthIterable {\r\n}"}]}],"linkedEditGroups":[{"positions":[{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":18662},{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478}],"length":23,"suggestions":[]}]}]}]}}
     // Same here?
[17:05:18]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":true}}}
[17:05:18]: <== {"event":"analysis.errors","params":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","errors":[{"severity":"ERROR","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Undefined class 'EfficientLengthIterable'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'EfficientLengthIterable'.","code":"undefined_class","hasFix":true},{"severity":"ERROR","type":"COMPILE_TIME_ERROR","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":2478,"length":23,"startLine":54,"startColumn":35},"message":"Classes can only implement other classes.","correction":"Try specifying a class, or remove the name from the list.","code":"implements_non_class","hasFix":false},{"severity":"ERROR","type":"STATIC_TYPE_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":5322,"length":22,"startLine":137,"startColumn":16},"message":"A value of type 'List<E> (M:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)' can't be assigned to a variable of type 'List<E> (m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)'.","correction":"Try changing the type of the variable, or casting the right-hand type to 'List<E> (m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart)'.","code":"invalid_assignment","hasFix":false},{"severity":"ERROR","type":"STATIC_WARNING","location":{"file":"m:\\Apps\\Dart-1.23.0-dev.2.0\\lib\\core\\list.dart","offset":8420,"length":6,"startLine":239,"startColumn":17},"message":"Undefined class 'Random'.","correction":"Try changing the name to the name of an existing class, or creating a class with the name 'Random'.","code":"undefined_class","hasFix":true}]}}
[17:05:18]: <== {"event":"server.status","params":{"analysis":{"isAnalyzing":false}}}

I can split that out into another issue if you want?

[scheglov]

Yes, it seems that we send errors after getFixes.

We wouldn't do this if the file was made a priority with analysis.setPriorityFiles, because we cache resolved ASTs for priority files, so additional requests, like getFixes or getAssists use this cached AST instead of analyzing again (and sending back found errors).

[DanTup]

We wouldn't do this if the file was made a priority with analysis.setPriorityFiles

Ah, interesting - looks like we currently don't include any files outside of the workspace root in priority files. I presume it would make sense for me to remove this restriction?

This doesn't happen in 1.22, but I wonder if that's a side-effect of it not returning any errors (therefore we don't send getFixes).

[DanTup]

Ah, in older SDKs we can't send priority files outside of the workspace:

Unanalyzed files cannot be a priority: 'm:\Apps\Dart-1.19.0\lib\core\list.dart'

That's why I added this code. I can do a version check and send them for >= 1.23 though I'd like confirmation that's the best fix first (this will presumably affect other IDE integrations too).

[scheglov]

Yes, I think it is better to remove this restriction.

Analysis Server now returns errors for files in analysis roots, and for open files.
Not sure if this is good though.
We need to be able to compute all errors for all files in the driver.
But this does not mean that we have to send this errors from Analysis Server.

[DanTup]

Yes, I think it is better to remove this restriction.

As mentioned above; I'll have to make this conditional because old SDKs return error if I send non-workspace files in setPriorityFiles. Is checking SDK version >= 1.23 the best way for me to do this?

[scheglov]

Yes, I think checking for >= 1.23 will work.

[bwilkerson]

That might work in this case (I don't know for sure either way), but in general if we're changing the behavior of the server API, then we need to change the server API version, and that's what needs to be tested.

[DanTup]

That makes sense; though if this behaviour change (the ability to send priority files that are outside the workspace root) came with the new driver, might it differ based on a switch within the same version? Or would the new driver have returned a different version number to the old driver within the same SDK?

[zoechi]

Looks like a dup of #28930

[DanTup]

For now (to fix the loop) I've stopped sending getFixes for non-workspace files (matching setPriorityFiles). I've also raised another case about changing the setPriortyFiles behaviour in a future version.

@scheglov I've pushed this in Dart Code v1.0.1 so if you're using Dart Code to debug this issue (with errors coming back unexpectedly) you should avoid updating.

[scheglov]

With this CL we will stop sending errors for files outside of analysis roots.

[scheglov]

I don't see errors and flickering with this change.

[DanTup]

@bwilkerson @scheglov Could you confirm the correct version of the Analzyer I should use when deciding whether to send files outside of analysis roots in priority files? I'm not sure exactly how this is versioned/released so I'm not sure which version to pick (I'm trying to implement https://github.com/Dart-Code/Dart-Code/issues/268, something which I suspect other IDEs will need to also do).

[scheglov]

If you go to the change, there is a list of versions in to which it is included.

[DanTup]

Brian said it's best to use the Analyzer API version for things like this but that's only the SDK version?

[bwilkerson]

Yes, I still think that DartCode should use the server API version to determine changes in behavior. Unfortunately, looking at the CL (https://codereview.chromium.org/2734673002), it doesn't look like we updated the server version. That seems like an oversight we should fix so that you have some way of knowing what to expect from server.

[DanTup]

@bwilkerson I think the important change was actually the one that allows non-workspace files to be sent to setPriorityFiles (since that's the behaviour I need to change based on the API version).

The version number in the spec was last increased by @devoncarew a month ago. I don't know whether that change was before or after the change to remove the error but I think it'll be safe for me to use 1.18 to trigger this change in Dart Code. At worst some old 1.23-dev SDKs might give errors depending on the ordering of those two changes but I'm not too concerned about that; people on dev SDKs are likely on newish versions of them and it's somewhat expected they may have issues.

Btw, I think the old behaviour probably wasn't documented correctly - in the spec for setPriorityFiles it said:

Note that this request determines the set of requested priority files. The actual set of priority files is the intersection of the requested set of priority files with the set of files currently subject to analysis. (See analysis.setSubscriptions for a description of files that are subject to analysis.)

This suggests to me that it wouldn't crash but would silently ignore priority files that aren't analyzed. I guess this means the spec doesn't need updating for the new behaviour but I wonder if it's worth adding a note that <= 1.17 analyzers will actually return an error if you send non-workspace files in case anyone is using the spec to build an integration and not testing with multiple SDKs.