-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add new rule RUF059: Unused unpacked assignment #16449
Conversation
Split from F841 following discussion in astral-sh#8884. Fixes astral-sh#8884.
@@ -48,6 +48,7 @@ pub(crate) fn deferred_scopes(checker: &Checker) { | |||
Rule::UnusedPrivateTypedDict, | |||
Rule::UnusedPrivateTypeVar, | |||
Rule::UnusedStaticMethodArgument, | |||
Rule::UnusedUnpackedVariable, |
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.
feedback/rant: took me forever to figure out that missing this line was why my tests weren't producing any lints. It makes sense in retrospect and I don't have a better idea for how to do it, but it was a bit frustrating.
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.
Sorry for that. Yeah, not sure what a good short-term fix for this is. My plan for Red Knot is to remove those manual checks entirely and instead have a more "formal" rule API that allows pre-computing in which phase a rule must run. This will hopefully help prevent such surprises
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
F841 | 1091 | 0 | 1091 | 0 | 0 |
RUF059 | 831 | 831 | 0 | 0 | 0 |
PLR0914 | 2 | 1 | 1 | 0 | 0 |
DOC201 | 2 | 1 | 1 | 0 | 0 |
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.
Thanks for pushing this over the line. I'll make a few small nit improvements but this looks good.
/// Under [preview mode](https://docs.astral.sh/ruff/preview), this rule also | ||
/// triggers on unused unpacked assignments (for example, `x, y = foo()`). | ||
/// |
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.
/// Under [preview mode](https://docs.astral.sh/ruff/preview), this rule also | |
/// triggers on unused unpacked assignments (for example, `x, y = foo()`). | |
/// |
@@ -48,6 +48,7 @@ pub(crate) fn deferred_scopes(checker: &Checker) { | |||
Rule::UnusedPrivateTypedDict, | |||
Rule::UnusedPrivateTypeVar, | |||
Rule::UnusedStaticMethodArgument, | |||
Rule::UnusedUnpackedVariable, |
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.
Sorry for that. Yeah, not sure what a good short-term fix for this is. My plan for Red Knot is to remove those manual checks entirely and instead have a more "formal" rule API that allows pre-computing in which phase a rule must run. This will hopefully help prevent such surprises
for (name, binding) in scope | ||
.bindings() | ||
.map(|(name, binding_id)| (name, checker.semantic().binding(binding_id))) | ||
.filter_map(|(name, binding)| { | ||
if checker.settings.preview.is_enabled() | ||
&& binding.is_unpacked_assignment() | ||
&& binding.is_unused() | ||
&& !binding.is_nonlocal() | ||
&& !binding.is_global() | ||
&& !checker.settings.dummy_variable_rgx.is_match(name) | ||
{ | ||
return Some((name, binding)); | ||
} | ||
|
||
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's a bit unfortunate that this repeats the entire check from F841 with only the unpack assignment being different. Let me see if we can make this more obvious
Thanks for the review and merge! |
Split from F841 following discussion in #8884.
Fixes #8884.
Summary
Add a new rule for unused assignments in tuples. Remove similar behavior from F841.
Test Plan
Adapt F841 tests and move them over to the new rule.