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.19k stars 1.56k forks source link

[Analyzer Plugins] Inaccurate/insufficient documentation and potential bugs #44546

Open zmeggyesi opened 3 years ago

zmeggyesi commented 3 years ago

I'm trying my hand at a custom Analyzer plugin, following the documentation in this repo, but I'm running into quite a few obstacles.

I'm trying to build an eventually-public plugin to assist in building localizable apps, primarily in Flutter, but for now, it's an uphill battle. Is there some more comprehensive documentation of the analyzer plugin system available, or a full minimalist plugin example/framework?

zmeggyesi commented 3 years ago

Some more gripes in naming that are making understanding the system harder than it ought to be:

zmeggyesi commented 3 years ago

This might actually be a bug, but the API documentation states

The set of errors included in the notification is always a complete list that supersedes any previously reported errors.

But my experience is that the Analysis Server retains the previous error reports for a given file, so the items accumulate by doubling on each change. I tried sending empty error reports, but it didn't do anything, the server did not clear out the issue display.

bwilkerson commented 3 years ago

... the Analysis Server retains the previous error reports for a given file ...

Yes, if the plugin id and the file path are the same, then that would be a bug.

In case you want to look into the bug before we have a chance to do so, the code that processes the notification from the plugin is here: https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/plugin/notification_manager.dart#L83.

You might also want to enable the generation of a protocol log (using --protocol-traffic-log=absoluteFilePath) so that you can see exactly what information is being passed between the plugin and the server.

zmeggyesi commented 3 years ago

@bwilkerson Thank you for responding, even during the holidays. Your dedication is appreciated!

Unfortunately, it seems like the plugin ID is the same, and the file path is definitely the same. I have filed another bug report as #44551.

That said, it looks like the function merging the errors only does so across a single run, and not across discrete analysis runs, and even then, deduplication seems to be considered, but not implemented (based on the TODO comment)?

bwilkerson commented 3 years ago

Sorry for the long delay this time.

That said, it looks like the function merging the errors only does so across a single run, and not across discrete analysis runs ...

I'm not sure what you mean by "discrete analysis run", but it might not correspond to the way the server works. When a file is changed the analysis server does the minimum amount of work necessary in order to update the client's view of the state of the code. In other words, the analysis server is doing incremental analysis across the code base. At the moment it's doing a full analysis of every file whose state might have changed, but that's an implementation detail, not a requirement of the server.

The server assumes the same is true of plugins. In order to handle this, when either the server or a plugin performs some analysis on a file and produces diagnostics, the notification manager replaces the previous diagnostics from that source with the new diagnostics, then merges the most recently reported diagnostics from all of the sources into a single list and sends that new list to the client.

Because the cache of diagnostics is indexed by both the file path and the plugin id, it's hard to imagine that the old results are not being replaced if the file path and plugin id are the same from one analysis to another.

... and even then, deduplication seems to be considered, but not implemented (based on the TODO comment)?

That's correct. Deduplication is not implemented.

bwilkerson commented 3 years ago

As for some of your other comments ...

The minimal plugin shown in the documentation doesn't seem to work.

That's true. The documentation was not intended to provide a working example. We had plans to create a working example in the examples directory, but have not yet had an opportunity to do so.

The Fixes Contributor example is showing a different signature for the overridden function ...

Thank you for letting us know. I will send out a CL shortly to fix this.

The same example mentions the MyErrorCode class, but the tutorial doesn't bring any examples of how the plugin can raise an analysis issue.

That's true. Unfortunately the documentation is incomplete. The expected behavior of a plugin is that when it receives an analysis.setContextRoots request that it will perform any appropriate analysis on the files in those analysis roots. It should also perform analysis when any file is changed via analysis.updateContent. In the course of performing that analysis is should create instances of AnalysisError, and when each file's analysis has completed it should use the analysis.errors notification to let the server know what diagnostics are being reported by the plugin.

The version field of the plugin is ...

Thank you for letting us know. The comment will be fixed.

The Package Structure documentation uses <host_package>/tools/analysis_plugin as the location for the canonical bootstrap package, where it's actually <host_package>/tools/analyzer_plugin.

Thank you for letting us know. The documentation will be fixed.

Further, the Dartdoc around the plugin_locator ...

Thank you for letting us know. I don't remember why the code was left commented out, other than the unhelpful comment in the TODO. As confusing as it is for the documentation to not match the implementation, I'm going to leave this one as-is until someone has a chance to look at it in more detail.

Speaking of the version field, it's not mentioned anywhere how exactly compatibility is determined ...

That's because the intent was to implement this such that plugin authors wouldn't need to be concerned with the details. As you probably know, it's a simple version comparison.

It is especially not mentioned that the Analysis Server uses a hardwired version of 1.0.0-alpha.0 in the request ...

The version in the request is the highest version of the plugin protocol that the server currently supports, so it makes sense that it's hard coded.

What might not be clear is that the comparison is based on the version of the protocol that server supports and the version of the protocol that the plugin is assuming, not the version of the server itself which is an independent version number.