Add `per-file-target-version` option #16257

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

@@ -129,7 +133,7 @@ fn get_prefix<'a>(settings: &'a LinterSettings, path: &Path) -> Option<&'a PathB
    prefix
 }

-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.

MichaReiser · 2025-02-21T07:13:06Z

crates/ruff_linter/src/settings/mod.rs

+    //redirect.github.com/ The non-path-resolved Python version specified by the `target-version` input option.
+    //redirect.github.com/
+    //redirect.github.com/ See [`LinterSettings::resolve_target_version`] for a way to obtain the Python version for a
+    //redirect.github.com/ given file, while respecting the overrides in `per_file_target_version`.
+    pub unresolved_target_version: PythonVersion,
+    //redirect.github.com/ Path-specific overrides to `unresolved_target_version`.
+    //redirect.github.com/
+    //redirect.github.com/ See [`LinterSettings::resolve_target_version`] for a way to check a given [`Path`]
+    //redirect.github.com/ against these patterns, while falling back to `unresolved_target_version` if none of them
+    //redirect.github.com/ match.


I'd probably mention Checker.target_version first because that's what most devs should use.

Oh, of course

MichaReiser · 2025-02-21T07:15:58Z

crates/ruff_linter/src/settings/types.rs

    }
 }

-impl Display for CompiledPerFileIgnoreList {
+impl<T: CacheKey + std::fmt::Debug + PerFileKind> CompiledPerFileList<T> {
+    pub(crate) fn iter_matches<'a, 'p>(&'a self, path: &'p Path) -> impl Iterator<Item = &'p T>


I'd probably add a third argument: debug_label: &'static str over introducing my own trait. Seems simpler

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

crates/ruff_linter/src/settings/types.rs

+
+impl<T> Display for CompiledPerFileList<T>
+where
+    T: Display + CacheKey,


Suggested change

T: Display + CacheKey,

T: Display,

MichaReiser · 2025-02-21T07:17:35Z

crates/ruff_linter/src/settings/types.rs

-pub struct CompiledPerFileIgnoreList {
-    // Ordered as (absolute path matcher, basename matcher, rules)
-    ignores: Vec<CompiledPerFileIgnore>,
+pub struct CompiledPerFileList<T: CacheKey> {


Suggested change

pub struct CompiledPerFileList<T: CacheKey> {

pub struct CompiledPerFileList<T> {

I prefer to avoid having type constraints in struct and instead, only add them on the impls where needed.

I think the downside of this is that you'd have to implement CacheKey manually (because we aren't using Rust edition 2024 yet)

Ah that makes sense, I did find it annoying to have to put : CacheKey everywhere. I took a brief look at adding the trait bounds in the derive macro, but I probably need a little more proc macro practice before tackling that!

I'm not sure if that's something that you even can do in the proc macro? I'd suggest to simply implement CacheKey manually.

That's what Clone does, so I thought it was possible. But I looked for the source code, and it said Clone was a compiler builtin, so maybe you're right. I already switched to the manual impl locally, I was just curious.

I think serde might be able to do it too, again just for my curiositiy, I don't think we need to do this here. CacheKey is easy to implement.

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.

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

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `per-file-target-version` option #16257

Add `per-file-target-version` option #16257

ntBre commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

MichaReiser commented Feb 19, 2025

ntBre commented Feb 19, 2025

ntBre commented Feb 19, 2025

MichaReiser commented Feb 19, 2025

ntBre commented Feb 19, 2025

MichaReiser left a comment

AlexWaygood left a comment

AlexWaygood Feb 20, 2025

ntBre commented Feb 20, 2025

ntBre commented Feb 20, 2025

MichaReiser Feb 21, 2025

ntBre Feb 21, 2025

MichaReiser Feb 21, 2025

MichaReiser Feb 21, 2025

MichaReiser Feb 21, 2025

ntBre Feb 21, 2025

MichaReiser Feb 21, 2025

ntBre Feb 21, 2025

ntBre Feb 21, 2025

MichaReiser commented Feb 21, 2025

dhruvmanila commented Feb 21, 2025

ntBre commented Feb 21, 2025

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

	// 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 {

	pub struct CompiledPerFileList<T: CacheKey> {
	pub struct CompiledPerFileList<T> {

Add per-file-target-version option #16257

Are you sure you want to change the base?

Add per-file-target-version option #16257

Conversation

ntBre commented Feb 19, 2025 • edited Loading

Summary

Test Plan

github-actions bot commented Feb 19, 2025 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

MichaReiser commented Feb 19, 2025

ntBre commented Feb 19, 2025

ntBre commented Feb 19, 2025

MichaReiser commented Feb 19, 2025

ntBre commented Feb 19, 2025

Footnotes

MichaReiser left a comment

Choose a reason for hiding this comment

AlexWaygood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntBre commented Feb 20, 2025

ntBre commented Feb 20, 2025

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

MichaReiser commented Feb 21, 2025

dhruvmanila commented Feb 21, 2025

ntBre commented Feb 21, 2025

Add `per-file-target-version` option #16257

Add `per-file-target-version` option #16257

ntBre commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

`ruff-ecosystem` results