Autofix Automatic/Suggested fixes with --fix

[evanrittenhouse]

Discussion started from here.

Copy-pasting the write-up that I posted there:

CLI

1. --fix - changed to fix only Automatic fixes and rules in the fixable set
2. --fix-unsafe - new option, will fix Automatic and Suggested fixes. While it differs from the implementation's naming conventions, as a user "safe" and "unsafe" make more sense, IMO. From the fixer's perspective, an "automatic" or "suggested" fix makes sense. I think it's clearer for users to consider fixes as "this is safe to implement" or "this is unsafe". Manual changes will never be fixed
3. --diff - will output only Automatic fixes and rules in the fixable set
4. --fix-only - will only fix Automatic fixes and rules in the fixable set

Rules with a Manual applicability will never be fixed, nor will rules in the unfixable set.

Configuration File

fixable/unfixable remain as normal, preserving backwards compatibility.

Implementation

Consider the example where we're enforcing default rules (e.g., E, F) and F841 (unused variable).

The Fix gets generated here - assume that we're going with the override field as well. The Fix::suggested() constructor would assign an override: None. The user then runs --fix. Ruff goes out and fixes all Automatic fixes, leaving F841 untouched.

Now assume that the user wants to fix all Automatic fixes, with F841 added. They F841 into the fixable set in their pyproject.toml file. The fixable/unfixable tables would essentially operate as ways to say "I always want this fixed" or "I never want this fixed".

We need to somehow tell F841 that its Applicability has to be overridden. This can be done in a few ways (and thanks for pushing back, I think the override field is worse):

1. An override field. We could add a set_override() method to Fix which allows you to pass an Applicability, setting override: Some(new_applicability). The downside here is going through and adding set_override() calls everywhere, and it adds memory, like you said.
2. preferred: Checker handles all fixing logic itself, now based on the CLI command and the contents of fixable/unfixable. The upside here is no change is required on a per-rule basis - checker.patch(Rule::UnusedVariable) would remain true and generate the fix.

If we go with the second option, Checker would see that the "base applicability" (determined by seeing that the user ran --fix and not --fix-unsafe) is Automatic, and that fixable contains F841. Regardless of applicability, patch() would therefore return true, and we'd generate the fix. All fix generation would be gated through Checker.patch().

What do you think?

---

@MichaReiser responded with 5 questions which I'll answer here.
1. --fix-unsafe: I think its good to have the option. We can align on the final naming later.
Cool. We could also call it --fix-all or something to denote that it fixes all Automatic and Suggested rules. But yeah, semantic changes should occur at the end.

2. --fix-only: Should --fix-unsafe enable fixing the suggested fixes too?
Yes. If you think of Applicability like a hierarchy, we should default to the highest level (i.e. Automatic - we don't want to do anything with Suggested if the user hasn't specified it) but always use the lowest level that the user specifies. If the user specifies --fix-unsafe, then --fix-only and --diff should include Suggested fixes.

3. --diff: Should --fix-unsafe enable the suggested fixes? I think it should be aligned with --fix-only, considering that --diff implies --fix-only.
See above

4. --diff: We could show diffs for manual fixes. I'm not recommending that we should do this. We may decide to only show the fixes as part of the MessageEmitters in the future.
If we go with what I described in option 2, then I don't think --diff should show them. In my opinion, --fix-only and --diff should always show the same rules, gated by what sort of "fix" command (e.g., --fix vs. --fix-unsafe) the user provides.

5. Configuration: How would I promote a single suggested rule?
The contents of the fixable/unfixable sets in pyproject.toml should trump any fix level that we've applied, IMO.

In the example above, the user wants to only promote F841, which we've marked as Suggested. Since it's Suggested, --fix won't do anything to it. If the user adds it to pyproject.toml#fixable, we will forcibly include it in the fixed set, even if the user calls --fix. Similarly, if a user wants to forcibly exclude a rule from the set, they can add it to unfixable. If a rule is in there, we will never fix it, even if it is Automatic.

[MichaReiser]

Thanks @evanrittenhouse. Sounds all good to me. A follow up on

5. Configuration: How would I promote a single suggested rule?

The way I understand fixable (I very well might be mistaken) is that adding a rule to fixable overrides Ruff's default. That means, promoting F841 requires me to add all automatic fixes to fixable and adding F841. We can decide that this is okay and that we defer adding a new setting to later.

[evanrittenhouse]

Spoke with @MichaReiser offline. We're leaning towards keeping this as an unsolved problem for now. P0 release won't include a way for users to pass --fix and manually include Applicability::Suggested fixes.

[MichaReiser]

preferred: Checker handles all fixing logic itself, now based on the CLI command and the contents of fixable/unfixable. The upside here is no change is required on a per-rule basis - checker.patch(Rule::UnusedVariable) would remain true and generate the fix.

