dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.2k stars 1.57k forks source link

Don't instantiate ErrorsResultImpl in analysis_server/ #54058

Open scheglov opened 11 months ago

scheglov commented 11 months ago

I looked shortly what should be done, and I think it is not much. Create separate analysis result class for a text file, and extend it as a Dart file analysis result. Update a few functions in analysis_server/ to use with the new text file result. In most places we don't need analysis session.

bwilkerson commented 11 months ago

Update a few functions in analysis_server/ to use with the new text file result.

I think we should seriously consider putting those functions on AnalysisSession and make them specific to the kind of file being analyzed: analysis_options.yaml or pubspec.yaml. I think the validators used to generate diagnostics are already in the analyzer.

One reason for this opinion is that I expect that plugin authors are going to want to look at these files (to get settings, etc.) and having a clean API for doing so would probably be a good idea. We need to flesh out the plugin support story to know whether that's true, of course. (Which suggests that it might want to be a YamlFileResult rather than just a text file result so that we can also return the result of parsing the YAML.)

In most places we don't need analysis session.

That's true, but from an API design perspective it might be cleaner if all of the getSomeKindOfResultObject methods were on the same interface.

scheglov commented 11 months ago

The functions in the analysis_server/ that I mentioned are for example

AnalysisError newAnalysisError_fromEngine(
    engine.AnalysisResultWithErrors result, engine.AnalysisError error,
    [engine.ErrorSeverity? errorSeverity]) {

And you probably reference analyzeAnalysisOptions that indeed already lives in package:analyzer. Do you suggest that we move analyzeAnalysisOptions from a top-level function into AnalysisSession?

I'm not sure what this will give us.

I was thinking only about internal use of these functions, and how we expose diagnostics in IDEs. If there are API uses, we probably need to know what they are to add something more.

In general I like YamlFileResult, as it follows the principle that I posted about some time ago - parse, don't validate. Maybe even specific subclasses that work with specific YAML formats - analysis_options.yaml and pubspec.yaml.

Still, both the current Dart results, and YamlFileResult are TextFileResults, so adding it as a common interface would be not wrong.

pq commented 9 months ago

See related discussion in https://dart-review.googlesource.com/c/sdk/+/347126.