Tweak some docs

Author: charliermarsh

crates/ruff_linter/src/rules/pylint/rules/modified_iterating_set.rs

@@ -7,11 +7,20 @@ use ruff_text_size::Ranged;
 use crate::checkers::ast::Checker;
 
 /// ## What it does
-/// Checks for `set` being modified during the iteration on the set.
+/// Checks for loops in which a `set` is modified during iteration.
 ///
 /// ## 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.
+/// 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
@@ -37,26 +46,17 @@ pub struct ModifiedIteratingSet {
 impl AlwaysFixableViolation for ModifiedIteratingSet {
     #[derive_message_formats]
     fn message(&self) -> String {
-        format!(
-            "Iterated set `{}` is being modified inside for loop body.",
-            self.name
-        )
+        let ModifiedIteratingSet { name } = self;
+        format!("Iterated set `{name}` is modified within the `for` loop",)
     }
 
     fn fix_title(&self) -> String {
-        format!("Consider iterating through a copy of it instead.")
+        let ModifiedIteratingSet { name } = self;
+        format!("Iterate over a copy of `{name}`")
     }
 }
 
-fn is_method_modifying(identifier: &str) -> bool {
-    (identifier == "add")
-        || (identifier == "clear")
-        || (identifier == "discard")
-        || (identifier == "pop")
-        || (identifier == "remove")
-}
-
-// PLE4703
+/// PLE4703
 pub(crate) fn modified_iterating_set(checker: &mut Checker, for_stmt: &StmtFor) {
     let Some(name) = for_stmt.iter.as_name_expr() else {
         return;
@@ -69,8 +69,7 @@ pub(crate) fn modified_iterating_set(checker: &mut Checker, for_stmt: &StmtFor)
         return;
     }
 
-    let mut is_modified = false;
-    for stmt in &for_stmt.body {
+    if for_stmt.body.iter().any(|stmt| {
         // name_of_set.modify_method()
         // ^---------^ ^-----------^
         //    value        attr
@@ -79,40 +78,42 @@ pub(crate) fn modified_iterating_set(checker: &mut Checker, for_stmt: &StmtFor)
         // ^-------------------------^
         //        expr, stmt
         let Stmt::Expr(ast::StmtExpr { value: expr, .. }) = stmt else {
-            continue;
+            return false;
         };
 
         let Some(func) = expr.as_call_expr().map(|exprcall| &exprcall.func) else {
-            continue;
+            return false;
         };
 
         let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
-            continue;
+            return false;
         };
 
         let Some(value) = value.as_name_expr() else {
-            continue;
+            return false;
         };
 
         let Some(binding_id_value) = checker.semantic().only_binding(value) else {
-            continue;
+            return false;
         };
-        if binding_id == binding_id_value && is_method_modifying(attr.as_str()) {
-            is_modified = true;
-        }
-    }
 
-    if is_modified {
+        binding_id == binding_id_value && modifies_set(attr.as_str())
+    }) {
         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),
+            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

@@ -1,7 +1,7 @@
 ---
 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.
+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:
@@ -10,7 +10,7 @@ modified_iterating_set.py:4:1: PLE4703 [*] Iterated set `nums` is being modified
 6 |   
 7 |   animals = {"dog", "cat", "cow"}
   |
-  = help: Consider iterating through a copy of it instead.
+  = help: Iterate over a copy of `nums`
 
 ℹ Unsafe fix
 1 1 | # Errors
@@ -22,7 +22,7 @@ modified_iterating_set.py:4:1: PLE4703 [*] Iterated set `nums` is being modified
 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.
+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:
@@ -31,7 +31,7 @@ modified_iterating_set.py:8:1: PLE4703 [*] Iterated set `animals` is being modif
 10 |   
 11 |   fruits = {"apple", "orange", "grape"}
    |
-   = help: Consider iterating through a copy of it instead.
+   = help: Iterate over a copy of `animals`
 
 ℹ Unsafe fix
 5 5 |     nums.add(num + 1)
@@ -43,7 +43,7 @@ modified_iterating_set.py:8:1: PLE4703 [*] Iterated set `animals` is being modif
 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.
+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:
@@ -52,7 +52,7 @@ modified_iterating_set.py:12:1: PLE4703 [*] Iterated set `fruits` is being modif
 14 |   
 15 |   planets = {"mercury", "venus", "earth"}
    |
-   = help: Consider iterating through a copy of it instead.
+   = help: Iterate over a copy of `fruits`
 
 ℹ Unsafe fix
 9  9  |     animals.pop("cow")
@@ -64,7 +64,7 @@ modified_iterating_set.py:12:1: PLE4703 [*] Iterated set `fruits` is being modif
 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.
+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:
@@ -73,7 +73,7 @@ modified_iterating_set.py:16:1: PLE4703 [*] Iterated set `planets` is being modi
 18 |   
 19 |   colors = {"red", "green", "blue"}
    |
-   = help: Consider iterating through a copy of it instead.
+   = help: Iterate over a copy of `planets`
 
 ℹ Unsafe fix
 13 13 |     fruits.clear()
@@ -85,7 +85,7 @@ modified_iterating_set.py:16:1: PLE4703 [*] Iterated set `planets` is being modi
 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.
+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:
@@ -94,7 +94,7 @@ modified_iterating_set.py:20:1: PLE4703 [*] Iterated set `colors` is being modif
 22 |   
 23 |   # OK
    |
-   = help: Consider iterating through a copy of it instead.
+   = help: Iterate over a copy of `colors`
 
 ℹ Unsafe fix
 17 17 |     planets.discard("mercury")