Avoid invalid fix for parenthesized values in F601 (#7559)

Closes https://github.com/astral-sh/ruff/issues/4897.
This commit is contained in:
Charlie Marsh 2023-09-20 21:28:11 -04:00 committed by GitHub
parent 621bed55c0
commit ad893f8295
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 88 additions and 17 deletions

View file

@ -53,3 +53,8 @@ x = {
('a', 'b'): 'asdf', ('a', 'b'): 'asdf',
('a', 'b'): 'qwer', ('a', 'b'): 'qwer',
} }
# Regression test for: https://github.com/astral-sh/ruff/issues/4897
t={"x":"test123", "x":("test123")}
t={"x":("test123"), "x":"test123"}

View file

@ -908,16 +908,18 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
flake8_logging::rules::exception_without_exc_info(checker, call); flake8_logging::rules::exception_without_exc_info(checker, call);
} }
} }
Expr::Dict(ast::ExprDict { Expr::Dict(
keys, dict @ ast::ExprDict {
values, keys,
range: _, values,
}) => { range: _,
},
) => {
if checker.any_enabled(&[ if checker.any_enabled(&[
Rule::MultiValueRepeatedKeyLiteral, Rule::MultiValueRepeatedKeyLiteral,
Rule::MultiValueRepeatedKeyVariable, Rule::MultiValueRepeatedKeyVariable,
]) { ]) {
pyflakes::rules::repeated_keys(checker, keys, values); pyflakes::rules::repeated_keys(checker, dict);
} }
if checker.enabled(Rule::UnnecessarySpread) { if checker.enabled(Rule::UnnecessarySpread) {
flake8_pie::rules::unnecessary_spread(checker, keys, values); flake8_pie::rules::unnecessary_spread(checker, keys, values);

View file

@ -1,14 +1,15 @@
use std::hash::BuildHasherDefault; use std::hash::BuildHasherDefault;
use ruff_python_ast::Expr;
use rustc_hash::{FxHashMap, FxHashSet}; use rustc_hash::{FxHashMap, FxHashSet};
use crate::autofix::snippet::SourceCodeSnippet;
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation}; use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr; 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 ruff_text_size::Ranged;
use crate::autofix::snippet::SourceCodeSnippet;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
use crate::registry::{AsRule, Rule}; use crate::registry::{AsRule, Rule};
@ -128,19 +129,19 @@ impl Violation for MultiValueRepeatedKeyVariable {
} }
/// F601, F602 /// F601, F602
pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option<Expr>], values: &[Expr]) { pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) {
// Generate a map from key to (index, value). // Generate a map from key to (index, value).
let mut seen: FxHashMap<ComparableExpr, FxHashSet<ComparableExpr>> = let mut seen: FxHashMap<ComparableExpr, FxHashSet<ComparableExpr>> =
FxHashMap::with_capacity_and_hasher(keys.len(), BuildHasherDefault::default()); FxHashMap::with_capacity_and_hasher(dict.keys.len(), BuildHasherDefault::default());
// Detect duplicate keys. // 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 { let Some(key) = key else {
continue; continue;
}; };
let comparable_key = ComparableExpr::from(key); 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 { let Some(seen_values) = seen.get_mut(&comparable_key) else {
seen.insert(comparable_key, FxHashSet::from_iter([comparable_value])); seen.insert(comparable_key, FxHashSet::from_iter([comparable_value]));
@ -159,8 +160,22 @@ pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option<Expr>], values
if checker.patch(diagnostic.kind.rule()) { if checker.patch(diagnostic.kind.rule()) {
if !seen_values.insert(comparable_value) { if !seen_values.insert(comparable_value) {
diagnostic.set_fix(Fix::suggested(Edit::deletion( diagnostic.set_fix(Fix::suggested(Edit::deletion(
values[i - 1].end(), parenthesized_range(
values[i].end(), (&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<Expr>], values
key.range(), key.range(),
); );
if checker.patch(diagnostic.kind.rule()) { 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) { if !seen_values.insert(comparable_value) {
diagnostic.set_fix(Fix::suggested(Edit::deletion( diagnostic.set_fix(Fix::suggested(Edit::deletion(
values[i - 1].end(), parenthesized_range(
values[i].end(), (&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(),
))); )));
} }
} }

View file

@ -281,4 +281,39 @@ F601.py:54:5: F601 Dictionary key literal `('a', 'b')` repeated
| |
= help: Remove repeated key literal `('a', 'b')` = 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")}