diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/modified_iterating_set.py b/crates/ruff_linter/resources/test/fixtures/pylint/modified_iterating_set.py new file mode 100644 index 0000000000000..7d3e7af8687b3 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/modified_iterating_set.py @@ -0,0 +1,60 @@ +# Errors + +nums = {1, 2, 3} +for num in nums: + nums.add(num + 1) + +animals = {"dog", "cat", "cow"} +for animal in animals: + animals.pop("cow") + +fruits = {"apple", "orange", "grape"} +for fruit in fruits: + fruits.clear() + +planets = {"mercury", "venus", "earth"} +for planet in planets: + planets.discard("mercury") + +colors = {"red", "green", "blue"} +for color in colors: + colors.remove("red") + +odds = {1, 3, 5} +for num in odds: + if num > 1: + odds.add(num + 1) + +# OK + +nums = {1, 2, 3} +for num in nums.copy(): + nums.add(nums + 3) + +animals = {"dog", "cat", "cow"} +for animal in animals: + print(animals - {animal}) + +fruits = {"apple", "orange", "grape"} +temp_fruits = set() +for fruit in fruits: + temp_fruits.add(fruit) + temp_fruits.remove(fruit) + temp_fruits.clear(fruit) + +colors = {"red", "green", "blue"} + + +def add_colors(): + colors = {"cyan", "magenta", "yellow"} + for color in colors: + + def add_color(): + global colors + colors.add(color) + + add_color() + + +add_colors() +print(colors) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 0b7839767356d..7e8293172916f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1293,6 +1293,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::ManualDictComprehension) { perflint::rules::manual_dict_comprehension(checker, target, body); } + if checker.enabled(Rule::ModifiedIteratingSet) { + pylint::rules::modified_iterating_set(checker, for_stmt); + } if checker.enabled(Rule::UnnecessaryListCast) { perflint::rules::unnecessary_list_cast(checker, iter, body); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index ce8b7c4661977..a6db70d943302 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -265,6 +265,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "E2513") => (RuleGroup::Stable, rules::pylint::rules::InvalidCharacterEsc), (Pylint, "E2514") => (RuleGroup::Stable, rules::pylint::rules::InvalidCharacterNul), (Pylint, "E2515") => (RuleGroup::Stable, rules::pylint::rules::InvalidCharacterZeroWidthSpace), + (Pylint, "E4703") => (RuleGroup::Preview, rules::pylint::rules::ModifiedIteratingSet), (Pylint, "R0124") => (RuleGroup::Stable, rules::pylint::rules::ComparisonWithItself), (Pylint, "R0133") => (RuleGroup::Stable, rules::pylint::rules::ComparisonOfConstant), (Pylint, "R0202") => (RuleGroup::Preview, rules::pylint::rules::NoClassmethodDecorator), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 7d9bc56575854..86d3d2e897e37 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -92,6 +92,7 @@ mod tests { #[test_case(Rule::LoggingTooFewArgs, Path::new("logging_too_few_args.py"))] #[test_case(Rule::LoggingTooManyArgs, Path::new("logging_too_many_args.py"))] #[test_case(Rule::MagicValueComparison, Path::new("magic_value_comparison.py"))] + #[test_case(Rule::ModifiedIteratingSet, Path::new("modified_iterating_set.py"))] #[test_case( Rule::NamedExprWithoutContext, Path::new("named_expr_without_context.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index c190406c67776..99bbb5063efdb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -37,6 +37,7 @@ pub(crate) use logging::*; pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; pub(crate) use misplaced_bare_raise::*; +pub(crate) use modified_iterating_set::*; pub(crate) use named_expr_without_context::*; pub(crate) use nan_comparison::*; pub(crate) use nested_min_max::*; @@ -130,6 +131,7 @@ mod logging; mod magic_value_comparison; mod manual_import_from; mod misplaced_bare_raise; +mod modified_iterating_set; mod named_expr_without_context; mod nan_comparison; mod nested_min_max; diff --git a/crates/ruff_linter/src/rules/pylint/rules/modified_iterating_set.rs b/crates/ruff_linter/src/rules/pylint/rules/modified_iterating_set.rs new file mode 100644 index 0000000000000..efc31b4e1cf08 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/modified_iterating_set.rs @@ -0,0 +1,111 @@ +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::any_over_body; +use ruff_python_ast::{self as ast, Expr, StmtFor}; +use ruff_python_semantic::analyze::typing::is_set; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for loops in which a `set` is modified during iteration. +/// +/// ## Why is this bad? +/// If a `set` is modified during iteration, it will cause a `RuntimeError`. +/// +/// If you need to modify a `set` within a loop, consider iterating over a copy +/// of the `set` instead. +/// +/// ## Known problems +/// This rule favors false negatives over false positives. Specifically, it +/// will only detect variables that can be inferred to be a `set` type based on +/// local type inference, and will only detect modifications that are made +/// directly on the variable itself (e.g., `set.add()`), as opposed to +/// modifications within other function calls (e.g., `some_function(set)`). +/// +/// ## Example +/// ```python +/// nums = {1, 2, 3} +/// for num in nums: +/// nums.add(num + 5) +/// ``` +/// +/// Use instead: +/// ```python +/// nums = {1, 2, 3} +/// for num in nums.copy(): +/// nums.add(num + 5) +/// ``` +/// +/// ## References +/// - [Python documentation: `set`](https://docs.python.org/3/library/stdtypes.html#set) +#[violation] +pub struct ModifiedIteratingSet { + name: String, +} + +impl AlwaysFixableViolation for ModifiedIteratingSet { + #[derive_message_formats] + fn message(&self) -> String { + let ModifiedIteratingSet { name } = self; + format!("Iterated set `{name}` is modified within the `for` loop",) + } + + fn fix_title(&self) -> String { + let ModifiedIteratingSet { name } = self; + format!("Iterate over a copy of `{name}`") + } +} + +/// PLE4703 +pub(crate) fn modified_iterating_set(checker: &mut Checker, for_stmt: &StmtFor) { + let Some(name) = for_stmt.iter.as_name_expr() else { + return; + }; + + let Some(binding_id) = checker.semantic().only_binding(name) else { + return; + }; + if !is_set(checker.semantic().binding(binding_id), checker.semantic()) { + return; + } + + let is_modified = any_over_body(&for_stmt.body, &|expr| { + let Some(func) = expr.as_call_expr().map(|call| &call.func) else { + return false; + }; + + let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else { + return false; + }; + + let Some(value) = value.as_name_expr() else { + return false; + }; + + let Some(value_id) = checker.semantic().only_binding(value) else { + return false; + }; + + binding_id == value_id && modifies_set(attr.as_str()) + }); + + if is_modified { + let mut diagnostic = Diagnostic::new( + ModifiedIteratingSet { + name: name.id.clone(), + }, + for_stmt.range(), + ); + diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement( + format!("{}.copy()", checker.locator().slice(name)), + name.range(), + ))); + checker.diagnostics.push(diagnostic); + } +} + +/// Returns `true` if the method modifies the set. +fn modifies_set(identifier: &str) -> bool { + matches!(identifier, "add" | "clear" | "discard" | "pop" | "remove") +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE4703_modified_iterating_set.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE4703_modified_iterating_set.py.snap new file mode 100644 index 0000000000000..0af466fe72d09 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE4703_modified_iterating_set.py.snap @@ -0,0 +1,129 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +modified_iterating_set.py:4:1: PLE4703 [*] Iterated set `nums` is modified within the `for` loop + | +3 | nums = {1, 2, 3} +4 | / for num in nums: +5 | | nums.add(num + 1) + | |_____________________^ PLE4703 +6 | +7 | animals = {"dog", "cat", "cow"} + | + = help: Iterate over a copy of `nums` + +ℹ Unsafe fix +1 1 | # Errors +2 2 | +3 3 | nums = {1, 2, 3} +4 |-for num in nums: + 4 |+for num in nums.copy(): +5 5 | nums.add(num + 1) +6 6 | +7 7 | animals = {"dog", "cat", "cow"} + +modified_iterating_set.py:8:1: PLE4703 [*] Iterated set `animals` is modified within the `for` loop + | + 7 | animals = {"dog", "cat", "cow"} + 8 | / for animal in animals: + 9 | | animals.pop("cow") + | |______________________^ PLE4703 +10 | +11 | fruits = {"apple", "orange", "grape"} + | + = help: Iterate over a copy of `animals` + +ℹ Unsafe fix +5 5 | nums.add(num + 1) +6 6 | +7 7 | animals = {"dog", "cat", "cow"} +8 |-for animal in animals: + 8 |+for animal in animals.copy(): +9 9 | animals.pop("cow") +10 10 | +11 11 | fruits = {"apple", "orange", "grape"} + +modified_iterating_set.py:12:1: PLE4703 [*] Iterated set `fruits` is modified within the `for` loop + | +11 | fruits = {"apple", "orange", "grape"} +12 | / for fruit in fruits: +13 | | fruits.clear() + | |__________________^ PLE4703 +14 | +15 | planets = {"mercury", "venus", "earth"} + | + = help: Iterate over a copy of `fruits` + +ℹ Unsafe fix +9 9 | animals.pop("cow") +10 10 | +11 11 | fruits = {"apple", "orange", "grape"} +12 |-for fruit in fruits: + 12 |+for fruit in fruits.copy(): +13 13 | fruits.clear() +14 14 | +15 15 | planets = {"mercury", "venus", "earth"} + +modified_iterating_set.py:16:1: PLE4703 [*] Iterated set `planets` is modified within the `for` loop + | +15 | planets = {"mercury", "venus", "earth"} +16 | / for planet in planets: +17 | | planets.discard("mercury") + | |______________________________^ PLE4703 +18 | +19 | colors = {"red", "green", "blue"} + | + = help: Iterate over a copy of `planets` + +ℹ Unsafe fix +13 13 | fruits.clear() +14 14 | +15 15 | planets = {"mercury", "venus", "earth"} +16 |-for planet in planets: + 16 |+for planet in planets.copy(): +17 17 | planets.discard("mercury") +18 18 | +19 19 | colors = {"red", "green", "blue"} + +modified_iterating_set.py:20:1: PLE4703 [*] Iterated set `colors` is modified within the `for` loop + | +19 | colors = {"red", "green", "blue"} +20 | / for color in colors: +21 | | colors.remove("red") + | |________________________^ PLE4703 +22 | +23 | odds = {1, 3, 5} + | + = help: Iterate over a copy of `colors` + +ℹ Unsafe fix +17 17 | planets.discard("mercury") +18 18 | +19 19 | colors = {"red", "green", "blue"} +20 |-for color in colors: + 20 |+for color in colors.copy(): +21 21 | colors.remove("red") +22 22 | +23 23 | odds = {1, 3, 5} + +modified_iterating_set.py:24:1: PLE4703 [*] Iterated set `odds` is modified within the `for` loop + | +23 | odds = {1, 3, 5} +24 | / for num in odds: +25 | | if num > 1: +26 | | odds.add(num + 1) + | |_________________________^ PLE4703 +27 | +28 | # OK + | + = help: Iterate over a copy of `odds` + +ℹ Unsafe fix +21 21 | colors.remove("red") +22 22 | +23 23 | odds = {1, 3, 5} +24 |-for num in odds: + 24 |+for num in odds.copy(): +25 25 | if num > 1: +26 26 | odds.add(num + 1) +27 27 | diff --git a/ruff.schema.json b/ruff.schema.json index 2858f60029dd4..1c26154c2d19f 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3311,6 +3311,10 @@ "PLE2513", "PLE2514", "PLE2515", + "PLE4", + "PLE47", + "PLE470", + "PLE4703", "PLR", "PLR0", "PLR01",