Add `per-file-target-version` option by ntBre · Pull Request #16257 · astral-sh/ruff

ntBre · 2025-02-19T15:08:31Z

Summary

This PR is another step in preparing to detect syntax errors in the parser. It introduces the new per-file-target-version top-level configuration option, which holds a mapping of compiled glob patterns to Python versions. I intend to use the LinterSettings::resolve_target_version method here to pass to the parser:

ruff/crates/ruff_linter/src/linter.rs

Lines 491 to 493 in f50849a

    
           // Parse once. 
        
           let parsed = 
        
               ruff_python_parser::parse_unchecked_source(transformed.source_code(), source_type);

Test Plan

I added two new CLI tests to show that the per-file-target-version is respected in both the formatter and the linter.

github-actions · 2025-02-19T15:15:34Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2025-02-19T16:27:12Z

We should respect the per-file-target-version when resolving the target version for a file being linted or formatted. Ideally, we'd add a CLI test that demonstrates that the per-file-target-version override is respected both for linting and formatting.

ntBre · 2025-02-19T16:33:17Z

That makes sense to me. How does the approach of making the current target_version fields private and requiring callers to use the resolve_target_version method sound to you? I only added LinterSettings::resolve_target_version so far, but I'm assuming a formatter analog would be easy too.

I wasn't sure how many changes to put in this PR, but I think it definitely makes sense to wire it up somehow for useful tests.

ntBre · 2025-02-19T16:34:38Z

I'll convert to a draft because I think it's incomplete based on your suggestion.

MichaReiser · 2025-02-19T17:31:51Z

Hmm. I haven't really thought about it. We probably want to avoid calling resolve_target_version everywhere where we compare the target versions. That would become expensive very quickly. Ideally, we would compute the target version for the current file once and then store it somewhere (on Checker?)

ntBre · 2025-02-19T19:33:46Z

Reopening for review. I added

Checker::target_version as a field and const method to return the resolved PythonVersion ¹ to be used in lint rules
Target version resolution to the formatter
Two new CLI tests showing that both the linter and formatter respect the new option

I tried also making the LinterSettings::target_version private, as I mentioned above, but it disrupts a ton of tests that create them directly with code like LinterSettings { ..LinterSettings::default() }. This could be solved with a bunch of with_* methods to build a LinterSettings without pub fields, but after adding two of those and seeing the number of remaining errors, I just made them pub again. I think I caught all of the (current) uses in lint rules while they were private, though. ↩

MichaReiser

This is great! I suggest renaming the field on the setting structs to e.g. default_target_version or global_target_version so that its name raises questions: Hmm, why global? maybe this is not the field I want. Let me check the documentation

We should also test this feature in the editor. I think it currently doesn't work for the formatter.

crates/ruff_workspace/src/settings.rs

crates/ruff_workspace/src/options.rs

crates/ruff_linter/src/settings/types.rs

crates/ruff_linter/src/checkers/ast/mod.rs

crates/ruff_workspace/src/settings.rs

crates/ruff/src/commands/format.rs

crates/ruff/tests/lint.rs

crates/ruff/tests/format.rs

AlexWaygood

I'll leave this one mostly to Micha, just a quick nit I spotted

AlexWaygood · 2025-02-20T11:47:26Z

crates/ruff_linter/src/rules/flake8_builtins/rules/stdlib_module_shadowing.rs

 }

