Improve rule config resolution #2312

not-my-profile · 2023-01-28T20:29:50Z

Ruff allows rules to be enabled with select and disabled with
ignore, where the more specific rule selector takes precedence,
for example:

`--select ALL --ignore E501` selects all rules except E501
`--ignore ALL --select E501` selects only E501

(If both selectors have the same specificity ignore selectors
take precedence.)

Ruff always had two quirks:

If pyproject.toml specified ignore = ["E501"] then you could
previously not override that with --select E501 on the command-line
(since the resolution didn't take into account that the select was
specified after the ignore).
If pyproject.toml specified select = ["E501"] then you could
previously not override that with --ignore E on the command-line
(since the resolution didn't take into account that the ignore was
specified after the select).

Since d067efe (#1245)
extend-select and extend-ignore always override
select and ignore and are applied iteratively in pairs,
which introduced another quirk:

If some pyproject.toml file specified extend-select
or extend-ignore, select and ignore became pretty much
unreliable after that with no way of resetting that.

This commit fixes all of these quirks by making later configuration
sources take precedence over earlier configuration sources.

While this is a breaking change, it fixes the broken status quo.

Fixes #2300.

not-my-profile · 2023-01-28T20:44:50Z

I think this makes much more sense and is much more user-friendly / understandable.

This obviously breaks existing configurations that relied on the previous weird behavior ... however I'd expect most ruff config files to not rely on this weird behavior ... but that's just a hunch.

How you want to go about this is very much your call. It would be possible to support both the old and the new config resolution at the same time and only use the new resolution based on some tool.ruff.edition setting but I am not really interested in implementing that.

charliermarsh · 2023-01-28T20:47:39Z

I just have to take a look at some of the more complex setups in the wild and see how they'd be affected.

src/settings/configuration.rs

charliermarsh · 2023-01-28T21:41:41Z

How you want to go about this is very much your call. It would be possible to support both the old and the new config resolution at the same time and only use the new resolution based on some tool.ruff.edition setting but I am not really interested in implementing that.

Don't worry, I definitely don't want to support both.

charliermarsh · 2023-01-28T21:45:07Z

src/settings/mod.rs

+        let mut select_set: FxHashSet<_> = defaults::PREFIXES.iter().flatten().collect();
+        let mut fixable_set: FxHashSet<_> = RuleSelector::All.into_iter().collect();
+
+        let mut redirects = FxHashMap::default();


Do you mind adding some comments throughout this method? Or explaining the logic in more detail here? I'm tracing it but having trouble following.

As a specific example: if I have select = ["F"] in my pyproject.toml, and I do --select A, then I end up with two rule selections:

[RuleSelection { select: Some([Prefix { prefix: F, redirected_from: None }]), ignore: Some([Prefix { prefix: A, redirected_from: None }]), extend_select: None, extend_ignore: None, fixable: None, unfixable: None }, RuleSelection { select: Some([Prefix { prefix: A, redirected_from: None }]), ignore: None, extend_select: None, extend_ignore: None, fixable: None, unfixable: None }]

But, how do the F rules not end up in the select set?

Oh, I see -- do we, in effect, only use the last RuleSelection that contains a non-empty select? The rest gets discarded?

If that's the correct interpretation how would I do the following: create a pyproject.toml that enables all the F rules, then extend it from another pyproject.toml to also include the E rules? I think I know the answer, but it would be helpful to hear it.

Separate question: under this scheme, is there ever any difference between specifying ignore and extend-ignore?

I added some comments :)

if I have select = ["F"] in my pyproject.toml, and I do --select A, [..] But, how do the F rules not end up in the select set?

If selection.select.is_some() then the select_set is overridden.

do we, in effect, only use the last RuleSelection that contains a non-empty select? The rest gets discarded?

No, multiple RuleSelections can all contribute to select_set if they only use extend_select / ignore but not select.

how would I do the following: create a pyproject.toml that enables all the F rules, then extend it from another pyproject.toml to also include the E rules?

