From bdca7694969af785d3eec6d99d75ed38a5b20b4e Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Tue, 26 Nov 2024 14:38:42 +0100 Subject: [PATCH 01/20] Implemented wps rule unused-variable- --- .../resources/test/fixtures/ruff/RUF052.py | 72 ++++++++++++++++++ .../src/checkers/ast/analyze/bindings.rs | 6 ++ crates/ruff_linter/src/codes.rs | 3 +- crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 3 + .../ruff/rules/unused_variable_accessed.rs | 73 +++++++++++++++++++ ..._rules__ruff__tests__RUF052_RUF052.py.snap | 55 ++++++++++++++ ruff.schema.json | 2 + 8 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py new file mode 100644 index 0000000000000..d2fcfc019300c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -0,0 +1,72 @@ +# Correct + +for _ in range(5): + pass + +_valid_type = int + +_valid_var_1: _valid_type + +_valid_var_1 = 1 + +_valid_var_2 = 2 + +_valid_var_3 = _valid_var_1 + _valid_var_2 + +def _valid_fun(): + pass + +_valid_fun() + +def fun(_valid_arg): + return _valid_arg + +def fun(_valid_arg): + _valid_unused_var = _valid_arg + pass + +class _ValidClass: + pass + +_ValidClass() + +class ClassOk: + _valid_private_cls_attr = 1 + + print(_valid_private_cls_attr) + + def __init__(self): + self._valid_private_ins_attr = 2 + print(self._valid_private_ins_attr) + + def _valid_method(self): + return self._valid_private_ins_attr + + def method(_valid_arg=3): + _valid_unused_var = _valid_arg + return + +# Incorrect + +def fun(_valid_arg): + _invalid_var = _valid_arg # [RUF052] + return _invalid_var + +class ClassOk: + + def __init__(self): + _ = 1 + __ = 2 # [RUF052] + ___ = 3 + _invalid_type = int # [RUF052] + isinstance(_, _invalid_type) + ___ = __ + _ + self._private_ins_attr_ok = 2 + + def met(_valid_arg=3): + _invalid_var_1 = _valid_arg # [RUF052] + + _invalid_var_2 = 1 # [RUF052] + if _valid_arg: + return _invalid_var_1 + return _invalid_var_2 \ No newline at end of file diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 586fa7ada081f..3cb6cad3409bc 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -15,6 +15,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::UnconventionalImportAlias, Rule::UnsortedDunderSlots, Rule::UnusedVariable, + Rule::UnusedVariableAccessed, ]) { return; } @@ -77,5 +78,10 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } + if checker.enabled(Rule::UnusedVariableAccessed) { + if let Some(diagnostic) = ruff::rules::unused_variable_accessed(checker, binding) { + checker.diagnostics.push(diagnostic); + } + } } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5dc33a1c36216..8265f28196fc3 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -976,9 +976,10 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "035") => (RuleGroup::Preview, rules::ruff::rules::UnsafeMarkupUse), (Ruff, "036") => (RuleGroup::Preview, rules::ruff::rules::NoneNotAtEndOfUnion), (Ruff, "038") => (RuleGroup::Preview, rules::ruff::rules::RedundantBoolLiteral), - (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), (Ruff, "039") => (RuleGroup::Preview, rules::ruff::rules::UnrawRePattern), (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument), + (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), + (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UnusedVariableAccessed), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index aad87d1d336c6..66ede94980cca 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -67,6 +67,7 @@ mod tests { #[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.py"))] #[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.pyi"))] #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] + #[test_case(Rule::UnusedVariableAccessed, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 95f79145d55b8..27ee20c327726 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -90,3 +90,6 @@ pub(crate) enum Context { Docstring, Comment, } +pub(crate) use unused_variable_accessed::*; + +mod unused_variable_accessed; diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs new file mode 100644 index 0000000000000..075d32b1284cf --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -0,0 +1,73 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_semantic::Binding; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for use of unused marked variables (leading with underscore, except '_') +/// Forbid using method and function variables that are marked as unused. +/// +/// ## Why is this bad? +/// Marking variables with a leading underscore conveys that they are 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" but subsequently used suggests oversight or unintentional use +/// and detracts from the clarity and maintainability of the codebase. +/// +/// ## Example +/// ```python +/// def function(): +/// _variable = 3 +/// return _variable + 1 +/// ``` +/// +/// Use instead: +/// ```python +/// def function(): +/// variable = 3 +/// return variable + 1 +/// ``` +#[violation] +pub struct UnusedVariableAccessed { + name: String, +} + +impl Violation for UnusedVariableAccessed { + #[derive_message_formats] + fn message(&self) -> String { + let name = &self.name; + format!("Local variable `{name}` is marked as unused but is used") + } + + fn fix_title(&self) -> Option { + let name = &self.name; + Some(format!("Remove leading underscores from variable `{name}`")) + } +} + +/// RUF052 +pub(crate) fn unused_variable_accessed(checker: &Checker, binding: &Binding) -> Option { + // only used variables + if binding.is_unused() || !binding.kind.is_assignment() { + return None; + } + + // leading with underscore and not '_' + let name = binding.name(checker.locator().contents()); + if !binding.is_private_declaration() || name == "_" { + return None; + } + + // current scope is method or function + if !checker.semantic().scopes[binding.scope].kind.is_function() { + return None; + } + + Some(Diagnostic::new( + UnusedVariableAccessed { + name: name.to_string(), + }, + binding.range(), + )) +} 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 new file mode 100644 index 0000000000000..856ab3321d794 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap @@ -0,0 +1,55 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF052.py:52:5: RUF052 Local variable `_invalid_var` is marked as unused but is used + | +51 | def fun(_valid_arg): +52 | _invalid_var = _valid_arg # [RUF052] + | ^^^^^^^^^^^^ RUF052 +53 | return _invalid_var + | + = help: Remove leading underscores from variable `_invalid_var` + +RUF052.py:59:9: RUF052 Local variable `__` is marked as unused but is used + | +57 | def __init__(self): +58 | _ = 1 +59 | __ = 2 # [RUF052] + | ^^ RUF052 +60 | ___ = 3 +61 | _invalid_type = int # [RUF052] + | + = help: Remove leading underscores from variable `__` + +RUF052.py:61:9: RUF052 Local variable `_invalid_type` is marked as unused but is used + | +59 | __ = 2 # [RUF052] +60 | ___ = 3 +61 | _invalid_type = int # [RUF052] + | ^^^^^^^^^^^^^ RUF052 +62 | isinstance(_, _invalid_type) +63 | ___ = __ + _ + | + = help: Remove leading underscores from variable `_invalid_type` + +RUF052.py:67:9: RUF052 Local variable `_invalid_var_1` is marked as unused but is used + | +66 | def met(_valid_arg=3): +67 | _invalid_var_1 = _valid_arg # [RUF052] + | ^^^^^^^^^^^^^^ RUF052 +68 | +69 | _invalid_var_2 = 1 # [RUF052] + | + = help: Remove leading underscores from variable `_invalid_var_1` + +RUF052.py:69:9: RUF052 Local variable `_invalid_var_2` is marked as unused but is used + | +67 | _invalid_var_1 = _valid_arg # [RUF052] +68 | +69 | _invalid_var_2 = 1 # [RUF052] + | ^^^^^^^^^^^^^^ RUF052 +70 | if _valid_arg: +71 | return _invalid_var_1 + | + = help: Remove leading underscores from variable `_invalid_var_2` diff --git a/ruff.schema.json b/ruff.schema.json index c70b5df565454..3742efc998f2c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3835,6 +3835,8 @@ "RUF04", "RUF040", "RUF048", + "RUF05", + "RUF052", "RUF1", "RUF10", "RUF100", From 9982fafe9cdd0c1bf8750c28a5769cfa9a71b2a3 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 10:45:53 +0100 Subject: [PATCH 02/20] Query reworked; changed fix_title --- .../resources/test/fixtures/ruff/RUF052.py | 79 ++++++++++------ .../src/checkers/ast/analyze/bindings.rs | 4 +- crates/ruff_linter/src/rules/ruff/mod.rs | 14 ++- .../ruff/rules/unused_variable_accessed.rs | 93 ++++++++++++++----- ...es__ruff__tests__dummy_variable_unset.snap | 66 +++++++++++++ 5 files changed, 203 insertions(+), 53 deletions(-) create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py index d2fcfc019300c..2b1799f33cd8c 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -18,11 +18,8 @@ def _valid_fun(): _valid_fun() -def fun(_valid_arg): - return _valid_arg - -def fun(_valid_arg): - _valid_unused_var = _valid_arg +def fun(arg): + _valid_unused_var = arg pass class _ValidClass: @@ -42,31 +39,55 @@ def __init__(self): def _valid_method(self): return self._valid_private_ins_attr - def method(_valid_arg=3): - _valid_unused_var = _valid_arg + def method(arg): + _valid_unused_var = arg return + +# Correct if dummy_variable_re = "_+" + +def fun(x): + _ = 1 + __ = 2 + ___ = 3 + if x == 1: + return _ + if x == 2: + return __ + if x == 3: + return ___ + return x # Incorrect -def fun(_valid_arg): - _invalid_var = _valid_arg # [RUF052] - return _invalid_var - -class ClassOk: - - def __init__(self): - _ = 1 - __ = 2 # [RUF052] - ___ = 3 - _invalid_type = int # [RUF052] - isinstance(_, _invalid_type) - ___ = __ + _ - self._private_ins_attr_ok = 2 - - def met(_valid_arg=3): - _invalid_var_1 = _valid_arg # [RUF052] - - _invalid_var_2 = 1 # [RUF052] - if _valid_arg: - return _invalid_var_1 - return _invalid_var_2 \ No newline at end of file +class Class_: + def fun(self): + _var = "method variable" + return _var # [RUF052] + +def fun(_var): + return _var # [RUF052] + +def fun(): + _list = "built-in" + return _list # [RUF052] + +x = "global" + +def fun(): + global x + _x = "shadows global" + return _x # [RUF052] + +def foo(): + x = "outer" + def bar(): + nonlocal x + _x = "shadows nonlocal" + return _x # [RUF052] + bar() + return x + +def fun(): + x = "local" + _x = "shadows local" + return _x # [RUF052] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 3cb6cad3409bc..992cd8abc5fd1 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -79,8 +79,8 @@ pub(crate) fn bindings(checker: &mut Checker) { } } if checker.enabled(Rule::UnusedVariableAccessed) { - if let Some(diagnostic) = ruff::rules::unused_variable_accessed(checker, binding) { - checker.diagnostics.push(diagnostic); + if let Some(diagnostics) = ruff::rules::unused_variable_accessed(checker, binding) { + checker.diagnostics.extend(diagnostics); } } } diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 66ede94980cca..5993d527b6da9 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -10,6 +10,7 @@ mod tests { use std::path::Path; use anyhow::Result; + use regex::Regex; use rustc_hash::FxHashSet; use test_case::test_case; @@ -67,7 +68,6 @@ mod tests { #[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.py"))] #[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.pyi"))] #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] - #[test_case(Rule::UnusedVariableAccessed, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( @@ -446,4 +446,16 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + #[test] + fn dummy_variable_unset() -> Result<()> { + let diagnostics = test_path( + Path::new("ruff/RUF052.py"), + &LinterSettings { + dummy_variable_rgx: Regex::new(r"^_+$").unwrap(), + ..LinterSettings::for_rule(Rule::UnusedVariableAccessed) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index 075d32b1284cf..b72e27a4e769d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -1,12 +1,13 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::Binding; +use ruff_python_stdlib::builtins::is_python_builtin; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for use of unused marked variables (leading with underscore, except '_') +/// Checks for usages of variables marked as unused (variable names starting with an underscore, except '_') in functions. /// Forbid using method and function variables that are marked as unused. /// /// ## Why is this bad? @@ -31,43 +32,93 @@ use crate::checkers::ast::Checker; #[violation] pub struct UnusedVariableAccessed { name: String, + fix: String, + shadowed_kind: ShadowedKind, } impl Violation for UnusedVariableAccessed { #[derive_message_formats] fn message(&self) -> String { - let name = &self.name; - format!("Local variable `{name}` is marked as unused but is used") + format!( + "Local variable `{}` marked as unused is accessed", + self.name + ) } fn fix_title(&self) -> Option { - let name = &self.name; - Some(format!("Remove leading underscores from variable `{name}`")) + Some(match self.shadowed_kind { + ShadowedKind::BuiltIn => { + "Consider using prefered trailing underscores to avoid shadowing a built-in." + .to_string() + } + ShadowedKind::Some => { + "Consider using prefered trailing underscores to avoid shadowing a variable." + .to_string() + } + ShadowedKind::None => "Consider removing leading underscores.".to_string(), + }) } } /// RUF052 -pub(crate) fn unused_variable_accessed(checker: &Checker, binding: &Binding) -> Option { +pub(crate) fn unused_variable_accessed( + checker: &Checker, + binding: &Binding, +) -> Option> { + let name = binding.name(checker.source()); + // only used variables - if binding.is_unused() || !binding.kind.is_assignment() { + if !name.starts_with('_') + || name == "_" + || binding.is_unused() + || binding.is_global() + || binding.is_nonlocal() + || (!binding.kind.is_argument() && !binding.kind.is_assignment()) + || !checker.semantic().scopes[binding.scope].kind.is_function() + || checker.settings.dummy_variable_rgx.is_match(name) + { return None; } - // leading with underscore and not '_' - let name = binding.name(checker.locator().contents()); - if !binding.is_private_declaration() || name == "_" { - return None; - } + let trimmed_name = name.trim_start_matches('_'); + let mut kind = ShadowedKind::None; + let mut fix = trimmed_name.to_string(); - // current scope is method or function - if !checker.semantic().scopes[binding.scope].kind.is_function() { - return None; + if !trimmed_name.is_empty() { + if is_python_builtin( + trimmed_name, + checker.settings.target_version.minor(), + checker.source_type.is_ipynb(), + ) { + kind = ShadowedKind::BuiltIn; + fix += "_"; + } else if checker.semantic().scopes[binding.scope].has(trimmed_name) { + kind = ShadowedKind::Some; + fix += "_"; + } } - Some(Diagnostic::new( - UnusedVariableAccessed { - name: name.to_string(), - }, - binding.range(), - )) + Some( + binding + .references + .iter() + .map(|ref_id| { + Diagnostic::new( + UnusedVariableAccessed { + name: name.to_string(), + fix: fix.clone(), + shadowed_kind: kind, + }, + checker.semantic().reference(*ref_id).range(), + ) + }) + .collect(), + ) +} + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum ShadowedKind { + Some, + BuiltIn, + None, } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap new file mode 100644 index 0000000000000..29571de4400d7 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap @@ -0,0 +1,66 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF052.py:65:16: RUF052 Local variable `_var` marked as unused is accessed + | +63 | def fun(self): +64 | _var = "method variable" +65 | return _var + | ^^^^ RUF052 +66 | +67 | def fun(_var): + | + = help: Consider removing leading underscores. + +RUF052.py:68:12: RUF052 Local variable `_var` marked as unused is accessed + | +67 | def fun(_var): +68 | return _var + | ^^^^ RUF052 +69 | +70 | def fun(): + | + = help: Consider removing leading underscores. + +RUF052.py:72:12: RUF052 Local variable `_list` marked as unused is accessed + | +70 | def fun(): +71 | _list = "built-in" +72 | return _list + | ^^^^^ RUF052 +73 | +74 | x = "global" + | + = help: Consider using prefered trailing underscores to avoid shadowing a built-in. + +RUF052.py:79:12: RUF052 Local variable `_x` marked as unused is accessed + | +77 | global x +78 | _x = "shadows global" +79 | return _x + | ^^ RUF052 +80 | +81 | def foo(): + | + = help: Consider using prefered trailing underscores to avoid shadowing a variable. + +RUF052.py:86:12: RUF052 Local variable `_x` marked as unused is accessed + | +84 | nonlocal x +85 | _x = "shadows nonlocal" +86 | return _x + | ^^ RUF052 +87 | bar() +88 | return x + | + = help: Consider using prefered trailing underscores to avoid shadowing a variable. + +RUF052.py:93:12: RUF052 Local variable `_x` marked as unused is accessed + | +91 | x = "local" +92 | _x = "shadows local" +93 | return _x + | ^^ RUF052 + | + = help: Consider using prefered trailing underscores to avoid shadowing a variable. From d4d52583830eded4997d421ac1bbbc66d19ba0c7 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 11:37:07 +0100 Subject: [PATCH 03/20] violation changes --- .../ruff/rules/unused_variable_accessed.rs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index b72e27a4e769d..e08f17145312b 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -1,5 +1,5 @@ use ruff_diagnostics::{Diagnostic, Violation}; -use ruff_macros::{derive_message_formats, violation}; +use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_semantic::Binding; use ruff_python_stdlib::builtins::is_python_builtin; use ruff_text_size::Ranged; @@ -29,10 +29,10 @@ use crate::checkers::ast::Checker; /// variable = 3 /// return variable + 1 /// ``` -#[violation] -pub struct UnusedVariableAccessed { +#[derive(ViolationMetadata)] +pub(crate) struct UnusedVariableAccessed { name: String, - fix: String, + // fix: String, shadowed_kind: ShadowedKind, } @@ -48,11 +48,11 @@ impl Violation for UnusedVariableAccessed { fn fix_title(&self) -> Option { Some(match self.shadowed_kind { ShadowedKind::BuiltIn => { - "Consider using prefered trailing underscores to avoid shadowing a built-in." + "Consider using preferred trailing underscores to avoid shadowing a built-in." .to_string() } ShadowedKind::Some => { - "Consider using prefered trailing underscores to avoid shadowing a variable." + "Consider using preferred trailing underscores to avoid shadowing a variable." .to_string() } ShadowedKind::None => "Consider removing leading underscores.".to_string(), @@ -82,7 +82,7 @@ pub(crate) fn unused_variable_accessed( let trimmed_name = name.trim_start_matches('_'); let mut kind = ShadowedKind::None; - let mut fix = trimmed_name.to_string(); + // let mut fix = trimmed_name.to_string(); if !trimmed_name.is_empty() { if is_python_builtin( @@ -91,10 +91,10 @@ pub(crate) fn unused_variable_accessed( checker.source_type.is_ipynb(), ) { kind = ShadowedKind::BuiltIn; - fix += "_"; + // fix += "_"; } else if checker.semantic().scopes[binding.scope].has(trimmed_name) { kind = ShadowedKind::Some; - fix += "_"; + // fix += "_"; } } @@ -106,7 +106,7 @@ pub(crate) fn unused_variable_accessed( Diagnostic::new( UnusedVariableAccessed { name: name.to_string(), - fix: fix.clone(), + // fix: fix.clone(), shadowed_kind: kind, }, checker.semantic().reference(*ref_id).range(), From dee88b348f4a9750ed288c10942fe5c1ad8728de Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 11:52:30 +0100 Subject: [PATCH 04/20] Updated snap --- ...es__ruff__tests__dummy_variable_unset.snap | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap index 29571de4400d7..91912cdd6f676 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap @@ -6,7 +6,7 @@ RUF052.py:65:16: RUF052 Local variable `_var` marked as unused is accessed | 63 | def fun(self): 64 | _var = "method variable" -65 | return _var +65 | return _var # [RUF052] | ^^^^ RUF052 66 | 67 | def fun(_var): @@ -16,7 +16,7 @@ RUF052.py:65:16: RUF052 Local variable `_var` marked as unused is accessed RUF052.py:68:12: RUF052 Local variable `_var` marked as unused is accessed | 67 | def fun(_var): -68 | return _var +68 | return _var # [RUF052] | ^^^^ RUF052 69 | 70 | def fun(): @@ -27,40 +27,40 @@ RUF052.py:72:12: RUF052 Local variable `_list` marked as unused is accessed | 70 | def fun(): 71 | _list = "built-in" -72 | return _list +72 | return _list # [RUF052] | ^^^^^ RUF052 73 | 74 | x = "global" | - = help: Consider using prefered trailing underscores to avoid shadowing a built-in. + = help: Consider using preferred trailing underscores to avoid shadowing a built-in. RUF052.py:79:12: RUF052 Local variable `_x` marked as unused is accessed | 77 | global x 78 | _x = "shadows global" -79 | return _x +79 | return _x # [RUF052] | ^^ RUF052 80 | 81 | def foo(): | - = help: Consider using prefered trailing underscores to avoid shadowing a variable. + = help: Consider using preferred trailing underscores to avoid shadowing a variable. RUF052.py:86:12: RUF052 Local variable `_x` marked as unused is accessed | 84 | nonlocal x 85 | _x = "shadows nonlocal" -86 | return _x +86 | return _x # [RUF052] | ^^ RUF052 87 | bar() 88 | return x | - = help: Consider using prefered trailing underscores to avoid shadowing a variable. + = help: Consider using preferred trailing underscores to avoid shadowing a variable. RUF052.py:93:12: RUF052 Local variable `_x` marked as unused is accessed | 91 | x = "local" 92 | _x = "shadows local" -93 | return _x +93 | return _x # [RUF052] | ^^ RUF052 | - = help: Consider using prefered trailing underscores to avoid shadowing a variable. + = help: Consider using preferred trailing underscores to avoid shadowing a variable. From 08f7dd33d696ecdb606510f10252036d5d7e03e9 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 12:13:37 +0100 Subject: [PATCH 05/20] removed old snap --- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 55 ------------------- 1 file changed, 55 deletions(-) delete mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap 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 deleted file mode 100644 index 856ab3321d794..0000000000000 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap +++ /dev/null @@ -1,55 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text ---- -RUF052.py:52:5: RUF052 Local variable `_invalid_var` is marked as unused but is used - | -51 | def fun(_valid_arg): -52 | _invalid_var = _valid_arg # [RUF052] - | ^^^^^^^^^^^^ RUF052 -53 | return _invalid_var - | - = help: Remove leading underscores from variable `_invalid_var` - -RUF052.py:59:9: RUF052 Local variable `__` is marked as unused but is used - | -57 | def __init__(self): -58 | _ = 1 -59 | __ = 2 # [RUF052] - | ^^ RUF052 -60 | ___ = 3 -61 | _invalid_type = int # [RUF052] - | - = help: Remove leading underscores from variable `__` - -RUF052.py:61:9: RUF052 Local variable `_invalid_type` is marked as unused but is used - | -59 | __ = 2 # [RUF052] -60 | ___ = 3 -61 | _invalid_type = int # [RUF052] - | ^^^^^^^^^^^^^ RUF052 -62 | isinstance(_, _invalid_type) -63 | ___ = __ + _ - | - = help: Remove leading underscores from variable `_invalid_type` - -RUF052.py:67:9: RUF052 Local variable `_invalid_var_1` is marked as unused but is used - | -66 | def met(_valid_arg=3): -67 | _invalid_var_1 = _valid_arg # [RUF052] - | ^^^^^^^^^^^^^^ RUF052 -68 | -69 | _invalid_var_2 = 1 # [RUF052] - | - = help: Remove leading underscores from variable `_invalid_var_1` - -RUF052.py:69:9: RUF052 Local variable `_invalid_var_2` is marked as unused but is used - | -67 | _invalid_var_1 = _valid_arg # [RUF052] -68 | -69 | _invalid_var_2 = 1 # [RUF052] - | ^^^^^^^^^^^^^^ RUF052 -70 | if _valid_arg: -71 | return _invalid_var_1 - | - = help: Remove leading underscores from variable `_invalid_var_2` From af20d137b0650aedb3af3f86be51f7b10cb35da5 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 12:50:42 +0100 Subject: [PATCH 06/20] Restructured filters, filtering out non-dummy variables --- .../resources/test/fixtures/ruff/RUF052.py | 11 ++++ crates/ruff_linter/src/rules/ruff/mod.rs | 14 +--- .../ruff/rules/unused_variable_accessed.rs | 47 +++++++------ ...es__ruff__tests__dummy_variable_unset.snap | 66 ------------------- 4 files changed, 38 insertions(+), 100 deletions(-) delete mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py index 2b1799f33cd8c..35ed0c8270494 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -22,6 +22,17 @@ def fun(arg): _valid_unused_var = arg pass +_x = "global" + +def fun(): + global _x + return _x + +def fun(): + global _x + _x = "reassigned global" + return _x + class _ValidClass: pass diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index df468a5d33390..62dc871b06cb6 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -10,7 +10,6 @@ mod tests { use std::path::Path; use anyhow::Result; - use regex::Regex; use rustc_hash::FxHashSet; use test_case::test_case; @@ -71,6 +70,7 @@ mod tests { #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))] + #[test_case(Rule::UnusedVariableAccessed, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( @@ -449,16 +449,4 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } - #[test] - fn dummy_variable_unset() -> Result<()> { - let diagnostics = test_path( - Path::new("ruff/RUF052.py"), - &LinterSettings { - dummy_variable_rgx: Regex::new(r"^_+$").unwrap(), - ..LinterSettings::for_rule(Rule::UnusedVariableAccessed) - }, - )?; - assert_messages!(diagnostics); - Ok(()) - } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index e08f17145312b..ccd266bfdecc0 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; -use ruff_python_semantic::Binding; +use ruff_python_ast::helpers::is_dunder; +use ruff_python_semantic::{Binding, BindingKind}; use ruff_python_stdlib::builtins::is_python_builtin; use ruff_text_size::Ranged; @@ -8,7 +9,6 @@ use crate::checkers::ast::Checker; /// ## What it does /// Checks for usages of variables marked as unused (variable names starting with an underscore, except '_') in functions. -/// Forbid using method and function variables that are marked as unused. /// /// ## Why is this bad? /// Marking variables with a leading underscore conveys that they are intentionally unused within the function or method. @@ -32,7 +32,6 @@ use crate::checkers::ast::Checker; #[derive(ViolationMetadata)] pub(crate) struct UnusedVariableAccessed { name: String, - // fix: String, shadowed_kind: ShadowedKind, } @@ -40,7 +39,7 @@ impl Violation for UnusedVariableAccessed { #[derive_message_formats] fn message(&self) -> String { format!( - "Local variable `{}` marked as unused is accessed", + "Local variable `{}` with leading underscore is accessed", self.name ) } @@ -48,14 +47,12 @@ impl Violation for UnusedVariableAccessed { fn fix_title(&self) -> Option { Some(match self.shadowed_kind { ShadowedKind::BuiltIn => { - "Consider using preferred trailing underscores to avoid shadowing a built-in." - .to_string() + "Prefer using trailing underscores to avoid shadowing a built-in".to_string() } ShadowedKind::Some => { - "Consider using preferred trailing underscores to avoid shadowing a variable." - .to_string() + "Prefer using trailing underscores to avoid shadowing a variable".to_string() } - ShadowedKind::None => "Consider removing leading underscores.".to_string(), + ShadowedKind::None => "Remove leading underscores".to_string(), }) } } @@ -67,16 +64,27 @@ pub(crate) fn unused_variable_accessed( ) -> Option> { let name = binding.name(checker.source()); + // only variables marked as private + if !name.starts_with('_') || name == "_" || is_dunder(name) { + return None; + } // only used variables - if !name.starts_with('_') - || name == "_" - || binding.is_unused() - || binding.is_global() - || binding.is_nonlocal() - || (!binding.kind.is_argument() && !binding.kind.is_assignment()) - || !checker.semantic().scopes[binding.scope].kind.is_function() - || checker.settings.dummy_variable_rgx.is_match(name) - { + if binding.is_unused() { + return None; + } + // Only variables defined via function arguments or assignments. + // This excludes `global` and `nonlocal` variables. + if !matches!( + binding.kind, + BindingKind::Argument | BindingKind::Assignment + ) { + return None; + } + // Only variables defined in function scopes + if !checker.semantic().scopes[binding.scope].kind.is_function() { + return None; + } + if checker.settings.dummy_variable_rgx.is_match(name) { return None; } @@ -91,10 +99,8 @@ pub(crate) fn unused_variable_accessed( checker.source_type.is_ipynb(), ) { kind = ShadowedKind::BuiltIn; - // fix += "_"; } else if checker.semantic().scopes[binding.scope].has(trimmed_name) { kind = ShadowedKind::Some; - // fix += "_"; } } @@ -106,7 +112,6 @@ pub(crate) fn unused_variable_accessed( Diagnostic::new( UnusedVariableAccessed { name: name.to_string(), - // fix: fix.clone(), shadowed_kind: kind, }, checker.semantic().reference(*ref_id).range(), diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap deleted file mode 100644 index 91912cdd6f676..0000000000000 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__dummy_variable_unset.snap +++ /dev/null @@ -1,66 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/ruff/mod.rs -snapshot_kind: text ---- -RUF052.py:65:16: RUF052 Local variable `_var` marked as unused is accessed - | -63 | def fun(self): -64 | _var = "method variable" -65 | return _var # [RUF052] - | ^^^^ RUF052 -66 | -67 | def fun(_var): - | - = help: Consider removing leading underscores. - -RUF052.py:68:12: RUF052 Local variable `_var` marked as unused is accessed - | -67 | def fun(_var): -68 | return _var # [RUF052] - | ^^^^ RUF052 -69 | -70 | def fun(): - | - = help: Consider removing leading underscores. - -RUF052.py:72:12: RUF052 Local variable `_list` marked as unused is accessed - | -70 | def fun(): -71 | _list = "built-in" -72 | return _list # [RUF052] - | ^^^^^ RUF052 -73 | -74 | x = "global" - | - = help: Consider using preferred trailing underscores to avoid shadowing a built-in. - -RUF052.py:79:12: RUF052 Local variable `_x` marked as unused is accessed - | -77 | global x -78 | _x = "shadows global" -79 | return _x # [RUF052] - | ^^ RUF052 -80 | -81 | def foo(): - | - = help: Consider using preferred trailing underscores to avoid shadowing a variable. - -RUF052.py:86:12: RUF052 Local variable `_x` marked as unused is accessed - | -84 | nonlocal x -85 | _x = "shadows nonlocal" -86 | return _x # [RUF052] - | ^^ RUF052 -87 | bar() -88 | return x - | - = help: Consider using preferred trailing underscores to avoid shadowing a variable. - -RUF052.py:93:12: RUF052 Local variable `_x` marked as unused is accessed - | -91 | x = "local" -92 | _x = "shadows local" -93 | return _x # [RUF052] - | ^^ RUF052 - | - = help: Consider using preferred trailing underscores to avoid shadowing a variable. From 87e8f821e3f37a7001af2deaa85e4a8f9ee297a5 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 13:03:18 +0100 Subject: [PATCH 07/20] Edited error msgs and filtering --- .../ruff/rules/unused_variable_accessed.rs | 9 ++- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 66 +++++++++++++++++++ 2 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index ccd266bfdecc0..d864d1694ba29 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; -use ruff_python_semantic::{Binding, BindingKind}; +use ruff_python_semantic::{Binding, BindingFlags, BindingKind}; use ruff_python_stdlib::builtins::is_python_builtin; use ruff_text_size::Ranged; @@ -73,18 +73,21 @@ pub(crate) fn unused_variable_accessed( return None; } // Only variables defined via function arguments or assignments. - // This excludes `global` and `nonlocal` variables. if !matches!( binding.kind, BindingKind::Argument | BindingKind::Assignment ) { return None; } + // This excludes `global` and `nonlocal` variables. + if binding.is_global() || binding.is_nonlocal() { + return None + } // Only variables defined in function scopes if !checker.semantic().scopes[binding.scope].kind.is_function() { return None; } - if checker.settings.dummy_variable_rgx.is_match(name) { + if !checker.settings.dummy_variable_rgx.is_match(name) { return None; } 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 new file mode 100644 index 0000000000000..abce30529f444 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap @@ -0,0 +1,66 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF052.py:76:16: RUF052 Local variable `_var` with leading underscore is accessed + | +74 | def fun(self): +75 | _var = "method variable" +76 | return _var # [RUF052] + | ^^^^ RUF052 +77 | +78 | def fun(_var): + | + = help: Remove leading underscores + +RUF052.py:79:12: RUF052 Local variable `_var` with leading underscore is accessed + | +78 | def fun(_var): +79 | return _var # [RUF052] + | ^^^^ RUF052 +80 | +81 | def fun(): + | + = help: Remove leading underscores + +RUF052.py:83:12: RUF052 Local variable `_list` with leading underscore is accessed + | +81 | def fun(): +82 | _list = "built-in" +83 | return _list # [RUF052] + | ^^^^^ RUF052 +84 | +85 | x = "global" + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +RUF052.py:90:12: RUF052 Local variable `_x` with leading underscore is accessed + | +88 | global x +89 | _x = "shadows global" +90 | return _x # [RUF052] + | ^^ RUF052 +91 | +92 | def foo(): + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:97:12: RUF052 Local variable `_x` with leading underscore is accessed + | +95 | nonlocal x +96 | _x = "shadows nonlocal" +97 | return _x # [RUF052] + | ^^ RUF052 +98 | bar() +99 | return x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:104:12: RUF052 Local variable `_x` with leading underscore is accessed + | +102 | x = "local" +103 | _x = "shadows local" +104 | return _x # [RUF052] + | ^^ RUF052 + | + = help: Prefer using trailing underscores to avoid shadowing a variable From c4a33f2589f0d9ca2011395f867f82afd8cb542d Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 13:15:49 +0100 Subject: [PATCH 08/20] Fmt --- .../src/rules/ruff/rules/unused_variable_accessed.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index d864d1694ba29..52df6d876f02d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -81,7 +81,7 @@ pub(crate) fn unused_variable_accessed( } // This excludes `global` and `nonlocal` variables. if binding.is_global() || binding.is_nonlocal() { - return None + return None; } // Only variables defined in function scopes if !checker.semantic().scopes[binding.scope].kind.is_function() { From 3a12196d4c8af4dcf4f37739c4060e1446140f0c Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 13:17:06 +0100 Subject: [PATCH 09/20] Removed unused code --- .../ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index 52df6d876f02d..a73c151ff48bc 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -93,7 +93,6 @@ pub(crate) fn unused_variable_accessed( let trimmed_name = name.trim_start_matches('_'); let mut kind = ShadowedKind::None; - // let mut fix = trimmed_name.to_string(); if !trimmed_name.is_empty() { if is_python_builtin( From 9faf7cd3b78f2cbba1a02d9bb8ff2122393532b0 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Thu, 28 Nov 2024 13:38:57 +0100 Subject: [PATCH 10/20] Catching all dummy vars --- .../resources/test/fixtures/ruff/RUF052.py | 6 +- .../ruff/rules/unused_variable_accessed.rs | 13 +++- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 74 +++++++++---------- 3 files changed, 52 insertions(+), 41 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py index 35ed0c8270494..ecbdb4826ace8 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -26,7 +26,11 @@ def fun(arg): def fun(): global _x - return _x + return _x + +def fun(): + __dunder__ = "dunder variable" + return __dunder__ def fun(): global _x diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs index a73c151ff48bc..9b874fc1422ba 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; -use ruff_python_semantic::{Binding, BindingFlags, BindingKind}; +use ruff_python_semantic::{Binding, BindingKind}; use ruff_python_stdlib::builtins::is_python_builtin; use ruff_text_size::Ranged; @@ -29,6 +29,9 @@ use crate::checkers::ast::Checker; /// variable = 3 /// return variable + 1 /// ``` +/// +/// ## Options +/// - [`lint.dummy-variable-rgx`] #[derive(ViolationMetadata)] pub(crate) struct UnusedVariableAccessed { name: String, @@ -64,8 +67,8 @@ pub(crate) fn unused_variable_accessed( ) -> Option> { let name = binding.name(checker.source()); - // only variables marked as private - if !name.starts_with('_') || name == "_" || is_dunder(name) { + // Ignore `_` and dunder variables + if name == "_" || is_dunder(name) { return None; } // only used variables @@ -123,9 +126,13 @@ pub(crate) fn unused_variable_accessed( ) } +/// 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 does not shadow any other symbols None, } 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 abce30529f444..5b3a83588c902 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 @@ -2,65 +2,65 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs snapshot_kind: text --- -RUF052.py:76:16: RUF052 Local variable `_var` with leading underscore is accessed +RUF052.py:80:16: RUF052 Local variable `_var` with leading underscore is accessed | -74 | def fun(self): -75 | _var = "method variable" -76 | return _var # [RUF052] +78 | def fun(self): +79 | _var = "method variable" +80 | return _var # [RUF052] | ^^^^ RUF052 -77 | -78 | def fun(_var): +81 | +82 | def fun(_var): | = help: Remove leading underscores -RUF052.py:79:12: RUF052 Local variable `_var` with leading underscore is accessed +RUF052.py:83:12: RUF052 Local variable `_var` with leading underscore is accessed | -78 | def fun(_var): -79 | return _var # [RUF052] +82 | def fun(_var): +83 | return _var # [RUF052] | ^^^^ RUF052 -80 | -81 | def fun(): +84 | +85 | def fun(): | = help: Remove leading underscores -RUF052.py:83:12: RUF052 Local variable `_list` with leading underscore is accessed +RUF052.py:87:12: RUF052 Local variable `_list` with leading underscore is accessed | -81 | def fun(): -82 | _list = "built-in" -83 | return _list # [RUF052] +85 | def fun(): +86 | _list = "built-in" +87 | return _list # [RUF052] | ^^^^^ RUF052 -84 | -85 | x = "global" +88 | +89 | x = "global" | = help: Prefer using trailing underscores to avoid shadowing a built-in -RUF052.py:90:12: RUF052 Local variable `_x` with leading underscore is accessed +RUF052.py:94:12: RUF052 Local variable `_x` with leading underscore is accessed | -88 | global x -89 | _x = "shadows global" -90 | return _x # [RUF052] +92 | global x +93 | _x = "shadows global" +94 | return _x # [RUF052] | ^^ RUF052 -91 | -92 | def foo(): +95 | +96 | def foo(): | = help: Prefer using trailing underscores to avoid shadowing a variable -RUF052.py:97:12: RUF052 Local variable `_x` with leading underscore is accessed - | -95 | nonlocal x -96 | _x = "shadows nonlocal" -97 | return _x # [RUF052] - | ^^ RUF052 -98 | bar() -99 | return x - | - = help: Prefer using trailing underscores to avoid shadowing a variable +RUF052.py:101:12: RUF052 Local variable `_x` with leading underscore is accessed + | + 99 | nonlocal x +100 | _x = "shadows nonlocal" +101 | return _x # [RUF052] + | ^^ RUF052 +102 | bar() +103 | return x + | + = help: Prefer using trailing underscores to avoid shadowing a variable -RUF052.py:104:12: RUF052 Local variable `_x` with leading underscore is accessed +RUF052.py:108:12: RUF052 Local variable `_x` with leading underscore is accessed | -102 | x = "local" -103 | _x = "shadows local" -104 | return _x # [RUF052] +106 | x = "local" +107 | _x = "shadows local" +108 | return _x # [RUF052] | ^^ RUF052 | = help: Prefer using trailing underscores to avoid shadowing a variable From 65e45a14849664fe7352e13130eed23a710a77bd Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Fri, 29 Nov 2024 21:33:49 +0100 Subject: [PATCH 11/20] Renamed rule to dummy-variable-accessed --- .../ruff_linter/src/checkers/ast/analyze/bindings.rs | 6 +++--- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 2 +- ...variable_accessed.rs => dummy_variable_accessed.rs} | 10 +++++----- crates/ruff_linter/src/rules/ruff/rules/mod.rs | 5 ++--- 5 files changed, 12 insertions(+), 13 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{unused_variable_accessed.rs => dummy_variable_accessed.rs} (92%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index bac6cbbc060fa..bfc690056b4b6 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -18,7 +18,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::UnsortedDunderSlots, Rule::UnusedVariable, Rule::UnquotedTypeAlias, - Rule::UnusedVariableAccessed, + Rule::DummyVariableAccessed, ]) { return; } @@ -88,8 +88,8 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } - if checker.enabled(Rule::UnusedVariableAccessed) { - if let Some(diagnostics) = ruff::rules::unused_variable_accessed(checker, binding) { + if checker.enabled(Rule::DummyVariableAccessed) { + if let Some(diagnostics) = ruff::rules::dummy_variable_accessed(checker, binding) { checker.diagnostics.extend(diagnostics); } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index cb66fff8cbfe8..3a13e58eafd91 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -984,7 +984,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument), (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral), (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), - (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UnusedVariableAccessed), + (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::DummyVariableAccessed), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 62dc871b06cb6..42bd9d0aa22de 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -70,7 +70,7 @@ mod tests { #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))] - #[test_case(Rule::UnusedVariableAccessed, Path::new("RUF052.py"))] + #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs similarity index 92% rename from crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs rename to crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 9b874fc1422ba..83f185c3ea80a 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -8,7 +8,7 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for usages of variables marked as unused (variable names starting with an underscore, except '_') in functions. +/// Checks for usages of variables marked as unused in functions (dummy variable names starting with an underscore, except '_'). /// /// ## Why is this bad? /// Marking variables with a leading underscore conveys that they are intentionally unused within the function or method. @@ -33,12 +33,12 @@ use crate::checkers::ast::Checker; /// ## Options /// - [`lint.dummy-variable-rgx`] #[derive(ViolationMetadata)] -pub(crate) struct UnusedVariableAccessed { +pub(crate) struct DummyVariableAccessed { name: String, shadowed_kind: ShadowedKind, } -impl Violation for UnusedVariableAccessed { +impl Violation for DummyVariableAccessed { #[derive_message_formats] fn message(&self) -> String { format!( @@ -61,7 +61,7 @@ impl Violation for UnusedVariableAccessed { } /// RUF052 -pub(crate) fn unused_variable_accessed( +pub(crate) fn dummy_variable_accessed( checker: &Checker, binding: &Binding, ) -> Option> { @@ -115,7 +115,7 @@ pub(crate) fn unused_variable_accessed( .iter() .map(|ref_id| { Diagnostic::new( - UnusedVariableAccessed { + DummyVariableAccessed { name: name.to_string(), shadowed_kind: kind, }, diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index e0854472b199b..f1189e1109dde 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -5,6 +5,7 @@ pub(crate) use asyncio_dangling_task::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use decimal_from_float_literal::*; pub(crate) use default_factory_kwarg::*; +pub(crate) use dummy_variable_accessed::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; pub(crate) use implicit_optional::*; @@ -48,6 +49,7 @@ mod collection_literal_concatenation; mod confusables; mod decimal_from_float_literal; mod default_factory_kwarg; +mod dummy_variable_accessed; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; mod helpers; @@ -92,6 +94,3 @@ pub(crate) enum Context { Docstring, Comment, } -pub(crate) use unused_variable_accessed::*; - -mod unused_variable_accessed; From 8ad9bb0648f242284e3c9aa6032c7f2f734d9fdf Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Sat, 30 Nov 2024 16:17:01 +0100 Subject: [PATCH 12/20] Fix is now available --- .../ruff/rules/dummy_variable_accessed.rs | 202 ++++++++++++++---- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 81 ++++++- 2 files changed, 230 insertions(+), 53 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 83f185c3ea80a..1b5d8f64fff8b 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -1,14 +1,15 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; -use ruff_python_semantic::{Binding, BindingKind}; -use ruff_python_stdlib::builtins::is_python_builtin; +use ruff_python_semantic::{Binding, BindingKind, Scope}; +use ruff_python_stdlib::{builtins::is_python_builtin, identifiers::is_identifier}; use ruff_text_size::Ranged; -use crate::checkers::ast::Checker; +use crate::{checkers::ast::Checker, renamer::Renamer}; /// ## What it does -/// Checks for usages of variables marked as unused in functions (dummy variable names starting with an underscore, except '_'). +/// Checks for accesses of local dummy variable (specified by `dummy-variable-rgx` setting), except dunder names and '_'. +/// Provides auto fix for identifiers with a leading underscore if possible. /// /// ## Why is this bad? /// Marking variables with a leading underscore conveys that they are intentionally unused within the function or method. @@ -35,28 +36,33 @@ use crate::checkers::ast::Checker; #[derive(ViolationMetadata)] pub(crate) struct DummyVariableAccessed { name: String, - shadowed_kind: ShadowedKind, + fix_kind: Option, } impl Violation for DummyVariableAccessed { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { - format!( - "Local variable `{}` with leading underscore is accessed", - self.name - ) + format!("Local dummy variable `{}` is accessed", self.name) } fn fix_title(&self) -> Option { - Some(match self.shadowed_kind { - ShadowedKind::BuiltIn => { - "Prefer using trailing underscores to avoid shadowing a built-in".to_string() - } - ShadowedKind::Some => { - "Prefer using trailing underscores to avoid shadowing a variable".to_string() - } - ShadowedKind::None => "Remove leading underscores".to_string(), - }) + if let Some(fix_kind) = self.fix_kind { + return Some(match fix_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 } } @@ -87,43 +93,49 @@ pub(crate) fn dummy_variable_accessed( return None; } // Only variables defined in function scopes - if !checker.semantic().scopes[binding.scope].kind.is_function() { + let scope = &checker.semantic().scopes[binding.scope]; + if !scope.kind.is_function() { return None; } if !checker.settings.dummy_variable_rgx.is_match(name) { return None; } - let trimmed_name = name.trim_start_matches('_'); - let mut kind = ShadowedKind::None; - - if !trimmed_name.is_empty() { - if is_python_builtin( - trimmed_name, - checker.settings.target_version.minor(), - checker.source_type.is_ipynb(), - ) { - kind = ShadowedKind::BuiltIn; - } else if checker.semantic().scopes[binding.scope].has(trimmed_name) { - kind = ShadowedKind::Some; + let possible_fix_kind = get_possible_fix_kind(name, scope, checker); + + let mut diagnostics: Vec = binding + .references + .iter() + .map(|ref_id| { + Diagnostic::new( + DummyVariableAccessed { + name: name.to_string(), + fix_kind: possible_fix_kind, + }, + checker.semantic().reference(*ref_id).range(), + ) + .with_parent(binding.start()) + }) + .collect(); + + // Try at most one auto-fix for first diagnostic instance + if let Some(fix_kind) = possible_fix_kind { + // Get the possible fix based on the scope + if let Some(fix) = get_possible_fix(name, fix_kind, scope) { + // Fix is available + if let Some(diagnostic) = diagnostics.first_mut() { + // Try to set the fix + diagnostic.try_set_fix(|| { + let (edit, rest) = + Renamer::rename(name, &fix, scope, checker.semantic(), checker.stylist())?; + let applicability = Applicability::Safe; + Ok(Fix::applicable_edits(edit, rest, applicability)) + }); + } } } - Some( - binding - .references - .iter() - .map(|ref_id| { - Diagnostic::new( - DummyVariableAccessed { - name: name.to_string(), - shadowed_kind: kind, - }, - checker.semantic().reference(*ref_id).range(), - ) - }) - .collect(), - ) + Some(diagnostics) } /// Enumeration of various ways in which a binding can shadow other variables @@ -133,6 +145,102 @@ enum ShadowedKind { 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, kind: ShadowedKind, scope: &Scope) -> 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 => { + format!("{trimmed_name}_") // Append an underscore + } + ShadowedKind::None => trimmed_name.to_string(), + }; + + // Ensure the fix name is not already taken in the scope + if scope.has(&fix_name) { + return None; + } + + // 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 get_possible_fix_kind(name: &str, scope: &Scope, checker: &Checker) -> 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 scope.has(trimmed_name) { + return Some(ShadowedKind::Some); + } + + // Default to no shadowing + Some(ShadowedKind::None) +} + +fn is_keyword(name: &str) -> bool { + matches!( + name, + "False" + | "None" + | "True" + | "and" + | "as" + | "assert" + | "async" + | "await" + | "break" + | "class" + | "continue" + | "def" + | "del" + | "elif" + | "else" + | "except" + | "finally" + | "for" + | "from" + | "global" + | "if" + | "import" + | "in" + | "is" + | "lambda" + | "nonlocal" + | "not" + | "or" + | "pass" + | "raise" + | "return" + | "try" + | "while" + | "with" + | "yield", + ) +} 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 5b3a83588c902..9741e049cdba3 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 @@ -2,7 +2,7 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs snapshot_kind: text --- -RUF052.py:80:16: RUF052 Local variable `_var` with leading underscore is accessed +RUF052.py:80:16: RUF052 [*] Local dummy variable `_var` is accessed | 78 | def fun(self): 79 | _var = "method variable" @@ -13,7 +13,19 @@ RUF052.py:80:16: RUF052 Local variable `_var` with leading underscore is accesse | = help: Remove leading underscores -RUF052.py:83:12: RUF052 Local variable `_var` with leading underscore is accessed +ℹ Safe fix +76 76 | +77 77 | class Class_: +78 78 | def fun(self): +79 |- _var = "method variable" +80 |- return _var # [RUF052] + 79 |+ var = "method variable" + 80 |+ return var # [RUF052] +81 81 | +82 82 | def fun(_var): +83 83 | return _var # [RUF052] + +RUF052.py:83:12: RUF052 [*] Local dummy variable `_var` is accessed | 82 | def fun(_var): 83 | return _var # [RUF052] @@ -23,7 +35,19 @@ RUF052.py:83:12: RUF052 Local variable `_var` with leading underscore is accesse | = help: Remove leading underscores -RUF052.py:87:12: RUF052 Local variable `_list` with leading underscore is accessed +ℹ Safe fix +79 79 | _var = "method variable" +80 80 | return _var # [RUF052] +81 81 | +82 |-def fun(_var): +83 |- return _var # [RUF052] + 82 |+def fun(var): + 83 |+ return var # [RUF052] +84 84 | +85 85 | def fun(): +86 86 | _list = "built-in" + +RUF052.py:87:12: RUF052 [*] Local dummy variable `_list` is accessed | 85 | def fun(): 86 | _list = "built-in" @@ -34,7 +58,19 @@ RUF052.py:87:12: RUF052 Local variable `_list` with leading underscore is access | = help: Prefer using trailing underscores to avoid shadowing a built-in -RUF052.py:94:12: RUF052 Local variable `_x` with leading underscore is accessed +ℹ Safe fix +83 83 | return _var # [RUF052] +84 84 | +85 85 | def fun(): +86 |- _list = "built-in" +87 |- return _list # [RUF052] + 86 |+ list_ = "built-in" + 87 |+ return list_ # [RUF052] +88 88 | +89 89 | x = "global" +90 90 | + +RUF052.py:94:12: RUF052 [*] Local dummy variable `_x` is accessed | 92 | global x 93 | _x = "shadows global" @@ -45,7 +81,19 @@ RUF052.py:94:12: RUF052 Local variable `_x` with leading underscore is accessed | = help: Prefer using trailing underscores to avoid shadowing a variable -RUF052.py:101:12: RUF052 Local variable `_x` with leading underscore is accessed +ℹ Safe fix +90 90 | +91 91 | def fun(): +92 92 | global x +93 |- _x = "shadows global" +94 |- return _x # [RUF052] + 93 |+ x_ = "shadows global" + 94 |+ return x_ # [RUF052] +95 95 | +96 96 | def foo(): +97 97 | x = "outer" + +RUF052.py:101:12: RUF052 [*] Local dummy variable `_x` is accessed | 99 | nonlocal x 100 | _x = "shadows nonlocal" @@ -56,7 +104,19 @@ RUF052.py:101:12: RUF052 Local variable `_x` with leading underscore is accessed | = help: Prefer using trailing underscores to avoid shadowing a variable -RUF052.py:108:12: RUF052 Local variable `_x` with leading underscore is accessed +ℹ Safe fix +97 97 | x = "outer" +98 98 | def bar(): +99 99 | nonlocal x +100 |- _x = "shadows nonlocal" +101 |- return _x # [RUF052] + 100 |+ x_ = "shadows nonlocal" + 101 |+ return x_ # [RUF052] +102 102 | bar() +103 103 | return x +104 104 | + +RUF052.py:108:12: RUF052 [*] Local dummy variable `_x` is accessed | 106 | x = "local" 107 | _x = "shadows local" @@ -64,3 +124,12 @@ RUF052.py:108:12: RUF052 Local variable `_x` with leading underscore is accessed | ^^ RUF052 | = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +104 104 | +105 105 | def fun(): +106 106 | x = "local" +107 |- _x = "shadows local" +108 |- return _x # [RUF052] + 107 |+ x_ = "shadows local" + 108 |+ return x_ # [RUF052] From 2b479758bd5cd6df06dec777ac932386697c220d Mon Sep 17 00:00:00 2001 From: Lokejoke <70384473+Lokejoke@users.noreply.github.com> Date: Sat, 30 Nov 2024 22:08:35 +0100 Subject: [PATCH 13/20] Update crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs Co-authored-by: Alex Waygood --- .../rules/ruff/rules/dummy_variable_accessed.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 1b5d8f64fff8b..49db0df75d529 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -8,14 +8,16 @@ use ruff_text_size::Ranged; use crate::{checkers::ast::Checker, renamer::Renamer}; /// ## What it does -/// Checks for accesses of local dummy variable (specified by `dummy-variable-rgx` setting), except dunder names and '_'. -/// Provides auto fix for identifiers with a leading underscore if possible. +/// Checks for accesses of local dummy variables, excluding `_` and dunder variables. +/// +/// By default, "dummy variables" are any variables with names that start with leading +/// underscores. However, this is customisable using the `dummy-variable-rgx` setting). /// /// ## Why is this bad? -/// Marking variables with a leading underscore conveys that they are intentionally unused within the function or method. +/// 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" but subsequently used suggests oversight or unintentional use -/// and detracts from the clarity and maintainability of the codebase. +/// 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. /// /// ## Example /// ```python @@ -31,6 +33,9 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// return variable + 1 /// ``` /// +/// ## Fix availability +/// An fix is only available for variables that start with leading underscores. +/// /// ## Options /// - [`lint.dummy-variable-rgx`] #[derive(ViolationMetadata)] From 2fb7615402e42ddee53a703bf9c82f8edde8f07a Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Sat, 30 Nov 2024 22:40:32 +0100 Subject: [PATCH 14/20] Diagnostic is applied only on the binding definition, changed visibility of is_keyword --- .../resources/test/fixtures/ruff/RUF052.py | 26 ++- .../src/checkers/ast/analyze/bindings.rs | 4 +- .../ruff/rules/dummy_variable_accessed.rs | 93 ++------- ..._rules__ruff__tests__RUF052_RUF052.py.snap | 187 +++++++++--------- crates/ruff_python_stdlib/src/keyword.rs | 2 +- 5 files changed, 127 insertions(+), 185 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py index ecbdb4826ace8..b99ce11108165 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py @@ -57,8 +57,6 @@ def _valid_method(self): def method(arg): _valid_unused_var = arg return - -# Correct if dummy_variable_re = "_+" def fun(x): _ = 1 @@ -76,33 +74,33 @@ def fun(x): class Class_: def fun(self): - _var = "method variable" - return _var # [RUF052] + _var = "method variable" # [RUF052] + return _var -def fun(_var): - return _var # [RUF052] +def fun(_var): # [RUF052] + return _var def fun(): - _list = "built-in" - return _list # [RUF052] + _list = "built-in" # [RUF052] + return _list x = "global" def fun(): global x - _x = "shadows global" - return _x # [RUF052] + _x = "shadows global" # [RUF052] + return _x def foo(): x = "outer" def bar(): nonlocal x - _x = "shadows nonlocal" - return _x # [RUF052] + _x = "shadows nonlocal" # [RUF052] + return _x bar() return x def fun(): x = "local" - _x = "shadows local" - return _x # [RUF052] + _x = "shadows local" # [RUF052] + return _x diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 8bfec4374213b..09783678b618a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -90,8 +90,8 @@ pub(crate) fn bindings(checker: &mut Checker) { } } if checker.enabled(Rule::DummyVariableAccessed) { - if let Some(diagnostics) = ruff::rules::dummy_variable_accessed(checker, binding) { - checker.diagnostics.extend(diagnostics); + if let Some(diagnostic) = ruff::rules::dummy_variable_accessed(checker, binding) { + checker.diagnostics.push(diagnostic); } } if checker.enabled(Rule::AssignmentInAssert) { diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 49db0df75d529..ca940bde353ec 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -2,7 +2,9 @@ use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violatio use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::helpers::is_dunder; use ruff_python_semantic::{Binding, BindingKind, Scope}; -use ruff_python_stdlib::{builtins::is_python_builtin, identifiers::is_identifier}; +use ruff_python_stdlib::{ + builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, +}; use ruff_text_size::Ranged; use crate::{checkers::ast::Checker, renamer::Renamer}; @@ -72,10 +74,7 @@ impl Violation for DummyVariableAccessed { } /// RUF052 -pub(crate) fn dummy_variable_accessed( - checker: &Checker, - binding: &Binding, -) -> Option> { +pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> Option { let name = binding.name(checker.source()); // Ignore `_` and dunder variables @@ -108,39 +107,28 @@ pub(crate) fn dummy_variable_accessed( let possible_fix_kind = get_possible_fix_kind(name, scope, checker); - let mut diagnostics: Vec = binding - .references - .iter() - .map(|ref_id| { - Diagnostic::new( - DummyVariableAccessed { - name: name.to_string(), - fix_kind: possible_fix_kind, - }, - checker.semantic().reference(*ref_id).range(), - ) - .with_parent(binding.start()) - }) - .collect(); - - // Try at most one auto-fix for first diagnostic instance + let mut diagnostic = Diagnostic::new( + DummyVariableAccessed { + name: name.to_string(), + fix_kind: possible_fix_kind, + }, + binding.range(), + ); + + // If fix available if let Some(fix_kind) = possible_fix_kind { // Get the possible fix based on the scope if let Some(fix) = get_possible_fix(name, fix_kind, scope) { - // Fix is available - if let Some(diagnostic) = diagnostics.first_mut() { - // Try to set the fix - diagnostic.try_set_fix(|| { - let (edit, rest) = - Renamer::rename(name, &fix, scope, checker.semantic(), checker.stylist())?; - let applicability = Applicability::Safe; - Ok(Fix::applicable_edits(edit, rest, applicability)) - }); - } + diagnostic.try_set_fix(|| { + let (edit, rest) = + Renamer::rename(name, &fix, scope, checker.semantic(), checker.stylist())?; + let applicability = Applicability::Safe; + Ok(Fix::applicable_edits(edit, rest, applicability)) + }); } } - Some(diagnostics) + Some(diagnostic) } /// Enumeration of various ways in which a binding can shadow other variables @@ -208,44 +196,3 @@ fn get_possible_fix_kind(name: &str, scope: &Scope, checker: &Checker) -> Option // Default to no shadowing Some(ShadowedKind::None) } - -fn is_keyword(name: &str) -> bool { - matches!( - name, - "False" - | "None" - | "True" - | "and" - | "as" - | "assert" - | "async" - | "await" - | "break" - | "class" - | "continue" - | "def" - | "del" - | "elif" - | "else" - | "except" - | "finally" - | "for" - | "from" - | "global" - | "if" - | "import" - | "in" - | "is" - | "lambda" - | "nonlocal" - | "not" - | "or" - | "pass" - | "raise" - | "return" - | "try" - | "while" - | "with" - | "yield", - ) -} 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 9741e049cdba3..ad8e5355311b1 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 @@ -2,134 +2,131 @@ source: crates/ruff_linter/src/rules/ruff/mod.rs snapshot_kind: text --- -RUF052.py:80:16: RUF052 [*] Local dummy variable `_var` is accessed +RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed | -78 | def fun(self): -79 | _var = "method variable" -80 | return _var # [RUF052] - | ^^^^ RUF052 -81 | -82 | def fun(_var): +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var | = help: Remove leading underscores ℹ Safe fix -76 76 | -77 77 | class Class_: -78 78 | def fun(self): -79 |- _var = "method variable" -80 |- return _var # [RUF052] - 79 |+ var = "method variable" - 80 |+ return var # [RUF052] -81 81 | -82 82 | def fun(_var): -83 83 | return _var # [RUF052] +74 74 | +75 75 | class Class_: +76 76 | def fun(self): +77 |- _var = "method variable" # [RUF052] +78 |- return _var + 77 |+ var = "method variable" # [RUF052] + 78 |+ return var +79 79 | +80 80 | def fun(_var): # [RUF052] +81 81 | return _var -RUF052.py:83:12: RUF052 [*] Local dummy variable `_var` is accessed +RUF052.py:80:9: RUF052 [*] Local dummy variable `_var` is accessed | -82 | def fun(_var): -83 | return _var # [RUF052] - | ^^^^ RUF052 -84 | -85 | def fun(): +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var | = help: Remove leading underscores ℹ Safe fix -79 79 | _var = "method variable" -80 80 | return _var # [RUF052] -81 81 | -82 |-def fun(_var): -83 |- return _var # [RUF052] - 82 |+def fun(var): - 83 |+ return var # [RUF052] -84 84 | -85 85 | def fun(): -86 86 | _list = "built-in" +77 77 | _var = "method variable" # [RUF052] +78 78 | return _var +79 79 | +80 |-def fun(_var): # [RUF052] +81 |- return _var + 80 |+def fun(var): # [RUF052] + 81 |+ return var +82 82 | +83 83 | def fun(): +84 84 | _list = "built-in" # [RUF052] -RUF052.py:87:12: RUF052 [*] Local dummy variable `_list` is accessed +RUF052.py:84:5: RUF052 [*] Local dummy variable `_list` is accessed | -85 | def fun(): -86 | _list = "built-in" -87 | return _list # [RUF052] - | ^^^^^ RUF052 -88 | -89 | x = "global" +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list | = help: Prefer using trailing underscores to avoid shadowing a built-in ℹ Safe fix -83 83 | return _var # [RUF052] -84 84 | -85 85 | def fun(): -86 |- _list = "built-in" -87 |- return _list # [RUF052] - 86 |+ list_ = "built-in" - 87 |+ return list_ # [RUF052] +81 81 | return _var +82 82 | +83 83 | def fun(): +84 |- _list = "built-in" # [RUF052] +85 |- return _list + 84 |+ list_ = "built-in" # [RUF052] + 85 |+ return list_ +86 86 | +87 87 | x = "global" 88 88 | -89 89 | x = "global" -90 90 | -RUF052.py:94:12: RUF052 [*] Local dummy variable `_x` is accessed +RUF052.py:91:5: RUF052 [*] Local dummy variable `_x` is accessed | -92 | global x -93 | _x = "shadows global" -94 | return _x # [RUF052] - | ^^ RUF052 -95 | -96 | def foo(): +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x | = help: Prefer using trailing underscores to avoid shadowing a variable ℹ Safe fix -90 90 | -91 91 | def fun(): -92 92 | global x -93 |- _x = "shadows global" -94 |- return _x # [RUF052] - 93 |+ x_ = "shadows global" - 94 |+ return x_ # [RUF052] -95 95 | -96 96 | def foo(): -97 97 | x = "outer" +88 88 | +89 89 | def fun(): +90 90 | global x +91 |- _x = "shadows global" # [RUF052] +92 |- return _x + 91 |+ x_ = "shadows global" # [RUF052] + 92 |+ return x_ +93 93 | +94 94 | def foo(): +95 95 | x = "outer" -RUF052.py:101:12: RUF052 [*] Local dummy variable `_x` is accessed +RUF052.py:98:5: RUF052 [*] Local dummy variable `_x` is accessed | - 99 | nonlocal x -100 | _x = "shadows nonlocal" -101 | return _x # [RUF052] - | ^^ RUF052 -102 | bar() -103 | return x + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() | = help: Prefer using trailing underscores to avoid shadowing a variable ℹ Safe fix -97 97 | x = "outer" -98 98 | def bar(): -99 99 | nonlocal x -100 |- _x = "shadows nonlocal" -101 |- return _x # [RUF052] - 100 |+ x_ = "shadows nonlocal" - 101 |+ return x_ # [RUF052] -102 102 | bar() -103 103 | return x -104 104 | +95 95 | x = "outer" +96 96 | def bar(): +97 97 | nonlocal x +98 |- _x = "shadows nonlocal" # [RUF052] +99 |- return _x + 98 |+ x_ = "shadows nonlocal" # [RUF052] + 99 |+ return x_ +100 100 | bar() +101 101 | return x +102 102 | -RUF052.py:108:12: RUF052 [*] Local dummy variable `_x` is accessed +RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed | -106 | x = "local" -107 | _x = "shadows local" -108 | return _x # [RUF052] - | ^^ RUF052 +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x | = help: Prefer using trailing underscores to avoid shadowing a variable ℹ Safe fix -104 104 | -105 105 | def fun(): -106 106 | x = "local" -107 |- _x = "shadows local" -108 |- return _x # [RUF052] - 107 |+ x_ = "shadows local" - 108 |+ return x_ # [RUF052] +102 102 | +103 103 | def fun(): +104 104 | x = "local" +105 |- _x = "shadows local" # [RUF052] +106 |- return _x + 105 |+ x_ = "shadows local" # [RUF052] + 106 |+ return x_ diff --git a/crates/ruff_python_stdlib/src/keyword.rs b/crates/ruff_python_stdlib/src/keyword.rs index 7f361c0b6988f..6057a0d9cf1dc 100644 --- a/crates/ruff_python_stdlib/src/keyword.rs +++ b/crates/ruff_python_stdlib/src/keyword.rs @@ -1,5 +1,5 @@ // See: https://github.com/python/cpython/blob/9d692841691590c25e6cf5b2250a594d3bf54825/Lib/keyword.py#L18 -pub(crate) fn is_keyword(name: &str) -> bool { +pub fn is_keyword(name: &str) -> bool { matches!( name, "False" From b387d5c240667bff35c91a6a8109e2c5cc5eeeef Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Sat, 30 Nov 2024 23:55:50 +0100 Subject: [PATCH 15/20] Last touches, no changes to snap though --- .../ruff/rules/dummy_variable_accessed.rs | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index ca940bde353ec..2b395310e55a9 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -1,7 +1,7 @@ -use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation}; +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, BindingKind, Scope}; +use ruff_python_semantic::{Binding, BindingKind, SemanticModel}; use ruff_python_stdlib::{ builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, }; @@ -21,6 +21,9 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// 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. /// +/// 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. +/// /// ## Example /// ```python /// def function(): @@ -40,6 +43,9 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// /// ## Options /// - [`lint.dummy-variable-rgx`] +/// +/// +/// [PEP 8]: https://peps.python.org/pep-0008/ #[derive(ViolationMetadata)] pub(crate) struct DummyVariableAccessed { name: String, @@ -96,8 +102,11 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O if binding.is_global() || binding.is_nonlocal() { return None; } + + let semantic = checker.semantic(); + // Only variables defined in function scopes - let scope = &checker.semantic().scopes[binding.scope]; + let scope = &semantic.scopes[binding.scope]; if !scope.kind.is_function() { return None; } @@ -105,7 +114,7 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O return None; } - let possible_fix_kind = get_possible_fix_kind(name, scope, checker); + let possible_fix_kind = get_possible_fix_kind(name, checker); let mut diagnostic = Diagnostic::new( DummyVariableAccessed { @@ -118,12 +127,10 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O // If fix available if let Some(fix_kind) = possible_fix_kind { // Get the possible fix based on the scope - if let Some(fix) = get_possible_fix(name, fix_kind, scope) { + if let Some(fix) = get_possible_fix(name, fix_kind, semantic) { diagnostic.try_set_fix(|| { - let (edit, rest) = - Renamer::rename(name, &fix, scope, checker.semantic(), checker.stylist())?; - let applicability = Applicability::Safe; - Ok(Fix::applicable_edits(edit, rest, applicability)) + Renamer::rename(name, &fix, scope, semantic, checker.stylist()) + .map(|(edit, rest)| Fix::safe_edits(edit, rest)) }); } } @@ -145,7 +152,7 @@ enum ShadowedKind { } /// Suggests a potential alternative name to resolve a shadowing conflict. -fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option { +fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> Option { // Remove leading underscores for processing let trimmed_name = name.trim_start_matches('_'); @@ -157,8 +164,8 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option trimmed_name.to_string(), }; - // Ensure the fix name is not already taken in the scope - if scope.has(&fix_name) { + // Ensure the fix name is not already taken in the scope or enclosing scopes + if !semantic.is_available(&fix_name) { return None; } @@ -167,7 +174,7 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option Option { +fn get_possible_fix_kind(name: &str, checker: &Checker) -> Option { // If the name starts with an underscore, we don't consider it if !name.starts_with('_') { return None; @@ -189,7 +196,7 @@ fn get_possible_fix_kind(name: &str, scope: &Scope, checker: &Checker) -> Option return Some(ShadowedKind::BuiltIn); } - if scope.has(trimmed_name) { + if !checker.semantic().is_available(trimmed_name) { return Some(ShadowedKind::Some); } From 0067de846b1853407dd381e954796f0fe7922b7b Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Sat, 30 Nov 2024 23:56:58 +0100 Subject: [PATCH 16/20] fmt --- .../ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 2b395310e55a9..759ebeb51cd96 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -43,7 +43,7 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// /// ## Options /// - [`lint.dummy-variable-rgx`] -/// +/// /// /// [PEP 8]: https://peps.python.org/pep-0008/ #[derive(ViolationMetadata)] From d9e69be7ff0d5d114cade4afb20c3c8e6f54ebe9 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Sun, 1 Dec 2024 11:04:16 +0100 Subject: [PATCH 17/20] Applied proposed changes --- .../ruff/rules/dummy_variable_accessed.rs | 22 +++++++++----- crates/ruff_python_semantic/src/model.rs | 30 +++++++++++++++---- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 759ebeb51cd96..51ef5d08a773c 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -1,7 +1,7 @@ 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, BindingKind, SemanticModel}; +use ruff_python_semantic::{Binding, BindingKind, ScopeId, SemanticModel}; use ruff_python_stdlib::{ builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, }; @@ -114,7 +114,7 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O return None; } - let possible_fix_kind = get_possible_fix_kind(name, checker); + let possible_fix_kind = get_possible_fix_kind(name, checker, binding.scope); let mut diagnostic = Diagnostic::new( DummyVariableAccessed { @@ -127,7 +127,7 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O // If fix available if let Some(fix_kind) = possible_fix_kind { // Get the possible fix based on the scope - if let Some(fix) = get_possible_fix(name, fix_kind, semantic) { + if let Some(fix) = get_possible_fix(name, fix_kind, binding.scope, semantic) { diagnostic.try_set_fix(|| { Renamer::rename(name, &fix, scope, semantic, checker.stylist()) .map(|(edit, rest)| Fix::safe_edits(edit, rest)) @@ -152,7 +152,12 @@ enum ShadowedKind { } /// Suggests a potential alternative name to resolve a shadowing conflict. -fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> Option { +fn get_possible_fix( + name: &str, + kind: ShadowedKind, + scope_id: ScopeId, + semantic: &SemanticModel, +) -> Option { // Remove leading underscores for processing let trimmed_name = name.trim_start_matches('_'); @@ -165,7 +170,7 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> }; // Ensure the fix name is not already taken in the scope or enclosing scopes - if !semantic.is_available(&fix_name) { + if !semantic.is_available_in_scope(&fix_name, scope_id) { return None; } @@ -174,7 +179,7 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> } /// Determines the kind of shadowing or conflict for a given variable name. -fn get_possible_fix_kind(name: &str, checker: &Checker) -> Option { +fn get_possible_fix_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; @@ -196,7 +201,10 @@ fn get_possible_fix_kind(name: &str, checker: &Checker) -> Option return Some(ShadowedKind::BuiltIn); } - if !checker.semantic().is_available(trimmed_name) { + if !checker + .semantic() + .is_available_in_scope(trimmed_name, scope_id) + { return Some(ShadowedKind::Some); } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 9be76dc208c03..ff3a1a10ae995 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -325,9 +325,15 @@ impl<'a> SemanticModel<'a> { } /// Return `true` if `member` is an "available" symbol, i.e., a symbol that has not been bound - /// in the current scope, or in any containing scope. + /// in the current scope currently being visited, or in any containing scope. pub fn is_available(&self, member: &str) -> bool { - self.lookup_symbol(member) + self.is_available_in_scope(member, self.scope_id) + } + + /// Return `true` if `member` is an "available" symbol in a given scope, i.e., + /// a symbol that has not been bound in that current scope, or in any containing scope. + pub fn is_available_in_scope(&self, member: &str, scope_id: ScopeId) -> bool { + self.lookup_symbol_in_scope(member, scope_id, false) .map(|binding_id| &self.bindings[binding_id]) .map_or(true, |binding| binding.kind.is_builtin()) } @@ -620,10 +626,22 @@ impl<'a> SemanticModel<'a> { } } - /// Lookup a symbol in the current scope. This is a carbon copy of [`Self::resolve_load`], but - /// doesn't add any read references to the resolved symbol. + /// Lookup a symbol in the current scope. pub fn lookup_symbol(&self, symbol: &str) -> Option { - if self.in_forward_reference() { + self.lookup_symbol_in_scope(symbol, self.scope_id, self.in_forward_reference()) + } + + /// Lookup a symbol in a certain scope + /// + /// This is a carbon copy of [`Self::resolve_load`], but + /// doesn't add any read references to the resolved symbol. + pub fn lookup_symbol_in_scope( + &self, + symbol: &str, + scope_id: ScopeId, + in_forward_reference: bool, + ) -> Option { + if in_forward_reference { if let Some(binding_id) = self.scopes.global().get(symbol) { if !self.bindings[binding_id].is_unbound() { return Some(binding_id); @@ -633,7 +651,7 @@ impl<'a> SemanticModel<'a> { let mut seen_function = false; let mut class_variables_visible = true; - for (index, scope_id) in self.scopes.ancestor_ids(self.scope_id).enumerate() { + for (index, scope_id) in self.scopes.ancestor_ids(scope_id).enumerate() { let scope = &self.scopes[scope_id]; if scope.kind.is_class() { if seen_function && matches!(symbol, "__class__") { From 977de563514a6dc775c1984142f08884f689259d Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Mon, 2 Dec 2024 12:06:00 +0100 Subject: [PATCH 18/20] Fix of dummy is not dummy. --- crates/ruff_linter/src/rules/ruff/mod.rs | 24 ++++ .../ruff/rules/dummy_variable_accessed.rs | 30 ++-- ...var_regexp_preset__RUF052_RUF052.py_1.snap | 132 ++++++++++++++++++ ...var_regexp_preset__RUF052_RUF052.py_2.snap | 121 ++++++++++++++++ 4 files changed, 296 insertions(+), 11 deletions(-) create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index eb1a9ff78f052..dea2e5c90c4ab 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -10,6 +10,7 @@ mod tests { use std::path::Path; use anyhow::Result; + use regex::Regex; use rustc_hash::FxHashSet; use test_case::test_case; @@ -450,4 +451,27 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"^_+", 1)] + #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"", 2)] + fn custom_regexp_preset(rule_code: Rule, path: &Path, regex_pattern: &str, id: u8) -> Result<()> { + // Compile the regex from the pattern string + let regex = Regex::new(regex_pattern).unwrap(); + + let snapshot = format!( + "custom_dummy_var_regexp_preset__{}_{}_{}", + rule_code.noqa_code(), + path.to_string_lossy(), + id, + ); + let diagnostics = test_path( + Path::new("ruff").join(path).as_path(), + &settings::LinterSettings { + dummy_variable_rgx: regex, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs index 51ef5d08a773c..a68f9325238ae 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs @@ -1,7 +1,7 @@ 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, BindingKind, ScopeId, SemanticModel}; +use ruff_python_semantic::{Binding, BindingKind, ScopeId}; use ruff_python_stdlib::{ builtins::is_python_builtin, identifiers::is_identifier, keyword::is_keyword, }; @@ -49,7 +49,7 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; #[derive(ViolationMetadata)] pub(crate) struct DummyVariableAccessed { name: String, - fix_kind: Option, + shadowed_kind: Option, } impl Violation for DummyVariableAccessed { @@ -61,8 +61,8 @@ impl Violation for DummyVariableAccessed { } fn fix_title(&self) -> Option { - if let Some(fix_kind) = self.fix_kind { - return Some(match fix_kind { + 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() } @@ -114,20 +114,20 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O return None; } - let possible_fix_kind = get_possible_fix_kind(name, checker, binding.scope); + let shadowed_kind = try_shadowed_kind(name, checker, binding.scope); let mut diagnostic = Diagnostic::new( DummyVariableAccessed { name: name.to_string(), - fix_kind: possible_fix_kind, + shadowed_kind, }, binding.range(), ); // If fix available - if let Some(fix_kind) = possible_fix_kind { + if let Some(shadowed_kind) = shadowed_kind { // Get the possible fix based on the scope - if let Some(fix) = get_possible_fix(name, fix_kind, binding.scope, semantic) { + 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::safe_edits(edit, rest)) @@ -156,7 +156,7 @@ fn get_possible_fix( name: &str, kind: ShadowedKind, scope_id: ScopeId, - semantic: &SemanticModel, + checker: &Checker, ) -> Option { // Remove leading underscores for processing let trimmed_name = name.trim_start_matches('_'); @@ -169,8 +169,16 @@ fn get_possible_fix( ShadowedKind::None => trimmed_name.to_string(), }; + // Check if the fix name is again dummy identifier + if checker.settings.dummy_variable_rgx.is_match(&fix_name) { + return None; + } + // Ensure the fix name is not already taken in the scope or enclosing scopes - if !semantic.is_available_in_scope(&fix_name, scope_id) { + if !checker + .semantic() + .is_available_in_scope(&fix_name, scope_id) + { return None; } @@ -179,7 +187,7 @@ fn get_possible_fix( } /// Determines the kind of shadowing or conflict for a given variable name. -fn get_possible_fix_kind(name: &str, checker: &Checker, scope_id: ScopeId) -> Option { +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; 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 new file mode 100644 index 0000000000000..ad8e5355311b1 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_1.snap @@ -0,0 +1,132 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF052.py:77:9: RUF052 [*] Local dummy variable `_var` is accessed + | +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +74 74 | +75 75 | class Class_: +76 76 | def fun(self): +77 |- _var = "method variable" # [RUF052] +78 |- return _var + 77 |+ var = "method variable" # [RUF052] + 78 |+ return var +79 79 | +80 80 | def fun(_var): # [RUF052] +81 81 | return _var + +RUF052.py:80:9: RUF052 [*] Local dummy variable `_var` is accessed + | +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var + | + = help: Remove leading underscores + +ℹ Safe fix +77 77 | _var = "method variable" # [RUF052] +78 78 | return _var +79 79 | +80 |-def fun(_var): # [RUF052] +81 |- return _var + 80 |+def fun(var): # [RUF052] + 81 |+ return var +82 82 | +83 83 | def fun(): +84 84 | _list = "built-in" # [RUF052] + +RUF052.py:84:5: RUF052 [*] Local dummy variable `_list` is accessed + | +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +ℹ Safe fix +81 81 | return _var +82 82 | +83 83 | def fun(): +84 |- _list = "built-in" # [RUF052] +85 |- return _list + 84 |+ list_ = "built-in" # [RUF052] + 85 |+ return list_ +86 86 | +87 87 | x = "global" +88 88 | + +RUF052.py:91:5: RUF052 [*] Local dummy variable `_x` is accessed + | +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +88 88 | +89 89 | def fun(): +90 90 | global x +91 |- _x = "shadows global" # [RUF052] +92 |- return _x + 91 |+ x_ = "shadows global" # [RUF052] + 92 |+ return x_ +93 93 | +94 94 | def foo(): +95 95 | x = "outer" + +RUF052.py:98:5: RUF052 [*] Local dummy variable `_x` is accessed + | + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +95 95 | x = "outer" +96 96 | def bar(): +97 97 | nonlocal x +98 |- _x = "shadows nonlocal" # [RUF052] +99 |- return _x + 98 |+ x_ = "shadows nonlocal" # [RUF052] + 99 |+ return x_ +100 100 | bar() +101 101 | return x +102 102 | + +RUF052.py:105:5: RUF052 [*] Local dummy variable `_x` is accessed + | +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +ℹ Safe fix +102 102 | +103 103 | def fun(): +104 104 | x = "local" +105 |- _x = "shadows local" # [RUF052] +106 |- return _x + 105 |+ x_ = "shadows local" # [RUF052] + 106 |+ return x_ 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 new file mode 100644 index 0000000000000..ea20bafed686e --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__custom_dummy_var_regexp_preset__RUF052_RUF052.py_2.snap @@ -0,0 +1,121 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +snapshot_kind: text +--- +RUF052.py:21:9: RUF052 Local dummy variable `arg` is accessed + | +19 | _valid_fun() +20 | +21 | def fun(arg): + | ^^^ RUF052 +22 | _valid_unused_var = arg +23 | pass + | + +RUF052.py:50:18: RUF052 Local dummy variable `self` is accessed + | +48 | print(_valid_private_cls_attr) +49 | +50 | def __init__(self): + | ^^^^ RUF052 +51 | self._valid_private_ins_attr = 2 +52 | print(self._valid_private_ins_attr) + | + +RUF052.py:54:23: RUF052 Local dummy variable `self` is accessed + | +52 | print(self._valid_private_ins_attr) +53 | +54 | def _valid_method(self): + | ^^^^ RUF052 +55 | return self._valid_private_ins_attr + | + +RUF052.py:57:16: RUF052 Local dummy variable `arg` is accessed + | +55 | return self._valid_private_ins_attr +56 | +57 | def method(arg): + | ^^^ RUF052 +58 | _valid_unused_var = arg +59 | return + | + +RUF052.py:61:9: RUF052 Local dummy variable `x` is accessed + | +59 | return +60 | +61 | def fun(x): + | ^ RUF052 +62 | _ = 1 +63 | __ = 2 + | + +RUF052.py:77:9: RUF052 Local dummy variable `_var` is accessed + | +75 | class Class_: +76 | def fun(self): +77 | _var = "method variable" # [RUF052] + | ^^^^ RUF052 +78 | return _var + | + = help: Remove leading underscores + +RUF052.py:80:9: RUF052 Local dummy variable `_var` is accessed + | +78 | return _var +79 | +80 | def fun(_var): # [RUF052] + | ^^^^ RUF052 +81 | return _var + | + = help: Remove leading underscores + +RUF052.py:84:5: RUF052 Local dummy variable `_list` is accessed + | +83 | def fun(): +84 | _list = "built-in" # [RUF052] + | ^^^^^ RUF052 +85 | return _list + | + = help: Prefer using trailing underscores to avoid shadowing a built-in + +RUF052.py:91:5: RUF052 Local dummy variable `_x` is accessed + | +89 | def fun(): +90 | global x +91 | _x = "shadows global" # [RUF052] + | ^^ RUF052 +92 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:95:3: RUF052 Local dummy variable `x` is accessed + | +94 | def foo(): +95 | x = "outer" + | ^ RUF052 +96 | def bar(): +97 | nonlocal x + | + +RUF052.py:98:5: RUF052 Local dummy variable `_x` is accessed + | + 96 | def bar(): + 97 | nonlocal x + 98 | _x = "shadows nonlocal" # [RUF052] + | ^^ RUF052 + 99 | return _x +100 | bar() + | + = help: Prefer using trailing underscores to avoid shadowing a variable + +RUF052.py:105:5: RUF052 Local dummy variable `_x` is accessed + | +103 | def fun(): +104 | x = "local" +105 | _x = "shadows local" # [RUF052] + | ^^ RUF052 +106 | return _x + | + = help: Prefer using trailing underscores to avoid shadowing a variable From d30972d179c2830d442d1d13ad20d5b9cdf52043 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Mon, 2 Dec 2024 12:09:53 +0100 Subject: [PATCH 19/20] fmt --- crates/ruff_linter/src/rules/ruff/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index dea2e5c90c4ab..f614d81733b56 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -454,7 +454,12 @@ mod tests { #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"^_+", 1)] #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"", 2)] - fn custom_regexp_preset(rule_code: Rule, path: &Path, regex_pattern: &str, id: u8) -> Result<()> { + fn custom_regexp_preset( + rule_code: Rule, + path: &Path, + regex_pattern: &str, + id: u8, + ) -> Result<()> { // Compile the regex from the pattern string let regex = Regex::new(regex_pattern).unwrap(); From 2bcab3ab71f04739454540bf6ae6dd343d74a0a9 Mon Sep 17 00:00:00 2001 From: Lokejoke Date: Mon, 2 Dec 2024 22:24:31 +0100 Subject: [PATCH 20/20] Renamed rule to used-dummy-variable --- crates/ruff_linter/src/checkers/ast/analyze/bindings.rs | 8 ++++---- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 6 +++--- crates/ruff_linter/src/rules/ruff/rules/mod.rs | 4 ++-- ...{dummy_variable_accessed.rs => used_dummy_variable.rs} | 8 ++++---- 5 files changed, 14 insertions(+), 14 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{dummy_variable_accessed.rs => used_dummy_variable.rs} (96%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs index 09783678b618a..a871f307e3bed 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/bindings.rs @@ -10,7 +10,7 @@ use crate::rules::{ /// Run lint rules over the [`Binding`]s. pub(crate) fn bindings(checker: &mut Checker) { if !checker.any_enabled(&[ - Rule::DummyVariableAccessed, + Rule::AssignmentInAssert, Rule::InvalidAllFormat, Rule::InvalidAllObject, Rule::NonAsciiName, @@ -19,7 +19,7 @@ pub(crate) fn bindings(checker: &mut Checker) { Rule::UnsortedDunderSlots, Rule::UnusedVariable, Rule::UnquotedTypeAlias, - Rule::AssignmentInAssert, + Rule::UsedDummyVariable, ]) { return; } @@ -89,8 +89,8 @@ pub(crate) fn bindings(checker: &mut Checker) { checker.diagnostics.push(diagnostic); } } - if checker.enabled(Rule::DummyVariableAccessed) { - if let Some(diagnostic) = ruff::rules::dummy_variable_accessed(checker, binding) { + if checker.enabled(Rule::UsedDummyVariable) { + if let Some(diagnostic) = ruff::rules::used_dummy_variable(checker, binding) { checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 8851373bc3ee4..ee3ac314a36bb 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -984,7 +984,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "040") => (RuleGroup::Preview, rules::ruff::rules::InvalidAssertMessageLiteralArgument), (Ruff, "041") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryNestedLiteral), (Ruff, "048") => (RuleGroup::Preview, rules::ruff::rules::MapIntVersionParsing), - (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::DummyVariableAccessed), + (Ruff, "052") => (RuleGroup::Preview, rules::ruff::rules::UsedDummyVariable), (Ruff, "055") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryRegularExpression), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index f614d81733b56..e5f783db960ae 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -71,7 +71,7 @@ mod tests { #[test_case(Rule::InvalidAssertMessageLiteralArgument, Path::new("RUF040.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.py"))] #[test_case(Rule::UnnecessaryNestedLiteral, Path::new("RUF041.pyi"))] - #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"))] + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( @@ -452,8 +452,8 @@ mod tests { Ok(()) } - #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"^_+", 1)] - #[test_case(Rule::DummyVariableAccessed, Path::new("RUF052.py"), r"", 2)] + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"), r"^_+", 1)] + #[test_case(Rule::UsedDummyVariable, Path::new("RUF052.py"), r"", 2)] fn custom_regexp_preset( rule_code: Rule, path: &Path, diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index c1df7ea217c92..69f92b8da2bab 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -5,7 +5,6 @@ pub(crate) use asyncio_dangling_task::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use decimal_from_float_literal::*; pub(crate) use default_factory_kwarg::*; -pub(crate) use dummy_variable_accessed::*; pub(crate) use explicit_f_string_type_conversion::*; pub(crate) use function_call_in_dataclass_default::*; pub(crate) use implicit_optional::*; @@ -39,6 +38,7 @@ pub(crate) use unraw_re_pattern::*; pub(crate) use unsafe_markup_use::*; pub(crate) use unused_async::*; pub(crate) use unused_noqa::*; +pub(crate) use used_dummy_variable::*; pub(crate) use useless_if_else::*; pub(crate) use zip_instead_of_pairwise::*; @@ -50,7 +50,6 @@ mod collection_literal_concatenation; mod confusables; mod decimal_from_float_literal; mod default_factory_kwarg; -mod dummy_variable_accessed; mod explicit_f_string_type_conversion; mod function_call_in_dataclass_default; mod helpers; @@ -87,6 +86,7 @@ mod unraw_re_pattern; mod unsafe_markup_use; mod unused_async; mod unused_noqa; +mod used_dummy_variable; mod useless_if_else; mod zip_instead_of_pairwise; diff --git a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs similarity index 96% rename from crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs rename to crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs index a68f9325238ae..d40716dbd36c8 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/used_dummy_variable.rs @@ -47,12 +47,12 @@ use crate::{checkers::ast::Checker, renamer::Renamer}; /// /// [PEP 8]: https://peps.python.org/pep-0008/ #[derive(ViolationMetadata)] -pub(crate) struct DummyVariableAccessed { +pub(crate) struct UsedDummyVariable { name: String, shadowed_kind: Option, } -impl Violation for DummyVariableAccessed { +impl Violation for UsedDummyVariable { const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] @@ -80,7 +80,7 @@ impl Violation for DummyVariableAccessed { } /// RUF052 -pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> Option { +pub(crate) fn used_dummy_variable(checker: &Checker, binding: &Binding) -> Option { let name = binding.name(checker.source()); // Ignore `_` and dunder variables @@ -117,7 +117,7 @@ pub(crate) fn dummy_variable_accessed(checker: &Checker, binding: &Binding) -> O let shadowed_kind = try_shadowed_kind(name, checker, binding.scope); let mut diagnostic = Diagnostic::new( - DummyVariableAccessed { + UsedDummyVariable { name: name.to_string(), shadowed_kind, },