Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable "fix all" options #161

Closed
rrousselGit opened this issue Jun 14, 2023 · 3 comments · Fixed by #223
Closed

Enable "fix all" options #161

rrousselGit opened this issue Jun 14, 2023 · 3 comments · Fixed by #223
Assignees
Labels
enhancement New feature or request

Comments

@rrousselGit
Copy link
Collaborator

Custom_lint should offer tools to more easily:

  • Enable fixing all fixable issues at once. Such as maybe with a custom_lint fix --apply command, similar to dart fix --apply
  • Enable fixing multiple issues within the same file, like "make provider final" vs "make all providers final in file"

The problem: FIx conflicts

There are a few difficulties with fixing multiple issues at once is:

  1. Some fixes may conflict with each other. For example, two fixes may edit the same element, but in different ways.
    In that case, it is unclear what should take over.

  2. After applying a fix, more fixes may need to be applied.
    For example, a fix may change a var into a final. And after that, a warning may suggest changing the final into `const.
    But in the initial run, we didn't know about the final->const change

Possible solutions

On fix conflict, apply the first fix, then recompute the list of fixes.

The idea is to apply fixes from top to bottom (relative to the lint start offset). Then, if a conflict between two fixes arise, we apply the first fix, and re-analyze the file. Once the file is re-analyzed, we reapply the skipped fixes. And we keep going until all fixes have been applied.

Possible concerns:

One worry is that it could cause some form of an infinite loop of Dart change. If a fix triggers new warnings, which involve new conflict on their quick fixes, it could go on indefinitely.

To solve that, we could have a few restrictions:

  • It is an error for a DartFix to conflict with itself.
    Although, what if a LintRule is associated with two DartFixes. And those two fixes conflict with each other in a way?
    Maybe the rule should be "fixes for a given LintRule shouldn't conflict with each other".
    Although it's unclear whether that's a real concern. Cf Problem 3

  • Once a fix has been applied, it cannot be re-applied again, even if there are remaining fixes found in the file.

The idea is that this approach may not fix all problems at once. But it likely would fix most of them.
If there are some unfixed issues, folks could run the command one more time.

Problem 2: Unknown message for "fix all in file" fixes

For implementing the "fix all in file" IDE part, we need to know the message we need to show users

One issue is, we don't necessarily know what the message for a "fix all" would be in the IDE.

To begin with,

Possible solutions: Give a default message based on the lint code.

We could have the message default to fix all <lint_code> in file.
This message may be less clear than a user-defined message, like "add all missing awaits in file". But it's a message that could be filled-in for all fixes.
Of course, we would still provide a way to customize the message.

A problem with this solution is that a LintRule may have two possible fixes, with different messages.
In that case, the "fix-all" would be conflicting with each other.

Solution 2: Have a message getter on DartFix/DartAssist

Currently, the fix message is a parameter of the createChangeBuilder.

Maybe an alternative would be to have the ChangeReporter reporter become a ChangeBuilder directly. And move the createChangeBuilder(message, priority) to be done by custom_lint itself.

That message/priority information would then be determined by getters on the DartFix/DartAssist objects:

class Example extends DartFix {
  @override
  String get message => 'Make provider private';
  @override
  int get priority => 40;

 ...
}

We could then have an optional String get batchMessage for applying the fix in batch.

And we could have the default message be get batchMessage => 'Apply all "$message"'

Problem 3: A LintRule may offer different solutions to the same problem.

Some problems have a less clear-cut solution and may offer multiple choices. In that situation, applying both fixes is unreasonable, as they are likely to conflict with each other.

We could do a few things to help with this:

  • Fixes could optionally be excluded from the "fix all" batch.
  • If a lint is associated with multiple non-skipped fixes (cf above), we could decide to not apply any fix at all in the CLI
  • ^ if we do so, we might want to show a warning in the CLI
  • In the IDE, we can still provide a "fix all" for both options
@dkbast
Copy link

dkbast commented Nov 8, 2023

This would be very useful to write custom migrations e.g. when switching from one package to another this would allow package authors to create an automatic migration strategy.

This could also help with semi automatic refactorings in a large code base where we can "simply" describe what we want to refactor and use custom_lint to execute the refactoring in a "reproducible" way, allowing us to start over "again and again" without loosing progress :)

@rrousselGit
Copy link
Collaborator Author

I would like to implement a fix-all too, yes. I have other things on my list though

@rrousselGit
Copy link
Collaborator Author

I found a way to apply fixes that's fairly simple. I'll give a go to custom_lint --fix :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants