[pylint] Implement modified-iterating-set (E4703)

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)

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);
             }

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),

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")

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;

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")
+}

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 |