From ad893f8295d108aef431d65e54cdae50945d571e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 20 Sep 2023 21:28:11 -0400 Subject: [PATCH] Avoid invalid fix for parenthesized values in F601 (#7559) Closes https://github.com/astral-sh/ruff/issues/4897. --- .../resources/test/fixtures/pyflakes/F601.py | 5 ++ .../src/checkers/ast/analyze/expression.rs | 14 ++--- .../src/rules/pyflakes/rules/repeated_keys.rs | 51 +++++++++++++++---- ..._rules__pyflakes__tests__F601_F601.py.snap | 35 +++++++++++++ 4 files changed, 88 insertions(+), 17 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py index e8e9e6a89f..3ef8f1e55d 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py @@ -53,3 +53,8 @@ x = { ('a', 'b'): 'asdf', ('a', 'b'): 'qwer', } + +# Regression test for: https://github.com/astral-sh/ruff/issues/4897 +t={"x":"test123", "x":("test123")} + +t={"x":("test123"), "x":"test123"} diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 7c9a0c8179..f2ed2125a5 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -908,16 +908,18 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { flake8_logging::rules::exception_without_exc_info(checker, call); } } - Expr::Dict(ast::ExprDict { - keys, - values, - range: _, - }) => { + Expr::Dict( + dict @ ast::ExprDict { + keys, + values, + range: _, + }, + ) => { if checker.any_enabled(&[ Rule::MultiValueRepeatedKeyLiteral, Rule::MultiValueRepeatedKeyVariable, ]) { - pyflakes::rules::repeated_keys(checker, keys, values); + pyflakes::rules::repeated_keys(checker, dict); } if checker.enabled(Rule::UnnecessarySpread) { flake8_pie::rules::unnecessary_spread(checker, keys, values); diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs b/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs index 28291e3600..6926d15d67 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs @@ -1,14 +1,15 @@ use std::hash::BuildHasherDefault; -use ruff_python_ast::Expr; use rustc_hash::{FxHashMap, FxHashSet}; -use crate::autofix::snippet::SourceCodeSnippet; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::parenthesize::parenthesized_range; +use ruff_python_ast::{self as ast, Expr}; use ruff_text_size::Ranged; +use crate::autofix::snippet::SourceCodeSnippet; use crate::checkers::ast::Checker; use crate::registry::{AsRule, Rule}; @@ -128,19 +129,19 @@ impl Violation for MultiValueRepeatedKeyVariable { } /// F601, F602 -pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option], values: &[Expr]) { +pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) { // Generate a map from key to (index, value). let mut seen: FxHashMap> = - FxHashMap::with_capacity_and_hasher(keys.len(), BuildHasherDefault::default()); + FxHashMap::with_capacity_and_hasher(dict.keys.len(), BuildHasherDefault::default()); // Detect duplicate keys. - for (i, key) in keys.iter().enumerate() { + for (i, (key, value)) in dict.keys.iter().zip(dict.values.iter()).enumerate() { let Some(key) = key else { continue; }; let comparable_key = ComparableExpr::from(key); - let comparable_value = ComparableExpr::from(&values[i]); + let comparable_value = ComparableExpr::from(value); let Some(seen_values) = seen.get_mut(&comparable_key) else { seen.insert(comparable_key, FxHashSet::from_iter([comparable_value])); @@ -159,8 +160,22 @@ pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option], values if checker.patch(diagnostic.kind.rule()) { if !seen_values.insert(comparable_value) { diagnostic.set_fix(Fix::suggested(Edit::deletion( - values[i - 1].end(), - values[i].end(), + parenthesized_range( + (&dict.values[i - 1]).into(), + dict.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(dict.values[i - 1].range()) + .end(), + parenthesized_range( + (&dict.values[i]).into(), + dict.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(dict.values[i].range()) + .end(), ))); } } @@ -176,11 +191,25 @@ pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option], values key.range(), ); if checker.patch(diagnostic.kind.rule()) { - let comparable_value: ComparableExpr = (&values[i]).into(); + let comparable_value: ComparableExpr = (&dict.values[i]).into(); if !seen_values.insert(comparable_value) { diagnostic.set_fix(Fix::suggested(Edit::deletion( - values[i - 1].end(), - values[i].end(), + parenthesized_range( + (&dict.values[i - 1]).into(), + dict.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(dict.values[i - 1].range()) + .end(), + parenthesized_range( + (&dict.values[i]).into(), + dict.into(), + checker.indexer().comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or(dict.values[i].range()) + .end(), ))); } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap index 60699ba130..f1d52d7d34 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap @@ -281,4 +281,39 @@ F601.py:54:5: F601 Dictionary key literal `('a', 'b')` repeated | = help: Remove repeated key literal `('a', 'b')` +F601.py:58:19: F601 [*] Dictionary key literal `"x"` repeated + | +57 | # Regression test for: https://github.com/astral-sh/ruff/issues/4897 +58 | t={"x":"test123", "x":("test123")} + | ^^^ F601 +59 | +60 | t={"x":("test123"), "x":"test123"} + | + = help: Remove repeated key literal `"x"` + +ℹ Suggested fix +55 55 | } +56 56 | +57 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/4897 +58 |-t={"x":"test123", "x":("test123")} + 58 |+t={"x":"test123"} +59 59 | +60 60 | t={"x":("test123"), "x":"test123"} + +F601.py:60:21: F601 [*] Dictionary key literal `"x"` repeated + | +58 | t={"x":"test123", "x":("test123")} +59 | +60 | t={"x":("test123"), "x":"test123"} + | ^^^ F601 + | + = help: Remove repeated key literal `"x"` + +ℹ Suggested fix +57 57 | # Regression test for: https://github.com/astral-sh/ruff/issues/4897 +58 58 | t={"x":"test123", "x":("test123")} +59 59 | +60 |-t={"x":("test123"), "x":"test123"} + 60 |+t={"x":("test123")} +