From bf40a37138926cdcd625d3e3a864c93f79716bd4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 30 Mar 2024 19:30:49 -0400 Subject: [PATCH] Tweak some docs --- .../pylint/rules/modified_iterating_set.rs | 61 ++++++++++--------- ...ts__PLE4703_modified_iterating_set.py.snap | 20 +++--- 2 files changed, 41 insertions(+), 40 deletions(-) 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 index 7c572f869f740..fae57ece50912 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/modified_iterating_set.rs +++ b/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,30 +78,27 @@ 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(), @@ -110,9 +106,14 @@ pub(crate) fn modified_iterating_set(checker: &mut Checker, for_stmt: &StmtFor) 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") +} 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 index e8fa5a989dcd5..84c42b5fa591a 100644 --- 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 @@ -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")