select can be extended via extend-select.

Separate question: under this scheme, is there ever any difference between specifying ignore and extend-ignore?

Very good catch! I didn't even notice that but indeed we can deprecate extend-ignore since it now can always be replaced with ignore. The reasoning behind that is that now ignore/extend-ignore can only ever deselect already selected rules ... it's impossible for an ignore option to take precedence over a latter select option. Since the "ignore set" isn't something that is kept track of anymore ... the function only keeps track of the effectively selected rules, extend-ignore doesn't fulfill any purpose anymore.

Definitely, will do. Mechanically, probably easiest for me to open those changes as a separate PR on main and then just rebase that PR after this merges...?

You have write permissions on my branch so you could just push your commits there :)

Ok, I went ahead and pushed a first-pass in the README.md and BREAKING_CHANGES.md. Feedback welcome.

Thank you so much, I think you described it very well :)

Great. I'll try to get this out tomorrow!

not-my-profile · 2023-01-29T20:30:29Z

(force pushed a small refactor that removed extend_ignore from the new RuleSelection struct and removed some needless Option wrapping around the fields that don't require it)

Previously we tested the resolve_codes helper function directly. Since we want to rewrite our resolution logic in the next commit, this commit changes the tests to test the more high-level From impl.

not-my-profile · 2023-01-30T04:21:39Z

(force pushed to fix the CI failure, which was caused by the README now including the --extend-ignore setting which this PR hides from the help output)

I think we should also mention in the new Rule resolution section in the README and the BREAKING_CHANGES.md entry that extend-ignore is now deprecated since it behaves exactly like ignore.

(This is now already mentioned in the documentation of the extend-ignore setting but I think it would make sense to repeat it ... note that I have also updated the CLI help to hide the --extend-ignore flag.)

charliermarsh · 2023-01-30T13:08:06Z

README.md

@@ -433,8 +433,6 @@ Rule selection:
          Comma-separated list of rule codes to disable
      --extend-select <RULE_CODE>
          Like --select, but adds additional rule codes on top of the selected ones
-      --extend-ignore <RULE_CODE>
-          Like --ignore, but adds additional rule codes on top of the ignored ones


I do kind of wonder if we should just leave this visible in the CLI to avoid confusion (due to the asymmetry with --select). What do you think? Bad idea?

If we leave it I think we should add DEPRECATED to the description ... however I don't think that would really reduce the confusion since we'd have to explain why it's deprecated ... which would probably be too verbose. I consider --extend-ignore to effectively be an alias for --ignore ... and we currently don't document e.g. that ruff clean has ruff --clean as an alias in the CLI help.

I don't have a strong opinion about this, if you think it should be listed we can add it back.

Note that I have also removed extend-ignore from the JSON schema ... which could also be confusing for users but I think it's worth it to stop it from being autocompleted for users writing new configs.

I think I prefer to hide it. I have updated the message to mention that it's deprecated since it still shows up in the clap shell completion.

Ruff allows rules to be enabled with `select` and disabled with `ignore`, where the more specific rule selector takes precedence, for example: `--select ALL --ignore E501` selects all rules except E501 `--ignore ALL --select E501` selects only E501 (If both selectors have the same specificity ignore selectors take precedence.) Ruff always had two quirks: * If `pyproject.toml` specified `ignore = ["E501"]` then you could previously not override that with `--select E501` on the command-line (since the resolution didn't take into account that the select was specified after the ignore). * If `pyproject.toml` specified `select = ["E501"]` then you could previously not override that with `--ignore E` on the command-line (since the resolution didn't take into account that the ignore was specified after the select). Since d067efe (astral-sh#1245) `extend-select` and `extend-ignore` always override `select` and `ignore` and are applied iteratively in pairs, which introduced another quirk: * If some `pyproject.toml` file specified `extend-select` or `extend-ignore`, `select` and `ignore` became pretty much unreliable after that with no way of resetting that. This commit fixes all of these quirks by making later configuration sources take precedence over earlier configuration sources. While this is a breaking change, we expect most ruff configuration files to not rely on the previous unintutive behavior.

Now that the option is deprecated we no longer want IDEs to suggest it in their autocompletion.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.237` -> `^0.0.238` | [![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.238/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.238/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.238/compatibility-slim/0.0.237)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.238/confidence-slim/0.0.237)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>charliermarsh/ruff</summary> ### [`v0.0.238`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.238) [Compare Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.237...v0.0.238)  #### What's Changed ##### ⚠️ Breaking Changes ⚠️ ##### `select`, `extend-select`, `ignore`, and `extend-ignore` have new semantics ([#2312](https://togithub.com/charliermarsh/ruff/pull/2312)) Previously, the interplay between `select` and its related options could lead to unexpected behavior. For example, `ruff --select E501 --ignore ALL` and `ruff --select E501 --extend-ignore ALL` behaved differently. (See [#2312](https://togithub.com/charliermarsh/ruff/pull/2312) for more examples.) The new semantics are such that Ruff uses the "highest-priority" `select` as the basis for the rule set, and then applies any `extend-select`, `ignore`, and `extend-ignore` adjustments. CLI options are given higher priority than `pyproject.toml` options, and the current `pyproject.toml` file is given higher priority than any inherited `pyproject.toml` files. As an example: `ruff --select F401` will select rule `F401`, and ignore any of the modifiers from the `pyproject.toml`, as the "highest-priorty" select kicks off the resolution chain. This change is largely backwards compatible -- most users should experience no change in behavior. For more, see [BREAKING_CHANGES.md](https://togithub.com/charliermarsh/ruff/blob/main/BREAKING_CHANGES.md#select-extend-select-ignore-and-extend-ignore-have-new-semantics-2312). ##### `remove-six-compat` (`UP016`) has been removed ([#2332](https://togithub.com/charliermarsh/ruff/pull/2332)) The `remove-six-compat` rule has been removed. This rule was only useful for one-time Python 2-to-3 upgrades. ##### Rules - Implement Pylint's `too-many-arguments` rule (`PLR0913`) by [@akhildevelops](https://togithub.com/akhildevelops) in [astral-sh/ruff#2308 - Extend conventional imports defaults to include TensorFlow et al by [@sbrugman](https://togithub.com/sbrugman) in [astral-sh/ruff#2353 ##### Settings - feat: add ruff --statistics by [@spaceone](https://togithub.com/spaceone) in [astral-sh/ruff#2284 - Ignore magic comparisons to bytes by default by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2365 - Implement `ruff linter` subcommand by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2294 - Improve rule config resolution by [@not-my-profile](https://togithub.com/not-my-profile) in [astral-sh/ruff#2312 ##### Bug Fixes - Refine criteria for `exc_info` logger rules by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2364 - Respect per-file-ignores when checking noqa by [@sciyoshi](https://togithub.com/sciyoshi) in [astral-sh/ruff#2309 - Place star before other member imports by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2320 - Allow list comprehensions for **all** assignment by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2326 - \[`TRY201`] don't check raise statements in nested exception handlers by [@sciyoshi](https://togithub.com/sciyoshi) in [astral-sh/ruff#2337 - include tomllib in standard lib by [@sbrugman](https://togithub.com/sbrugman) in [astral-sh/ruff#2345 - Avoid removing trailing comments when autofixing by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2352 - \[`I001`] fix isort for files with tab-based indentation by [@sciyoshi](https://togithub.com/sciyoshi) in [astral-sh/ruff#2361 - Disable incompatible rules rather than merely warning by [@charliermarsh](https://togithub.com/charliermarsh) in [astral-sh/ruff#2369 #### New Contributors - [@chirag127](https://togithub.com/chirag127) made their first contribution in [astral-sh/ruff#2307 - [@akhildevelops](https://togithub.com/akhildevelops) made their first contribution in [astral-sh/ruff#2308 **Full Changelog**: astral-sh/ruff@v0.0.237...v0.0.238 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).  Signed-off-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

not-my-profile changed the title ~~Improve rule resolution~~ Improve rule config resolution Jan 28, 2023

not-my-profile mentioned this pull request Jan 28, 2023

Disable D203, D212, D213 and S101 by default #2292

Closed

charliermarsh reviewed Jan 28, 2023

View reviewed changes

src/settings/configuration.rs Show resolved Hide resolved

charliermarsh reviewed Jan 28, 2023

View reviewed changes

not-my-profile force-pushed the improve-rule-resolution branch 4 times, most recently from 02c0ea2 to 99887d8 Compare January 29, 2023 03:57

not-my-profile requested a review from charliermarsh January 29, 2023 04:02

not-my-profile force-pushed the improve-rule-resolution branch from 99887d8 to db633ae Compare January 29, 2023 20:27

not-my-profile added 2 commits January 30, 2023 05:17

refactor: test impl From<&Configuration> for RuleTable

91ab755

Previously we tested the resolve_codes helper function directly. Since we want to rewrite our resolution logic in the next commit, this commit changes the tests to test the more high-level From impl.

Test that more specific select wins over less specific ignore

12d5832

not-my-profile force-pushed the improve-rule-resolution branch from c024514 to 26ba908 Compare January 30, 2023 04:19

not-my-profile force-pushed the improve-rule-resolution branch from 26ba908 to 61822bd Compare January 30, 2023 04:29

charliermarsh reviewed Jan 30, 2023

View reviewed changes

not-my-profile force-pushed the improve-rule-resolution branch from 61822bd to 3947ad0 Compare January 30, 2023 20:56

not-my-profile and others added 3 commits January 30, 2023 21:57

Exclude deprecated extend-ignore from the JSON schema

7e47d62

Now that the option is deprecated we no longer want IDEs to suggest it in their autocompletion.

Document new rule config resolution

0264772

not-my-profile force-pushed the improve-rule-resolution branch from 3947ad0 to 0264772 Compare January 30, 2023 20:57

charliermarsh merged commit ca1129a into astral-sh:main Jan 30, 2023

not-my-profile mentioned this pull request Feb 2, 2023

No longer possible to pass --select=ALL and honor pyproject.toml's tool.ruff.ignore #2461

Closed

charliermarsh mentioned this pull request Nov 18, 2023

Please consider changing configuration merging semantics #8751

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rule config resolution #2312

Improve rule config resolution #2312

not-my-profile commented Jan 28, 2023

not-my-profile commented Jan 28, 2023

charliermarsh commented Jan 28, 2023

charliermarsh commented Jan 28, 2023

charliermarsh Jan 28, 2023 •

edited

charliermarsh Jan 28, 2023

charliermarsh Jan 28, 2023

charliermarsh Jan 28, 2023

not-my-profile Jan 29, 2023

charliermarsh Jan 29, 2023

not-my-profile Jan 29, 2023

charliermarsh Jan 29, 2023

not-my-profile Jan 30, 2023

charliermarsh Jan 30, 2023

not-my-profile commented Jan 29, 2023

not-my-profile commented Jan 30, 2023 •

edited

charliermarsh Jan 30, 2023

not-my-profile Jan 30, 2023 •

edited

not-my-profile Jan 30, 2023

Improve rule config resolution #2312

Improve rule config resolution #2312

Conversation

not-my-profile commented Jan 28, 2023

not-my-profile commented Jan 28, 2023

charliermarsh commented Jan 28, 2023

charliermarsh commented Jan 28, 2023

charliermarsh Jan 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-my-profile commented Jan 29, 2023

not-my-profile commented Jan 30, 2023 • edited

Choose a reason for hiding this comment

not-my-profile Jan 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh Jan 28, 2023 •

edited

not-my-profile commented Jan 30, 2023 •

edited

not-my-profile Jan 30, 2023 •

edited