Provide a way to allow lint to provide quick fix

[a14n]

I saw some quick fixes for lint in the sdk but it would be really cleaner to have those fixes in the linter package next to/into the lint code.

[pq]

cc @bwilkerson

[bwilkerson]

I agree, but given our current priorities that's a slightly longer term project. We are getting close to beta for the new plugin architecture, and I'm hoping that we can turn the linter into a new-style plugin after that project is done (I'm just not sure how soon).

[a14n]

Would it be possible to design an API (actually the type of an additionnal quickfix optional named argument on LintRule constructor) that would allow to start writing quickfixes inside rule today and bind it inside analyzer later?

[bwilkerson]

Possibly (I'm not sure I fully understand what you have in mind), but I don't think it would be worth the effort to both write it now and remove it later. It really isn't that hard to contribute code to the 'sdk' repo, so I'd encourage you to try doing that before trying the more heroic effort of adding additional hooks.

[a14n]

:)
I'm not that heroic.

[pq]

I've wanted an API for quickfixes since the beginning of this project. Unfortunately as time has marched on priorities just haven't aligned. Since there is a path forward and better ergonomics are not on a current road-map I'm going to go ahead and close this.

Shedding a little tear as I do... 😢

[a14n]

😢

[pq]

OK, so I don't really want to see this closed either. Re-opening and flagging as a long-term goal.

[pq]

(Also flagging as "help wanted" 😄)

@scheglov / @bwilkerson : when you have a moment, perhaps update with current thinking? Given how many contributions we see here (vs. in the SDK), a more ergonomic way to add fixes might be a big win.

Thanks!

[a14n]

I'd also like to know if/when we will update linter to become a analyzer plugin. In this case does the plugin architecture allow to plugin to provide quickfixes?

[bwilkerson]

I can give you my current thoughts. :-)

... a more ergonomic way to add fixes might be a big win.

I think it's much easier than most people realize to add a quick fix for a lint. Maybe we need to document this somewhere (other than here :-) to help allay concerns. Basically:

1. Define a fix kind similar to this one: https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/services/correction/fix.dart#L171. If there's more than one fix you're adding for a given lint then you'd add more than one fix kind.

2. Add a new if similar to this one: https://github.com/dart-lang/sdk/blob/master/pkg/analysis_server/lib/src/services/correction/fix_internal.dart#L527 that calls one or more _addFix methods (one method per fix kind).

3. Implement the invoked methods to check whether the associated fix being added can be applied at the current location, and if so, generate a change and add it.

Note that all of that work would be the same even if the linter were a plugin. Only the file paths would change.

As for getting it committed. Just create a PR in the SDK repo. It will automatically be turned into a Gerrit CL. (I can't remember whether review comments are copied back to the PR, so you might have to deal with Gerrit to see them.) If changes need to be made, just update the PR and the updates will automatically be copied over. The barrier to entry here is fairly low.

I'd also like to know if/when we will update linter to become a analyzer plugin. In this case does the plugin architecture allow to plugin to provide quickfixes?

Yes, the plugin architecture allows plugins to provide quick fixes, and making the linter a proper plugin would satisfy this request.

We have discussed several times whether to make the linter a proper plugin. There are four issues.

The first is that the command-line analyzer doesn't yet support plugins. This is a major hole in our support, but one that we just don't have the resources to fix at the moment. I consider this to be a blocking issue (though I'll mention a possible alternative in the next item).

The second is usability. Users have to opt in to plugins. That means that every user would be required to add lines to their analysis options file to enable the linter, and would be required to have a dev dependency on the linter package. That's a significant step backward. (That said, I've considered before making the linter a "built-in" plugin; adding support to server to load the linter without requiring users to add either the enable directive or the dependency. I don't know how much work that would end up being or what would be required to do so, and we haven't had time to pursue this line of inquiry.)

The third is performance. We have not measured the performance of plugins compared to in-process analysis. I don't expect performance to be hurt by making the linter a plugin, but I'd like to see that confirmed before committing to it. (And on the plus side, if we accidentally shipped a broken lint, server would likely be more stable because it's written to be defensive wrt broken plugins.)

The fourth is resources. We don't currently have resources to apply to converting the linter into a plugin.

[a14n]

Thanks a lot for all those informations! This comment should even be copy-paste to doc/WritingLints.MD.

After some contributions to the sdk I find the workflow with Github better:

• on sdk I have to update pubspec.yaml
• on Gerrit I cannot have a quick feedback about CI tests/format like with Travis
• the github PR don't mention the Gerrit link and if the Gerrit mail is delete, it's not simple to find your own CL.

So even if it's not that hard I'd prefer to make changes in a single simple place (the linter codebase on github)

The fourth is resources. We don't currently have resources to apply to converting the linter into a plugin.

It could be worth to have some guidelines in an issue about how to move the linter into a analyzer plugin. Someone could perhaps provide some help...

[pq]

Thanks for this feedback @a14n. It's really useful to get a better perspective on SDK contribution friction. (And you're one of the perseverant ones!)

FYI @JekCharlsonYu and @devoncarew

[bwilkerson]

It could be worth to have some guidelines in an issue about how to move the linter into a analyzer plugin.

If you're talking about the style of plugin supported today, there are doc for this in the package 'analyzer_plugin'. I'm sure they could be improved, so if you take a look at them I'd love to get feedback.

[scheglov]

I think that performance is a significant problem.

Lints operate over fully resolved ASTs, and moving linter into a plugin means that we will double the number of expensive operations to parse and resolve files.

[pq]

Now that the linter is in the SDK, I think this is less of an issue. (Or at least different enough that we can close out this conversation in favor of the various ones ongoing about plugin support.)