From 9d6d309b9e8beb6f72cf00380d54fb027e254813 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 4 Feb 2024 19:30:50 -0500 Subject: [PATCH] Remove one usage of LibCST --- .../fixtures/flake8_comprehensions/C403.py | 8 ++ .../src/checkers/ast/analyze/expression.rs | 4 +- .../src/rules/flake8_comprehensions/fixes.rs | 87 ++++++++++--------- .../rules/unnecessary_collection_call.rs | 2 +- .../unnecessary_list_comprehension_set.rs | 55 ++++++++---- ...8_comprehensions__tests__C403_C403.py.snap | 64 ++++++++++++++ 6 files changed, 156 insertions(+), 64 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C403.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C403.py index 993c31e32f8a2..989581c5bc2ad 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C403.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C403.py @@ -13,3 +13,11 @@ def f(x): s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" + +s = set( # comment + [x for x in range(3)] +) + +s = set([ # comment + x for x in range(3) +]) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 55ccc7d0162a9..7329b9b9bf333 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -653,9 +653,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::UnnecessaryListComprehensionSet) { - flake8_comprehensions::rules::unnecessary_list_comprehension_set( - checker, expr, func, args, keywords, - ); + flake8_comprehensions::rules::unnecessary_list_comprehension_set(checker, call); } if checker.enabled(Rule::UnnecessaryListComprehensionDict) { flake8_comprehensions::rules::unnecessary_list_comprehension_dict( diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs index b177a494009f5..aebd3e6b15825 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs @@ -1,3 +1,5 @@ +use std::iter; + use anyhow::{bail, Result}; use itertools::Itertools; use libcst_native::{ @@ -7,7 +9,6 @@ use libcst_native::{ RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace, TrailingWhitespace, Tuple, }; -use std::iter; use ruff_diagnostics::{Edit, Fix}; use ruff_python_ast::Expr; @@ -151,46 +152,6 @@ pub(crate) fn fix_unnecessary_generator_dict(expr: &Expr, checker: &Checker) -> )) } -/// (C403) Convert `set([x for x in y])` to `{x for x in y}`. -pub(crate) fn fix_unnecessary_list_comprehension_set( - expr: &Expr, - checker: &Checker, -) -> Result { - let locator = checker.locator(); - let stylist = checker.stylist(); - // Expr(Call(ListComp)))) -> - // Expr(SetComp))) - let module_text = locator.slice(expr); - let mut tree = match_expression(module_text)?; - let call = match_call_mut(&mut tree)?; - let arg = match_arg(call)?; - - let list_comp = match_list_comp(&arg.value)?; - - tree = Expression::SetComp(Box::new(SetComp { - elt: list_comp.elt.clone(), - for_in: list_comp.for_in.clone(), - lbrace: LeftCurlyBrace { - whitespace_after: call.whitespace_before_args.clone(), - }, - rbrace: RightCurlyBrace { - whitespace_before: arg.whitespace_after_arg.clone(), - }, - lpar: list_comp.lpar.clone(), - rpar: list_comp.rpar.clone(), - })); - - Ok(Edit::range_replacement( - pad_expression( - tree.codegen_stylist(stylist), - expr.range(), - checker.locator(), - checker.semantic(), - ), - expr.range(), - )) -} - /// (C404) Convert `dict([(i, i) for i in range(3)])` to `{i: i for i in /// range(3)}`. pub(crate) fn fix_unnecessary_list_comprehension_dict( @@ -544,7 +505,7 @@ pub(crate) fn fix_unnecessary_collection_call(expr: &Expr, checker: &Checker) -> /// However, this is a syntax error under the f-string grammar. As such, /// this method will pad the start and end of an expression as needed to /// avoid producing invalid syntax. -fn pad_expression( +pub(crate) fn pad_expression( content: String, range: TextRange, locator: &Locator, @@ -575,6 +536,48 @@ fn pad_expression( } } +/// Like [`pad_expression`], but only pads the start of the expression. +pub(crate) fn pad_start( + content: &str, + range: TextRange, + locator: &Locator, + semantic: &SemanticModel, +) -> String { + if !semantic.in_f_string() { + return content.into(); + } + + // If the expression is immediately preceded by an opening brace, then + // we need to add a space before the expression. + let prefix = locator.up_to(range.start()); + if matches!(prefix.chars().next_back(), Some('{')) { + format!(" {content}") + } else { + content.into() + } +} + +/// Like [`pad_expression`], but only pads the end of the expression. +pub(crate) fn pad_end( + content: &str, + range: TextRange, + locator: &Locator, + semantic: &SemanticModel, +) -> String { + if !semantic.in_f_string() { + return content.into(); + } + + // If the expression is immediately preceded by an opening brace, then + // we need to add a space before the expression. + let suffix = locator.after(range.end()); + if matches!(suffix.chars().next(), Some('}')) { + format!("{content} ") + } else { + content.into() + } +} + /// (C409) Convert `tuple([1, 2])` to `tuple(1, 2)` pub(crate) fn fix_unnecessary_literal_within_tuple_call( expr: &Expr, 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 6e3eb4f4a3a62..4e21015c9f222 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 @@ -76,7 +76,7 @@ pub(crate) fn unnecessary_collection_call( { // `dict()` or `dict(a=1)` (as opposed to `dict(**a)`) } - "list" | "tuple" => { + "list" | "tuple" if keywords.is_empty() => { // `list()` or `tuple()` } _ => return, diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs index 3c791385696a1..2919688f2d83b 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_list_comprehension_set.rs @@ -1,12 +1,10 @@ -use ruff_python_ast::{Expr, Keyword}; - -use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -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 super::helpers; @@ -45,25 +43,46 @@ impl AlwaysFixableViolation for UnnecessaryListComprehensionSet { } /// C403 (`set([...])`) -pub(crate) fn unnecessary_list_comprehension_set( - checker: &mut Checker, - expr: &Expr, - func: &Expr, - args: &[Expr], - keywords: &[Keyword], -) { - let Some(argument) = - helpers::exactly_one_argument_with_matching_function("set", func, args, keywords) - else { +pub(crate) fn unnecessary_list_comprehension_set(checker: &mut Checker, call: &ast::ExprCall) { + let Some(argument) = helpers::exactly_one_argument_with_matching_function( + "set", + &call.func, + &call.arguments.args, + &call.arguments.keywords, + ) else { return; }; if !checker.semantic().is_builtin("set") { return; } if argument.is_list_comp_expr() { - let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, expr.range()); + let mut diagnostic = Diagnostic::new(UnnecessaryListComprehensionSet, call.range()); diagnostic.try_set_fix(|| { - fixes::fix_unnecessary_list_comprehension_set(expr, checker).map(Fix::unsafe_edit) + // Replace `set(` with `{`. + let call_start = Edit::replacement( + pad_start("{", call.range(), checker.locator(), checker.semantic()), + call.start(), + call.arguments.start() + TextSize::from(1), + ); + + // Replace `)` with `}`. + let call_end = Edit::replacement( + pad_end("}", call.range(), checker.locator(), checker.semantic()), + call.arguments.end() - TextSize::from(1), + call.end(), + ); + + // Delete the open bracket (`[`). + let argument_start = + Edit::deletion(argument.start(), argument.start() + TextSize::from(1)); + + // Delete the close bracket (`]`). + let argument_end = Edit::deletion(argument.end() - TextSize::from(1), argument.end()); + + Ok(Fix::unsafe_edits( + call_start, + [argument_start, argument_end, call_end], + )) }); checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C403_C403.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C403_C403.py.snap index 13ff3ffd6a3fc..8b30f0a8e2537 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C403_C403.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C403_C403.py.snap @@ -120,6 +120,8 @@ C403.py:14:9: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comp 14 |-s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 14 |+s = f"{ {x for x in 'ab'} | set([x for x in 'ab']) }" 15 15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" +16 16 | +17 17 | s = set( # comment C403.py:14:34: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comprehension) | @@ -138,12 +140,16 @@ C403.py:14:34: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` com 14 |-s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 14 |+s = f"{ set([x for x in 'ab']) | {x for x in 'ab'} }" 15 15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" +16 16 | +17 17 | s = set( # comment C403.py:15:8: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comprehension) | 14 | s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" | ^^^^^^^^^^^^^^^^^^^^^^ C403 +16 | +17 | s = set( # comment | = help: Rewrite as a `set` comprehension @@ -153,12 +159,17 @@ C403.py:15:8: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comp 14 14 | s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 15 |-s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" 15 |+s = f"{ {x for x in 'ab'} | set([x for x in 'ab'])}" +16 16 | +17 17 | s = set( # comment +18 18 | [x for x in range(3)] C403.py:15:33: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comprehension) | 14 | s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" | ^^^^^^^^^^^^^^^^^^^^^^ C403 +16 | +17 | s = set( # comment | = help: Rewrite as a `set` comprehension @@ -168,5 +179,58 @@ C403.py:15:33: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` com 14 14 | s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" 15 |-s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" 15 |+s = f"{set([x for x in 'ab']) | {x for x in 'ab'} }" +16 16 | +17 17 | s = set( # comment +18 18 | [x for x in range(3)] + +C403.py:17:5: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comprehension) + | +15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" +16 | +17 | s = set( # comment + | _____^ +18 | | [x for x in range(3)] +19 | | ) + | |_^ C403 +20 | +21 | s = set([ # comment + | + = help: Rewrite as a `set` comprehension + +ℹ Unsafe fix +14 14 | s = f"{ set([x for x in 'ab']) | set([x for x in 'ab']) }" +15 15 | s = f"{set([x for x in 'ab']) | set([x for x in 'ab'])}" +16 16 | +17 |-s = set( # comment +18 |- [x for x in range(3)] +19 |-) + 17 |+s = { # comment + 18 |+ x for x in range(3) + 19 |+} +20 20 | +21 21 | s = set([ # comment +22 22 | x for x in range(3) + +C403.py:21:5: C403 [*] Unnecessary `list` comprehension (rewrite as a `set` comprehension) + | +19 | ) +20 | +21 | s = set([ # comment + | _____^ +22 | | x for x in range(3) +23 | | ]) + | |__^ C403 + | + = help: Rewrite as a `set` comprehension + +ℹ Unsafe fix +18 18 | [x for x in range(3)] +19 19 | ) +20 20 | +21 |-s = set([ # comment + 21 |+s = { # comment +22 22 | x for x in range(3) +23 |-]) + 23 |+}