-fn is_allowed_module(settings: &LinterSettings, module: &str) -> bool {
+fn is_allowed_module(settings: &LinterSettings, minor_version: u8, module: &str) -> bool {


I'd prefer to pass around a PythonVersion in the ruff_linter crate where possible (only converting it to a u8 representing the minor version when crossing the crate boundary and calling a function from the ruff_python_stdlib crate) as it's more strongly typed

Suggested change

fn is_allowed_module(settings: &LinterSettings, minor_version: u8, module: &str) -> bool {

fn is_allowed_module(settings: &LinterSettings, version: PythonVersion, module: &str) -> bool {

ntBre · 2025-02-20T22:45:47Z

Wow I didn't know eliding the lifetime in a const was a recent feature. I thought clippy used to suggest that 🤷 Maybe I need to start building with 1.80 locally instead of 1.82...

crates/ruff_wasm/src/lib.rs

ntBre · 2025-02-20T23:08:07Z

Thanks @MichaReiser for the thorough review. I think it's in a much better place now. The only open question I see now is how to add tests for the editor integration (if that's possible).

I also feel like there might be a more elegant solution to the generic PerFile<T> stuff, but I haven't found it yet. Probably we could remove some of the wrapper newtypes and just use the generic versions directly at least, if we want to.

crates/ruff_linter/src/settings/mod.rs

crates/ruff_linter/src/settings/types.rs

MichaReiser · 2025-02-21T07:21:20Z

The only open question I see now is how to add tests for the editor integration (if that's possible).

There are a few end to end tests in the VS code extension but it isn't something we have good infrastructure today. But @dhruvmanila knows better than I.

dhruvmanila · 2025-02-21T14:02:40Z

There are a few end to end tests in the VS code extension but it isn't something we have good infrastructure today. But @dhruvmanila knows better than I.

I've talked with Brent in regards to this on Discord. Looking at the change that fixed the issue which you were pointing out earlier there are two options:

Add unit tests in https://github.com/astral-sh/ruff/blob/df9fbdce3af5af826115780f2d8c5cc4d91be650/crates/ruff_server/src/format.rs for format function specifically for per-file-target-version
Add a E2E test in https://github.com/astral-sh/ruff-vscode/blob/main/src/test/e2e.test.ts with a fixture that sets this new option but this would only work after the Ruff release with this option is went out. I wouldn't recommend using this for a new option in Ruff.

I'd suggest (1) but I'm longing to create a mock server xD

ntBre · 2025-02-21T15:16:27Z

Yes, thanks @dhruvmanila! I've added unit tests for format and format_range for scripts that parallel the other linter and formatter tests I added, so I think this was a really nice idea.

It looks like notebooks also go through this code path, but I'm happy to add notebook-specific tests if either of you want. It shouldn't be too much trouble if I reuse the create_test_notebook infrastructure from notebook.rs.

Similarly, it looks like linting goes straight through check_path, where the target version resolution happens and is tested in the linter. But again, I'd be happy to add linter tests in the server crate if you want.

dhruvmanila

Thank you for adding those test cases in the language server, they look good.

* main: (38 commits) [red-knot] Use arena-allocated association lists for narrowing constraints (#16306) [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321) Add issue templates (#16213) Normalize inconsistent markdown headings in docstrings (#16364) [red-knot] Better diagnostics for method calls (#16362) [red-knot] Add argfile and windows glob path support (#16353) [red-knot] Handle pipe-errors gracefully (#16354) Rename `venv-path` to `python` (#16347) [red-knot] Fixup some formatting in `infer.rs` (#16348) [red-knot] Restrict visibility of more things in `class.rs` (#16346) [red-knot] Add diagnostic for class-object access to pure instance variables (#16036) Add `per-file-target-version` option (#16257) [PLW1507] Mark fix unsafe (#16343) [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326) Extract class and instance types (#16337) Re-order changelog entries for 0.9.7 (#16344) [red-knot] Add support for `@classmethod`s (#16305) Update Salsa (#16338) Update Salsa part 1 (#16340) Upgrade Rust toolchain to 1.85.0 (#16339) ...

MichaReiser added the configuration Related to settings and configuration label Feb 19, 2025

ntBre marked this pull request as draft February 19, 2025 16:33

ntBre marked this pull request as ready for review February 19, 2025 19:33

ntBre requested a review from AlexWaygood as a code owner February 19, 2025 19:33

MichaReiser requested changes Feb 20, 2025

View reviewed changes

AlexWaygood reviewed Feb 20, 2025

View reviewed changes

ntBre added 18 commits February 20, 2025 15:33

add option

8f4dacc

use compiled patterns for resolving

377b6bd

use Checker::target_version, LinterSettings::resolve_target_version

b418e31

allow resolving per_file_target_versions in the formatter

89ac994

resolve target_version once for Checker

9f7e057

test that the linter respects the per-file version

90f6493

handle base and absolute paths like PerFileIgnore

aceca51

convert to Vec<PerFileVersion> earlier like PerFileIgnore

f369358

test that the formatter respects the per-file version

ee992b3

delete unused Deref impl

d9f1c17

impl Display for CompiledPerFileVersion and use in lint and format

3bcf6c3

add error case for linter test

f30a3aa

add other case for formatter test

4c64314

pass the whole version to is_allowed_module

09d2989

pass target_version down to A005

c1bd3a0

extract PerFile<T> and share negation with PerFileVersion

89bee8d

add some docs

b364a4a

factor out CompiledPerFile<T>

6cf6ccb

ntBre added 10 commits February 20, 2025 17:09

add generic List type and factor out uses

1c85afa

remove unused CompiledPerFile<T> newtypes

77653e6

add PerFileKind to fix labels

920df2d

rename to PerFileTargetVersion

5d02c60

add context on glob failure

276f3ab

also add context to resolve call itself

699f273

expand docs on per-file-target-version

5f7e8b0

rename target_version fields and document them along with per-file

1f4dced

pass Option<&Path> to to_format_options

d9d6022

tidy up

778c559

ntBre force-pushed the brent/per-file-target-version branch from f8ab420 to 778c559 Compare February 20, 2025 22:33

fix ci

d4f2bba

ntBre commented Feb 20, 2025

View reviewed changes

crates/ruff_wasm/src/lib.rs Show resolved Hide resolved

MichaReiser approved these changes Feb 21, 2025

View reviewed changes

ntBre added 3 commits February 21, 2025 08:40

mention Checker

80699a1

switch to debug_label

23f370e

impl CacheKey manually to avoid constraints on structs

375fd5c

test script formatting in the server

34345de

ntBre mentioned this pull request Feb 22, 2025

[WIP] Detect several simple syntax errors in the parser #16308

Closed

dhruvmanila approved these changes Feb 24, 2025

View reviewed changes

ntBre merged commit e7a6c19 into main Feb 24, 2025
21 checks passed

ntBre deleted the brent/per-file-target-version branch February 24, 2025 13:47

BrewTestBot mentioned this pull request Feb 27, 2025

ruff 0.9.8 Homebrew/homebrew-core#209149

Merged

	// Parse once.
	let parsed =
	ruff_python_parser::parse_unchecked_source(transformed.source_code(), source_type);

	fn is_allowed_module(settings: &LinterSettings, minor_version: u8, module: &str) -> bool {
	fn is_allowed_module(settings: &LinterSettings, version: PythonVersion, module: &str) -> bool {

Comments

Conversation

ntBre commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser commented Feb 19, 2025

Uh oh!

ntBre commented Feb 19, 2025

Uh oh!

ntBre commented Feb 19, 2025

Uh oh!

MichaReiser commented Feb 19, 2025

Uh oh!

ntBre commented Feb 19, 2025

Footnotes

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

ntBre commented Feb 20, 2025

Uh oh!

Uh oh!

ntBre commented Feb 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Feb 21, 2025

Uh oh!

dhruvmanila commented Feb 21, 2025

Uh oh!

ntBre commented Feb 21, 2025

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ntBre commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

`ruff-ecosystem` results