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

crates/ruff_linter/resources/test/fixtures/pylint/modified_iterating_set.py

@@ -0,0 +1,55 @@
+# 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")
+
+# 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,118 @@
+use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, Expr, Stmt, StmtFor};
+use ruff_python_semantic::analyze::typing::is_set;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for `set` being modified during the iteration on the set.
+///
+/// ## Why is this bad?
+/// If `set` is modified during the iteration, it will cause `RuntimeError`.
+/// This could be fixed by using temporal copy of the set to iterate.
+///
+/// ## 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 {
+        format!(
+            "Iterated set `{}` is being modified inside for loop body.",
+            self.name
+        )
+    }
+
+    fn fix_title(&self) -> String {
+        format!("Consider iterating through a copy of it instead.")
+    }
+}
+
+fn is_method_modifying(identifier: &str) -> bool {
+    (identifier == "add")
+        || (identifier == "clear")
+        || (identifier == "discard")
+        || (identifier == "pop")
+        || (identifier == "remove")
+}
+
+// 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 mut is_modified = false;
+    for stmt in &for_stmt.body {
+        // name_of_set.modify_method()
+        // ^---------^ ^-----------^
+        //    value        attr
+        // ^-----------------------^
+        //           func
+        // ^-------------------------^
+        //        expr, stmt
+        let Stmt::Expr(ast::StmtExpr { value: expr, .. }) = stmt else {
+            continue;
+        };
+
+        let Some(func) = expr.as_call_expr().map(|exprcall| &exprcall.func) else {
+            continue;
+        };
+
+        let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
+            continue;
+        };
+
+        let Some(value) = value.as_name_expr() else {
+            continue;
+        };
+
+        let Some(binding_id_value) = checker.semantic().only_binding(value) else {
+            continue;
+        };
+        if binding_id == binding_id_value && is_method_modifying(attr.as_str()) {
+            is_modified = true;
+        }
+    }
+
+    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()", name.id),
+            name.range(),
+        )));
+        checker.diagnostics.push(diagnostic);
+    }
+}

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE4703_modified_iterating_set.py.snap

@@ -0,0 +1,107 @@
+---
+source: crates/ruff_linter/src/rules/pylint/mod.rs
+---
+modified_iterating_set.py:4:1: PLE4703 [*] Iterated set `nums` is being modified inside for loop body.
+  |
+3 |   nums = {1, 2, 3}
+4 | / for num in nums:
+5 | |     nums.add(num + 1)
+  | |_____________________^ PLE4703
+6 |   
+7 |   animals = {"dog", "cat", "cow"}
+  |
+  = help: Consider iterating through a copy of it instead.
+
+ℹ 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 being modified inside for loop body.
+   |
+ 7 |   animals = {"dog", "cat", "cow"}
+ 8 | / for animal in animals:
+ 9 | |     animals.pop("cow")
+   | |______________________^ PLE4703
+10 |   
+11 |   fruits = {"apple", "orange", "grape"}
+   |
+   = help: Consider iterating through a copy of it instead.
+
+ℹ 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 being modified inside for loop body.
+   |
+11 |   fruits = {"apple", "orange", "grape"}
+12 | / for fruit in fruits:
+13 | |     fruits.clear()
+   | |__________________^ PLE4703
+14 |   
+15 |   planets = {"mercury", "venus", "earth"}
+   |
+   = help: Consider iterating through a copy of it instead.
+
+ℹ 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 being modified inside for loop body.
+   |
+15 |   planets = {"mercury", "venus", "earth"}
+16 | / for planet in planets:
+17 | |     planets.discard("mercury")
+   | |______________________________^ PLE4703
+18 |   
+19 |   colors = {"red", "green", "blue"}
+   |
+   = help: Consider iterating through a copy of it instead.
+
+ℹ 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 being modified inside for loop body.
+   |
+19 |   colors = {"red", "green", "blue"}
+20 | / for color in colors:
+21 | |     colors.remove("red")
+   | |________________________^ PLE4703
+22 |   
+23 |   # OK
+   |
+   = help: Consider iterating through a copy of it instead.
+
+ℹ 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 | # OK