[pylint] Implement redefined-argument-from-local (R1704) #8159
Conversation
…ed-argument-from-local
…ed-argument-from-local
…ed-argument-from-local # Conflicts: # crates/ruff_linter/src/checkers/ast/analyze/statement.rs # crates/ruff_linter/src/codes.rs
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
| scope = &checker.semantic().scopes[scope_id]; | ||
| } else { | ||
| break; | ||
| }; |
There was a problem hiding this comment.
I'm wondering if there's a slightly different way to structure this rule. Bear with me...
Instead of analyzing the AST, I think we can achieve the same effect by looking at bindings.
When we have something like:
def f(x):
for x in y:
...We create a binding for the x in def f(x), and then later, a binding for x in for x in y, and we track that this second binding shadows the first.
If you look in deferred_scopes.rs, you'll see we have some rules based on looking at these shadows:
if checker.enabled(Rule::ImportShadowedByLoopVar) {
for (name, binding_id) in scope.bindings() {
for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) {
// If the shadowing binding isn't a loop variable, abort.
let binding = &checker.semantic.bindings[shadow.binding_id()];
if !binding.kind.is_loop_var() {
continue;
}
// If the shadowed binding isn't an import, abort.
let shadowed = &checker.semantic.bindings[shadow.shadowed_id()];
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) {
continue;
}
// If the bindings are in different forks, abort.
if shadowed.source.map_or(true, |left| {
binding.source.map_or(true, |right| {
checker.semantic.different_branches(left, right)
})
}) {
continue;
}
#[allow(deprecated)]
let line = checker.locator.compute_line_index(shadowed.start());
checker.diagnostics.push(Diagnostic::new(
pyflakes::rules::ImportShadowedByLoopVar {
name: name.to_string(),
line,
},
binding.range(),
));
}
}
}I think this rule could be structured similarly: iterate over the pairs of shadowed bindings, and look for pairs in which the original binding is BindingKind::Argument, and the new binding is BindingKind::LoopVar.
|
Appreciate the comment. |
This reverts commit ab14d52.
…ed-argument-from-local
|
Applied the comment In the current code, it looks there's no BindingKind for withitem variable. Could you give an advice for the task? |
| let shadowed = &checker.semantic.bindings[shadow.shadowed_id()]; | ||
| if !shadowed.kind.is_argument() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Is this supposed to check if the argument is unused? Or does it fire in either case?
There was a problem hiding this comment.
It's not checking the usage.
It looks sensible to check binding.is_used() as below
if !shadowed.kind.is_argument() || !binding.is_used() {
continue;
}In this case, results are following
def foo(i):
for i in range(10): # emit error
print(i)
...
def foo(i):
for i in range(10): # no error because it's unused
...Is it good to go?
There was a problem hiding this comment.
I was referring to the inverse -- should this depend on the argument being unused? I'll check Pylint.
There was a problem hiding this comment.
I erred on the side of removing the !binding.is_used() piece here. I think it still makes sense to raise, even if the new binding is unused, since it will cause the same problems?
|
Thanks @danbi2990! Overall looks good. Let me take a look at adding a binding kind for |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLR1704 | 24 | 24 | 0 | 0 | 0 |
|
@danbi2990 - I added a with-item kind in #8594. Can you try updating against |
|
It's working with updated main. |
Summary
It implements Pylint rule R1704: redefined-argument-from-local
Problematic code:
Correct code:
References:
Pylint documentation
Related Issue
Test Plan
cargo test