From 6bb32355ef9f2fd350a148a1b0d30b78875646a1 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:22:57 -0500 Subject: [PATCH] [`pyupgrade`] Rename private type parameters in PEP 695 generics (`UP049`) (#15862) ## Summary This is a new rule to implement the renaming of PEP 695 type parameters with leading underscores after they have (presumably) been converted from standalone type variables by either UP046 or UP047. Part of #15642. I'm not 100% sure the fix is always safe, but I haven't come up with any counterexamples yet. `Renamer` seems pretty precise, so I don't think the usual issues with comments apply. I initially tried writing this as a rule that receives a `Stmt` rather than a `Binding`, but in that case the `checker.semantic().current_scope()` was the global scope, rather than the scope of the type parameters as I needed. Most of the other rules using `Renamer` also used `Binding`s, but it does have the downside of offering separate diagnostics for each parameter to rename. ## Test Plan New snapshot tests for UP049 alone and the combination of UP046, UP049, and PYI018. --------- Co-authored-by: Alex Waygood --- crates/ruff/tests/lint.rs | 51 ++++ .../test/fixtures/pyupgrade/UP049_0.py | 30 +++ .../test/fixtures/pyupgrade/UP049_1.py | 56 ++++ .../src/checkers/ast/analyze/bindings.rs | 8 +- crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/renamer.rs | 52 ++++ crates/ruff_linter/src/rules/pyupgrade/mod.rs | 2 + .../src/rules/pyupgrade/rules/pep695/mod.rs | 2 + .../rules/pep695/private_type_parameter.rs | 149 +++++++++++ ...__rules__pyupgrade__tests__UP049_0.py.snap | 109 ++++++++ ...__rules__pyupgrade__tests__UP049_1.py.snap | 245 ++++++++++++++++++ .../rules/ruff/rules/used_dummy_variable.rs | 98 +++---- ruff.schema.json | 1 + 13 files changed, 736 insertions(+), 68 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_0.py create mode 100644 crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_1.py create mode 100644 crates/ruff_linter/src/rules/pyupgrade/rules/pep695/private_type_parameter.rs create mode 100644 crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_0.py.snap create mode 100644 crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_1.py.snap diff --git a/crates/ruff/tests/lint.rs b/crates/ruff/tests/lint.rs index c9b136bcf7..4aa40ea1d5 100644 --- a/crates/ruff/tests/lint.rs +++ b/crates/ruff/tests/lint.rs @@ -2174,3 +2174,54 @@ fn flake8_import_convention_unused_aliased_import() { .arg("-") .pass_stdin("1")); } + +/// Test that private, old-style `TypeVar` generics +/// 1. Get replaced with PEP 695 type parameters (UP046, UP047) +/// 2. Get renamed to remove leading underscores (UP049) +/// 3. Emit a warning that the standalone type variable is now unused (PYI018) +/// 4. Remove the now-unused `Generic` import +#[test] +fn pep695_generic_rename() { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .args(STDIN_BASE_OPTIONS) + .args(["--select", "F401,PYI018,UP046,UP047,UP049"]) + .args(["--stdin-filename", "test.py"]) + .arg("--unsafe-fixes") + .arg("--fix") + .arg("--preview") + .arg("--target-version=py312") + .arg("-") + .pass_stdin( + r#" +from typing import Generic, TypeVar +_T = TypeVar("_T") + +class OldStyle(Generic[_T]): + var: _T + +def func(t: _T) -> _T: + x: _T + return x +"# + ), + @r#" + success: false + exit_code: 1 + ----- stdout ----- + + from typing import TypeVar + _T = TypeVar("_T") + + class OldStyle[T]: + var: T + + def func[T](t: T) -> T: + x: T + return x + + ----- stderr ----- + test.py:3:1: PYI018 Private TypeVar `_T` is never used + Found 6 errors (5 fixed, 1 remaining). + "# + ); +} diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_0.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_0.py new file mode 100644 index 0000000000..217dbbef84 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_0.py @@ -0,0 +1,30 @@ +# simple case, replace _T in signature and body +class Generic[_T]: + buf: list[_T] + + def append(self, t: _T): + self.buf.append(t) + + +# simple case, replace _T in signature and body +def second[_T](var: tuple[_T]) -> _T: + y: _T = var[1] + return y + + +# one diagnostic for each variable, comments are preserved +def many_generics[ + _T, # first generic + _U, # second generic +](args): + return args + + +# neither of these are currently renamed +from typing import Literal, cast + + +def f[_T](v): + cast("_T", v) + cast("Literal['_T']") + cast("list[_T]", v) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_1.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_1.py new file mode 100644 index 0000000000..5df573a1f7 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_1.py @@ -0,0 +1,56 @@ +# bound +class Foo[_T: str]: + var: _T + + +# constraint +class Foo[_T: (str, bytes)]: + var: _T + + +# python 3.13+ default +class Foo[_T = int]: + var: _T + + +# tuple +class Foo[*_Ts]: + var: tuple[*_Ts] + + +# paramspec +class C[**_P]: + var: _P + + +from typing import Callable + + +# each of these will get a separate diagnostic, but at least they'll all get +# fixed +class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + @staticmethod + def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + return None + + +# this should not be fixed because the new name is a keyword, but we still +# offer a diagnostic +class F[_async]: ... + + +# and this should not be fixed because of the conflict with the outer X, but it +# also gets a diagnostic +def f(): + type X = int + + class ScopeConflict[_X]: + var: _X + x: X + + +# these cases should be skipped entirely +def f[_](x: _) -> _: ... +def g[__](x: __) -> __: ... +def h[_T_](x: _T_) -> _T_: ... +def i[__T__](x: __T__) -> __T__: ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 3815ea21e3..a042f7506b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -5,7 +5,7 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::rules::{ flake8_import_conventions, flake8_pyi, flake8_pytest_style, flake8_type_checking, pyflakes, - pylint, refurb, ruff, + pylint, pyupgrade, refurb, ruff, }; /// Run lint rules over the [`Binding`]s. @@ -24,6 +24,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::PytestUnittestRaisesAssertion, Rule::ForLoopWrites, Rule::CustomTypeVarForSelf, + Rule::PrivateTypeParameter, ]) { return; } @@ -123,5 +124,10 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::PrivateTypeParameter) { + if let Some(diagnostic) = pyupgrade::rules::private_type_parameter(checker, binding) { + checker.diagnostics.push(diagnostic); + } + } } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 09af31938a..b9be03c40a 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -542,6 +542,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pyupgrade, "045") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP604AnnotationOptional), (Pyupgrade, "046") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP695GenericClass), (Pyupgrade, "047") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP695GenericFunction), + (Pyupgrade, "049") => (RuleGroup::Preview, rules::pyupgrade::rules::PrivateTypeParameter), // pydocstyle (Pydocstyle, "100") => (RuleGroup::Stable, rules::pydocstyle::rules::UndocumentedPublicModule), diff --git a/crates/ruff_linter/src/renamer.rs b/crates/ruff_linter/src/renamer.rs index a5308d9029..fb87c83ca3 100644 --- a/crates/ruff_linter/src/renamer.rs +++ b/crates/ruff_linter/src/renamer.rs @@ -7,8 +7,11 @@ use ruff_diagnostics::Edit; use ruff_python_ast as ast; use ruff_python_codegen::Stylist; use ruff_python_semantic::{Binding, BindingKind, Scope, ScopeId, SemanticModel}; +use ruff_python_stdlib::{builtins::is_python_builtin, keyword::is_keyword}; use ruff_text_size::Ranged; +use crate::checkers::ast::Checker; + pub(crate) struct Renamer; impl Renamer { @@ -369,3 +372,52 @@ impl Renamer { } } } + +/// Enumeration of various ways in which a binding can shadow other variables +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub(crate) enum ShadowedKind { + /// The variable shadows a global, nonlocal or local symbol + Some, + /// The variable shadows a builtin symbol + BuiltIn, + /// The variable shadows a keyword + Keyword, + /// The variable does not shadow any other symbols + None, +} + +impl ShadowedKind { + /// Determines the kind of shadowing or conflict for a given variable name. + /// + /// This function is useful for checking whether or not the `target` of a [`Rename::rename`] + /// will shadow another binding. + pub(crate) fn new(name: &str, checker: &Checker, scope_id: ScopeId) -> ShadowedKind { + // Check the kind in order of precedence + if is_keyword(name) { + return ShadowedKind::Keyword; + } + + if is_python_builtin( + name, + checker.settings.target_version.minor(), + checker.source_type.is_ipynb(), + ) { + return ShadowedKind::BuiltIn; + } + + if !checker.semantic().is_available_in_scope(name, scope_id) { + return ShadowedKind::Some; + } + + // Default to no shadowing + ShadowedKind::None + } + + /// Returns `true` if `self` shadows any global, nonlocal, or local symbol, keyword, or builtin. + pub(crate) const fn shadows_any(self) -> bool { + matches!( + self, + ShadowedKind::Some | ShadowedKind::BuiltIn | ShadowedKind::Keyword + ) + } +} diff --git a/crates/ruff_linter/src/rules/pyupgrade/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/mod.rs index f10421e4ff..e033cd3fb9 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/mod.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/mod.rs @@ -106,6 +106,8 @@ mod tests { #[test_case(Rule::NonPEP695GenericClass, Path::new("UP046_0.py"))] #[test_case(Rule::NonPEP695GenericClass, Path::new("UP046_1.py"))] #[test_case(Rule::NonPEP695GenericFunction, Path::new("UP047.py"))] + #[test_case(Rule::PrivateTypeParameter, Path::new("UP049_0.py"))] + #[test_case(Rule::PrivateTypeParameter, Path::new("UP049_1.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = path.to_string_lossy().to_string(); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs index 273c0867d4..68db219871 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs @@ -18,12 +18,14 @@ use ruff_text_size::{Ranged, TextRange}; pub(crate) use non_pep695_generic_class::*; pub(crate) use non_pep695_generic_function::*; pub(crate) use non_pep695_type_alias::*; +pub(crate) use private_type_parameter::*; use crate::checkers::ast::Checker; mod non_pep695_generic_class; mod non_pep695_generic_function; mod non_pep695_type_alias; +mod private_type_parameter; #[derive(Debug)] enum TypeVarRestriction<'a> { diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/private_type_parameter.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/private_type_parameter.rs new file mode 100644 index 0000000000..ad0cd13a70 --- /dev/null +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/private_type_parameter.rs @@ -0,0 +1,149 @@ +use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_ast::Stmt; +use ruff_python_semantic::Binding; + +use crate::{ + checkers::ast::Checker, + renamer::{Renamer, ShadowedKind}, +}; + +/// ## What it does +/// +/// Checks for use of [PEP 695] type parameters with leading underscores in generic classes and +/// functions. +/// +/// ## Why is this bad? +/// +/// [PEP 695] type parameters are already restricted in scope to the class or function in which they +/// appear, so leading underscores just hurt readability without the usual privacy benefits. +/// +/// However, neither a diagnostic nor a fix will be emitted for "sunder" (`_T_`) or "dunder" +/// (`__T__`) type parameter names as these are not considered private. +/// +/// ## Example +/// +/// ```python +/// class GenericClass[_T]: +/// var: _T +/// +/// +/// def generic_function[_T](var: _T) -> list[_T]: +/// return var[0] +/// ``` +/// +/// Use instead: +/// +/// ```python +/// class GenericClass[T]: +/// var: T +/// +/// +/// def generic_function[T](var: T) -> list[T]: +/// return var[0] +/// ``` +/// +/// ## Fix availability +/// +/// If the name without an underscore would shadow a builtin or another variable, would be a +/// keyword, or would otherwise be an invalid identifier, a fix will not be available. In these +/// situations, you can consider using a trailing underscore or a different name entirely to satisfy +/// the lint rule. +/// +/// ## See also +/// +/// This rule renames private [PEP 695] type parameters but doesn't convert pre-[PEP 695] generics +/// to the new format. See [`non-pep695-generic-function`] and [`non-pep695-generic-class`] for +/// rules that will make this transformation. Those rules do not remove unused type variables after +/// their changes, so you may also want to consider enabling [`unused-private-type-var`] to complete +/// the transition to [PEP 695] generics. +/// +/// [PEP 695]: https://peps.python.org/pep-0695/ +/// [non-pep695-generic-function]: https://docs.astral.sh/ruff/rules/non-pep695-generic-function +/// [non-pep695-generic-class]: https://docs.astral.sh/ruff/rules/non-pep695-generic-class +/// [unused-private-type-var]: https://docs.astral.sh/ruff/rules/unused-private-type-var +#[derive(ViolationMetadata)] +pub(crate) struct PrivateTypeParameter { + kind: ParamKind, +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum ParamKind { + Class, + Function, +} + +impl ParamKind { + const fn as_str(self) -> &'static str { + match self { + ParamKind::Class => "class", + ParamKind::Function => "function", + } + } +} + +impl Violation for PrivateTypeParameter { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] + fn message(&self) -> String { + format!( + "Generic {} uses private type parameters", + self.kind.as_str() + ) + } + + fn fix_title(&self) -> Option { + Some("Remove the leading underscores".to_string()) + } +} + +/// UP049 +pub(crate) fn private_type_parameter(checker: &Checker, binding: &Binding) -> Option { + let semantic = checker.semantic(); + let stmt = binding.statement(semantic)?; + if !binding.kind.is_type_param() { + return None; + } + + let kind = match stmt { + Stmt::FunctionDef(_) => ParamKind::Function, + Stmt::ClassDef(_) => ParamKind::Class, + _ => return None, + }; + + let old_name = binding.name(checker.source()); + + if !old_name.starts_with('_') { + return None; + } + + // Sunder `_T_`, dunder `__T__`, and all all-under `_` or `__` cases should all be skipped, as + // these are not "private" names + if old_name.ends_with('_') { + return None; + } + + let mut diagnostic = Diagnostic::new(PrivateTypeParameter { kind }, binding.range); + + let new_name = old_name.trim_start_matches('_'); + + // if the new name would shadow another variable, keyword, or builtin, emit a diagnostic without + // a suggested fix + if ShadowedKind::new(new_name, checker, binding.scope).shadows_any() { + return Some(diagnostic); + } + + diagnostic.try_set_fix(|| { + let (first, rest) = Renamer::rename( + old_name, + new_name, + &semantic.scopes[binding.scope], + checker.semantic(), + checker.stylist(), + )?; + + Ok(Fix::safe_edits(first, rest)) + }); + + Some(diagnostic) +} diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_0.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_0.py.snap new file mode 100644 index 0000000000..76c5f7864f --- /dev/null +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_0.py.snap @@ -0,0 +1,109 @@ +--- +source: crates/ruff_linter/src/rules/pyupgrade/mod.rs +--- +UP049_0.py:2:15: UP049 [*] Generic class uses private type parameters + | +1 | # simple case, replace _T in signature and body +2 | class Generic[_T]: + | ^^ UP049 +3 | buf: list[_T] + | + = help: Remove the leading underscores + +ℹ Safe fix +1 1 | # simple case, replace _T in signature and body +2 |-class Generic[_T]: +3 |- buf: list[_T] + 2 |+class Generic[T]: + 3 |+ buf: list[T] +4 4 | +5 |- def append(self, t: _T): + 5 |+ def append(self, t: T): +6 6 | self.buf.append(t) +7 7 | +8 8 | + +UP049_0.py:10:12: UP049 [*] Generic function uses private type parameters + | + 9 | # simple case, replace _T in signature and body +10 | def second[_T](var: tuple[_T]) -> _T: + | ^^ UP049 +11 | y: _T = var[1] +12 | return y + | + = help: Remove the leading underscores + +ℹ Safe fix +7 7 | +8 8 | +9 9 | # simple case, replace _T in signature and body +10 |-def second[_T](var: tuple[_T]) -> _T: +11 |- y: _T = var[1] + 10 |+def second[T](var: tuple[T]) -> T: + 11 |+ y: T = var[1] +12 12 | return y +13 13 | +14 14 | + +UP049_0.py:17:5: UP049 [*] Generic function uses private type parameters + | +15 | # one diagnostic for each variable, comments are preserved +16 | def many_generics[ +17 | _T, # first generic + | ^^ UP049 +18 | _U, # second generic +19 | ](args): + | + = help: Remove the leading underscores + +ℹ Safe fix +14 14 | +15 15 | # one diagnostic for each variable, comments are preserved +16 16 | def many_generics[ +17 |- _T, # first generic + 17 |+ T, # first generic +18 18 | _U, # second generic +19 19 | ](args): +20 20 | return args + +UP049_0.py:18:5: UP049 [*] Generic function uses private type parameters + | +16 | def many_generics[ +17 | _T, # first generic +18 | _U, # second generic + | ^^ UP049 +19 | ](args): +20 | return args + | + = help: Remove the leading underscores + +ℹ Safe fix +15 15 | # one diagnostic for each variable, comments are preserved +16 16 | def many_generics[ +17 17 | _T, # first generic +18 |- _U, # second generic + 18 |+ U, # second generic +19 19 | ](args): +20 20 | return args +21 21 | + +UP049_0.py:27:7: UP049 [*] Generic function uses private type parameters + | +27 | def f[_T](v): + | ^^ UP049 +28 | cast("_T", v) +29 | cast("Literal['_T']") + | + = help: Remove the leading underscores + +ℹ Safe fix +24 24 | from typing import Literal, cast +25 25 | +26 26 | +27 |-def f[_T](v): +28 |- cast("_T", v) + 27 |+def f[T](v): + 28 |+ cast("T", v) +29 29 | cast("Literal['_T']") +30 |- cast("list[_T]", v) + 30 |+ cast("list[T]", v) diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_1.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_1.py.snap new file mode 100644 index 0000000000..96df294002 --- /dev/null +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP049_1.py.snap @@ -0,0 +1,245 @@ +--- +source: crates/ruff_linter/src/rules/pyupgrade/mod.rs +--- +UP049_1.py:2:11: UP049 [*] Generic class uses private type parameters + | +1 | # bound +2 | class Foo[_T: str]: + | ^^ UP049 +3 | var: _T + | + = help: Remove the leading underscores + +ℹ Safe fix +1 1 | # bound +2 |-class Foo[_T: str]: +3 |- var: _T + 2 |+class Foo[T: str]: + 3 |+ var: T +4 4 | +5 5 | +6 6 | # constraint + +UP049_1.py:7:11: UP049 [*] Generic class uses private type parameters + | +6 | # constraint +7 | class Foo[_T: (str, bytes)]: + | ^^ UP049 +8 | var: _T + | + = help: Remove the leading underscores + +ℹ Safe fix +4 4 | +5 5 | +6 6 | # constraint +7 |-class Foo[_T: (str, bytes)]: +8 |- var: _T + 7 |+class Foo[T: (str, bytes)]: + 8 |+ var: T +9 9 | +10 10 | +11 11 | # python 3.13+ default + +UP049_1.py:12:11: UP049 [*] Generic class uses private type parameters + | +11 | # python 3.13+ default +12 | class Foo[_T = int]: + | ^^ UP049 +13 | var: _T + | + = help: Remove the leading underscores + +ℹ Safe fix +9 9 | +10 10 | +11 11 | # python 3.13+ default +12 |-class Foo[_T = int]: +13 |- var: _T + 12 |+class Foo[T = int]: + 13 |+ var: T +14 14 | +15 15 | +16 16 | # tuple + +UP049_1.py:17:12: UP049 [*] Generic class uses private type parameters + | +16 | # tuple +17 | class Foo[*_Ts]: + | ^^^ UP049 +18 | var: tuple[*_Ts] + | + = help: Remove the leading underscores + +ℹ Safe fix +14 14 | +15 15 | +16 16 | # tuple +17 |-class Foo[*_Ts]: +18 |- var: tuple[*_Ts] + 17 |+class Foo[*Ts]: + 18 |+ var: tuple[*Ts] +19 19 | +20 20 | +21 21 | # paramspec + +UP049_1.py:22:11: UP049 [*] Generic class uses private type parameters + | +21 | # paramspec +22 | class C[**_P]: + | ^^ UP049 +23 | var: _P + | + = help: Remove the leading underscores + +ℹ Safe fix +19 19 | +20 20 | +21 21 | # paramspec +22 |-class C[**_P]: +23 |- var: _P + 22 |+class C[**P]: + 23 |+ var: P +24 24 | +25 25 | +26 26 | from typing import Callable + +UP049_1.py:31:18: UP049 [*] Generic class uses private type parameters + | +29 | # each of these will get a separate diagnostic, but at least they'll all get +30 | # fixed +31 | class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + | ^^ UP049 +32 | @staticmethod +33 | def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + | + = help: Remove the leading underscores + +ℹ Safe fix +28 28 | +29 29 | # each of these will get a separate diagnostic, but at least they'll all get +30 30 | # fixed +31 |-class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + 31 |+class Everything[T, _U: str, _V: (int, float), *_W, **_X]: +32 32 | @staticmethod +33 |- def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + 33 |+ def transform(t: T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, T] | None: +34 34 | return None +35 35 | +36 36 | + +UP049_1.py:31:22: UP049 [*] Generic class uses private type parameters + | +29 | # each of these will get a separate diagnostic, but at least they'll all get +30 | # fixed +31 | class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + | ^^ UP049 +32 | @staticmethod +33 | def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + | + = help: Remove the leading underscores + +ℹ Safe fix +28 28 | +29 29 | # each of these will get a separate diagnostic, but at least they'll all get +30 30 | # fixed +31 |-class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + 31 |+class Everything[_T, U: str, _V: (int, float), *_W, **_X]: +32 32 | @staticmethod +33 |- def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + 33 |+ def transform(t: _T, u: U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: +34 34 | return None +35 35 | +36 36 | + +UP049_1.py:31:31: UP049 [*] Generic class uses private type parameters + | +29 | # each of these will get a separate diagnostic, but at least they'll all get +30 | # fixed +31 | class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + | ^^ UP049 +32 | @staticmethod +33 | def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + | + = help: Remove the leading underscores + +ℹ Safe fix +28 28 | +29 29 | # each of these will get a separate diagnostic, but at least they'll all get +30 30 | # fixed +31 |-class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + 31 |+class Everything[_T, _U: str, V: (int, float), *_W, **_X]: +32 32 | @staticmethod +33 |- def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + 33 |+ def transform(t: _T, u: _U, v: V) -> tuple[*_W] | Callable[_X, _T] | None: +34 34 | return None +35 35 | +36 36 | + +UP049_1.py:31:50: UP049 [*] Generic class uses private type parameters + | +29 | # each of these will get a separate diagnostic, but at least they'll all get +30 | # fixed +31 | class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + | ^^ UP049 +32 | @staticmethod +33 | def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + | + = help: Remove the leading underscores + +ℹ Safe fix +28 28 | +29 29 | # each of these will get a separate diagnostic, but at least they'll all get +30 30 | # fixed +31 |-class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + 31 |+class Everything[_T, _U: str, _V: (int, float), *W, **_X]: +32 32 | @staticmethod +33 |- def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + 33 |+ def transform(t: _T, u: _U, v: _V) -> tuple[*W] | Callable[_X, _T] | None: +34 34 | return None +35 35 | +36 36 | + +UP049_1.py:31:56: UP049 [*] Generic class uses private type parameters + | +29 | # each of these will get a separate diagnostic, but at least they'll all get +30 | # fixed +31 | class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + | ^^ UP049 +32 | @staticmethod +33 | def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + | + = help: Remove the leading underscores + +ℹ Safe fix +28 28 | +29 29 | # each of these will get a separate diagnostic, but at least they'll all get +30 30 | # fixed +31 |-class Everything[_T, _U: str, _V: (int, float), *_W, **_X]: + 31 |+class Everything[_T, _U: str, _V: (int, float), *_W, **X]: +32 32 | @staticmethod +33 |- def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None: + 33 |+ def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[X, _T] | None: +34 34 | return None +35 35 | +36 36 | + +UP049_1.py:39:9: UP049 Generic class uses private type parameters + | +37 | # this should not be fixed because the new name is a keyword, but we still +38 | # offer a diagnostic +39 | class F[_async]: ... + | ^^^^^^ UP049 + | + = help: Remove the leading underscores + +UP049_1.py:47:25: UP049 Generic class uses private type parameters + | +45 | type X = int +46 | +47 | class ScopeConflict[_X]: + | ^^ UP049 +48 | var: _X +49 | x: X + | + = help: Remove the leading underscores diff --git a/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs index 525aba5f26..6687983ad7 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs @@ -2,12 +2,13 @@ use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; use ruff_python_semantic::{Binding, BindingId, ScopeId}; -use ruff_python_stdlib::{ - builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, -}; +use ruff_python_stdlib::identifiers::is_identifier; use ruff_text_size::Ranged; -use crate::{checkers::ast::Checker, renamer::Renamer}; +use crate::{ + checkers::ast::Checker, + renamer::{Renamer, ShadowedKind}, +}; /// ## What it does /// Checks for "dummy variables" (variables that are named as if to indicate they are unused) @@ -164,53 +165,50 @@ pub(crate) fn used_dummy_variable( return None; } - let shadowed_kind = try_shadowed_kind(name, checker, binding.scope); + // If the name doesn't start with an underscore, we don't consider it for a fix + if !name.starts_with('_') { + return Some(Diagnostic::new( + UsedDummyVariable { + name: name.to_string(), + shadowed_kind: None, + }, + binding.range(), + )); + } + + // Trim the leading underscores for further checks + let trimmed_name = name.trim_start_matches('_'); + + let shadowed_kind = ShadowedKind::new(trimmed_name, checker, binding.scope); let mut diagnostic = Diagnostic::new( UsedDummyVariable { name: name.to_string(), - shadowed_kind, + shadowed_kind: Some(shadowed_kind), }, binding.range(), ); - // If fix available - if let Some(shadowed_kind) = shadowed_kind { - // Get the possible fix based on the scope - if let Some(fix) = get_possible_fix(name, shadowed_kind, binding.scope, checker) { - diagnostic.try_set_fix(|| { - Renamer::rename(name, &fix, scope, semantic, checker.stylist()) - .map(|(edit, rest)| Fix::unsafe_edits(edit, rest)) - }); - } + // Get the possible fix based on the scope + if let Some(new_name) = + get_possible_new_name(trimmed_name, shadowed_kind, binding.scope, checker) + { + diagnostic.try_set_fix(|| { + Renamer::rename(name, &new_name, scope, semantic, checker.stylist()) + .map(|(edit, rest)| Fix::unsafe_edits(edit, rest)) + }); } Some(diagnostic) } -/// Enumeration of various ways in which a binding can shadow other variables -#[derive(Debug, PartialEq, Eq, Copy, Clone)] -enum ShadowedKind { - /// The variable shadows a global, nonlocal or local symbol - Some, - /// The variable shadows a builtin symbol - BuiltIn, - /// The variable shadows a keyword - Keyword, - /// The variable does not shadow any other symbols - None, -} - /// Suggests a potential alternative name to resolve a shadowing conflict. -fn get_possible_fix( - name: &str, +fn get_possible_new_name( + trimmed_name: &str, kind: ShadowedKind, scope_id: ScopeId, checker: &Checker, ) -> Option { - // Remove leading underscores for processing - let trimmed_name = name.trim_start_matches('_'); - // Construct the potential fix name based on ShadowedKind let fix_name = match kind { ShadowedKind::Some | ShadowedKind::BuiltIn | ShadowedKind::Keyword => { @@ -235,37 +233,3 @@ fn get_possible_fix( // Check if the fix name is a valid identifier is_identifier(&fix_name).then_some(fix_name) } - -/// Determines the kind of shadowing or conflict for a given variable name. -fn try_shadowed_kind(name: &str, checker: &Checker, scope_id: ScopeId) -> Option { - // If the name starts with an underscore, we don't consider it - if !name.starts_with('_') { - return None; - } - - // Trim the leading underscores for further checks - let trimmed_name = name.trim_start_matches('_'); - - // Check the kind in order of precedence - if is_keyword(trimmed_name) { - return Some(ShadowedKind::Keyword); - } - - if is_python_builtin( - trimmed_name, - checker.settings.target_version.minor(), - checker.source_type.is_ipynb(), - ) { - return Some(ShadowedKind::BuiltIn); - } - - if !checker - .semantic() - .is_available_in_scope(trimmed_name, scope_id) - { - return Some(ShadowedKind::Some); - } - - // Default to no shadowing - Some(ShadowedKind::None) -} diff --git a/ruff.schema.json b/ruff.schema.json index 48042711f2..05cecddeb1 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4218,6 +4218,7 @@ "UP045", "UP046", "UP047", + "UP049", "W", "W1", "W19",