From f41494d6353f4cc022e45ea818d83ee0343600c3 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sat, 1 Jun 2024 23:53:17 -0500 Subject: [PATCH 01/18] implement-consider-dict-items --- crates/ruff_linter/src/codes.rs | 1 + .../rules/pylint/rules/consider_dict_items.rs | 108 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ruff.schema.json | 1 + 4 files changed, 112 insertions(+) create mode 100644 crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 9c8385ac17d74..87c8fd6c23936 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -222,6 +222,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0131") => (RuleGroup::Stable, rules::pylint::rules::TypeBivariance), (Pylint, "C0132") => (RuleGroup::Stable, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Stable, rules::pylint::rules::SingleStringSlots), + (Pylint, "C0206") => (RuleGroup::Stable, rules::pylint::rules::ConsiderDictItems), (Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet), (Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias), (Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel), diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs new file mode 100644 index 0000000000000..db2b19a39d17f --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -0,0 +1,108 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for usages where +/// a value is extracted through the key of dictionary in an iteration, when it simply be extracted using `.items()` +/// +/// ## Why is this bad? +/// Instead of unnecsarily indexing the the dictionary, it's more semantically clear to extract the value +/// one-per-one with the key, increasing readability. +/// +/// +/// ## Example +/// ```python +/// ORCHESTRA = { +/// "violin": "strings", +/// "oboe": "woodwind", +/// "tuba": "brass", +/// "gong": "percussion", +/// } +/// +/// for instrument in ORCHESTRA: +/// print(f"{instrument}: {ORCHESTRA[instrument]}") +/// ``` +/// +/// Use instead: +/// ```python +/// ORCHESTRA = { +/// "violin": "strings", +/// "oboe": "woodwind", +/// "tuba": "brass", +/// "gong": "percussion", +/// } +/// +/// for instrument, section in ORCHESTRA.items(): +/// print(f"{instrument}: {section}") +/// ``` + +#[violation] +pub struct ConsiderDictItems; + +impl Violation for ConsiderDictItems { + #[derive_message_formats] + fn message(&self) -> String { + format!("Extracting value from dictionary in iteration without calling `.items()`") + } +} + +/// PLC206 +pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { + let ast::StmtFor { + target, + iter, + body, + orelse, + is_async: _, + range: _, + } = stmt_for; + + let Some(iter_obj_name) = iter.as_name_expr() else { + return; + }; + + for child in body { + if let ast::Expr::Subscript(ast::ExprSubscript { + value, + slice, + ctx: _, + range: _, + }) = child + { + if !value.is_name_expr() && !value.is_attribute_expr() { + return; + } + + // Check that the sliced value is the same as the target of the for loop. + let slice_name = slice.as_name_expr(); + let target_name = target.as_name_expr(); + + if slice_name.is_none() || target_name.is_none() { + return; + } + + let slice_name = slice_name.unwrap(); + let target_name = target_name.unwrap(); + + if slice_name.id != target_name.id { + return; + } + + // Check that the sliced dict name is the same as the iterated object name. + if !(slice + .as_name_expr() + .is_some_and(|name| name.id == iter_obj_name.id)) + { + return; + } + + let mut diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); + checker.diagnostics.push(diagnostic); + } else { + return; + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 16f31a9d5a2f1..e8eee8b879c51 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -13,6 +13,7 @@ pub(crate) use collapsible_else_if::*; pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; pub(crate) use comparison_with_itself::*; +pub(crate) use consider_dict_items::*; pub(crate) use continue_in_finally::*; pub(crate) use dict_iter_missing_items::*; pub(crate) use duplicate_bases::*; @@ -115,6 +116,7 @@ mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; +mod consider_dict_items; mod continue_in_finally; mod dict_iter_missing_items; mod duplicate_bases; diff --git a/ruff.schema.json b/ruff.schema.json index b146d9b74bcc2..261117e95eb9c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3232,6 +3232,7 @@ "PLC02", "PLC020", "PLC0205", + "PLC0206", "PLC0208", "PLC04", "PLC041", From 8d92035c076cf187979d06eab7af3d16bf1b0cb7 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 00:09:53 -0500 Subject: [PATCH 02/18] Test --- .../fixtures/pylint/consider_dict_items.py | 23 +++++++++++++++++++ crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/consider_dict_items.rs | 11 ++++++--- 3 files changed, 32 insertions(+), 3 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py new file mode 100644 index 0000000000000..a4e8ada0f6f48 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -0,0 +1,23 @@ +ORCHESTRA = { + "violin": "strings", + "oboe": "woodwind", + "tuba": "brass", + "gong": "percussion", +} + +# Errors +for instrument in ORCHESTRA: + print(f"{instrument}: {ORCHESTRA[instrument]}") + +for instrument in ORCHESTRA.keys(): + print(f"{instrument}: {ORCHESTRA[instrument]}") + +for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + print(f"{instrument}: {temp_orchestra[instrument]}") + +# Non errors +for instrument, section in ORCHESTRA.items(): + print(f"{instrument}: {section}") + +for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): + print(f"{instrument}: {section}") \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index b20b17dac58be..d5e42b94e784e 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -196,6 +196,7 @@ mod tests { #[test_case(Rule::SelfOrClsAssignment, Path::new("self_or_cls_assignment.py"))] #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] + #[test_case(Rule::ConsiderDictItems, Path::new("consider_dict_items.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index db2b19a39d17f..ea18b88ef7d2d 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -55,7 +55,7 @@ pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor target, iter, body, - orelse, + orelse: _, is_async: _, range: _, } = stmt_for; @@ -65,12 +65,17 @@ pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor }; for child in body { + // If it isn't an expression, skip it. + let Some(child_expr) = child.as_expr_stmt() else { + continue; + }; + let expr_value = &child_expr.value; if let ast::Expr::Subscript(ast::ExprSubscript { value, slice, ctx: _, range: _, - }) = child + }) = &**expr_value { if !value.is_name_expr() && !value.is_attribute_expr() { return; @@ -99,7 +104,7 @@ pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor return; } - let mut diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); + let diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); checker.diagnostics.push(diagnostic); } else { return; From 3f8d370e37924ab3f44d452b464eaa0a7f1502cb Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 00:17:01 -0500 Subject: [PATCH 03/18] Fixes --- .../resources/test/fixtures/pylint/consider_dict_items.py | 2 +- crates/ruff_linter/src/codes.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py index a4e8ada0f6f48..fa0f324b00b1f 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -20,4 +20,4 @@ print(f"{instrument}: {section}") for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): - print(f"{instrument}: {section}") \ No newline at end of file + print(f"{instrument}: {section}") diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 87c8fd6c23936..f8f6ecfcf1f27 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -222,7 +222,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0131") => (RuleGroup::Stable, rules::pylint::rules::TypeBivariance), (Pylint, "C0132") => (RuleGroup::Stable, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Stable, rules::pylint::rules::SingleStringSlots), - (Pylint, "C0206") => (RuleGroup::Stable, rules::pylint::rules::ConsiderDictItems), + (Pylint, "C0206") => (RuleGroup::Preview, rules::pylint::rules::ConsiderDictItems), (Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet), (Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias), (Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel), From b13f16d0e553e9e4b916fccf71adbfdf5b765641 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 00:20:19 -0500 Subject: [PATCH 04/18] Fix test --- .../src/checkers/ast/analyze/deferred_for_loops.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index 0aad5987dc1ce..41809dba5bc91 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs @@ -2,7 +2,7 @@ use ruff_python_ast::Stmt; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pyupgrade, refurb}; +use crate::rules::{flake8_bugbear, flake8_simplify, perflint, pylint, pyupgrade, refurb}; /// Run lint rules over all deferred for-loops in the [`SemanticModel`]. pub(crate) fn deferred_for_loops(checker: &mut Checker) { @@ -33,6 +33,9 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { if checker.enabled(Rule::LoopIteratorMutation) { flake8_bugbear::rules::loop_iterator_mutation(checker, stmt_for); } + if checker.enabled(Rule::ConsiderDictItems) { + pylint::rules::consider_dict_items(checker, stmt_for); + } } } } From 7783075fa71804046f8e9a14dd025cd55c20256b Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 00:31:54 -0500 Subject: [PATCH 05/18] Tweaks --- .../resources/test/fixtures/pylint/consider_dict_items.py | 2 +- .../ruff_linter/src/rules/pylint/rules/consider_dict_items.rs | 2 +- ..._rules__pylint__tests__PLC0206_consider_dict_items.py.snap | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py index fa0f324b00b1f..62bfc27e7f586 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -15,7 +15,7 @@ for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): print(f"{instrument}: {temp_orchestra[instrument]}") -# Non errors +# OK for instrument, section in ORCHESTRA.items(): print(f"{instrument}: {section}") diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index ea18b88ef7d2d..3a83178413a93 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -49,7 +49,7 @@ impl Violation for ConsiderDictItems { } } -/// PLC206 +/// PLC0206 pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { let ast::StmtFor { target, diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap new file mode 100644 index 0000000000000..6c123427ab2b8 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- + From 4cbde72028948c5c7c6379a8e66d4e35767fa681 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 01:57:04 -0500 Subject: [PATCH 06/18] Fixes --- .../fixtures/pylint/consider_dict_items.py | 18 ++++ .../src/checkers/ast/analyze/statement.rs | 1 + .../rules/pylint/rules/consider_dict_items.rs | 86 +++++++++++-------- 3 files changed, 70 insertions(+), 35 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py index 62bfc27e7f586..96dfd6f4faafd 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -9,15 +9,33 @@ for instrument in ORCHESTRA: print(f"{instrument}: {ORCHESTRA[instrument]}") +for instrument in ORCHESTRA: + ORCHESTRA[instrument] + for instrument in ORCHESTRA.keys(): print(f"{instrument}: {ORCHESTRA[instrument]}") +for instrument in ORCHESTRA.keys(): + ORCHESTRA[instrument] + for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): print(f"{instrument}: {temp_orchestra[instrument]}") +for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + temp_orchestra[instrument] + # OK for instrument, section in ORCHESTRA.items(): print(f"{instrument}: {section}") +for instrument, section in ORCHESTRA.items(): + section + for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): print(f"{instrument}: {section}") + +for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): + section + +for instrument in ORCHESTRA: + ORCHESTRA[instrument] = 3 diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 94419de40fbb0..934d6b94bf012 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1335,6 +1335,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::UnnecessaryEnumerate, Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, + Rule::ConsiderDictItems, ]) { checker.analyze.for_loops.push(checker.semantic.snapshot()); } diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index 3a83178413a93..de1051060dbda 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -1,6 +1,9 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast}; +use ruff_python_ast::{ + self as ast, + visitor::{self, Visitor}, +}; use crate::checkers::ast::Checker; @@ -49,41 +52,35 @@ impl Violation for ConsiderDictItems { } } -/// PLC0206 -pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { - let ast::StmtFor { - target, - iter, - body, - orelse: _, - is_async: _, - range: _, - } = stmt_for; +struct SubscriptVisitor<'a> { + target: &'a ast::Expr, + iter_obj_name: &'a ast::ExprName, + has_violation: bool, +} - let Some(iter_obj_name) = iter.as_name_expr() else { - return; - }; +impl<'a> SubscriptVisitor<'a> { + fn new(target: &'a ast::Expr, iter_obj_name: &'a ast::ExprName) -> Self { + Self { + target, + iter_obj_name, + has_violation: false, + } + } +} - for child in body { - // If it isn't an expression, skip it. - let Some(child_expr) = child.as_expr_stmt() else { - continue; - }; - let expr_value = &child_expr.value; - if let ast::Expr::Subscript(ast::ExprSubscript { - value, - slice, - ctx: _, - range: _, - }) = &**expr_value - { +impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { + fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { + visitor::walk_stmt(self, stmt); + } + + fn visit_expr(&mut self, expr: &'a ast::Expr) { + if let ast::Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr { if !value.is_name_expr() && !value.is_attribute_expr() { return; } - // Check that the sliced value is the same as the target of the for loop. let slice_name = slice.as_name_expr(); - let target_name = target.as_name_expr(); + let target_name = self.target.as_name_expr(); if slice_name.is_none() || target_name.is_none() { return; @@ -95,19 +92,38 @@ pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor if slice_name.id != target_name.id { return; } - // Check that the sliced dict name is the same as the iterated object name. - if !(slice + if !(value .as_name_expr() - .is_some_and(|name| name.id == iter_obj_name.id)) + .is_some_and(|name| name.id == self.iter_obj_name.id)) { return; } - let diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); - checker.diagnostics.push(diagnostic); + self.has_violation = true; } else { - return; + visitor::walk_expr(self, expr); } } } + +/// PLC0206 +pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { + let ast::StmtFor { + target, iter, body, .. + } = stmt_for; + + let Some(iter_obj_name) = iter.as_name_expr() else { + return; + }; + + let mut visitor = SubscriptVisitor::new(target, iter_obj_name); + for stmt in body { + visitor.visit_stmt(stmt); + } + + if visitor.has_violation { + let diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); + checker.diagnostics.push(diagnostic); + } +} From 0d8354dd069bad894f03798aa80edec04481eda6 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 02:38:13 -0500 Subject: [PATCH 07/18] Fixes --- .../fixtures/pylint/consider_dict_items.py | 2 +- .../rules/pylint/rules/consider_dict_items.rs | 31 ++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py index 96dfd6f4faafd..70648f530a04c 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -24,7 +24,7 @@ for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): temp_orchestra[instrument] -# OK +# # OK for instrument, section in ORCHESTRA.items(): print(f"{instrument}: {section}") diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index de1051060dbda..c98ebe4fbf3cd 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -92,6 +92,7 @@ impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { if slice_name.id != target_name.id { return; } + // Check that the sliced dict name is the same as the iterated object name. if !(value .as_name_expr() @@ -107,13 +108,41 @@ impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { } } +/// Extracts the name of the dictionary from the expression. +fn extract_dict_name(expr: &ast::Expr) -> Option<&ast::ExprName> { + if let Some(name_expr) = expr.as_name_expr() { + return Some(name_expr); + } + + // Handle `dict.keys()` case. + if let ast::Expr::Call(ast::ExprCall { func, .. }) = expr { + if let ast::Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() { + if attr == "keys" { + if let ast::Expr::Name(var_name) = value.as_ref() { + return Some(var_name); + } + } + } + } + + // Handle `my_dict := {"foo": "bar"}` case. + if let ast::Expr::Named(ast::ExprNamed { target, value, .. }) = expr { + if let ast::Expr::Dict(ast::ExprDict { .. }) = value.as_ref() { + if let ast::Expr::Name(var_name) = target.as_ref() { + return Some(var_name); + } + } + } + None +} + /// PLC0206 pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { let ast::StmtFor { target, iter, body, .. } = stmt_for; - let Some(iter_obj_name) = iter.as_name_expr() else { + let Some(iter_obj_name) = extract_dict_name(&iter) else { return; }; From 8e55fb3897e5378adbf33415cb57c454d3a7b7f8 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 02:42:29 -0500 Subject: [PATCH 08/18] Finalize --- .../src/rules/pylint/rules/consider_dict_items.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index c98ebe4fbf3cd..ca23b0af39168 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -70,6 +70,14 @@ impl<'a> SubscriptVisitor<'a> { impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { + // Skip if the assignment is a subscript expression. + if let ast::Stmt::Assign(assign) = stmt { + if let Some(target) = assign.targets.first() { + if target.is_subscript_expr() { + return; + } + } + } visitor::walk_stmt(self, stmt); } From 183bc52711d0ed666bf2fdb009cadd3cebfb0883 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:03:29 -0500 Subject: [PATCH 09/18] Finalize --- .../fixtures/pylint/consider_dict_items.py | 10 ++++++++ .../rules/pylint/rules/consider_dict_items.rs | 25 ++++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py index 70648f530a04c..71697bec6b498 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py @@ -39,3 +39,13 @@ for instrument in ORCHESTRA: ORCHESTRA[instrument] = 3 + + +# Shouldn't trigger for non-dict types +items = {1, 2, 3, 4} +for i in items: + items[i] + +items = [1, 2, 3, 4] +for i in items: + items[i] diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index ca23b0af39168..d5752ffe214c6 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -4,6 +4,8 @@ use ruff_python_ast::{ self as ast, visitor::{self, Visitor}, }; +use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; +use ruff_python_semantic::analyze::typing::is_dict; use crate::checkers::ast::Checker; @@ -144,15 +146,36 @@ fn extract_dict_name(expr: &ast::Expr) -> Option<&ast::ExprName> { None } +fn is_inferred_dict(name: &ast::ExprName, checker: &Checker) -> bool { + let binding = checker + .semantic() + .only_binding(name) + .map(|id| checker.semantic().binding(id)); + if binding.is_none() { + return false; + } + let binding = binding.unwrap(); + is_dict(binding, checker.semantic()) +} + /// PLC0206 pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { let ast::StmtFor { target, iter, body, .. } = stmt_for; - let Some(iter_obj_name) = extract_dict_name(&iter) else { + // Check if the right hand side is a dict literal (i.e. `for key in (dict := {"a": 1}):`). + let is_dict_literal = matches!( + ResolvedPythonType::from(&**iter), + ResolvedPythonType::Atom(PythonType::Dict), + ); + + let Some(iter_obj_name) = extract_dict_name(iter) else { return; }; + if !is_inferred_dict(iter_obj_name, checker) && !is_dict_literal { + return; + } let mut visitor = SubscriptVisitor::new(target, iter_obj_name); for stmt in body { From 520c8eb2ceb67ac6b58dd49f8f423113a8e97c6c Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:06:30 -0500 Subject: [PATCH 10/18] Implement checks --- ...tests__PLC0206_consider_dict_items.py.snap | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap index 6c123427ab2b8..7cd5862e54cd1 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap @@ -1,4 +1,67 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- +consider_dict_items.py:9:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | + 8 | # Errors + 9 | / for instrument in ORCHESTRA: +10 | | print(f"{instrument}: {ORCHESTRA[instrument]}") + | |___________________________________________________^ PLC0206 +11 | +12 | for instrument in ORCHESTRA: + | +consider_dict_items.py:12:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | +10 | print(f"{instrument}: {ORCHESTRA[instrument]}") +11 | +12 | / for instrument in ORCHESTRA: +13 | | ORCHESTRA[instrument] + | |_________________________^ PLC0206 +14 | +15 | for instrument in ORCHESTRA.keys(): + | + +consider_dict_items.py:15:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | +13 | ORCHESTRA[instrument] +14 | +15 | / for instrument in ORCHESTRA.keys(): +16 | | print(f"{instrument}: {ORCHESTRA[instrument]}") + | |___________________________________________________^ PLC0206 +17 | +18 | for instrument in ORCHESTRA.keys(): + | + +consider_dict_items.py:18:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | +16 | print(f"{instrument}: {ORCHESTRA[instrument]}") +17 | +18 | / for instrument in ORCHESTRA.keys(): +19 | | ORCHESTRA[instrument] + | |_________________________^ PLC0206 +20 | +21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + | + +consider_dict_items.py:21:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | +19 | ORCHESTRA[instrument] +20 | +21 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): +22 | | print(f"{instrument}: {temp_orchestra[instrument]}") + | |________________________________________________________^ PLC0206 +23 | +24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): + | + +consider_dict_items.py:24:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` + | +22 | print(f"{instrument}: {temp_orchestra[instrument]}") +23 | +24 | / for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): +25 | | temp_orchestra[instrument] + | |______________________________^ PLC0206 +26 | +27 | # # OK + | From 7efde9d8adb7b2c1126e0562adc50bcb05e6ece0 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:12:28 -0500 Subject: [PATCH 11/18] Clarify example --- .../ruff_linter/src/rules/pylint/rules/consider_dict_items.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs index d5752ffe214c6..4e7cc110805ea 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs @@ -10,8 +10,8 @@ use ruff_python_semantic::analyze::typing::is_dict; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for usages where -/// a value is extracted through the key of dictionary in an iteration, when it simply be extracted using `.items()` +/// Checks when iterating over keys of a dictionary and extracting the value from the dictionary +/// through indexing the key, instead of calling `.items()` on the dictionary. /// /// ## Why is this bad? /// Instead of unnecsarily indexing the the dictionary, it's more semantically clear to extract the value From 92eb527a3b6a9d8c2001fe208b23d3d1ea371d18 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:22:17 -0500 Subject: [PATCH 12/18] Fixes --- ...der_dict_items.py => dict_index_missing_items.py} | 0 .../src/checkers/ast/analyze/deferred_for_loops.rs | 4 ++-- .../src/checkers/ast/analyze/statement.rs | 2 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/pylint/mod.rs | 2 +- ...der_dict_items.rs => dict_index_missing_items.rs} | 8 ++++---- crates/ruff_linter/src/rules/pylint/rules/mod.rs | 4 ++-- ...ylint__tests__PLC0206_consider_dict_items.py.snap | 12 ++++++------ 8 files changed, 17 insertions(+), 17 deletions(-) rename crates/ruff_linter/resources/test/fixtures/pylint/{consider_dict_items.py => dict_index_missing_items.py} (100%) rename crates/ruff_linter/src/rules/pylint/rules/{consider_dict_items.rs => dict_index_missing_items.rs} (95%) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/pylint/consider_dict_items.py rename to crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs index 41809dba5bc91..35fe8168e0eaa 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_for_loops.rs @@ -33,8 +33,8 @@ pub(crate) fn deferred_for_loops(checker: &mut Checker) { if checker.enabled(Rule::LoopIteratorMutation) { flake8_bugbear::rules::loop_iterator_mutation(checker, stmt_for); } - if checker.enabled(Rule::ConsiderDictItems) { - pylint::rules::consider_dict_items(checker, stmt_for); + if checker.enabled(Rule::DictIndexMissingItems) { + pylint::rules::dict_index_missing_items(checker, stmt_for); } } } diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 934d6b94bf012..f597767ee5fda 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1335,7 +1335,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { Rule::UnnecessaryEnumerate, Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, - Rule::ConsiderDictItems, + Rule::DictIndexMissingItems, ]) { checker.analyze.for_loops.push(checker.semantic.snapshot()); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index f8f6ecfcf1f27..32af0c5509a2c 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -222,7 +222,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0131") => (RuleGroup::Stable, rules::pylint::rules::TypeBivariance), (Pylint, "C0132") => (RuleGroup::Stable, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Stable, rules::pylint::rules::SingleStringSlots), - (Pylint, "C0206") => (RuleGroup::Preview, rules::pylint::rules::ConsiderDictItems), + (Pylint, "C0206") => (RuleGroup::Preview, rules::pylint::rules::DictIndexMissingItems), (Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet), (Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias), (Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index d5e42b94e784e..c65624ae576e6 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -195,8 +195,8 @@ mod tests { #[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))] #[test_case(Rule::SelfOrClsAssignment, Path::new("self_or_cls_assignment.py"))] #[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))] + #[test_case(Rule::DictIndexMissingItems, Path::new("dict_index_missing_items.py"))] #[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))] - #[test_case(Rule::ConsiderDictItems, Path::new("consider_dict_items.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs similarity index 95% rename from crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs rename to crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs index 4e7cc110805ea..66889b0efeb82 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/consider_dict_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs @@ -45,9 +45,9 @@ use crate::checkers::ast::Checker; /// ``` #[violation] -pub struct ConsiderDictItems; +pub struct DictIndexMissingItems; -impl Violation for ConsiderDictItems { +impl Violation for DictIndexMissingItems { #[derive_message_formats] fn message(&self) -> String { format!("Extracting value from dictionary in iteration without calling `.items()`") @@ -159,7 +159,7 @@ fn is_inferred_dict(name: &ast::ExprName, checker: &Checker) -> bool { } /// PLC0206 -pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { +pub(crate) fn dict_index_missing_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { let ast::StmtFor { target, iter, body, .. } = stmt_for; @@ -183,7 +183,7 @@ pub(crate) fn consider_dict_items(checker: &mut Checker, stmt_for: &ast::StmtFor } if visitor.has_violation { - let diagnostic = Diagnostic::new(ConsiderDictItems, stmt_for.range); + let diagnostic = Diagnostic::new(DictIndexMissingItems, stmt_for.range); checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index e8eee8b879c51..620c0abd4d858 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -13,7 +13,6 @@ pub(crate) use collapsible_else_if::*; pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; pub(crate) use comparison_with_itself::*; -pub(crate) use consider_dict_items::*; pub(crate) use continue_in_finally::*; pub(crate) use dict_iter_missing_items::*; pub(crate) use duplicate_bases::*; @@ -93,6 +92,7 @@ pub(crate) use unnecessary_dunder_call::*; pub(crate) use unnecessary_lambda::*; pub(crate) use unnecessary_list_index_lookup::*; pub(crate) use unspecified_encoding::*; +pub(crate) use dict_index_missing_items::*; pub(crate) use useless_else_on_loop::*; pub(crate) use useless_exception_statement::*; pub(crate) use useless_import_alias::*; @@ -116,7 +116,6 @@ mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; -mod consider_dict_items; mod continue_in_finally; mod dict_iter_missing_items; mod duplicate_bases; @@ -196,6 +195,7 @@ mod unnecessary_dunder_call; mod unnecessary_lambda; mod unnecessary_list_index_lookup; mod unspecified_encoding; +mod dict_index_missing_items; mod useless_else_on_loop; mod useless_exception_statement; mod useless_import_alias; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap index 7cd5862e54cd1..c12db45610fa5 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -consider_dict_items.py:9:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:9:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 8 | # Errors 9 | / for instrument in ORCHESTRA: @@ -11,7 +11,7 @@ consider_dict_items.py:9:1: PLC0206 Extracting value from dictionary in iteratio 12 | for instrument in ORCHESTRA: | -consider_dict_items.py:12:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:12:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 10 | print(f"{instrument}: {ORCHESTRA[instrument]}") 11 | @@ -22,7 +22,7 @@ consider_dict_items.py:12:1: PLC0206 Extracting value from dictionary in iterati 15 | for instrument in ORCHESTRA.keys(): | -consider_dict_items.py:15:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:15:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 13 | ORCHESTRA[instrument] 14 | @@ -33,7 +33,7 @@ consider_dict_items.py:15:1: PLC0206 Extracting value from dictionary in iterati 18 | for instrument in ORCHESTRA.keys(): | -consider_dict_items.py:18:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:18:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 16 | print(f"{instrument}: {ORCHESTRA[instrument]}") 17 | @@ -44,7 +44,7 @@ consider_dict_items.py:18:1: PLC0206 Extracting value from dictionary in iterati 21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | -consider_dict_items.py:21:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:21:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 19 | ORCHESTRA[instrument] 20 | @@ -55,7 +55,7 @@ consider_dict_items.py:21:1: PLC0206 Extracting value from dictionary in iterati 24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | -consider_dict_items.py:24:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:24:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` | 22 | print(f"{instrument}: {temp_orchestra[instrument]}") 23 | From e3671c6cc78bac267b3e61473e1226d88033ef2b Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:25:43 -0500 Subject: [PATCH 13/18] Linting fix --- crates/ruff_linter/src/rules/pylint/rules/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 620c0abd4d858..53f808be427b0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -14,6 +14,7 @@ pub(crate) use compare_to_empty_string::*; pub(crate) use comparison_of_constant::*; pub(crate) use comparison_with_itself::*; pub(crate) use continue_in_finally::*; +pub(crate) use dict_index_missing_items::*; pub(crate) use dict_iter_missing_items::*; pub(crate) use duplicate_bases::*; pub(crate) use empty_comment::*; @@ -92,7 +93,6 @@ pub(crate) use unnecessary_dunder_call::*; pub(crate) use unnecessary_lambda::*; pub(crate) use unnecessary_list_index_lookup::*; pub(crate) use unspecified_encoding::*; -pub(crate) use dict_index_missing_items::*; pub(crate) use useless_else_on_loop::*; pub(crate) use useless_exception_statement::*; pub(crate) use useless_import_alias::*; @@ -117,6 +117,7 @@ mod compare_to_empty_string; mod comparison_of_constant; mod comparison_with_itself; mod continue_in_finally; +mod dict_index_missing_items; mod dict_iter_missing_items; mod duplicate_bases; mod empty_comment; @@ -195,7 +196,6 @@ mod unnecessary_dunder_call; mod unnecessary_lambda; mod unnecessary_list_index_lookup; mod unspecified_encoding; -mod dict_index_missing_items; mod useless_else_on_loop; mod useless_exception_statement; mod useless_import_alias; From a06bc0960c05dac2e67900da98d385f2d1cb06c3 Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:27:50 -0500 Subject: [PATCH 14/18] Add snapshot --- ...ules__pylint__tests__PLC0206_dict_index_missing_items.py.snap} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename crates/ruff_linter/src/rules/pylint/snapshots/{ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap => ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap} (100%) diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap similarity index 100% rename from crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_consider_dict_items.py.snap rename to crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap From f3e9940c6c1c46f5eae0caee8db3f0b150db293b Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:28:55 -0500 Subject: [PATCH 15/18] Fix docs --- .../src/rules/pylint/rules/dict_index_missing_items.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs index 66889b0efeb82..4367ae57f8203 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs @@ -14,8 +14,8 @@ use crate::checkers::ast::Checker; /// through indexing the key, instead of calling `.items()` on the dictionary. /// /// ## Why is this bad? -/// Instead of unnecsarily indexing the the dictionary, it's more semantically clear to extract the value -/// one-per-one with the key, increasing readability. +/// Instead of unnecsarily unnecessarily the the dictionary, it's semantically clearer to extract the value +/// one-per-one with the key with `.items()`, increasing readability. /// /// /// ## Example From 0faead572690d1d4d7a788ea7258cc9ba0eb0bda Mon Sep 17 00:00:00 2001 From: Maxwell Muoto <41130755+max-muoto@users.noreply.github.com> Date: Sun, 2 Jun 2024 11:29:05 -0500 Subject: [PATCH 16/18] Fix --- .../src/rules/pylint/rules/dict_index_missing_items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs index 4367ae57f8203..c598e08ecafc7 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs @@ -14,7 +14,7 @@ use crate::checkers::ast::Checker; /// through indexing the key, instead of calling `.items()` on the dictionary. /// /// ## Why is this bad? -/// Instead of unnecsarily unnecessarily the the dictionary, it's semantically clearer to extract the value +/// Instead of unnecsarily indexing the the dictionary, it's semantically clearer to extract the value /// one-per-one with the key with `.items()`, increasing readability. /// /// From 809dccce510e29b5574df32f6293a58f7a9ded75 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 6 Jun 2024 00:07:20 -0400 Subject: [PATCH 17/18] Format --- .../test/fixtures/pylint/dict_index_missing_items.py | 8 ++++++-- crates/ruff_linter/src/checkers/ast/analyze/statement.rs | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py b/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py index 71697bec6b498..35bc2b0f897d7 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/dict_index_missing_items.py @@ -31,10 +31,14 @@ for instrument, section in ORCHESTRA.items(): section -for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): +for instrument, section in ( + temp_orchestra := {"violin": "strings", "oboe": "woodwind"} +).items(): print(f"{instrument}: {section}") -for instrument, section in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}).items(): +for instrument, section in ( + temp_orchestra := {"violin": "strings", "oboe": "woodwind"} +).items(): section for instrument in ORCHESTRA: diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index f597767ee5fda..c07f41c77d497 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1329,13 +1329,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { pylint::rules::too_many_nested_blocks(checker, stmt); } if checker.any_enabled(&[ + Rule::DictIndexMissingItems, Rule::EnumerateForLoop, Rule::IncorrectDictIterator, Rule::LoopIteratorMutation, Rule::UnnecessaryEnumerate, Rule::UnusedLoopControlVariable, Rule::YieldInForLoop, - Rule::DictIndexMissingItems, ]) { checker.analyze.for_loops.push(checker.semantic.snapshot()); } From 9d7d527ada48ae7e8e1aa2c09979ccf2fead83e5 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 6 Jun 2024 00:20:21 -0400 Subject: [PATCH 18/18] Simplify visitor --- .../pylint/rules/dict_index_missing_items.rs | 175 +++++++++--------- .../pylint/rules/dict_iter_missing_items.rs | 4 +- ...__PLC0206_dict_index_missing_items.py.snap | 12 +- 3 files changed, 92 insertions(+), 99 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs index c598e08ecafc7..bdedf21f58c54 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_index_missing_items.rs @@ -1,22 +1,25 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::{ self as ast, visitor::{self, Visitor}, + Expr, ExprContext, }; use ruff_python_semantic::analyze::type_inference::{PythonType, ResolvedPythonType}; use ruff_python_semantic::analyze::typing::is_dict; +use ruff_python_semantic::SemanticModel; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks when iterating over keys of a dictionary and extracting the value from the dictionary -/// through indexing the key, instead of calling `.items()` on the dictionary. +/// Checks for dictionary iterations that extract the dictionary value +/// via explicit indexing, instead of using `.items()`. /// /// ## Why is this bad? -/// Instead of unnecsarily indexing the the dictionary, it's semantically clearer to extract the value -/// one-per-one with the key with `.items()`, increasing readability. -/// +/// Iterating over a dictionary with `.items()` is semantically clearer +/// and more efficient than extracting the value with the key. /// /// ## Example /// ```python @@ -50,64 +53,86 @@ pub struct DictIndexMissingItems; impl Violation for DictIndexMissingItems { #[derive_message_formats] fn message(&self) -> String { - format!("Extracting value from dictionary in iteration without calling `.items()`") + format!("Extracting value from dictionary without calling `.items()`") + } +} + +/// PLC0206 +pub(crate) fn dict_index_missing_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { + let ast::StmtFor { + target, iter, body, .. + } = stmt_for; + + // Extract the name of the iteration object (e.g., `obj` in `for key in obj:`). + let Some(dict_name) = extract_dict_name(iter) else { + return; + }; + + // Determine if the right-hand side is a dictionary literal (i.e. `for key in (dict := {"a": 1}):`). + let is_dict_literal = matches!( + ResolvedPythonType::from(&**iter), + ResolvedPythonType::Atom(PythonType::Dict), + ); + + if !is_dict_literal && !is_inferred_dict(dict_name, checker.semantic()) { + return; + } + + let has_violation = { + let mut visitor = SubscriptVisitor::new(target, dict_name); + for stmt in body { + visitor.visit_stmt(stmt); + } + visitor.has_violation + }; + + if has_violation { + let diagnostic = Diagnostic::new(DictIndexMissingItems, stmt_for.range()); + checker.diagnostics.push(diagnostic); } } +/// A visitor to detect subscript operations on a target dictionary. struct SubscriptVisitor<'a> { - target: &'a ast::Expr, - iter_obj_name: &'a ast::ExprName, + /// The target of the for loop (e.g., `key` in `for key in obj:`). + target: &'a Expr, + /// The name of the iterated object (e.g., `obj` in `for key in obj:`). + dict_name: &'a ast::ExprName, + /// Whether a violation has been detected. has_violation: bool, } impl<'a> SubscriptVisitor<'a> { - fn new(target: &'a ast::Expr, iter_obj_name: &'a ast::ExprName) -> Self { + fn new(target: &'a Expr, dict_name: &'a ast::ExprName) -> Self { Self { target, - iter_obj_name, + dict_name, has_violation: false, } } } -impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { - fn visit_stmt(&mut self, stmt: &'a ast::Stmt) { - // Skip if the assignment is a subscript expression. - if let ast::Stmt::Assign(assign) = stmt { - if let Some(target) = assign.targets.first() { - if target.is_subscript_expr() { - return; - } - } - } - visitor::walk_stmt(self, stmt); - } - - fn visit_expr(&mut self, expr: &'a ast::Expr) { - if let ast::Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr { - if !value.is_name_expr() && !value.is_attribute_expr() { +impl<'a> Visitor<'a> for SubscriptVisitor<'a> { + fn visit_expr(&mut self, expr: &'a Expr) { + // Given `obj[key]`, `value` must be `obj` and `slice` must be `key`. + if let Expr::Subscript(ast::ExprSubscript { + value, + slice, + ctx: ExprContext::Load, + .. + }) = expr + { + let Expr::Name(name) = value.as_ref() else { return; - } - // Check that the sliced value is the same as the target of the for loop. - let slice_name = slice.as_name_expr(); - let target_name = self.target.as_name_expr(); - - if slice_name.is_none() || target_name.is_none() { - return; - } + }; - let slice_name = slice_name.unwrap(); - let target_name = target_name.unwrap(); - - if slice_name.id != target_name.id { + // Check that the sliced dictionary name is the same as the iterated object name. + if name.id != self.dict_name.id { return; } - // Check that the sliced dict name is the same as the iterated object name. - if !(value - .as_name_expr() - .is_some_and(|name| name.id == self.iter_obj_name.id)) - { + // Check that the sliced value is the same as the target of the `for` loop. + if ComparableExpr::from(slice) != ComparableExpr::from(self.target) { return; } @@ -119,71 +144,39 @@ impl<'a> visitor::Visitor<'a> for SubscriptVisitor<'a> { } /// Extracts the name of the dictionary from the expression. -fn extract_dict_name(expr: &ast::Expr) -> Option<&ast::ExprName> { +fn extract_dict_name(expr: &Expr) -> Option<&ast::ExprName> { + // Ex) `for key in obj:` if let Some(name_expr) = expr.as_name_expr() { return Some(name_expr); } - // Handle `dict.keys()` case. - if let ast::Expr::Call(ast::ExprCall { func, .. }) = expr { - if let ast::Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() { + // Ex) `for key in obj.keys():` + if let Expr::Call(ast::ExprCall { func, .. }) = expr { + if let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() { if attr == "keys" { - if let ast::Expr::Name(var_name) = value.as_ref() { + if let Expr::Name(var_name) = value.as_ref() { return Some(var_name); } } } } - // Handle `my_dict := {"foo": "bar"}` case. - if let ast::Expr::Named(ast::ExprNamed { target, value, .. }) = expr { - if let ast::Expr::Dict(ast::ExprDict { .. }) = value.as_ref() { - if let ast::Expr::Name(var_name) = target.as_ref() { + // Ex) `for key in (my_dict := {"foo": "bar"}):` + if let Expr::Named(ast::ExprNamed { target, value, .. }) = expr { + if let Expr::Dict(ast::ExprDict { .. }) = value.as_ref() { + if let Expr::Name(var_name) = target.as_ref() { return Some(var_name); } } } + None } -fn is_inferred_dict(name: &ast::ExprName, checker: &Checker) -> bool { - let binding = checker - .semantic() +/// Returns `true` if the binding is a dictionary, inferred from the type. +fn is_inferred_dict(name: &ast::ExprName, semantic: &SemanticModel) -> bool { + semantic .only_binding(name) - .map(|id| checker.semantic().binding(id)); - if binding.is_none() { - return false; - } - let binding = binding.unwrap(); - is_dict(binding, checker.semantic()) -} - -/// PLC0206 -pub(crate) fn dict_index_missing_items(checker: &mut Checker, stmt_for: &ast::StmtFor) { - let ast::StmtFor { - target, iter, body, .. - } = stmt_for; - - // Check if the right hand side is a dict literal (i.e. `for key in (dict := {"a": 1}):`). - let is_dict_literal = matches!( - ResolvedPythonType::from(&**iter), - ResolvedPythonType::Atom(PythonType::Dict), - ); - - let Some(iter_obj_name) = extract_dict_name(iter) else { - return; - }; - if !is_inferred_dict(iter_obj_name, checker) && !is_dict_literal { - return; - } - - let mut visitor = SubscriptVisitor::new(target, iter_obj_name); - for stmt in body { - visitor.visit_stmt(stmt); - } - - if visitor.has_violation { - let diagnostic = Diagnostic::new(DictIndexMissingItems, stmt_for.range); - checker.diagnostics.push(diagnostic); - } + .map(|id| semantic.binding(id)) + .is_some_and(|binding| is_dict(binding, semantic)) } diff --git a/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs b/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs index 28870dda84c7d..03016aa5ca0d1 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/dict_iter_missing_items.rs @@ -73,7 +73,7 @@ pub(crate) fn dict_iter_missing_items(checker: &mut Checker, target: &Expr, iter } // If we can reliably determine that a dictionary has keys that are tuples of two we don't warn - if is_dict_key_tuple_with_two_elements(checker.semantic(), binding) { + if is_dict_key_tuple_with_two_elements(binding, checker.semantic()) { return; } @@ -86,7 +86,7 @@ pub(crate) fn dict_iter_missing_items(checker: &mut Checker, target: &Expr, iter } /// Returns true if the binding is a dictionary where each key is a tuple with two elements. -fn is_dict_key_tuple_with_two_elements(semantic: &SemanticModel, binding: &Binding) -> bool { +fn is_dict_key_tuple_with_two_elements(binding: &Binding, semantic: &SemanticModel) -> bool { let Some(statement) = binding.statement(semantic) else { return false; }; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap index c12db45610fa5..49e59491f1f25 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLC0206_dict_index_missing_items.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -dict_index_missing_items.py:9:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:9:1: PLC0206 Extracting value from dictionary without calling `.items()` | 8 | # Errors 9 | / for instrument in ORCHESTRA: @@ -11,7 +11,7 @@ dict_index_missing_items.py:9:1: PLC0206 Extracting value from dictionary in ite 12 | for instrument in ORCHESTRA: | -dict_index_missing_items.py:12:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:12:1: PLC0206 Extracting value from dictionary without calling `.items()` | 10 | print(f"{instrument}: {ORCHESTRA[instrument]}") 11 | @@ -22,7 +22,7 @@ dict_index_missing_items.py:12:1: PLC0206 Extracting value from dictionary in it 15 | for instrument in ORCHESTRA.keys(): | -dict_index_missing_items.py:15:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:15:1: PLC0206 Extracting value from dictionary without calling `.items()` | 13 | ORCHESTRA[instrument] 14 | @@ -33,7 +33,7 @@ dict_index_missing_items.py:15:1: PLC0206 Extracting value from dictionary in it 18 | for instrument in ORCHESTRA.keys(): | -dict_index_missing_items.py:18:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:18:1: PLC0206 Extracting value from dictionary without calling `.items()` | 16 | print(f"{instrument}: {ORCHESTRA[instrument]}") 17 | @@ -44,7 +44,7 @@ dict_index_missing_items.py:18:1: PLC0206 Extracting value from dictionary in it 21 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | -dict_index_missing_items.py:21:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:21:1: PLC0206 Extracting value from dictionary without calling `.items()` | 19 | ORCHESTRA[instrument] 20 | @@ -55,7 +55,7 @@ dict_index_missing_items.py:21:1: PLC0206 Extracting value from dictionary in it 24 | for instrument in (temp_orchestra := {"violin": "strings", "oboe": "woodwind"}): | -dict_index_missing_items.py:24:1: PLC0206 Extracting value from dictionary in iteration without calling `.items()` +dict_index_missing_items.py:24:1: PLC0206 Extracting value from dictionary without calling `.items()` | 22 | print(f"{instrument}: {temp_orchestra[instrument]}") 23 |