From 1fec402e1c40a7f4d2673484426dc3c37c5132c1 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 13 Oct 2024 14:45:58 -0400 Subject: [PATCH 1/4] [`pylint`] - restrict `iteration-over-set` to only work on sets of literals (`PLC0208`) --- .../resources/test/fixtures/pylint/iteration_over_set.py | 6 ++++++ .../src/rules/pylint/rules/iteration_over_set.rs | 5 ++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py index 38a20dcc1ed55..1bd112925bea4 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py @@ -50,3 +50,9 @@ for item in {*numbers_set, 4, 5, 6}: # set unpacking is fine print(f"I like {item}.") + +for item in {1, 2, 3, 4, 5, 6, 2 // 1}: # operations in set literals are fine + print(f"I like {item}.") + +for item in {1, 2, 3, 4, 5, 6, int("7")}: # calls in set literals are fine + print(f"I like {item}.") diff --git a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs index 82023c5ad8320..5d08b06c728ec 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs @@ -46,7 +46,10 @@ pub(crate) fn iteration_over_set(checker: &mut Checker, expr: &Expr) { return; }; - if set.iter().any(Expr::is_starred_expr) { + // Only suggest a fix if all elements are literals. + // This is because we can't determine if the set is used to de-dupe + // the output values of a call, operation, etc. + if !set.iter().all(Expr::is_literal_expr) { return; } From b07a74068aa2cdc3dc46c9ffa3720720f01b5156 Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 17 Oct 2024 23:59:53 -0400 Subject: [PATCH 2/4] only diagnose on unique sets --- .../fixtures/pylint/iteration_over_set.py | 4 ++++ .../rules/pylint/rules/iteration_over_set.rs | 23 ++++++++++++++----- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py index 1bd112925bea4..f8d32e59a572b 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/iteration_over_set.py @@ -56,3 +56,7 @@ for item in {1, 2, 3, 4, 5, 6, int("7")}: # calls in set literals are fine print(f"I like {item}.") + +for item in {1, 2, 2}: # duplicate literals will be ignored + # B033 catches this + print(f"I like {item}.") diff --git a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs index 5d08b06c728ec..7a80fdeedbd3f 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs @@ -1,7 +1,8 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; +use ruff_python_ast::{comparable::ComparableExpr, Expr}; use ruff_text_size::Ranged; +use rustc_hash::FxHashSet; use crate::checkers::ast::Checker; @@ -46,11 +47,21 @@ pub(crate) fn iteration_over_set(checker: &mut Checker, expr: &Expr) { return; }; - // Only suggest a fix if all elements are literals. - // This is because we can't determine if the set is used to de-dupe - // the output values of a call, operation, etc. - if !set.iter().all(Expr::is_literal_expr) { - return; + let mut seen_values: FxHashSet = FxHashSet::default(); + + for value in set { + if value.is_literal_expr() { + let comparable_value = ComparableExpr::from(value); + + if !seen_values.insert(comparable_value) { + // if the set contains a duplicate literal value, early exit. + // rule `B033` can catch that. + return; + } + } else { + // If the set contains a non-literal expression, early exit. + return; + } } let mut diagnostic = Diagnostic::new(IterationOverSet, expr.range()); From de8762d0e638fad20675ec695a880b30564cb8d6 Mon Sep 17 00:00:00 2001 From: Steve C Date: Fri, 18 Oct 2024 02:39:19 -0400 Subject: [PATCH 3/4] optimize! --- .../rules/pylint/rules/iteration_over_set.rs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs index 7a80fdeedbd3f..619c8cda45c0e 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::{comparable::ComparableExpr, Expr}; use ruff_text_size::Ranged; -use rustc_hash::FxHashSet; +use rustc_hash::{FxBuildHasher, FxHashSet}; use crate::checkers::ast::Checker; @@ -47,19 +47,16 @@ pub(crate) fn iteration_over_set(checker: &mut Checker, expr: &Expr) { return; }; - let mut seen_values: FxHashSet = FxHashSet::default(); + if set.iter().any(|value| !value.is_literal_expr()) { + return; + } + let mut seen_values = FxHashSet::with_capacity_and_hasher(set.len(), FxBuildHasher); for value in set { - if value.is_literal_expr() { - let comparable_value = ComparableExpr::from(value); - - if !seen_values.insert(comparable_value) { - // if the set contains a duplicate literal value, early exit. - // rule `B033` can catch that. - return; - } - } else { - // If the set contains a non-literal expression, early exit. + let comparable_value = ComparableExpr::from(value); + if !seen_values.insert(comparable_value) { + // if the set contains a duplicate literal value, early exit. + // rule `B033` can catch that. return; } } From d332727f00cafc52cc85e84d4e93aa13cb94813e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 21 Oct 2024 12:09:00 +0100 Subject: [PATCH 4/4] update docs --- .../ruff_linter/src/rules/pylint/rules/iteration_over_set.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs index 619c8cda45c0e..0abc2d89b0e5a 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/iteration_over_set.rs @@ -7,7 +7,8 @@ use rustc_hash::{FxBuildHasher, FxHashSet}; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for iterations over `set` literals. +/// Checks for iteration over a `set` literal where each element in the set is +/// itself a literal value. /// /// ## Why is this bad? /// Iterating over a `set` is less efficient than iterating over a sequence