From 36e0926f00d4ed9910d738c0bfa8b103dfe3cb01 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 4 Feb 2024 21:33:15 -0500 Subject: [PATCH] Remove CST-based fixer for C408 --- .../fixtures/flake8_comprehensions/C408.py | 7 + .../src/checkers/ast/analyze/expression.rs | 5 +- .../src/rules/flake8_comprehensions/fixes.rs | 170 +++++++----------- .../rules/unnecessary_collection_call.rs | 84 +++++++-- ...8_comprehensions__tests__C408_C408.py.snap | 60 +++++++ ...low_dict_calls_with_keyword_arguments.snap | 44 +++++ 6 files changed, 240 insertions(+), 130 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C408.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C408.py index 81540af3654f5..da0ebcaf713dc 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C408.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C408.py @@ -20,3 +20,10 @@ def list(): f"{ dict(x='y') | dict(y='z') }" f"a {dict(x='y') | dict(y='z')} b" f"a { dict(x='y') | dict(y='z') } b" + +dict( + # comment +) + +tuple( # comment +) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index df9f36e533070..6e44c85c154d4 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -667,10 +667,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryCollectionCall) { flake8_comprehensions::rules::unnecessary_collection_call( checker, - expr, - func, - args, - keywords, + call, &checker.settings.flake8_comprehensions, ); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index 65063ddee24ce..1febae119798b 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -5,13 +5,13 @@ use itertools::Itertools; use libcst_native::{ Arg, AssignEqual, AssignTargetExpression, Call, Comment, CompFor, Dict, DictComp, DictElement, Element, EmptyLine, Expression, GeneratorExp, LeftCurlyBrace, LeftParen, LeftSquareBracket, - List, ListComp, Name, ParenthesizableWhitespace, ParenthesizedNode, ParenthesizedWhitespace, + ListComp, Name, ParenthesizableWhitespace, ParenthesizedNode, ParenthesizedWhitespace, RightCurlyBrace, RightParen, RightSquareBracket, SetComp, SimpleString, SimpleWhitespace, TrailingWhitespace, Tuple, }; use ruff_diagnostics::{Edit, Fix}; -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::Stylist; use ruff_python_semantic::SemanticModel; use ruff_source_file::Locator; @@ -25,7 +25,7 @@ use crate::{ checkers::ast::Checker, cst::matchers::{ match_arg, match_call, match_call_mut, match_expression, match_generator_exp, match_lambda, - match_list_comp, match_name, match_tuple, + match_list_comp, match_tuple, }, }; @@ -213,126 +213,82 @@ pub(crate) fn fix_unnecessary_literal_dict(expr: &Expr, checker: &Checker) -> Re )) } -/// (C408) -pub(crate) fn fix_unnecessary_collection_call(expr: &Expr, checker: &Checker) -> Result { - enum Collection { - Tuple, - List, - Dict, - } - +/// (C408) Convert `dict(a=1, b=2)` to `{"a": 1, "b": 2}`. +pub(crate) fn fix_unnecessary_collection_call( + expr: &ast::ExprCall, + checker: &Checker, +) -> Result { let locator = checker.locator(); let stylist = checker.stylist(); - // Expr(Call("list" | "tuple" | "dict")))) -> Expr(List|Tuple|Dict) + // Expr(Call("dict")))) -> Expr(Dict) let module_text = locator.slice(expr); let mut tree = match_expression(module_text)?; let call = match_call(&tree)?; - let name = match_name(&call.func)?; - let collection = match name.value { - "tuple" => Collection::Tuple, - "list" => Collection::List, - "dict" => Collection::Dict, - _ => bail!("Expected 'tuple', 'list', or 'dict'"), - }; // Arena allocator used to create formatted strings of sufficient lifetime, // below. let mut arena: Vec = vec![]; - match collection { - Collection::Tuple => { - tree = Expression::Tuple(Box::new(Tuple { - elements: vec![], - lpar: vec![LeftParen::default()], - rpar: vec![RightParen::default()], - })); - } - Collection::List => { - tree = Expression::List(Box::new(List { - elements: vec![], - lbracket: LeftSquareBracket::default(), - rbracket: RightSquareBracket::default(), + let quote = checker.f_string_quote_style().unwrap_or(stylist.quote()); + + // Quote each argument. + for arg in &call.args { + let quoted = format!( + "{}{}{}", + quote, + arg.keyword + .as_ref() + .expect("Expected dictionary argument to be kwarg") + .value, + quote, + ); + arena.push(quoted); + } + + let elements = call + .args + .iter() + .enumerate() + .map(|(i, arg)| DictElement::Simple { + key: Expression::SimpleString(Box::new(SimpleString { + value: &arena[i], lpar: vec![], rpar: vec![], - })); - } - Collection::Dict => { - if call.args.is_empty() { - tree = Expression::Dict(Box::new(Dict { - elements: vec![], - lbrace: LeftCurlyBrace::default(), - rbrace: RightCurlyBrace::default(), - lpar: vec![], - rpar: vec![], - })); - } else { - let quote = checker.f_string_quote_style().unwrap_or(stylist.quote()); - - // Quote each argument. - for arg in &call.args { - let quoted = format!( - "{}{}{}", - quote, - arg.keyword - .as_ref() - .expect("Expected dictionary argument to be kwarg") - .value, - quote, - ); - arena.push(quoted); - } - - let elements = call - .args - .iter() - .enumerate() - .map(|(i, arg)| DictElement::Simple { - key: Expression::SimpleString(Box::new(SimpleString { - value: &arena[i], - lpar: vec![], - rpar: vec![], - })), - value: arg.value.clone(), - comma: arg.comma.clone(), - whitespace_before_colon: ParenthesizableWhitespace::default(), - whitespace_after_colon: ParenthesizableWhitespace::SimpleWhitespace( - SimpleWhitespace(" "), - ), - }) - .collect(); + })), + value: arg.value.clone(), + comma: arg.comma.clone(), + whitespace_before_colon: ParenthesizableWhitespace::default(), + whitespace_after_colon: ParenthesizableWhitespace::SimpleWhitespace(SimpleWhitespace( + " ", + )), + }) + .collect(); - tree = Expression::Dict(Box::new(Dict { - elements, - lbrace: LeftCurlyBrace { - whitespace_after: call.whitespace_before_args.clone(), - }, - rbrace: RightCurlyBrace { - whitespace_before: call - .args - .last() - .expect("Arguments should be non-empty") - .whitespace_after_arg - .clone(), - }, - lpar: vec![], - rpar: vec![], - })); - } - } - }; + tree = Expression::Dict(Box::new(Dict { + elements, + lbrace: LeftCurlyBrace { + whitespace_after: call.whitespace_before_args.clone(), + }, + rbrace: RightCurlyBrace { + whitespace_before: call + .args + .last() + .expect("Arguments should be non-empty") + .whitespace_after_arg + .clone(), + }, + lpar: vec![], + rpar: vec![], + })); Ok(Edit::range_replacement( - if matches!(collection, Collection::Dict) { - pad_expression( - tree.codegen_stylist(stylist), - expr.range(), - checker.locator(), - checker.semantic(), - ) - } else { - tree.codegen_stylist(stylist) - }, + pad_expression( + tree.codegen_stylist(stylist), + expr.range(), + checker.locator(), + checker.semantic(), + ), expr.range(), )) } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs index 4e21015c9f222..fa23032a34542 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs @@ -1,11 +1,11 @@ -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Expr, Keyword}; -use ruff_text_size::Ranged; +use ruff_python_ast as ast; +use ruff_text_size::{Ranged, TextSize}; use crate::checkers::ast::Checker; - use crate::rules::flake8_comprehensions::fixes; +use crate::rules::flake8_comprehensions::fixes::{pad_end, pad_start}; use crate::rules::flake8_comprehensions::settings::Settings; /// ## What it does @@ -56,42 +56,88 @@ impl AlwaysFixableViolation for UnnecessaryCollectionCall { /// C408 pub(crate) fn unnecessary_collection_call( checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], + call: &ast::ExprCall, settings: &Settings, ) { - if !args.is_empty() { + if !call.arguments.args.is_empty() { return; } - let Some(func) = func.as_name_expr() else { + let Some(func) = call.func.as_name_expr() else { return; }; - match func.id.as_str() { + let collection = match func.id.as_str() { "dict" - if keywords.is_empty() + if call.arguments.keywords.is_empty() || (!settings.allow_dict_calls_with_keyword_arguments - && keywords.iter().all(|kw| kw.arg.is_some())) => + && call.arguments.keywords.iter().all(|kw| kw.arg.is_some())) => { // `dict()` or `dict(a=1)` (as opposed to `dict(**a)`) + Collection::Dict + } + "list" if call.arguments.keywords.is_empty() => { + // `list() + Collection::List } - "list" | "tuple" if keywords.is_empty() => { - // `list()` or `tuple()` + "tuple" if call.arguments.keywords.is_empty() => { + // `tuple()` + Collection::Tuple } _ => return, }; if !checker.semantic().is_builtin(func.id.as_str()) { return; } + let mut diagnostic = Diagnostic::new( UnnecessaryCollectionCall { obj_type: func.id.to_string(), }, - expr.range(), + call.range(), ); - diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_collection_call(expr, checker).map(Fix::unsafe_edit) - }); + + // Convert `dict()` to `{}`. + if call.arguments.keywords.is_empty() { + diagnostic.set_fix({ + // Replace from the start of the call to the start of the argument. + let call_start = Edit::replacement( + match collection { + Collection::Dict => { + pad_start("{", call.range(), checker.locator(), checker.semantic()) + } + Collection::List => "[".to_string(), + Collection::Tuple => "(".to_string(), + }, + call.start(), + call.arguments.start() + TextSize::from(1), + ); + + // Replace from the end of the inner list or tuple to the end of the call with `}`. + let call_end = Edit::replacement( + match collection { + Collection::Dict => { + pad_end("}", call.range(), checker.locator(), checker.semantic()) + } + Collection::List => "]".to_string(), + Collection::Tuple => ")".to_string(), + }, + call.arguments.end() - TextSize::from(1), + call.end(), + ); + + Fix::unsafe_edits(call_start, [call_end]) + }); + } else { + // Convert `dict(a=1, b=2)` to `{"a": 1, "b": 2}`. + diagnostic.try_set_fix(|| { + fixes::fix_unnecessary_collection_call(call, checker).map(Fix::unsafe_edit) + }); + } + checker.diagnostics.push(diagnostic); } + +enum Collection { + Tuple, + List, + Dict, +} diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py.snap index 755112e2e7c0f..6b4414d1b4a87 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py.snap @@ -217,6 +217,7 @@ C408.py:20:5: C408 [*] Unnecessary `dict` call (rewrite as a literal) 20 |+f"{ {'x': 'y'} | dict(y='z') }" 21 21 | f"a {dict(x='y') | dict(y='z')} b" 22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | C408.py:20:19: C408 [*] Unnecessary `dict` call (rewrite as a literal) | @@ -236,6 +237,7 @@ C408.py:20:19: C408 [*] Unnecessary `dict` call (rewrite as a literal) 20 |+f"{ dict(x='y') | {'y': 'z'} }" 21 21 | f"a {dict(x='y') | dict(y='z')} b" 22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | C408.py:21:6: C408 [*] Unnecessary `dict` call (rewrite as a literal) | @@ -254,6 +256,8 @@ C408.py:21:6: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 |-f"a {dict(x='y') | dict(y='z')} b" 21 |+f"a { {'x': 'y'} | dict(y='z')} b" 22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | +24 24 | dict( C408.py:21:20: C408 [*] Unnecessary `dict` call (rewrite as a literal) | @@ -272,6 +276,8 @@ C408.py:21:20: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 |-f"a {dict(x='y') | dict(y='z')} b" 21 |+f"a {dict(x='y') | {'y': 'z'} } b" 22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | +24 24 | dict( C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal) | @@ -279,6 +285,8 @@ C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 | f"a {dict(x='y') | dict(y='z')} b" 22 | f"a { dict(x='y') | dict(y='z') } b" | ^^^^^^^^^^^ C408 +23 | +24 | dict( | = help: Rewrite as a literal @@ -288,6 +296,9 @@ C408.py:22:7: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 21 | f"a {dict(x='y') | dict(y='z')} b" 22 |-f"a { dict(x='y') | dict(y='z') } b" 22 |+f"a { {'x': 'y'} | dict(y='z') } b" +23 23 | +24 24 | dict( +25 25 | # comment C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal) | @@ -295,6 +306,8 @@ C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 | f"a {dict(x='y') | dict(y='z')} b" 22 | f"a { dict(x='y') | dict(y='z') } b" | ^^^^^^^^^^^ C408 +23 | +24 | dict( | = help: Rewrite as a literal @@ -304,5 +317,52 @@ C408.py:22:21: C408 [*] Unnecessary `dict` call (rewrite as a literal) 21 21 | f"a {dict(x='y') | dict(y='z')} b" 22 |-f"a { dict(x='y') | dict(y='z') } b" 22 |+f"a { dict(x='y') | {'y': 'z'} } b" +23 23 | +24 24 | dict( +25 25 | # comment + +C408.py:24:1: C408 [*] Unnecessary `dict` call (rewrite as a literal) + | +22 | f"a { dict(x='y') | dict(y='z') } b" +23 | +24 | / dict( +25 | | # comment +26 | | ) + | |_^ C408 +27 | +28 | tuple( # comment + | + = help: Rewrite as a literal + +ℹ Unsafe fix +21 21 | f"a {dict(x='y') | dict(y='z')} b" +22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | +24 |-dict( + 24 |+{ +25 25 | # comment +26 |-) + 26 |+} +27 27 | +28 28 | tuple( # comment +29 29 | ) + +C408.py:28:1: C408 [*] Unnecessary `tuple` call (rewrite as a literal) + | +26 | ) +27 | +28 | / tuple( # comment +29 | | ) + | |_^ C408 + | + = help: Rewrite as a literal + +ℹ Unsafe fix +25 25 | # comment +26 26 | ) +27 27 | +28 |-tuple( # comment + 28 |+( # comment +29 29 | ) diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py_allow_dict_calls_with_keyword_arguments.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py_allow_dict_calls_with_keyword_arguments.snap index 43df1eb9fb14f..59c761f4aa603 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py_allow_dict_calls_with_keyword_arguments.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C408_C408.py_allow_dict_calls_with_keyword_arguments.snap @@ -96,4 +96,48 @@ C408.py:17:6: C408 [*] Unnecessary `dict` call (rewrite as a literal) 19 19 | f"{dict(x='y') | dict(y='z')}" 20 20 | f"{ dict(x='y') | dict(y='z') }" +C408.py:24:1: C408 [*] Unnecessary `dict` call (rewrite as a literal) + | +22 | f"a { dict(x='y') | dict(y='z') } b" +23 | +24 | / dict( +25 | | # comment +26 | | ) + | |_^ C408 +27 | +28 | tuple( # comment + | + = help: Rewrite as a literal + +ℹ Unsafe fix +21 21 | f"a {dict(x='y') | dict(y='z')} b" +22 22 | f"a { dict(x='y') | dict(y='z') } b" +23 23 | +24 |-dict( + 24 |+{ +25 25 | # comment +26 |-) + 26 |+} +27 27 | +28 28 | tuple( # comment +29 29 | ) + +C408.py:28:1: C408 [*] Unnecessary `tuple` call (rewrite as a literal) + | +26 | ) +27 | +28 | / tuple( # comment +29 | | ) + | |_^ C408 + | + = help: Rewrite as a literal + +ℹ Unsafe fix +25 25 | # comment +26 26 | ) +27 27 | +28 |-tuple( # comment + 28 |+( # comment +29 29 | ) +