From 009430e0342dca2cb181ec2f8d69f87a6d3d1c1a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 12 Jan 2024 14:33:45 -0500 Subject: [PATCH] [`ruff`] Avoid treating named expressions as static keys (`RUF011`) (#9494) Closes https://github.com/astral-sh/ruff/issues/9487. --- .../resources/test/fixtures/ruff/RUF011.py | 4 + .../rules/unused_loop_control_variable.rs | 4 +- .../rules/static_key_dict_comprehension.rs | 6 +- ..._rules__ruff__tests__RUF011_RUF011.py.snap | 98 ++++++++++--------- crates/ruff_python_ast/src/helpers.rs | 21 ++++ 5 files changed, 84 insertions(+), 49 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF011.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF011.py index dec601bd685aa..bbd9082486d96 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF011.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF011.py @@ -10,6 +10,8 @@ {value.attribute: value.upper() for value in data for constant in data} {constant[value]: value.upper() for value in data for constant in data} {value[constant]: value.upper() for value in data for constant in data} +{local_id: token for token in tokens if (local_id := _extract_local_id(token)) is not None} +{key: kwargs.get(key) for key in kwargs.keys() if not params.get(key)} # Errors {"key": value.upper() for value in data} @@ -20,3 +22,5 @@ {constant + constant: value.upper() for value in data} {constant.attribute: value.upper() for value in data} {constant[0]: value.upper() for value in data} +{tokens: token for token in tokens} + diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs index 58c037e2c1750..709aef683a0a1 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/unused_loop_control_variable.rs @@ -2,7 +2,7 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::helpers; -use ruff_python_ast::helpers::NameFinder; +use ruff_python_ast::helpers::{NameFinder, StoredNameFinder}; use ruff_python_ast::visitor::Visitor; use ruff_text_size::Ranged; @@ -80,7 +80,7 @@ impl Violation for UnusedLoopControlVariable { /// B007 pub(crate) fn unused_loop_control_variable(checker: &mut Checker, stmt_for: &ast::StmtFor) { let control_names = { - let mut finder = NameFinder::default(); + let mut finder = StoredNameFinder::default(); finder.visit_expr(stmt_for.target.as_ref()); finder.names }; diff --git a/crates/ruff_linter/src/rules/ruff/rules/static_key_dict_comprehension.rs b/crates/ruff_linter/src/rules/ruff/rules/static_key_dict_comprehension.rs index cdcc672105428..1da5864e0927d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/static_key_dict_comprehension.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/static_key_dict_comprehension.rs @@ -2,7 +2,7 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::helpers::NameFinder; +use ruff_python_ast::helpers::StoredNameFinder; use ruff_python_ast::visitor::Visitor; use ruff_python_ast::{self as ast, Expr}; use ruff_text_size::Ranged; @@ -51,9 +51,9 @@ impl Violation for StaticKeyDictComprehension { pub(crate) fn static_key_dict_comprehension(checker: &mut Checker, dict_comp: &ast::ExprDictComp) { // Collect the bound names in the comprehension's generators. let names = { - let mut visitor = NameFinder::default(); + let mut visitor = StoredNameFinder::default(); for generator in &dict_comp.generators { - visitor.visit_expr(&generator.target); + visitor.visit_comprehension(generator); } visitor.names }; diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF011_RUF011.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF011_RUF011.py.snap index f7a7667d1e131..14b57eb0cd1f1 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF011_RUF011.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF011_RUF011.py.snap @@ -1,80 +1,90 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF011.py:15:2: RUF011 Dictionary comprehension uses static key: `"key"` +RUF011.py:17:2: RUF011 Dictionary comprehension uses static key: `"key"` | -14 | # Errors -15 | {"key": value.upper() for value in data} +16 | # Errors +17 | {"key": value.upper() for value in data} | ^^^^^ RUF011 -16 | {True: value.upper() for value in data} -17 | {0: value.upper() for value in data} +18 | {True: value.upper() for value in data} +19 | {0: value.upper() for value in data} | -RUF011.py:16:2: RUF011 Dictionary comprehension uses static key: `True` +RUF011.py:18:2: RUF011 Dictionary comprehension uses static key: `True` | -14 | # Errors -15 | {"key": value.upper() for value in data} -16 | {True: value.upper() for value in data} +16 | # Errors +17 | {"key": value.upper() for value in data} +18 | {True: value.upper() for value in data} | ^^^^ RUF011 -17 | {0: value.upper() for value in data} -18 | {(1, "a"): value.upper() for value in data} # Constant tuple +19 | {0: value.upper() for value in data} +20 | {(1, "a"): value.upper() for value in data} # Constant tuple | -RUF011.py:17:2: RUF011 Dictionary comprehension uses static key: `0` +RUF011.py:19:2: RUF011 Dictionary comprehension uses static key: `0` | -15 | {"key": value.upper() for value in data} -16 | {True: value.upper() for value in data} -17 | {0: value.upper() for value in data} +17 | {"key": value.upper() for value in data} +18 | {True: value.upper() for value in data} +19 | {0: value.upper() for value in data} | ^ RUF011 -18 | {(1, "a"): value.upper() for value in data} # Constant tuple -19 | {constant: value.upper() for value in data} +20 | {(1, "a"): value.upper() for value in data} # Constant tuple +21 | {constant: value.upper() for value in data} | -RUF011.py:18:2: RUF011 Dictionary comprehension uses static key: `(1, "a")` +RUF011.py:20:2: RUF011 Dictionary comprehension uses static key: `(1, "a")` | -16 | {True: value.upper() for value in data} -17 | {0: value.upper() for value in data} -18 | {(1, "a"): value.upper() for value in data} # Constant tuple +18 | {True: value.upper() for value in data} +19 | {0: value.upper() for value in data} +20 | {(1, "a"): value.upper() for value in data} # Constant tuple | ^^^^^^^^ RUF011 -19 | {constant: value.upper() for value in data} -20 | {constant + constant: value.upper() for value in data} +21 | {constant: value.upper() for value in data} +22 | {constant + constant: value.upper() for value in data} | -RUF011.py:19:2: RUF011 Dictionary comprehension uses static key: `constant` +RUF011.py:21:2: RUF011 Dictionary comprehension uses static key: `constant` | -17 | {0: value.upper() for value in data} -18 | {(1, "a"): value.upper() for value in data} # Constant tuple -19 | {constant: value.upper() for value in data} +19 | {0: value.upper() for value in data} +20 | {(1, "a"): value.upper() for value in data} # Constant tuple +21 | {constant: value.upper() for value in data} | ^^^^^^^^ RUF011 -20 | {constant + constant: value.upper() for value in data} -21 | {constant.attribute: value.upper() for value in data} +22 | {constant + constant: value.upper() for value in data} +23 | {constant.attribute: value.upper() for value in data} | -RUF011.py:20:2: RUF011 Dictionary comprehension uses static key: `constant + constant` +RUF011.py:22:2: RUF011 Dictionary comprehension uses static key: `constant + constant` | -18 | {(1, "a"): value.upper() for value in data} # Constant tuple -19 | {constant: value.upper() for value in data} -20 | {constant + constant: value.upper() for value in data} +20 | {(1, "a"): value.upper() for value in data} # Constant tuple +21 | {constant: value.upper() for value in data} +22 | {constant + constant: value.upper() for value in data} | ^^^^^^^^^^^^^^^^^^^ RUF011 -21 | {constant.attribute: value.upper() for value in data} -22 | {constant[0]: value.upper() for value in data} +23 | {constant.attribute: value.upper() for value in data} +24 | {constant[0]: value.upper() for value in data} | -RUF011.py:21:2: RUF011 Dictionary comprehension uses static key: `constant.attribute` +RUF011.py:23:2: RUF011 Dictionary comprehension uses static key: `constant.attribute` | -19 | {constant: value.upper() for value in data} -20 | {constant + constant: value.upper() for value in data} -21 | {constant.attribute: value.upper() for value in data} +21 | {constant: value.upper() for value in data} +22 | {constant + constant: value.upper() for value in data} +23 | {constant.attribute: value.upper() for value in data} | ^^^^^^^^^^^^^^^^^^ RUF011 -22 | {constant[0]: value.upper() for value in data} +24 | {constant[0]: value.upper() for value in data} +25 | {tokens: token for token in tokens} | -RUF011.py:22:2: RUF011 Dictionary comprehension uses static key: `constant[0]` +RUF011.py:24:2: RUF011 Dictionary comprehension uses static key: `constant[0]` | -20 | {constant + constant: value.upper() for value in data} -21 | {constant.attribute: value.upper() for value in data} -22 | {constant[0]: value.upper() for value in data} +22 | {constant + constant: value.upper() for value in data} +23 | {constant.attribute: value.upper() for value in data} +24 | {constant[0]: value.upper() for value in data} | ^^^^^^^^^^^ RUF011 +25 | {tokens: token for token in tokens} + | + +RUF011.py:25:2: RUF011 Dictionary comprehension uses static key: `tokens` + | +23 | {constant.attribute: value.upper() for value in data} +24 | {constant[0]: value.upper() for value in data} +25 | {tokens: token for token in tokens} + | ^^^^^^ RUF011 | diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 8926f54138cbd..98370dfa9b1bc 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -914,6 +914,27 @@ where } } +/// A [`Visitor`] to collect all stored [`Expr::Name`] nodes in an AST. +#[derive(Debug, Default)] +pub struct StoredNameFinder<'a> { + /// A map from identifier to defining expression. + pub names: FxHashMap<&'a str, &'a ast::ExprName>, +} + +impl<'a, 'b> Visitor<'b> for StoredNameFinder<'a> +where + 'b: 'a, +{ + fn visit_expr(&mut self, expr: &'a Expr) { + if let Expr::Name(name) = expr { + if name.ctx.is_store() { + self.names.insert(&name.id, name); + } + } + crate::visitor::walk_expr(self, expr); + } +} + /// A [`StatementVisitor`] that collects all `return` statements in a function or method. #[derive(Default)] pub struct ReturnStatementVisitor<'a> {