[ruff] Implemented used-dummy-variable (RUF052)#14611
[ruff] Implemented used-dummy-variable (RUF052)#14611MichaReiser merged 22 commits intoastral-sh:mainfrom
ruff] Implemented used-dummy-variable (RUF052)#14611Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| RUF052 | 760 | 760 | 0 | 0 | 0 |
| DOC201 | 2 | 1 | 1 | 0 | 0 |
There was a problem hiding this comment.
Thanks. This rule does make sense to me but I'm a bit concerned by the number of violations. What do you think @AlexWaygood?
I think the rule should respect https://docs.astral.sh/ruff/settings/#lint_dummy-variable-rgx as it is the "opposite" of the unused variable rule
I think we should change the rule to report the use of the binding rather than the variable declaration because I noticed when reviewing the ecosystem changes, that I always had to search for the variable use to assess whether the violation is correct. Doing so would match clippy's behavior
We should exclude bindings that are declared as global or nonlocal because it's not in the functions control to change the name https://github.com/RasaHQ/rasa/blob/b8de3b231126747ff74b2782cb25cb22d2d898d7/rasa/core/jobs.py#L22
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
.../ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF052_RUF052.py.snap
Outdated
Show resolved
Hide resolved
|
This rule makes sense to me as well. I agree with @MichaReiser's comment here however:
Here's another interesting case: https://github.com/DisnakeDev/disnake/blob/2d0f91ad7042d4b331d448678fc6f88b74973947/disnake/guild.py#L5119. I'm not sure whether the rule should or shouldn't flag this -- they've used the leading underscore here specifically because otherwise the variable would shadow a Python builtin. PEP 8 recommends using a trailing underscore for situations like this rather than a leading underscore ( If we want to go that route, you can use this function to detect whether the symbol would shadow a Python builtin without the leading underscore: ruff/crates/ruff_python_stdlib/src/builtins.rs Lines 219 to 222 in c40b37a |
AlexWaygood
left a comment
There was a problem hiding this comment.
Thank you! This is starting to look pretty good
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
|
It's a pity that the dummy regex, by default, marks every name with a leading underscore as a dummy variable. |
Oh -- I think @MichaReiser possibly was suggesting for you to do the opposite of what you're doing there right now? It makes sense to me that this rule would flag all variables that match the dummy regex (in addition/instead of flagging all variables that have leading underscores) rather than not flagging all variables that match the dummy regex. |
|
Ok, this does make sense. |
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/unused_variable_accessed.rs
Outdated
Show resolved
Hide resolved
|
We discussed the rule more internally and concluded that, considering the many violations, we should provide an auto fix. You could reuse some logic from here We must be careful only to offer a fix if the |
|
Do I list all options? For each dummy, we would match against: Should we go back to one diagnostic with one fix renaming the variable? |
The fix infrastructure should detect that it already applied the fix (or at least, it detects that the two fixes overlap) and skips it. It then rechecks the file, which should not yield any violations after the rename. What we should do is set
Generatign one fix each should be fine
But we can also start with adding a fix for dummy only, and then check the ecosystem results to see if it covers most variables. |
…d.rs Co-authored-by: Alex Waygood <[email protected]>
…ity of is_keyword
|
@Lokejoke I wanted to apply some very minor last touchups and then merge, but GitHub won't let me push to your branch (not sure why). The final changes I wanted to apply were these, if you'd be able to apply them. Mostly they're cosmetic, but I switched the use of Details--- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py
+++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF052.py
@@ -88,19 +88,39 @@ x = "global"
def fun():
global x
- _x = "shadows global" # [RUF052]
+ _x = "shadows global" # [RUF052] (but unfixable)
return _x
def foo():
x = "outer"
def bar():
nonlocal x
- _x = "shadows nonlocal" # [RUF052]
+ _x = "shadows nonlocal" # [RUF052] (but unfixable)
return _x
bar()
return x
def fun():
x = "local"
- _x = "shadows local" # [RUF052]
+ _x = "shadows local" # [RUF052] (but unfixable)
+ return _x
+
+def fun2():
+ x = "local"
+ x_ = "also local"
+ _x = "shadows local" # [RUF052] (but unfixable)
return _x
+
+def fun3():
+ global x_
+ x = "local"
+ _x = "shadows local" # [RUF052] (but unfixable)
+ return _x
+
+def fun4():
+ x_ = 42
+ def fun5():
+ x = "local"
+ _x = "shadows local" # RUF052 (but unfixable)
+ print(x_)
+ return _x
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 ca940bde3..e4a8dd28f 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,8 @@ 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 +101,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 +113,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 +126,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 +151,7 @@ enum ShadowedKind {
}
/// Suggests a potential alternative name to resolve a shadowing conflict.
-fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option<String> {
+fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> Option<String> {
// Remove leading underscores for processing
let trimmed_name = name.trim_start_matches('_');
@@ -157,8 +163,8 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option<Str
ShadowedKind::None => 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 +173,7 @@ fn get_possible_fix(name: &str, kind: ShadowedKind, scope: &Scope) -> Option<Str
}
/// Determines the kind of shadowing or conflict for a given variable name.
-fn get_possible_fix_kind(name: &str, scope: &Scope, checker: &Checker) -> Option<ShadowedKind> {
+fn get_possible_fix_kind(name: &str, checker: &Checker) -> Option<ShadowedKind> {
// If the name starts with an underscore, we don't consider it
if !name.starts_with('_') {
return None;
@@ -189,7 +195,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);
}If you'd be able to make these final changes, I'm happy to merge now :-) (Making these changes will also need some updates to the snapshots, since it makes the autofix more cautious -- some things in the fixtures which were previously deemed fixable are now no longer fixable.) |
|
Does the |
Oh, great catch! I've stepped on that footgun before, as well. We can fix that by making some small tweaks to the semantic model. You can add the |
we'd then need to create our own version of |
|
Sounds fantastic! Though, I'll leave it for tomorrow. |
|
I think something like this should work. It compiles, but I haven't tested it: Detailsdiff --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 e4a8dd28f..d90cf4ce5 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,
};
@@ -113,7 +113,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 {
@@ -126,7 +126,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))
@@ -151,7 +151,12 @@ enum ShadowedKind {
}
/// Suggests a potential alternative name to resolve a shadowing conflict.
-fn get_possible_fix(name: &str, kind: ShadowedKind, semantic: &SemanticModel) -> Option<String> {
+fn get_possible_fix(
+ name: &str,
+ kind: ShadowedKind,
+ scope_id: ScopeId,
+ semantic: &SemanticModel,
+) -> Option<String> {
// Remove leading underscores for processing
let trimmed_name = name.trim_start_matches('_');
@@ -164,7 +169,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;
}
@@ -173,7 +178,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<ShadowedKind> {
+fn get_possible_fix_kind(name: &str, checker: &Checker, scope_id: ScopeId) -> Option<ShadowedKind> {
// If the name starts with an underscore, we don't consider it
if !name.starts_with('_') {
return None;
@@ -195,7 +200,10 @@ fn get_possible_fix_kind(name: &str, checker: &Checker) -> Option<ShadowedKind>
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 9be76dc20..ff3a1a10a 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<BindingId> {
- 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<BindingId> {
+ 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__") { |
|
Thank you very much @AlexWaygood! I have applied all of your proposed changes, which seemed very intuitive. |
crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/dummy_variable_accessed.rs
Outdated
Show resolved
Hide resolved
|
This is great :) |
|
Considering that the rule is named I think I'd prefer that because the violation isn't the assignment to the variable, but that I'm using a variable that's marked as intentionally unused. That would also make reviewing the ecosystem changes much easier, because I wouldn't have to scan for the use :) |
See my earlier comment at #14611 (comment) @MichaReiser. I think the thing we're recommending to the user that they should change (and the thing that we do change in the autofix) is the naming of the variable: we don't try to remove uses of the variable! So I much prefer it as it is currently, with the diagnostic pointing to the binding rather than the use of the binding. |
ruff] Implemented unused-variable-accessed (RUF052)ruff] Implemented used-dummy-variable (RUF052)
|
Thanks for seeing this through! |
|
@AlexWaygood I know that you considered making some documentation changes to the rule but I thought I'd merge it and we can make those changes as separate PRs (this also solves the problem that you can't push to this branch 😆) |
|
Congrats @Lokejoke — there were a lot of unexpected complications to this one! This is great |
* main: [`ruff`] Extend unnecessary-regular-expression to non-literal strings (`RUF055`) (#14679) Minor followups to RUF052 (#14755) [red-knot] Property tests (#14178) [red-knot] `is_subtype_of` fix for `KnownInstance` types (#14750) Improve docs for flake8-use-pathlib rules (#14741) [`ruff`] Implemented `used-dummy-variable` (`RUF052`) (#14611) [red-knot] Simplify tuples containing `Never` (#14744) Possible fix for flaky file watching test (#14543) [`flake8-import-conventions`] Improve syntax check for aliases supplied in configuration for `unconventional-import-alias (ICN001)` (#14745) [red-knot] Deeper understanding of `LiteralString` (#14649) red-knot: support narrowing for bool(E) (#14668) [`refurb`] Handle non-finite decimals in `verbose-decimal-constructor (FURB157)` (#14596) [red-knot] Re-enable linter corpus tests (#14736)
Summary
This PR implements
wpsrule unused-variable-accessed; discussed here.Test Plan
cargo test