Use `ast::PythonVersion` internally in the formatter and linter #16170

ntBre · 2025-02-14T20:48:49Z

Summary

This PR updates the formatter and linter to use the PythonVersion struct from the ruff_python_ast crate internally. While this doesn't remove the need for the linter::PythonVersion enum, it does remove the formatter::PythonVersion enum and limits the use in the linter to deserializing from CLI arguments and config files and moves most of the remaining methods to the ast::PythonVersion struct.

Test Plan

Existing tests, with some inputs and outputs updated to reflect the new (de)serialization format. I think these are test-specific and shouldn't affect any external (de)serialization.

ntBre · 2025-02-14T20:57:49Z

Apparently I also messed up some cargo features here, like those that caused #16169. Sorry Micha! I guess these pass locally because I always run clippy and nextest on the whole workspace.

crates/ruff/tests/snapshots/show_settings__display_default_settings.snap

crates/ruff_python_ast/src/python_version.rs

MichaReiser · 2025-02-17T07:57:43Z

crates/ruff_python_formatter/src/options.rs

+    #[serde(deserialize_with = "adapter::deserialize::<_, PythonVersion>")]
+    #[serde(serialize_with = "adapter::serialize::<_, PythonVersion>")]


Is this type used anywhere in a user facing facade? If not, I then think it's fine if the representation changes.

It comes out in ruff check --show-setttings, but the version that's printed now (Py39) can't be directly pasted back into a config file anyway, so I think it should be fine, like you said.

ruff.schema.json

github-actions · 2025-02-17T14:25:14Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

the python version is only used internally, so we can just switch to the dotted representation

this Default impl is only used in one test, so we could possibly get rid of it as well if we updated the test

crates/ruff_linter/src/settings/types.rs

AlexWaygood

This is fantastic!

ntBre · 2025-02-18T14:46:51Z

Thank you all for the reviews! I think I took care of all the comments. If you review again, you may want to skip the last commit. I made ast::python_version private since Alex suggested pub use python_version::* and replaced all of the ruff_python_ast::python_version imports with just ruff_python_ast. I also merged a couple of separate ruff_python_ast imports I noticed while I was doing it.

ntBre · 2025-02-18T14:47:51Z

Oh, maybe it won't be the last commit anymore. Let me merge main.

AlexWaygood · 2025-02-18T15:44:08Z

crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs

