What is the purpose of analyzer_plugin's AnalysisErrorFixes.error?

[dark...@gmail.com]

Hello!

I was wondering, what's the purpose of the AnalysisErrorFixes.error property?

It isn't clear to me why plugins need to send a complete AnalaysisError when emitting fixes.

I would expect only the list of PrioritizedSourceChange to matter.

Best regards

[Brian Wilkerson]

This is in the plugin protocol in order to support the legacy protocol for the analysis server (which is used by IntelliJ and AndroidStudio). I don't know whether either of those clients are using the information, but I know that we needed it for some client at some point in the past. Until we remove it from the legacy protocol we can't remove it from the plugin protocol.

--
You received this message because you are subscribed to the Google Groups "Dart Analyzer Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to analyzer-discu...@dartlang.org.
To view this discussion on the web visit https://groups.google.com/a/dartlang.org/d/msgid/analyzer-discuss/fda46f29-046a-492b-933d-2081d7e48620n%40dartlang.org.

[Danny Tuppeny]

This field is still in use by VS Code (both pre-LSP, and now in the LSP server):

https://github.com/dart-lang/sdk/blob/abafed8cfcdd97ad06221a484caa54dedda39fe8/pkg/analysis_server/lib/src/lsp/handlers/code_actions/plugins.dart#L84

The reason for this is to allow you to see a filtered list of fixes from the Problems view. Diagnostics and quick-fix ranges could overlap, and although it's fine to show all fixes in the lightbulb menu from a location in the editor, it's confusing if you saw fixes for different diagnostics when you open the lightbulb menu for a specific diagnostic from the Problems view.

That said, it appears that this behaviour is currently not working in VS Code:

It seems like there's an open issue in VS Code about this, but I'll make a note to double-check it's not extension-specific and something we're not handling correctly.

[Danny Tuppeny]

Re-reading that issue, it seems there's a second way this could be achieved (VS Code also sends the diagnostics to the server when asking for CodeActions) and that's what was actually discussed in the linked issue. It's not clear to me which of these mechanisms is intended to be used, so I've pinged to see if I can get clarification.

That said, I think unless the plugin API was changed to support server forwarding those diagnostics from the incoming request to the plugin, the plugin would still need to return the diagnostic with its fixes for any filtering to happen at all.

[dark...@gmail.com]

I see, this makes sense thanks.

What triggered this question is that the need to specify AnalysisErrorFixes.error makes implementing "getFixes" a lot more complex,

as that method is by default unaware of the AnalysisErrors at the given offset.

It demands "getFixes" to implement some form of caching, to keep a reference on the last emitted AnalysisErrors at a given library.

That, or have getFixes recompute the list of AnalysisErrors, which could be inefficient.

My 2cents on this would be:

- I'd expect the getFixes request to contain the list of errorCodes at the given offset
- I'd expect AnalysisErrorFixes to contain the AnalysisError.errorCode only, instead of the whole AnalysisError

This would enable getFixes to return a list of fixes, without necessarily having to know how to compute AnalysisErrors. 

[Danny Tuppeny]

> What triggered this question is that the need to specify AnalysisErrorFixes.error makes implementing "getFixes" a lot more complex,
> as that method is by default unaware of the AnalysisErrors at the given offset.

Don't you already know the set of errors to produce fixes? That seems to be what server does - it enumarates the errors and checks whether there are fixes for that kind of error.

> My 2cents on this would be:

> - I'd expect the getFixes request to contain the list of errorCodes at the given offset

It seems like that might be what VS Code intends to do, but it's not clear whether it should be the full list or only a single item if you invoke the quick fix list from a single diagnostic on the Problems view (hopefully I'll get a response to this on the issue linked above).

> - I'd expect AnalysisErrorFixes to contain the AnalysisError.errorCode only, instead of the whole AnalysisError

I don't know about the use of this in other editors, but for VS Code we only have the option of providing a full diagnostic. Without knowing how they're using this (again, hopefully a response to my question will give some more clarification) I don't know whether they need all the other information or a "stub" error with just a code would work. Or maybe they don't use it at all and we could drop it without any impact. My guess is that if it is used, they'd want the whole error because I guess in theory you could have multiple overlapping errors that have the same error code and they want to match them up exactly.

[Brian Wilkerson]

I didn't know (or remember) that LSP was also using that information. Thanks.

The plugin protocol is definitely not optimal. It was designed before we had support for LSP and hasn't been updated since. If we decide to support plugins, I expect that we'll do a complete redesign of the protocol (or API, or whatever we have based on the way plugins are supported) to (a) optimize the flow of information between the server and plugins and (b) make it much easier to write plugins than it is today.

I'm sorry that it's so painful to write a plugin, but that's part of the reason that we strongly discourage anyone from writing plugins at this point in time.

[dark...@gmail.com]

> I'm sorry that it's so painful to write a plugin, but that's part of the reason that we strongly discourage anyone from writing plugins at this point in time.

Right. But this is exactly what I'm trying to fix.

I'm aware that the Dart team lacks the resources to work on this, so I'm doing what I can to fix this on my own. Hence this question :)

The Dart community needs something. So much progress is locked behind having a good plugin mechanism that the community can play with.

Having worked on custom_lint for nearly a year now, I'm confident that most issues with analyzer_plugin can be fixed without touching analyzer_plugin at all.