Where do you think it would be best to add the logic to filter out suggested fixes?

[evanrittenhouse]

@MichaReiser if that sounds acceptable, I can go ahead and start on the implementation.

[MichaReiser]

To get the Applicability over to Checker, I think it makes sense to put the base_applicability on Settings.

I wouldn't add the base_applicability to Settings because Settings only contains fields that are present in the ruff.toml. But what I understand is that you're now recommending adding it to FixMode instead. Adding it to FixMode does make sense to me, but:

• I don't think we need to store the applicability for Generate because we can show all fixes in this mode.
• For Apply (and maybe Diff); I recommend a boolean flag instead: apply_suggested because that's the only information we need

Since, at this point, we have access to the base_applicability and all diagnostics, it should be relatively simple to filter either during or after Message construction (the latter could be done in Emitter.emit(), since we have access to the underlying fix).

What's the use case for filtering messages? My understanding is that we always show all fixes. --fix-unsafe only controls whether we apply suggested fixes (not whether we show them)

We also need to be careful about caching or include --fix-unsafe in the cache key (I would prefer not to do so)

[evanrittenhouse]

@MichaReiser The message filtering would occur with --fix-only and --diff:

1. --fix-unsafe: I think its good to have the option. We can align on the final naming later.
   Cool. We could also call it --fix-all or something to denote that it fixes all Automatic and Suggested rules. But yeah, semantic changes should occur at the end.
   
   2. --fix-only: Should --fix-unsafe enable fixing the suggested fixes too?
   Yes. If you think of Applicability like a hierarchy, we should default to the highest level (i.e. Automatic - we don't want to do anything with Suggested if the user hasn't specified it) but always use the lowest level that the user specifies. If the user specifies --fix-unsafe, then --fix-only and --diff should include Suggested fixes.
   
   3. --diff: Should --fix-unsafe enable the suggested fixes? I think it should be aligned with --fix-only, considering that --diff implies --fix-only.
   See above

So the way I envision it is if they pass --fix-only without --fix-unsafe, it'd fix Automatic fixes (same with --diff). That means that we'd need to filter out Suggested fixes from that output.

[MichaReiser]

That means that we'd need to filter out Suggested fixes from that output.

I think we mean the same thing but use different terminology.

What I mean by output is the diagnostics that we render to the users. Or the message when applying --fix: Fixed 5, 1 remaining.

I expect suggested fixes to be reported in the CLI output regardless of the --fix-unsafe option.

But we should only apply the automatic fixes for --fix and only print the diffs for the automatic fixes for --diff.

[evanrittenhouse]

Yep, agreed - sorry for the confusion!

[MichaReiser]

Fix Applicability API Changes

• Change the semantics of --diff, --fix and --fix-only
  • They only apply automatic fixes (without providing the --fix-unsafe option)
• Add --fix-unsafe or --fix-suggested option
  • Enables the suggested fixes for --fix-only, --fix, and --diff
  • Future: Has no effect on the fixes ruff shows in diagnostics
• Configuration:
  • Keep as is.
  • No support for manually demoting an automatic fix to a suggested fix
  • Promoting a suggested fix to an automatix fix is done by adding it to fixable. Requires adding all other fixable rules.
• Implementation
  • Extend FixMode and add apply_suggested to Fix and Diff
  • Filter out suggested fixes before applying them or printing them in Diff.

@evanrittenhouse is this summary accurate? I want to propose the change to the team and want to make sure I have all the facts right

[MichaReiser]

@evanrittenhouse these are just some scratches and unfinished thoughts.

The change looks good overall. Nice job! The things that came up in the discussion are:

• We agreed on the semantics
• --fix-unsafe feels strange when using in conjunction with --diff. Potentially remove the --fix prefix (downside, unclear that this is a fix specific option)
• Using fixable to promote a rule feels strange because it breaks with the symmetry of fixable and unfixable. Also, requiring to add all automatic fixes when promoting a suggested rule feels very cumbersome. That's why we're leaning towards removing that feature and wait to see if users need a way for promoting rules.

That said, this change breaks the workflow of users that use fix = true in their configuration because they can't opt in to the same set of rules as they used to before (some of them may now be unsafe). We discussed a few options but didn't come to a conclusion:

