From 81bfcc9899eb80a2d996989db09c3581fafd5dd4 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 3 Dec 2024 13:33:29 +0000 Subject: [PATCH] Minor followups to RUF052 (#14755) ## Summary Just some minor followups to the recently merged RUF052 rule, that was added in bf0fd04: - Some small tweaks to the docs - A minor code-style nit - Some more tests for my peace of mind, just to check that the new methods on the semantic model are working correctly I'm adding the "internal" label as this doesn't deserve a changelog entry. RUF052 is a new rule that hasn't been released yet. ## Test Plan `cargo test -p ruff_linter` --- .../resources/test/fixtures/ruff/RUF052.py | 31 +++++++++++-- .../rules/ruff/rules/used_dummy_variable.rs | 44 ++++++++++--------- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 42 +++++++++++++++++- ...var_regexp_preset__RUF052_RUF052.py_1.snap | 42 +++++++++++++++++- ...var_regexp_preset__RUF052_RUF052.py_2.snap | 43 ++++++++++++++++-- 5 files changed, 173 insertions(+), 29 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py index b99ce11108..4f37b1d7fa 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -14,7 +14,7 @@ _valid_var_2 = 2 _valid_var_3 = _valid_var_1 + _valid_var_2 def _valid_fun(): - pass + pass _valid_fun() @@ -35,7 +35,7 @@ def fun(): def fun(): global _x _x = "reassigned global" - return _x + return _x class _ValidClass: pass @@ -56,7 +56,7 @@ class ClassOk: def method(arg): _valid_unused_var = arg - return + return def fun(x): _ = 1 @@ -104,3 +104,28 @@ def fun(): x = "local" _x = "shadows local" # [RUF052] return _x + + +GLOBAL_1 = "global 1" +GLOBAL_1_ = "global 1 with trailing underscore" + +def unfixables(): + _GLOBAL_1 = "foo" + # unfixable because the rename would shadow a global variable + print(_GLOBAL_1) # [RUF052] + + local = "local" + local_ = "local2" + + # unfixable because the rename would shadow a local variable + _local = "local3" # [RUF052] + print(_local) + + def nested(): + _GLOBAL_1 = "foo" + # unfixable because the rename would shadow a global variable + print(_GLOBAL_1) # [RUF052] + + # unfixable because the rename would shadow a variable from the outer function + _local = "local4" + print(_local) 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 d40716dbd3..3be6928f8a 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 @@ -10,16 +10,19 @@ use ruff_text_size::Ranged; use crate::{checkers::ast::Checker, renamer::Renamer}; /// ## What it does -/// Checks for accesses of local dummy variables, excluding `_` and dunder variables. +/// Checks for "dummy variables" (variables that are named as if to indicate they are unused) +/// that are in fact used. /// /// By default, "dummy variables" are any variables with names that start with leading -/// underscores. However, this is customisable using the `dummy-variable-rgx` setting). +/// underscores. However, this is customisable using the [`lint.dummy-variable-rgx`] setting). +/// +/// Dunder variables are ignored by this rule, as are variables named `_`. /// /// ## Why is this bad? /// Marking a variable with a leading underscore conveys that it is intentionally unused within the function or method. /// When these variables are later referenced in the code, it causes confusion and potential misunderstandings about -/// the code's intention. A variable marked as "unused" being subsequently used suggests oversight or unintentional use. -/// This detracts from the clarity and maintainability of the codebase. +/// the code's intention. If a variable marked as "unused" is subsequently used, it suggests that either the variable +/// could be given a clearer name, or that the code is accidentally making use of the wrong variable. /// /// Sometimes leading underscores are used to avoid variables shadowing other variables, Python builtins, or Python /// keywords. However, [PEP 8] recommends to use trailing underscores for this rather than leading underscores. @@ -39,12 +42,14 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// ``` /// /// ## Fix availability -/// An fix is only available for variables that start with leading underscores. +/// The rule's fix is only available for variables that start with leading underscores. +/// It will also only be available if the "obvious" new name for the variable +/// would not shadow any other known variables already accessible from the scope +/// in which the variable is defined. /// /// ## Options /// - [`lint.dummy-variable-rgx`] /// -/// /// [PEP 8]: https://peps.python.org/pep-0008/ #[derive(ViolationMetadata)] pub(crate) struct UsedDummyVariable { @@ -61,21 +66,18 @@ impl Violation for UsedDummyVariable { } fn fix_title(&self) -> Option { - if let Some(shadowed_kind) = self.shadowed_kind { - return Some(match shadowed_kind { - ShadowedKind::BuiltIn => { - "Prefer using trailing underscores to avoid shadowing a built-in".to_string() - } - ShadowedKind::Keyword => { - "Prefer using trailing underscores to avoid shadowing a keyword".to_string() - } - ShadowedKind::Some => { - "Prefer using trailing underscores to avoid shadowing a variable".to_string() - } - ShadowedKind::None => "Remove leading underscores".to_string(), - }); - } - None + self.shadowed_kind.map(|kind| match kind { + ShadowedKind::BuiltIn => { + "Prefer using trailing underscores to avoid shadowing a built-in".to_string() + } + ShadowedKind::Keyword => { + "Prefer using trailing underscores to avoid shadowing a keyword".to_string() + } + ShadowedKind::Some => { + "Prefer using trailing underscores to avoid shadowing a variable".to_string() + } + ShadowedKind::None => "Remove leading underscores".to_string(), + }) } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap index ad8e535531..f81ceaccac 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text --- RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed | @@ -130,3 +129,44 @@ RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed 106 |- return _x 105 |+ x_ = "shadows local" # [RUF052] 106 |+ return x_ +107 107 | +108 108 | +109 109 | GLOBAL_1 = "global 1" + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap index ad8e535531..f81ceaccac 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text --- RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed | @@ -130,3 +129,44 @@ RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed 106 |- return _x 105 |+ x_ = "shadows local" # [RUF052] 106 |+ return x_ +107 107 | +108 108 | +109 109 | GLOBAL_1 = "global 1" + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap index ea20bafed6..b519bae0f8 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap @@ -1,6 +1,5 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text --- RUF052.py:21:9: RUF052 Local dummy variable `arg` is accessed | @@ -38,12 +37,12 @@ RUF052.py:57:16: RUF052 Local dummy variable `arg` is accessed 57 | def method(arg): | ^^^ RUF052 58 | _valid_unused_var = arg -59 | return +59 | return | RUF052.py:61:9: RUF052 Local dummy variable `x` is accessed | -59 | return +59 | return 60 | 61 | def fun(x): | ^ RUF052 @@ -119,3 +118,41 @@ RUF052.py:105:5: RUF052 Local dummy variable `_x` is accessed 106 | return _x | = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:113:5: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +112 | def unfixables(): +113 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +114 | # unfixable because the rename would shadow a global variable +115 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:121:5: RUF052 Local dummy variable `_local` is accessed + | +120 | # unfixable because the rename would shadow a local variable +121 | _local = "local3" # [RUF052] + | ^^^^^^ RUF052 +122 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:125:9: RUF052 Local dummy variable `_GLOBAL_1` is accessed + | +124 | def nested(): +125 | _GLOBAL_1 = "foo" + | ^^^^^^^^^ RUF052 +126 | # unfixable because the rename would shadow a global variable +127 | print(_GLOBAL_1) # [RUF052] + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:130:9: RUF052 Local dummy variable `_local` is accessed + | +129 | # unfixable because the rename would shadow a variable from the outer function +130 | _local = "local4" + | ^^^^^^ RUF052 +131 | print(_local) + | + = help: Prefer using trailing underscores to avoid shadowing a variable