-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix false positive in PLR6301
#7933
Fix false positive in PLR6301
#7933
Conversation
Don't report a diagnostic if the method contains a `super()` call.
struct CallVisitor { | ||
is_super: bool, | ||
} | ||
|
||
impl<'a> Visitor<'a> for CallVisitor { | ||
fn visit_expr(&mut self, expr: &'a Expr) { | ||
match expr { | ||
Expr::Call(ast::ExprCall { func, .. }) => match func.as_ref() { | ||
Expr::Name(expr_name) => { | ||
self.is_super = expr_name.id == "super"; | ||
} | ||
_ => { | ||
visitor::walk_expr(self, expr); | ||
} | ||
}, | ||
_ => visitor::walk_expr(self, expr), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, it would be better to use the function scope to check if the method contains a super()
call in the body, but the function scope only gives me access to its parameters. So when using scope.get("super")
it would only return None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that super
is actually a global builtin in Python, so the usages we need are on the global scope. I tweaked the logic to use the technique you suggested above. I guess the downside is that we can't ensure that the super
usage was on a super()
call, but perhaps that's fine. (We don't store a NodeId
on Reference
, though we could in the future.)
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
aa8ff07
to
fe5f699
Compare
return; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LaBatata101 - What do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Didn't know about the binding.references()
function, will be very helpful when implementing other rules in the future.
Summary
Don't report a diagnostic if the method contains a
super()
call.Closes #6961
Test Plan
cargo test