From a3e88edf7db99ff036b89be6e876c4d3ed9d5559 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Mon, 5 Aug 2024 11:25:41 -0500 Subject: [PATCH 1/3] omit set comprehensions from check for sum and update test snapshot --- .../fixtures/flake8_comprehensions/C419.py | 14 +++ .../unnecessary_comprehension_in_call.rs | 111 ++++++++++++++++-- ...8_comprehensions__tests__C419_C419.py.snap | 46 +++++++- 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py index b0a15cf2d6aac..311364095af1e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C419.py @@ -41,3 +41,17 @@ async def f() -> bool: i.bit_count() for i in range(5) # rbracket comment ] # rpar comment ) + +## Set comprehensions should only be linted +## when function is invariant under duplication of inputs + +# should be linted... +any({x.id for x in bar}) +all({x.id for x in bar}) + +# should be linted in preview... +min({x.id for x in bar}) +max({x.id for x in bar}) + +# should not be linted... +sum({x.id for x in bar}) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 0ce5f88f1a3ca..1ccd5d2113bc2 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -1,3 +1,5 @@ +use std::fmt::Display; + use ruff_python_ast::{self as ast, Expr, Keyword}; use ruff_diagnostics::{Diagnostic, FixAvailability}; @@ -11,7 +13,10 @@ use crate::checkers::ast::Checker; use crate::rules::flake8_comprehensions::fixes; /// ## What it does -/// Checks for unnecessary list comprehensions passed to builtin functions that take an iterable. +/// Checks for unnecessary list or set comprehensions passed to builtin functions that take an iterable. +/// +/// Set comprehensions are only a violation in the case where the builtin function does not care about +/// duplication of elements in the passed iterable. /// /// ## Why is this bad? /// Many builtin functions (this rule currently covers `any` and `all` in stable, along with `min`, @@ -65,18 +70,23 @@ use crate::rules::flake8_comprehensions::fixes; /// /// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] -pub struct UnnecessaryComprehensionInCall; +pub struct UnnecessaryComprehensionInCall { + comprehension_kind: ComprehensionKind, +} impl Violation for UnnecessaryComprehensionInCall { const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary list comprehension") + format!("Unnecessary {} comprehension", self.comprehension_kind) } fn fix_title(&self) -> Option { - Some("Remove unnecessary list comprehension".to_string()) + Some(format!( + "Remove unnecessary {} comprehension", + self.comprehension_kind + )) } } @@ -102,18 +112,42 @@ pub(crate) fn unnecessary_comprehension_in_call( if contains_await(elt) { return; } - let Some(builtin_function) = checker.semantic().resolve_builtin_symbol(func) else { + let Some(Ok(builtin_function)) = checker + .semantic() + .resolve_builtin_symbol(func) + .map(SupportedBuiltins::try_from) + else { return; }; - if !(matches!(builtin_function, "any" | "all") - || (checker.settings.preview.is_enabled() - && matches!(builtin_function, "sum" | "min" | "max"))) + if !(matches!( + builtin_function, + SupportedBuiltins::Any | SupportedBuiltins::All + ) || (checker.settings.preview.is_enabled() + && matches!( + builtin_function, + SupportedBuiltins::Sum | SupportedBuiltins::Min | SupportedBuiltins::Max + ))) { return; } - let mut diagnostic = Diagnostic::new(UnnecessaryComprehensionInCall, arg.range()); - + let mut diagnostic = match (arg, builtin_function.duplication_variance()) { + (Expr::ListComp(_), _) => Diagnostic::new( + UnnecessaryComprehensionInCall { + comprehension_kind: ComprehensionKind::List, + }, + arg.range(), + ), + (Expr::SetComp(_), DuplicationVariance::Invariant) => Diagnostic::new( + UnnecessaryComprehensionInCall { + comprehension_kind: ComprehensionKind::Set, + }, + arg.range(), + ), + _ => { + return; + } + }; if args.len() == 1 { // If there's only one argument, remove the list or set brackets. diagnostic.try_set_fix(|| { @@ -144,3 +178,60 @@ pub(crate) fn unnecessary_comprehension_in_call( fn contains_await(expr: &Expr) -> bool { any_over_expr(expr, &Expr::is_await_expr) } + +#[derive(Debug, PartialEq, Eq)] +enum DuplicationVariance { + Invariant, + Variant, +} + +#[derive(Debug, PartialEq, Eq)] +enum ComprehensionKind { + List, + Set, +} + +#[derive(Debug, PartialEq, Eq)] +enum SupportedBuiltins { + All, + Any, + Sum, + Min, + Max, +} +impl TryFrom<&str> for SupportedBuiltins { + type Error = &'static str; + + fn try_from(value: &str) -> Result { + match value { + "all" => Ok(Self::All), + "any" => Ok(Self::Any), + "sum" => Ok(Self::Sum), + "min" => Ok(Self::Min), + "max" => Ok(Self::Max), + _ => Err("Unsupported builtin for `unnecessary-comprehension-in-call`."), + } + } +} + +impl SupportedBuiltins { + fn duplication_variance(&self) -> DuplicationVariance { + match self { + SupportedBuiltins::All + | SupportedBuiltins::Any + | SupportedBuiltins::Min + | SupportedBuiltins::Max => DuplicationVariance::Invariant, + SupportedBuiltins::Sum => DuplicationVariance::Variant, + } + } +} + +impl Display for ComprehensionKind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let text = match self { + ComprehensionKind::List => "list", + ComprehensionKind::Set => "set", + }; + write!(f, "{text}") + } +} diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap index 4f47e3af10fe2..ba01b0c21c316 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap @@ -77,7 +77,7 @@ C419.py:7:5: C419 [*] Unnecessary list comprehension 9 9 | any({x.id for x in bar}) 10 10 | -C419.py:9:5: C419 [*] Unnecessary list comprehension +C419.py:9:5: C419 [*] Unnecessary set comprehension | 7 | [x.id for x in bar], # second comment 8 | ) # third comment @@ -86,7 +86,7 @@ C419.py:9:5: C419 [*] Unnecessary list comprehension 10 | 11 | # OK | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary set comprehension ℹ Unsafe fix 6 6 | all( # first comment @@ -160,3 +160,45 @@ C419.py:39:5: C419 [*] Unnecessary list comprehension 41 |+# second line comment 42 |+i.bit_count() for i in range(5) # rbracket comment # rpar comment 43 43 | ) +44 44 | +45 45 | ## Set comprehensions should only be linted + +C419.py:49:5: C419 [*] Unnecessary set comprehension + | +48 | # should be linted... +49 | any({x.id for x in bar}) + | ^^^^^^^^^^^^^^^^^^^ C419 +50 | all({x.id for x in bar}) + | + = help: Remove unnecessary set comprehension + +ℹ Unsafe fix +46 46 | ## when function is invariant under duplication of inputs +47 47 | +48 48 | # should be linted... +49 |-any({x.id for x in bar}) + 49 |+any(x.id for x in bar) +50 50 | all({x.id for x in bar}) +51 51 | +52 52 | # should be linted in preview... + +C419.py:50:5: C419 [*] Unnecessary set comprehension + | +48 | # should be linted... +49 | any({x.id for x in bar}) +50 | all({x.id for x in bar}) + | ^^^^^^^^^^^^^^^^^^^ C419 +51 | +52 | # should be linted in preview... + | + = help: Remove unnecessary set comprehension + +ℹ Unsafe fix +47 47 | +48 48 | # should be linted... +49 49 | any({x.id for x in bar}) +50 |-all({x.id for x in bar}) + 50 |+all(x.id for x in bar) +51 51 | +52 52 | # should be linted in preview... +53 53 | min({x.id for x in bar}) From ee8bfb5af8919829e85e0a5ab980a5381f618d81 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 5 Aug 2024 22:24:52 -0400 Subject: [PATCH 2/3] Inline kind --- .../unnecessary_comprehension_in_call.rs | 35 ++++++------------- ...8_comprehensions__tests__C419_C419.py.snap | 18 +++++----- ...sions__tests__preview__C419_C419_1.py.snap | 10 +++--- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index 1ccd5d2113bc2..eba4a059b5c00 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -1,15 +1,11 @@ -use std::fmt::Display; - -use ruff_python_ast::{self as ast, Expr, Keyword}; - use ruff_diagnostics::{Diagnostic, FixAvailability}; use ruff_diagnostics::{Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::any_over_expr; +use ruff_python_ast::{self as ast, Expr, Keyword}; use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; - use crate::rules::flake8_comprehensions::fixes; /// ## What it does @@ -79,14 +75,14 @@ impl Violation for UnnecessaryComprehensionInCall { #[derive_message_formats] fn message(&self) -> String { - format!("Unnecessary {} comprehension", self.comprehension_kind) + match self.comprehension_kind { + ComprehensionKind::List => format!("Unnecessary list comprehension"), + ComprehensionKind::Set => format!("Unnecessary set comprehension"), + } } fn fix_title(&self) -> Option { - Some(format!( - "Remove unnecessary {} comprehension", - self.comprehension_kind - )) + Some("Remove unnecessary comprehension".to_string()) } } @@ -179,19 +175,19 @@ fn contains_await(expr: &Expr) -> bool { any_over_expr(expr, &Expr::is_await_expr) } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] enum DuplicationVariance { Invariant, Variant, } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] enum ComprehensionKind { List, Set, } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] enum SupportedBuiltins { All, Any, @@ -199,6 +195,7 @@ enum SupportedBuiltins { Min, Max, } + impl TryFrom<&str> for SupportedBuiltins { type Error = &'static str; @@ -209,7 +206,7 @@ impl TryFrom<&str> for SupportedBuiltins { "sum" => Ok(Self::Sum), "min" => Ok(Self::Min), "max" => Ok(Self::Max), - _ => Err("Unsupported builtin for `unnecessary-comprehension-in-call`."), + _ => Err("Unsupported builtin for `unnecessary-comprehension-in-call`"), } } } @@ -225,13 +222,3 @@ impl SupportedBuiltins { } } } - -impl Display for ComprehensionKind { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let text = match self { - ComprehensionKind::List => "list", - ComprehensionKind::Set => "set", - }; - write!(f, "{text}") - } -} diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap index ba01b0c21c316..d1b04aebaa8bf 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C419_C419.py.snap @@ -8,7 +8,7 @@ C419.py:1:5: C419 [*] Unnecessary list comprehension 2 | all([x.id for x in bar]) 3 | any( # first comment | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 |-any([x.id for x in bar]) @@ -25,7 +25,7 @@ C419.py:2:5: C419 [*] Unnecessary list comprehension 3 | any( # first comment 4 | [x.id for x in bar], # second comment | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 1 | any([x.id for x in bar]) @@ -44,7 +44,7 @@ C419.py:4:5: C419 [*] Unnecessary list comprehension 5 | ) # third comment 6 | all( # first comment | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 1 | any([x.id for x in bar]) @@ -65,7 +65,7 @@ C419.py:7:5: C419 [*] Unnecessary list comprehension 8 | ) # third comment 9 | any({x.id for x in bar}) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 4 4 | [x.id for x in bar], # second comment @@ -86,7 +86,7 @@ C419.py:9:5: C419 [*] Unnecessary set comprehension 10 | 11 | # OK | - = help: Remove unnecessary set comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 6 6 | all( # first comment @@ -113,7 +113,7 @@ C419.py:28:5: C419 [*] Unnecessary list comprehension 34 | # trailing comment 35 | ) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 25 25 | @@ -145,7 +145,7 @@ C419.py:39:5: C419 [*] Unnecessary list comprehension | |_____^ C419 43 | ) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 36 36 | @@ -170,7 +170,7 @@ C419.py:49:5: C419 [*] Unnecessary set comprehension | ^^^^^^^^^^^^^^^^^^^ C419 50 | all({x.id for x in bar}) | - = help: Remove unnecessary set comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 46 46 | ## when function is invariant under duplication of inputs @@ -191,7 +191,7 @@ C419.py:50:5: C419 [*] Unnecessary set comprehension 51 | 52 | # should be linted in preview... | - = help: Remove unnecessary set comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 47 47 | diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap index 1c30178ac47d2..9bc26685fbd88 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__preview__C419_C419_1.py.snap @@ -8,7 +8,7 @@ C419_1.py:1:5: C419 [*] Unnecessary list comprehension 2 | min([x.val for x in bar]) 3 | max([x.val for x in bar]) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 |-sum([x.val for x in bar]) @@ -25,7 +25,7 @@ C419_1.py:2:5: C419 [*] Unnecessary list comprehension 3 | max([x.val for x in bar]) 4 | sum([x.val for x in bar], 0) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 1 | sum([x.val for x in bar]) @@ -43,7 +43,7 @@ C419_1.py:3:5: C419 [*] Unnecessary list comprehension | ^^^^^^^^^^^^^^^^^^^^ C419 4 | sum([x.val for x in bar], 0) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 1 | sum([x.val for x in bar]) @@ -63,7 +63,7 @@ C419_1.py:4:5: C419 [*] Unnecessary list comprehension 5 | 6 | # OK | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 1 1 | sum([x.val for x in bar]) @@ -89,7 +89,7 @@ C419_1.py:14:5: C419 [*] Unnecessary list comprehension 19 | dt.timedelta(), 20 | ) | - = help: Remove unnecessary list comprehension + = help: Remove unnecessary comprehension ℹ Unsafe fix 11 11 | From f1e2e94ea032332978d1abd899f10d195242a6ae Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 5 Aug 2024 22:26:35 -0400 Subject: [PATCH 3/3] Fix Clippy --- .../rules/unnecessary_comprehension_in_call.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs index eba4a059b5c00..6897e224f3bbe 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_comprehension_in_call.rs @@ -212,7 +212,7 @@ impl TryFrom<&str> for SupportedBuiltins { } impl SupportedBuiltins { - fn duplication_variance(&self) -> DuplicationVariance { + fn duplication_variance(self) -> DuplicationVariance { match self { SupportedBuiltins::All | SupportedBuiltins::Any