[ruff] Add rule-codes-in-selectors (RUF201) - #26772
Conversation
ntBre
added
rule
Summary -- This PR adds a new lint rule for TOML config files that checks for the use of rule codes as selectors and offers a fix to replace them with human-readable names. At a high-level, the implementation just deserializes a TOML config into a stripped down version of our normal configuration types and then checks each of its `Spanned` selector entries. Unless I oversimplified this too much, it didn't end up being as tricky as I thought. The main tricky part was adding basic support for fixes in TOML files, but even this was pretty straightforward. Codex was concerned about calling this a safe fix because applying the fix with `--preview`, or another temporary way of enabling preview, would break the config if preview isn't always enabled. That seemed to go beyond our usual fix safety policy to me, but I'm also happy to mark the fix as unsafe if that's preferable. Test Plan -- New tests covering all of the expected selectors and new CLI tests to make sure that `ruff.toml` files get linted and respect various ways of suppressing fixes. Previously we only linted `pyproject.toml` files for `RUF200` and bypassed some of the checks in `LintContext`. I left the change to use `LintContext` as a separate commit since it also affects `RUF200` and I briefly considered splitting it off into a separate PR.
6ab1f76 to
6a2d6a5
Compare
Memory usage reportMemory usage unchanged ✅ |
|
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| rule-codes-in-selectors | 1861 | 1861 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
|
I was highly alarmed by the ecosystem results, which show completely wrong line and column numbers, but we rewrite config files as part of our ecosystem analysis to change any necessary settings: and tomli/tomli_w don't support preserving comments: https://github.com/hukkin/tomli#is-comment-preserving-round-trip-parsing-supported. I was hoping they might at least preserve some kind of A CLI override would be a better way around this (#10345), but after #22410, it seems that $ printf 'required-version = ">999"\n' > ruff.toml
$ touch test.py
$ ruff check --config 'required-version = ">=0"' .
ruff failed
Cause: Failed to load configuration `/private/var/folders/h6/f4fnzd4j1j5062v0hf073bm40000gn/T/tmp.htAWvk7AUu/ruff.toml`
Cause: Required version `>999` does not match the running version `0.15.15`
$ uvx ruff@0.14 check --config 'required-version = ">=0"' .
All checks passed!We should probably fix these, especially if we add more TOML rules, but it seems beyond the scope of this PR. The other ecosystem results I've looked at otherwise appear to be correct. |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This cool.
I think this PR would have been slightly easier to review if split into smaller PRs. But I can see how this is awkward in some sense, because you can only start adding tests in later PRs. But roughly, the work here is
- Add support for linting
ruff.toml, includingruff-test - Add the new rule (maybe this could be one PR with add linting support)
- Add support for fixing in toml files
I leave it up to you if you want to split the PR or leave it as one (I have reviewed most of it by now)
I do think we should make a few changes to this PR:
- Migrate testing to
ruff-test - Move parsing out of the lint rule into
lint_toml. Each rule parsing the configuration on its own doesn't scale - If we can, avoid duplicating the
Configurationstruct. This may require addingSpan's to `Configuration. If so, it would probably be good to do this as a separate PR - Maybe, only change the default include under preview.
I also suggest testing whether the new lint and fixes work in Ruff's LSP. We should make a decision if this is something we want to fix, but I think it's fine to do this as a separate PR.
| } | ||
|
|
||
| /// Generate [`Diagnostic`]s for a TOML configuration file, iteratively fixing until stable. | ||
| pub fn lint_fix_toml<'a>( |
There was a problem hiding this comment.
It's a bit unfortunate that we need separate lint_toml and lint_fix_toml functions. It would be nice if they could share more, but I think that's only really possible by introducing more abstraction or a more lazy model, like Salsa's.
Given that this PR is already large, I think the current approach is fine
| let Ok(Some(config_file)) = ConfigFile::from_toml_str(source_file.source_text(), source_type) | ||
| else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
I don't think each rule parsing the toml file itself scales well. We should move this out into lint_toml and pass the parsed configuration instead of the raw source type.
| /// For more information on the glob syntax, refer to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax). | ||
| #[option( | ||
| default = r#"["*.py", "*.pyi", "*.pyw", "*.ipynb", "*.md", "**/pyproject.toml"]"#, | ||
| default = r#"["*.py", "*.pyi", "*.pyw", "*.ipynb", "*.md", "**/pyproject.toml", "**/ruff.toml", "**/.ruff.toml"]"#, |
There was a problem hiding this comment.
Do we need to gate this behind preview? Or is the rule itself gated behind preview? Do we need to mention that this only applies in preview?
There was a problem hiding this comment.
Yes, the rule itself is in preview. I saw *.md here, which is also still in preview, and thought that I should include the other new formats too.
| } | ||
| } | ||
|
|
||
| fn toml_string_content_range(source: &str, range: Range<usize>) -> Option<Range<usize>> { |
There was a problem hiding this comment.
Should this return a TextRange instead (and take a TextRange as input?)
There was a problem hiding this comment.
I inlined this into a RuleCode::from_spanned helper that ties the rule code and range together. I think that makes the relationship to the Rule::from_code call clearer too #26772 (comment).
| // Keep these fields in sync with the selector-valued fields in | ||
| // `ruff_workspace::options::LintCommonOptions`. |
There was a problem hiding this comment.
The comment here is not relevant. Same in the Ruff section. But I am concerned that we'll fail to keep this struct up to date with our main Configuration.
| let resolved_file = resolved_file.context("Iterating the files in the repository failed")?; | ||
| // For some reason it does not filter in the beginning | ||
| if resolved_file.file_name() == "pyproject.toml" { | ||
| if ["pyproject.toml", "ruff.toml", ".ruff.toml"] |
There was a problem hiding this comment.
What about ruff format?
Codex reports
[P2] Adding Ruff configs to stable discovery breaks formatter range mode. settings.rs:146
With one Python file and ruff.toml, ruff format --check --range 1:1-1:9 DIR now fails because the formatter validates the discovered-file count before filtering TOML. Previously the directory resolved to one formattable file. Filter TOML before the range check or avoid adding these files to stable formatter discovery.
There was a problem hiding this comment.
Interesting, none of my Codex reviews ever caught this. Possibly because this reproduces on main with pyproject.toml:
$ touch pyproject.toml
$ touch try.py
$ uvx ruff@latest format --range 1:1-1:9 .
ruff failed
Cause: The `--range` option is only supported when formatting a single file but the specified paths resolve to 2 files.Is passing a directory to --range meant to be a supported operation?
There was a problem hiding this comment.
not really... I should have noticed this
| let (diagnostics, fixed) = | ||
| if matches!(fix_mode, flags::FixMode::Apply | flags::FixMode::Diff) { |
There was a problem hiding this comment.
It would be nice if we could reuse more with the python code path below. The structure here now feels very similar. I don't think we should push it very hard, but if you see an opportunity to do so
Summary -- This PR was split off from #26772 to add support for testing TOML lint rules to mdtest. We use the toml_parser crate to extract comments and now call `lint_pyproject_toml` in ruff_mdtest. Because we already use `toml` blocks for test configuration and because Ruff's current TOML lints need to know the filename, I opted to require a filename for linted TOML blocks. For example, ````markdown ```toml lint.select = ["F401"] ``` ```` remains a configuration block, but ````markdown `ruff.toml`: ```toml lint.select = ["F401"] ``` ```` will be linted. Test Plan -- Added one test for RUF200 and unit tests for the parsing itself.
Summary -- This PR was spun off from #26772 to handle some of the refactors to RUF200. The first non-empty commit is purely moving the implementation of RUF200 from the general `lint_pyproject_toml` function into its own rule file. In #26772, `lint_pyproject_toml` will become `lint_toml` and invoke multiple rules. The second commit drops the overly defensive range check, as noted in #26772 (comment). The third commit refactors the rule to use a `LintContext`. This not only sets up to run multiple rules in this function, but also fixes a latent bug in rule suppression for RUF200. Because the rule previously checked whether it was enabled via the `LinterSettings::rules.enabled` directly, it didn't take into account settings like `per-file-ignores` (this also affects `unfixable` and other fix-related settings for RUF201). The fourth commit contains a new mdtest for this that currently fails on main. Finally, the fifth commit moves parsing out of RUF200. We still parse into a `PyProjectToml` for now, but at least this sets us up to share parsing between rules once we find the right shared representation. Test Plan -- New mdtest mentioned above and then rebasing #26772 onto this.
#26772 (comment) update stable include snapshots
This reverts commit 0388bfc.
|
Thanks for the review! I think I've addressed the feedback. In particular, I split off PRs for adding TOML support to mdtests and refactoring RUF200 to use a
I also looked into sharing more of the Similarly, the lint and fixes do not currently work in the LSP, as I think you suspected. I can follow up on that since this PR is already quite large (although much smaller now!). |
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Nice solution by using Table directly.
|
|
||
| ```toml | ||
| [lint] | ||
| select = ["'F401'"] |
There was a problem hiding this comment.
Should we extend this example with a non-invalid-quotes examples?
There was a problem hiding this comment.
Added a valid rule snapshot to the same example.
| 2 | lint.select = ["F401"] | ||
| | ^^^^ | ||
| | | ||
| help: Replace rule code with name |
There was a problem hiding this comment.
I'm not convinced we should do this, but raising in case you have an opinion/idea on how to improve this.
Should we mention the rule name here? It feels a bit redundant if the fix is enabled too, but seems useful if the fix is missing.
There was a problem hiding this comment.
Sure, I like that idea.
| let Some(values) = value.get_ref().as_array() else { | ||
| continue; | ||
| }; | ||
| check_selector_array(context, values, selector, in_lint_table); |
There was a problem hiding this comment.
Hmm, I think we should split SELECTORS into two lists. One for arrays, and another for tables. Or make SELECTORS store paths, e.g. ["per-file-ignores", "file"] to make the path unambigous. Right now, the rule would rewrite a select = { blabla = [] } (blabla also works for per-file-ignores.). So I do think we need to make sure that we only rewrite allowlisted arrays.
There was a problem hiding this comment.
Interesting. Codex mentioned something similar, but I dismissed it because select = { blabla = [] } is an invalid config and should be rejected by normal config loading before we run any rules. But it's probably safer to specify which settings are expected to be tables anyway.
| } | ||
| } | ||
|
|
||
| const SELECTORS: &[&str] = &[ |
There was a problem hiding this comment.
Should we add some documentation to RuleSelector or elsewhere to hint towards updating this rule?
There was a problem hiding this comment.
Added to UnresolvedRuleSelector since hopefully that's the type someone would reach for when adding new selectors.
Rebasing the fork's commits onto each new upstream release could not work.
A 3-way merge conflicts even when both sides only *add* code near each
other, so upstream inserting a function next to `extract_python` was
enough to stop it, and every release bumps `version` in pyproject.toml
directly below the fork's `name` edit. The job had been failing nightly
since 0.15.15, leaving the fork nine releases behind.
Each release is now rebuilt from a clean checkout of the upstream tag,
producing exactly one commit on top of it. fork/apply.sh does four dumb
things, in increasing order of how much each can go wrong:
1. copy in workflows that exist only in this fork
2. rewrite two known-constant lines (pyproject name, PACKAGE_NAME)
3. rewrite release.yml and check.rs structurally
4. apply fork/fork.patch with `patch --fuzz=3`
Nothing merges anywhere. Steps 1-3 either succeed or abort with a specific
message. Only step 4 can reject a hunk, and a rejection does not abort,
because a hunk also rejects when upstream has implemented that change
itself: 0.16.0 added `*.md` to the default include list and removed the
markdown preview gate, making two of the fork's patches obsolete.
Whether the result is actually good is decided by building it and checking
that a `.md` file still lints, which gates tagging and publishing. That
matters more than it sounds — a tree with the check.rs hunk rejected still
compiles, it just silently stops linting markdown.
Anything anchored to something upstream keeps extending is a structural
rewrite rather than a patch hunk:
- release.yml: depot runners, plus `if: false` on every job outside a
six-job allowlist. An allowlist, so a job upstream adds later is off
by default. 0.15.19 added `custom-publish-crates`, which would
otherwise have tried to publish to crates.io from this fork.
- check.rs: appends to the `matches!(SourceType::from(path), ..)` arm.
Upstream extends that list whenever ruff learns a new file type
(astral-sh#26772 added `TomlSourceType::Ruff`), and this was the only hunk in
fork.patch that ever broke.
Verified by replaying all nine releases from 0.15.15 to 0.16.0: zero
rejected hunks in every one, against 7/9 for the same patch before
check.rs was moved out of it. On 0.16.0 the result builds, passes
`cargo test -p ruff_markdown`, is rustfmt-clean, and reports F401 on the
correct line inside a fenced code block.
fork/fork.patch is the source of truth for the fork's source changes
rather than the branch history, since the branch is force-pushed to each
new release. fork/regen.sh rewrites it from a working tree. Each release
commit carries fork/ and auto-release.yml forward, so it contains the
machinery that produces the next one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary -- Rough draft of RUF200/RUF201 support in the LSP, currently stacked on #26772. Test Plan -- Codex added some e2e tests, which I need to review, but I also want to test manually in VS Code.
2 participants
Summary
This PR adds a new lint rule for TOML config files that checks for the use of rule codes as
selectors and offers a fix to replace them with human-readable names. At a high-level, the
implementation just deserializes a TOML config into a stripped down version of our normal
configuration types and then checks each of its
Spannedselector entries. Unless I oversimplifiedthis too much, it didn't end up being as tricky as I thought. The main tricky part was adding basic
support for fixes in TOML files, but even this was pretty straightforward.
Codex was concerned about calling this a safe fix because applying the fix with
--preview, oranother temporary way of enabling preview, would break the config if preview isn't always enabled.
That seemed to go beyond our usual fix safety policy to me, but I'm also happy to mark the fix as
unsafe if that's preferable.
Test Plan
New tests covering all of the expected selectors and new CLI tests to make sure that
ruff.tomlfiles get linted and respect various ways of suppressing fixes. Previously we only linted
pyproject.tomlfiles forRUF200and bypassed some of the checks inLintContext. I left thechange to use
LintContextas a separate commit since it also affectsRUF200and I brieflyconsidered splitting it off into a separate PR.