mirror of https://github.com/astral-sh/ruff
Remove `discard`, `remove`, and `pop` allowance for `loop-iterator-mutation` (#12365)
## Summary Pretty sure this should still be an error, but also, I think I added this because of ecosystem CI? So want to see what pops up. Closes https://github.com/astral-sh/ruff/issues/12164.
This commit is contained in:
parent
e39298dcbc
commit
1435b0f022
|
|
@ -152,13 +152,16 @@ for elem in some_list:
|
||||||
else:
|
else:
|
||||||
break
|
break
|
||||||
|
|
||||||
# should not error
|
# should error
|
||||||
for elem in some_list:
|
for elem in some_list:
|
||||||
del some_list[elem]
|
del some_list[elem]
|
||||||
some_list[elem] = 1
|
|
||||||
some_list.remove(elem)
|
some_list.remove(elem)
|
||||||
some_list.discard(elem)
|
some_list.discard(elem)
|
||||||
|
|
||||||
|
# should not error
|
||||||
|
for elem in some_list:
|
||||||
|
some_list[elem] = 1
|
||||||
|
|
||||||
# should error
|
# should error
|
||||||
for i, elem in enumerate(some_list):
|
for i, elem in enumerate(some_list):
|
||||||
some_list.pop(0)
|
some_list.pop(0)
|
||||||
|
|
@ -169,4 +172,4 @@ for i, elem in enumerate(some_list):
|
||||||
|
|
||||||
# should not error (dict)
|
# should not error (dict)
|
||||||
for i, elem in enumerate(some_list):
|
for i, elem in enumerate(some_list):
|
||||||
some_list[elem] = 1
|
some_list[elem] = 1
|
||||||
|
|
@ -5,8 +5,8 @@ use ruff_python_ast::comparable::ComparableExpr;
|
||||||
use ruff_python_ast::name::UnqualifiedName;
|
use ruff_python_ast::name::UnqualifiedName;
|
||||||
use ruff_python_ast::{
|
use ruff_python_ast::{
|
||||||
visitor::{self, Visitor},
|
visitor::{self, Visitor},
|
||||||
Arguments, Expr, ExprAttribute, ExprCall, ExprSubscript, ExprTuple, Stmt, StmtAssign,
|
Expr, ExprAttribute, ExprCall, ExprSubscript, ExprTuple, Stmt, StmtAssign, StmtAugAssign,
|
||||||
StmtAugAssign, StmtBreak, StmtDelete, StmtFor, StmtIf,
|
StmtBreak, StmtDelete, StmtFor, StmtIf,
|
||||||
};
|
};
|
||||||
use ruff_text_size::TextRange;
|
use ruff_text_size::TextRange;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
|
|
@ -175,18 +175,13 @@ impl<'a> LoopMutationsVisitor<'a> {
|
||||||
if let Expr::Subscript(ExprSubscript {
|
if let Expr::Subscript(ExprSubscript {
|
||||||
range: _,
|
range: _,
|
||||||
value,
|
value,
|
||||||
slice,
|
slice: _,
|
||||||
ctx: _,
|
ctx: _,
|
||||||
}) = target
|
}) = target
|
||||||
{
|
{
|
||||||
// Find, e.g., `del items[0]`.
|
// Find, e.g., `del items[0]`.
|
||||||
if ComparableExpr::from(self.iter) == ComparableExpr::from(value) {
|
if ComparableExpr::from(self.iter) == ComparableExpr::from(value) {
|
||||||
// But allow, e.g., `for item in items: del items[item]`.
|
self.add_mutation(range);
|
||||||
if ComparableExpr::from(self.index) != ComparableExpr::from(slice)
|
|
||||||
&& ComparableExpr::from(self.target) != ComparableExpr::from(slice)
|
|
||||||
{
|
|
||||||
self.add_mutation(range);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -223,7 +218,7 @@ impl<'a> LoopMutationsVisitor<'a> {
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Handle, e.g., `items.append(1)`.
|
/// Handle, e.g., `items.append(1)`.
|
||||||
fn handle_call(&mut self, func: &Expr, arguments: &Arguments) {
|
fn handle_call(&mut self, func: &Expr) {
|
||||||
if let Expr::Attribute(ExprAttribute {
|
if let Expr::Attribute(ExprAttribute {
|
||||||
range,
|
range,
|
||||||
value,
|
value,
|
||||||
|
|
@ -234,20 +229,6 @@ impl<'a> LoopMutationsVisitor<'a> {
|
||||||
if is_mutating_function(attr.as_str()) {
|
if is_mutating_function(attr.as_str()) {
|
||||||
// Find, e.g., `items.remove(1)`.
|
// Find, e.g., `items.remove(1)`.
|
||||||
if ComparableExpr::from(self.iter) == ComparableExpr::from(value) {
|
if ComparableExpr::from(self.iter) == ComparableExpr::from(value) {
|
||||||
// But allow, e.g., `for item in items: items.remove(item)`.
|
|
||||||
if matches!(attr.as_str(), "remove" | "discard" | "pop") {
|
|
||||||
if arguments.len() == 1 {
|
|
||||||
if let [arg] = &*arguments.args {
|
|
||||||
if ComparableExpr::from(self.index) == ComparableExpr::from(arg)
|
|
||||||
|| ComparableExpr::from(self.target)
|
|
||||||
== ComparableExpr::from(arg)
|
|
||||||
{
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
self.add_mutation(*range);
|
self.add_mutation(*range);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -323,11 +304,8 @@ impl<'a> Visitor<'a> for LoopMutationsVisitor<'a> {
|
||||||
|
|
||||||
fn visit_expr(&mut self, expr: &'a Expr) {
|
fn visit_expr(&mut self, expr: &'a Expr) {
|
||||||
// Ex) `items.append(1)`
|
// Ex) `items.append(1)`
|
||||||
if let Expr::Call(ExprCall {
|
if let Expr::Call(ExprCall { func, .. }) = expr {
|
||||||
func, arguments, ..
|
self.handle_call(func);
|
||||||
}) = expr
|
|
||||||
{
|
|
||||||
self.handle_call(func, arguments);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
visitor::walk_expr(self, expr);
|
visitor::walk_expr(self, expr);
|
||||||
|
|
|
||||||
|
|
@ -340,12 +340,41 @@ B909.py:150:8: B909 Mutation to loop iterable `some_list` during iteration
|
||||||
152 | else:
|
152 | else:
|
||||||
|
|
|
|
||||||
|
|
||||||
B909.py:164:5: B909 Mutation to loop iterable `some_list` during iteration
|
B909.py:157:5: B909 Mutation to loop iterable `some_list` during iteration
|
||||||
|
|
|
|
||||||
162 | # should error
|
155 | # should error
|
||||||
163 | for i, elem in enumerate(some_list):
|
156 | for elem in some_list:
|
||||||
164 | some_list.pop(0)
|
157 | del some_list[elem]
|
||||||
|
| ^^^^^^^^^^^^^^^^^^^ B909
|
||||||
|
158 | some_list.remove(elem)
|
||||||
|
159 | some_list.discard(elem)
|
||||||
|
|
|
||||||
|
|
||||||
|
B909.py:158:5: B909 Mutation to loop iterable `some_list` during iteration
|
||||||
|
|
|
||||||
|
156 | for elem in some_list:
|
||||||
|
157 | del some_list[elem]
|
||||||
|
158 | some_list.remove(elem)
|
||||||
|
| ^^^^^^^^^^^^^^^^ B909
|
||||||
|
159 | some_list.discard(elem)
|
||||||
|
|
|
||||||
|
|
||||||
|
B909.py:159:5: B909 Mutation to loop iterable `some_list` during iteration
|
||||||
|
|
|
||||||
|
157 | del some_list[elem]
|
||||||
|
158 | some_list.remove(elem)
|
||||||
|
159 | some_list.discard(elem)
|
||||||
|
| ^^^^^^^^^^^^^^^^^ B909
|
||||||
|
160 |
|
||||||
|
161 | # should not error
|
||||||
|
|
|
||||||
|
|
||||||
|
B909.py:167:5: B909 Mutation to loop iterable `some_list` during iteration
|
||||||
|
|
|
||||||
|
165 | # should error
|
||||||
|
166 | for i, elem in enumerate(some_list):
|
||||||
|
167 | some_list.pop(0)
|
||||||
| ^^^^^^^^^^^^^ B909
|
| ^^^^^^^^^^^^^ B909
|
||||||
165 |
|
168 |
|
||||||
166 | # should not error (list)
|
169 | # should not error (list)
|
||||||
|
|
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue