From bdff4a66ac516567715c866f7fd412cccbdfeffd Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Thu, 1 Jun 2023 22:26:23 +0100 Subject: [PATCH] Add Pylint rule `C0208` (`use-sequence-for-iteration`) as `PLC0208` (`iteration-over-set`) (#4706) --- .../fixtures/pylint/iteration_over_set.py | 38 ++++++++++ crates/ruff/src/checkers/ast/mod.rs | 18 +++++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/iteration_over_set.rs | 62 ++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ..._tests__PLC0208_iteration_over_set.py.snap | 71 +++++++++++++++++++ ruff.schema.json | 3 + 9 files changed, 197 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/iteration_over_set.py create mode 100644 crates/ruff/src/rules/pylint/rules/iteration_over_set.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0208_iteration_over_set.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/iteration_over_set.py b/crates/ruff/resources/test/fixtures/pylint/iteration_over_set.py new file mode 100644 index 0000000000000..a27313b4d8048 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/iteration_over_set.py @@ -0,0 +1,38 @@ +# Errors + +for item in {"apples", "lemons", "water"}: # flags in-line set literals + print(f"I like {item}.") + +for item in set(("apples", "lemons", "water")): # flags set() calls + print(f"I like {item}.") + +for number in {i for i in range(10)}: # flags set comprehensions + print(number) + +numbers_list = [i for i in {1, 2, 3}] # flags sets in list comprehensions + +numbers_set = {i for i in {1, 2, 3}} # flags sets in set comprehensions + +numbers_dict = {str(i): i for i in {1, 2, 3}} # flags sets in dict comprehensions + +numbers_gen = (i for i in {1, 2, 3}) # flags sets in generator expressions + +# Non-errors + +items = {"apples", "lemons", "water"} +for item in items: # only complains about in-line sets (as per Pylint) + print(f"I like {item}.") + +for item in ["apples", "lemons", "water"]: # lists are fine + print(f"I like {item}.") + +for item in ("apples", "lemons", "water"): # tuples are fine + print(f"I like {item}.") + +numbers_list = [i for i in [1, 2, 3]] # lists in comprehensions are fine + +numbers_set = {i for i in (1, 2, 3)} # tuples in comprehensions are fine + +numbers_dict = {str(i): i for i in [1, 2, 3]} # lists in dict comprehensions are fine + +numbers_gen = (i for i in (1, 2, 3)) # tuples in generator expressions are fine diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 0dbde7739c0e0..b36b3017295df 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1554,6 +1554,9 @@ where if self.enabled(Rule::RedefinedLoopName) { pylint::rules::redefined_loop_name(self, &Node::Stmt(stmt)); } + if self.enabled(Rule::IterationOverSet) { + pylint::rules::iteration_over_set(self, iter); + } if matches!(stmt, Stmt::For(_)) { if self.enabled(Rule::ReimplementedBuiltin) { flake8_simplify::rules::convert_for_loop_to_any_all( @@ -3502,6 +3505,11 @@ where ); } } + if self.enabled(Rule::IterationOverSet) { + for generator in generators { + pylint::rules::iteration_over_set(self, &generator.iter); + } + } } Expr::DictComp(ast::ExprDictComp { key, @@ -3526,6 +3534,11 @@ where ); } } + if self.enabled(Rule::IterationOverSet) { + for generator in generators { + pylint::rules::iteration_over_set(self, &generator.iter); + } + } } Expr::GeneratorExp(ast::ExprGeneratorExp { generators, @@ -3544,6 +3557,11 @@ where ); } } + if self.enabled(Rule::IterationOverSet) { + for generator in generators { + pylint::rules::iteration_over_set(self, &generator.iter); + } + } } Expr::BoolOp(ast::ExprBoolOp { op, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index c5ee5769e6fc7..582a10f0ea948 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -150,6 +150,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0414") => (RuleGroup::Unspecified, Rule::UselessImportAlias), (Pylint, "C1901") => (RuleGroup::Unspecified, Rule::CompareToEmptyString), (Pylint, "C3002") => (RuleGroup::Unspecified, Rule::UnnecessaryDirectLambdaCall), + (Pylint, "C0208") => (RuleGroup::Unspecified, Rule::IterationOverSet), (Pylint, "E0100") => (RuleGroup::Unspecified, Rule::YieldInInit), (Pylint, "E0101") => (RuleGroup::Unspecified, Rule::ReturnInInit), (Pylint, "E0116") => (RuleGroup::Unspecified, Rule::ContinueInFinally), diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 197d0de0a12eb..2f4c024abdfe0 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -163,6 +163,7 @@ ruff_macros::register_rules!( rules::pylint::rules::DuplicateValue, rules::pylint::rules::DuplicateBases, rules::pylint::rules::NamedExprWithoutContext, + rules::pylint::rules::IterationOverSet, // flake8-async rules::flake8_async::rules::BlockingHttpCallInAsyncFunction, rules::flake8_async::rules::OpenSleepOrSubprocessInAsyncFunction, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index c3ac58c2fd7ce..29145a3b3528f 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -64,6 +64,7 @@ mod tests { )] #[test_case(Rule::InvalidEnvvarDefault, Path::new("invalid_envvar_default.py"))] #[test_case(Rule::InvalidEnvvarValue, Path::new("invalid_envvar_value.py"))] + #[test_case(Rule::IterationOverSet, Path::new("iteration_over_set.py"))] #[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"))] diff --git a/crates/ruff/src/rules/pylint/rules/iteration_over_set.rs b/crates/ruff/src/rules/pylint/rules/iteration_over_set.rs new file mode 100644 index 0000000000000..9d76516a68316 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/iteration_over_set.rs @@ -0,0 +1,62 @@ +use rustpython_parser::ast::{Expr, ExprName, Ranged}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for iterations over `set` literals and comprehensions. +/// +/// ## Why is this bad? +/// Iterating over a `set` is less efficient than iterating over a sequence +/// type, like `list` or `tuple`. +/// +/// ## Example +/// ```python +/// for number in {1, 2, 3}: +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// for number in (1, 2, 3): +/// ... +/// ``` +/// +/// ## References +/// - [Python documentation: `set`](https://docs.python.org/3/library/stdtypes.html#set) +#[violation] +pub struct IterationOverSet; + +impl Violation for IterationOverSet { + #[derive_message_formats] + fn message(&self) -> String { + format!("Use a sequence type instead of a `set` when iterating over values") + } +} + +/// PLC0208 +pub(crate) fn iteration_over_set(checker: &mut Checker, expr: &Expr) { + let is_set = match expr { + // Ex) `for i in {1, 2, 3}` + Expr::Set(_) => true, + // Ex)` for i in {n for n in range(1, 4)}` + Expr::SetComp(_) => true, + // Ex) `for i in set(1, 2, 3)` + Expr::Call(call) => { + if let Expr::Name(ExprName { id, .. }) = call.func.as_ref() { + id.as_str() == "set" && checker.semantic_model().is_builtin("set") + } else { + false + } + } + _ => false, + }; + + if is_set { + checker + .diagnostics + .push(Diagnostic::new(IterationOverSet, expr.range())); + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index f59172ad6a277..5a26cdb2ebe40 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -21,6 +21,7 @@ pub(crate) use invalid_string_characters::{ invalid_string_characters, InvalidCharacterBackspace, InvalidCharacterEsc, InvalidCharacterNul, InvalidCharacterSub, InvalidCharacterZeroWidthSpace, }; +pub(crate) use iteration_over_set::{iteration_over_set, IterationOverSet}; pub(crate) use load_before_global_declaration::{ load_before_global_declaration, LoadBeforeGlobalDeclaration, }; @@ -73,6 +74,7 @@ mod invalid_all_object; mod invalid_envvar_default; mod invalid_envvar_value; mod invalid_string_characters; +mod iteration_over_set; mod load_before_global_declaration; mod logging; mod magic_value_comparison; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0208_iteration_over_set.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0208_iteration_over_set.py.snap new file mode 100644 index 0000000000000..5fa6435a0daeb --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0208_iteration_over_set.py.snap @@ -0,0 +1,71 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +iteration_over_set.py:3:13: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +3 | # Errors +4 | +5 | for item in {"apples", "lemons", "water"}: # flags in-line set literals + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0208 +6 | print(f"I like {item}.") + | + +iteration_over_set.py:6:13: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +6 | print(f"I like {item}.") +7 | +8 | for item in set(("apples", "lemons", "water")): # flags set() calls + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLC0208 +9 | print(f"I like {item}.") + | + +iteration_over_set.py:9:15: PLC0208 Use a sequence type instead of a `set` when iterating over values + | + 9 | print(f"I like {item}.") +10 | +11 | for number in {i for i in range(10)}: # flags set comprehensions + | ^^^^^^^^^^^^^^^^^^^^^^ PLC0208 +12 | print(number) + | + +iteration_over_set.py:12:28: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +12 | print(number) +13 | +14 | numbers_list = [i for i in {1, 2, 3}] # flags sets in list comprehensions + | ^^^^^^^^^ PLC0208 +15 | +16 | numbers_set = {i for i in {1, 2, 3}} # flags sets in set comprehensions + | + +iteration_over_set.py:14:27: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +14 | numbers_list = [i for i in {1, 2, 3}] # flags sets in list comprehensions +15 | +16 | numbers_set = {i for i in {1, 2, 3}} # flags sets in set comprehensions + | ^^^^^^^^^ PLC0208 +17 | +18 | numbers_dict = {str(i): i for i in {1, 2, 3}} # flags sets in dict comprehensions + | + +iteration_over_set.py:16:36: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +16 | numbers_set = {i for i in {1, 2, 3}} # flags sets in set comprehensions +17 | +18 | numbers_dict = {str(i): i for i in {1, 2, 3}} # flags sets in dict comprehensions + | ^^^^^^^^^ PLC0208 +19 | +20 | numbers_gen = (i for i in {1, 2, 3}) # flags sets in generator expressions + | + +iteration_over_set.py:18:27: PLC0208 Use a sequence type instead of a `set` when iterating over values + | +18 | numbers_dict = {str(i): i for i in {1, 2, 3}} # flags sets in dict comprehensions +19 | +20 | numbers_gen = (i for i in {1, 2, 3}) # flags sets in generator expressions + | ^^^^^^^^^ PLC0208 +21 | +22 | # Non-errors + | + + diff --git a/ruff.schema.json b/ruff.schema.json index bd19906af2b99..4a863b2d89782 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2006,6 +2006,9 @@ "PL", "PLC", "PLC0", + "PLC02", + "PLC020", + "PLC0208", "PLC04", "PLC041", "PLC0414",