• Add unsafe = true to the setting: This would mirror the CLI API. Micha's concern is that we shouldn't encourage to run all rules by default (because it's unsafe after all).
• Add an option to manually promote suggested rules other than using fixable (e.g. fix.safe = [] or fix.extend_safe=[])
• ...

We'll need to solve this before shipping but it doesn't prevent us from working on a prototype implementation.

[evanrittenhouse]

Cool, I'll get to work on a prototype then.

IMHO, I think the easiest solution would be to add unsafe = false by default and add something like fix.safe = [] to the config. Users could then specify individual rules via the latter option if they want to, and we protect the base case with unsafe defaulting to false. That's just me though :) I'm sure additional options will come up as we iterate through the PR.

[MichaReiser]

I realised that there's another reason why we shouldn't use fixable to promote suggested fixes. The reason is that the fixability doesn't depend on the rule. Ruff has rules where the fix can either be automatic, suggested, or manual, depending on the specific violation. This makes it ambiguous what adding a rule to fixable means. Do only the automatic fixes become fixable or should the suggested fixes be applied to?

That's why I think a fix.safe = [] option (we could potentially allow fix.safe = "YOLO" (for @charliermarsh :P) is preferred. This still has the downside that it's an all-or-nothing opt-in for a specific rule. Meaning, they can't specify that the fix is safe only in circumstance x.

One concern that I have with safe is that:

The discussed semantic of safe is that it adds a suggested to the automatic rules set. However, the name gives the impression that only the listed rules are safe, replacing all other safe fixes.

Maybe we need a pair of option, similar to fixable and unfixable:

• fix.unsafe: Initialized with all suggested rules by default. Lists the fixes that are not applied without using --unsafe. Similar to fixable that gets initialized with all rules

• fix.safe: Empty by default. Promotes suggested fixes to be applied without the --unsafe. Similar to --unfixable that is empty by default

• You can enable all suggested rules by setting fix.unsafe to an empty list.

• You can demote an automatic rule by adding it to fix.unsafe. We may need a extend_unsafe so that you can do this without having to list all suggested rules. But we may want to leave this for later.

• You can promote a suggested rule by adding it to fix.safe.

My main struggle with this is that it is somewhat unintuitive. My expectation would be that fix.safe gets initialized with all *automaticrules andfix.unsafeis initialized with all *suggested* rules. The problem with that is that either settingsafeorunsafe` overrides all other rules, which is annoying to use.

[MichaReiser]

fix.unsafe and fix.safe would only work if they are initialized as empty lists, because initializing safe with all rules would promote all rules to safe, and initializing unsafe with all rules would demote all fixes. We could initialize them as empty lists, but we would then need to come up with better names: promoted, demoted, promote_suggested = [], demote_automatic = []?

An alternative format, that has no precedence in our configuration, is to use a dictionary instead:

fix.overrides = {
	"RUF100": "automatic",
	"RUF101: "suggested",
}

[evanrittenhouse]

@MichaReiser IMHO a dictionary is cleaner. Seems to be the easiest way to express what we want to express. What do you think?

[zanieb]

I've been thinking about this as part of our overall CLI design, here's a relevant excerpt:

Opt-in to applicability

A --fix-suggested option should be added to ruff check to support opt-in to fixes with a “Suggested” applicability level. This flag implies --fix.

Why not unsafe?
Previously, a --fix-unsafe flag was suggested. However, “unsafe” fails to differentiate between manual and suggested fixes and introduces a split in the user-facing and internal names. By using “suggested” and “manual” separately, we are explicit about what behavior will change.

Adjusting fix applicability by rule

Settings should be added to support adjusting the applicability of fixes for rules:

• extend-automatic-fix: Promote suggested fixes for a set of rules to be treated as automatic fixes
• extend-suggested-fix: Demote automatic fixes for a set of rules to be treated as suggested fixes
• extend-manual-fix: Demote suggested or automatic fixes for a set of rules to be treated as manual fixes

Rules can have fixes at different applicability levels depending on the context. Fixes will never be promoted from manual, if attempted a warning will be displayed (we cannot know until a violation is detected if a rule's fix will be manual — if we include the possible applicability values on the rule itself we can error earlier). Fixes can always be demoted. If the applicability for a rule is adjusted, all fixes from that rule will not exceed the adjusted applicability.

These settings should not be exposed as command line options — they’re not particularly useful for one-off invocations.

Why not use fixable and unfixable?
There are existing fixable and unfixable settings that are used to disable or enable fixes for rules. We could use unfixable to represent the same behavior as extend-manual-fix and fixable to represent the same behavior as extend-automatic-fix. While this excludes the ability to demote an automatic fix to suggested, it covers most of the functionality without introducing new options. However,

1. These settings have a clear purpose as is and would be more complex
2. These settings do not use extend semantics and would require enumeration of all rules
3. When we display fixes in the ruff check output, we may want to use unfixable to hide fixes for a rule whereas extend-manual-fix would still display the fix
4. Setting fixable has an unclear intent when a rule has fixes at multiple applicability levels (raised by @MichaReiser)

cc @evanrittenhouse would love to hear your thoughts

[evanrittenhouse]

Yeah sounds good! I think that's pretty aligned with what I was thinking originally :D

I'll work on getting the implementation to match - I may do the promotion/demotion stuff in a follow-up PR, if that works?

[zanieb]

Wonderful! Yeah, I think promotion and demotion is definitely not needed to start.