@@ -523,7 +523,7 @@ fn check_dynamically_typed<F>(
            if type_hint_resolves_to_any(
                parsed_annotation.expression(),
                checker,
-                checker.settings.target_version.minor(),
+                checker.settings.target_version.minor,


type_hint_resolves_to_any() should probably take a version: ast::PythonVersion rather than a minor_version: u8 argument, looking at this. It would make the method more strongly typed.

Patch

diff --git a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs index 692ed478d..e637f0ea1 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -523,7 +523,7 @@ fn check_dynamically_typed<F>( if type_hint_resolves_to_any( parsed_annotation.expression(), checker, - checker.settings.target_version.minor, + checker.settings.target_version, ) { diagnostics.push(Diagnostic::new( AnyType { name: func() }, @@ -532,7 +532,7 @@ fn check_dynamically_typed<F>( } } } else { - if type_hint_resolves_to_any(annotation, checker, checker.settings.target_version.minor) { + if type_hint_resolves_to_any(annotation, checker, checker.settings.target_version) { diagnostics.push(Diagnostic::new( AnyType { name: func() }, annotation.range(), diff --git a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs index 6c10ae503..de90e3b6e 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/implicit_optional.rs @@ -177,7 +177,7 @@ pub(crate) fn implicit_optional(checker: &Checker, parameters: &Parameters) { let Some(expr) = type_hint_explicitly_allows_none( parsed_annotation.expression(), checker, - checker.settings.target_version.minor, + checker.settings.target_version, ) else { continue; }; @@ -195,7 +195,7 @@ pub(crate) fn implicit_optional(checker: &Checker, parameters: &Parameters) { let Some(expr) = type_hint_explicitly_allows_none( annotation, checker, - checker.settings.target_version.minor, + checker.settings.target_version, ) else { continue; }; diff --git a/crates/ruff_linter/src/rules/ruff/typing.rs b/crates/ruff_linter/src/rules/ruff/typing.rs index 3faea2fa8..a12ab9b6d 100644 --- a/crates/ruff_linter/src/rules/ruff/typing.rs +++ b/crates/ruff_linter/src/rules/ruff/typing.rs @@ -10,10 +10,10 @@ use crate::checkers::ast::Checker; /// /// A known type is either a builtin type, any object from the standard library, /// or a type from the `typing_extensions` module. -fn is_known_type(qualified_name: &QualifiedName, minor_version: u8) -> bool { +fn is_known_type(qualified_name: &QualifiedName, version: ast::PythonVersion) -> bool { match qualified_name.segments() { ["" | "typing_extensions", ..] => true, - [module, ..] => is_known_standard_library(minor_version, module), + [module, ..] => is_known_standard_library(version.minor, module), _ => false, } } @@ -70,7 +70,11 @@ enum TypingTarget<'a> { } impl<'a> TypingTarget<'a> { - fn try_from_expr(expr: &'a Expr, checker: &'a Checker, minor_version: u8) -> Option<Self> { + fn try_from_expr( + expr: &'a Expr, + checker: &'a Checker, + version: ast::PythonVersion, + ) -> Option<Self> { let semantic = checker.semantic(); match expr { Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { @@ -91,7 +95,7 @@ impl<'a> TypingTarget<'a> { resolve_slice_value(slice.as_ref()).next(), )) } else { - if is_known_type(&qualified_name, minor_version) { + if is_known_type(&qualified_name, version) { Some(TypingTarget::Known) } else { Some(TypingTarget::Unknown) @@ -137,7 +141,7 @@ impl<'a> TypingTarget<'a> { ) { Some(TypingTarget::Hashable) - } else if !is_known_type(&qualified_name, minor_version) { + } else if !is_known_type(&qualified_name, version) { // If it's not a known type, we assume it's `Any`. Some(TypingTarget::Unknown) } else { @@ -149,7 +153,7 @@ impl<'a> TypingTarget<'a> { } /// Check if the [`TypingTarget`] explicitly allows `None`. - fn contains_none(&self, checker: &Checker, minor_version: u8) -> bool { + fn contains_none(&self, checker: &Checker, version: ast::PythonVersion) -> bool { match self { TypingTarget::None | TypingTarget::Optional(_) @@ -162,10 +166,10 @@ impl<'a> TypingTarget<'a> { resolve_slice_value(slice).any(|element| { // Literal can only contain `None`, a literal value, other `Literal` // or an enum value. - match TypingTarget::try_from_expr(element, checker, minor_version) { + match TypingTarget::try_from_expr(element, checker, version) { None | Some(TypingTarget::None) => true, Some(new_target @ TypingTarget::Literal(_)) => { - new_target.contains_none(checker, minor_version) + new_target.contains_none(checker, version) } _ => false, } @@ -173,35 +177,32 @@ impl<'a> TypingTarget<'a> { }), TypingTarget::Union(slice) => slice.is_some_and(|slice| { resolve_slice_value(slice).any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) + TypingTarget::try_from_expr(element, checker, version) .map_or(true, |new_target| { - new_target.contains_none(checker, minor_version) + new_target.contains_none(checker, version) }) }) }), TypingTarget::PEP604Union(left, right) => [left, right].iter().any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_none(checker, minor_version) - }) + TypingTarget::try_from_expr(element, checker, version).map_or(true, |new_target| { + new_target.contains_none(checker, version) + }) }), TypingTarget::Annotated(expr) => expr.is_some_and(|expr| { - TypingTarget::try_from_expr(expr, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_none(checker, minor_version) - }) + TypingTarget::try_from_expr(expr, checker, version).map_or(true, |new_target| { + new_target.contains_none(checker, version) + }) }), TypingTarget::ForwardReference(expr) => { - TypingTarget::try_from_expr(expr, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_none(checker, minor_version) - }) + TypingTarget::try_from_expr(expr, checker, version).map_or(true, |new_target| { + new_target.contains_none(checker, version) + }) } } } /// Check if the [`TypingTarget`] explicitly allows `Any`. - fn contains_any(&self, checker: &Checker, minor_version: u8) -> bool { + fn contains_any(&self, checker: &Checker, version: ast::PythonVersion) -> bool { match self { TypingTarget::Any => true, // `Literal` cannot contain `Any` as it's a dynamic value. @@ -213,31 +214,23 @@ impl<'a> TypingTarget<'a> { | TypingTarget::Unknown => false, TypingTarget::Union(slice) => slice.is_some_and(|slice| { resolve_slice_value(slice).any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + TypingTarget::try_from_expr(element, checker, version) + .map_or(true, |new_target| new_target.contains_any(checker, version)) }) }), TypingTarget::PEP604Union(left, right) => [left, right].iter().any(|element| { - TypingTarget::try_from_expr(element, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + TypingTarget::try_from_expr(element, checker, version) + .map_or(true, |new_target| new_target.contains_any(checker, version)) }), TypingTarget::Annotated(expr) | TypingTarget::Optional(expr) => { expr.is_some_and(|expr| { - TypingTarget::try_from_expr(expr, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + TypingTarget::try_from_expr(expr, checker, version) + .map_or(true, |new_target| new_target.contains_any(checker, version)) }) } TypingTarget::ForwardReference(expr) => { - TypingTarget::try_from_expr(expr, checker, minor_version) - .map_or(true, |new_target| { - new_target.contains_any(checker, minor_version) - }) + TypingTarget::try_from_expr(expr, checker, version) + .map_or(true, |new_target| new_target.contains_any(checker, version)) } } } @@ -253,9 +246,9 @@ impl<'a> TypingTarget<'a> { pub(crate) fn type_hint_explicitly_allows_none<'a>( annotation: &'a Expr, checker: &'a Checker, - minor_version: u8, + version: ast::PythonVersion, ) -> Option<&'a Expr> { - match TypingTarget::try_from_expr(annotation, checker, minor_version) { + match TypingTarget::try_from_expr(annotation, checker, version) { None | // Short circuit on top level `None`, `Any` or `Optional` Some(TypingTarget::None | TypingTarget::Optional(_) | TypingTarget::Any) => None, @@ -264,10 +257,10 @@ pub(crate) fn type_hint_explicitly_allows_none<'a>( // is found nested inside another type, then the outer type should // be returned. Some(TypingTarget::Annotated(expr)) => { - expr.and_then(|expr| type_hint_explicitly_allows_none(expr, checker, minor_version)) + expr.and_then(|expr| type_hint_explicitly_allows_none(expr, checker, version)) } Some(target) => { - if target.contains_none(checker, minor_version) { + if target.contains_none(checker, version) { return None; } Some(annotation) @@ -281,9 +274,9 @@ pub(crate) fn type_hint_explicitly_allows_none<'a>( pub(crate) fn type_hint_resolves_to_any( annotation: &Expr, checker: &Checker, - minor_version: u8, + version: ast::PythonVersion, ) -> bool { - match TypingTarget::try_from_expr(annotation, checker, minor_version) { + match TypingTarget::try_from_expr(annotation, checker, version) { // Short circuit on top level `Any` None | Some(TypingTarget::Any) => true, // `Optional` is `Optional[Any]` which is `Any | None`. @@ -291,39 +284,43 @@ pub(crate) fn type_hint_resolves_to_any( // Top-level `Annotated` node should check if the inner type resolves // to `Any`. Some(TypingTarget::Annotated(expr)) => { - expr.is_some_and(|expr| type_hint_resolves_to_any(expr, checker, minor_version)) + expr.is_some_and(|expr| type_hint_resolves_to_any(expr, checker, version)) } - Some(target) => target.contains_any(checker, minor_version), + Some(target) => target.contains_any(checker, version), } } #[cfg(test)] mod tests { use super::is_known_type; + use ruff_python_ast as ast; use ruff_python_ast::name::QualifiedName; #[test] fn test_is_known_type() { - assert!(is_known_type(&QualifiedName::builtin("int"), 11)); + assert!(is_known_type( + &QualifiedName::builtin("int"), + ast::PythonVersion::PY311 + )); assert!(is_known_type( &QualifiedName::from_iter(["builtins", "int"]), - 11 + ast::PythonVersion::PY311 )); assert!(is_known_type( &QualifiedName::from_iter(["typing", "Optional"]), - 11 + ast::PythonVersion::PY311 )); assert!(is_known_type( &QualifiedName::from_iter(["typing_extensions", "Literal"]), - 11 + ast::PythonVersion::PY311 )); assert!(is_known_type( &QualifiedName::from_iter(["zoneinfo", "ZoneInfo"]), - 11 + ast::PythonVersion::PY311 )); assert!(!is_known_type( &QualifiedName::from_iter(["zoneinfo", "ZoneInfo"]), - 8 + ast::PythonVersion::PY38 )); } }

Should probably be a followup, though; this is already a big PR and the change isn't directly related

Ooh good idea. I'll save this as a fast follow-up.

crates/ruff_linter/src/settings/types.rs

crates/ruff_python_formatter/tests/fixtures.rs

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> This is a follow-up to #16170 to use `ast::PythonVersion` in the `type_hint_resolves_to_any` call chain, as suggested (and implemented!) by Alex [here](#16170 (comment)).

This is a follow-up to #16170 to use `ast::PythonVersion` in the `type_hint_resolves_to_any` call chain, as suggested (and implemented!) by Alex [here](#16170 (comment)). Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

* main: [red-knot] Allow any `Ranged` argument for `report_lint` and `report_diagnostic` (#16252) [pycodestyle] Exempt `site.addsitedir(...)` calls (E402) (#16251) red_knot_python_semantic: improve diagnostic message for "invalid argument type" ruff_db: add "secondary" messages to `Diagnostic` trait ruff_db: refactor snippet rendering red_knot_python_semantic: remove `Ranged` impl for `TypeCheckDiagnostic` [red-knot] Refactor `infer_chained_boolean_types` to have access to `TypeInferenceBuilder` (#16222) Add `red_knot/README.md` (#16230) [airflow] move class attributed related cases to AIR302_class_attribute (AIR302) (#16226) [red-knot] Update tests for attributes inferred from parameters (#16208) [red-knot] update TODO comment in mdtest (#16242) [`refurb`] Correctly handle lengths of literal strings in `slice-to-remove-prefix-or-suffix` (`FURB188`) (#16237) Pass `ast::PythonVersion` to `type_hint_resolves_to_any` (#16236) Use `ast::PythonVersion` internally in the formatter and linter (#16170) Add `SECURITY.md` (#16224)

ntBre added 6 commits February 14, 2025 13:21

add serde adapter module, use PythonVersion in formatter

48e7160

convert to old PythonVersion for formatter snapshots

ad7f688

accept ruff settings snapshot (bad idea?)

55d4b5e

tidy formatter PythonVersion

84f518a

use (de)serialize_with in linter too

f40253c

delete separate enum code

50fd040

ntBre added the internal An internal refactor or improvement label Feb 14, 2025

MichaReiser reviewed Feb 17, 2025

View reviewed changes

crates/ruff/tests/snapshots/show_settings__display_default_settings.snap Outdated Show resolved Hide resolved

MichaReiser reviewed Feb 17, 2025

View reviewed changes

crates/ruff_python_ast/src/python_version.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Feb 17, 2025

View reviewed changes

crates/ruff_python_ast/src/python_version.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Feb 17, 2025

View reviewed changes

ruff.schema.json Outdated Show resolved Hide resolved

ntBre added 5 commits February 17, 2025 08:21

use Display instead of Debug for showing settings

a38c577

only use AstPythonVersion internally, move pep440 to external side

195dd99

move adapter functions to serde module and rename

e218330

update schema

7ef41dc

gate CacheKey behind existing cache feature

67b2ffc

ntBre added 10 commits February 17, 2025 10:14

move serde helpers to formatter, where they are used

da98908

remove serde adapters

e12fe0a

the python version is only used internally, so we can just switch to the dotted representation

print the filename for failed format tests

8349aab

update formatter json files for new serialization format

a0c5cd6

use PythonVersion Display impl instead of converting, update snaps

ee268a4

remove unused TryFrom

af041b6

remove latest comment, the method no longer exists

4ede555

simplify From now that as_tuple is back

33caff6

remove unused Default impl for formatter::PythonVersion

e9be081

update comment

02297d4

this Default impl is only used in one test, so we could possibly get rid of it as well if we updated the test

ntBre commented Feb 17, 2025

View reviewed changes

crates/ruff_linter/src/settings/types.rs Outdated Show resolved Hide resolved

AlexWaygood reviewed Feb 18, 2025

View reviewed changes

crates/ruff_linter/src/settings/types.rs Show resolved Hide resolved

AlexWaygood reviewed Feb 18, 2025

View reviewed changes

crates/ruff_linter/src/settings/types.rs Outdated Show resolved Hide resolved

AlexWaygood approved these changes Feb 18, 2025

View reviewed changes

ntBre added 11 commits February 18, 2025 08:53

restore try_from and use it for default

55e76fa

test that PythonVersion::default doesn't panic

0de7783

pub use python_version and use ast::PythonVersion

d64deb0

serde mod doesn't need pub now

020ffdf

remove PartialOrd and Ord

5f7ee7a

remove CacheKey

bad3bb7

move get_minimum_supported_version to where it's used

3231dde

update import script

0abcc2c

delete formatter PythonVersion

6dc5bf9

fix imports

3c90da8

use PythonVersion from ruff_python_ast, not ast::python_version

b75d033

ntBre requested review from carljm and sharkdp as code owners February 18, 2025 14:46

ntBre added 2 commits February 18, 2025 09:51

Merge branch 'main' into brent/formatter-version

5acc2ae

tidy up after merge

8d7cc7a

AlexWaygood approved these changes Feb 18, 2025

View reviewed changes

ntBre and others added 2 commits February 18, 2025 10:59

fix panic message on invalid JSON file

f8f162f

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

document unwrap safety

4e98d1a

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

ntBre merged commit a9efdea into main Feb 18, 2025
21 checks passed

ntBre deleted the brent/formatter-version branch February 18, 2025 17:03

ntBre mentioned this pull request Feb 18, 2025

Pass ast::PythonVersion to type_hint_resolves_to_any #16236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `ast::PythonVersion` internally in the formatter and linter #16170

Use `ast::PythonVersion` internally in the formatter and linter #16170

ntBre commented Feb 14, 2025 •

edited

Loading

ntBre commented Feb 14, 2025

MichaReiser Feb 17, 2025

ntBre Feb 17, 2025

github-actions bot commented Feb 17, 2025 •

edited

Loading

AlexWaygood left a comment

ntBre commented Feb 18, 2025

ntBre commented Feb 18, 2025

AlexWaygood Feb 18, 2025

ntBre Feb 18, 2025

		#[serde(deserialize_with = "adapter::deserialize::<_, PythonVersion>")]
		#[serde(serialize_with = "adapter::serialize::<_, PythonVersion>")]

Use ast::PythonVersion internally in the formatter and linter #16170

Use ast::PythonVersion internally in the formatter and linter #16170

Conversation

ntBre commented Feb 14, 2025 • edited Loading

Summary

Test Plan

ntBre commented Feb 14, 2025

MichaReiser Feb 17, 2025

Choose a reason for hiding this comment

ntBre Feb 17, 2025

Choose a reason for hiding this comment

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

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

AlexWaygood left a comment

Choose a reason for hiding this comment

ntBre commented Feb 18, 2025

ntBre commented Feb 18, 2025

AlexWaygood Feb 18, 2025

Choose a reason for hiding this comment

ntBre Feb 18, 2025

Choose a reason for hiding this comment

Use `ast::PythonVersion` internally in the formatter and linter #16170

Use `ast::PythonVersion` internally in the formatter and linter #16170

ntBre commented Feb 14, 2025 •

edited

Loading

github-actions bot commented Feb 17, 2025 •

edited

Loading

`ruff-ecosystem` results