Skip to content

Commit

Permalink
Remove CST-based fixers for C405 and C409 (#9821)
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh authored Feb 5, 2024
1 parent c5fa0cc commit a6bc4b2
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 195 deletions.
8 changes: 8 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C409.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,11 @@
t5 = tuple(
(1, 2)
)

tuple( # comment
[1, 2]
)

tuple([ # comment
1, 2
])
8 changes: 2 additions & 6 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -657,9 +657,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
);
}
if checker.enabled(Rule::UnnecessaryLiteralSet) {
flake8_comprehensions::rules::unnecessary_literal_set(
checker, expr, func, args, keywords,
);
flake8_comprehensions::rules::unnecessary_literal_set(checker, call);
}
if checker.enabled(Rule::UnnecessaryLiteralDict) {
flake8_comprehensions::rules::unnecessary_literal_dict(
Expand All @@ -677,9 +675,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
);
}
if checker.enabled(Rule::UnnecessaryLiteralWithinTupleCall) {
flake8_comprehensions::rules::unnecessary_literal_within_tuple_call(
checker, expr, func, args, keywords,
);
flake8_comprehensions::rules::unnecessary_literal_within_tuple_call(checker, call);
}
if checker.enabled(Rule::UnnecessaryLiteralWithinListCall) {
flake8_comprehensions::rules::unnecessary_literal_within_list_call(checker, call);
Expand Down
141 changes: 1 addition & 140 deletions crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ 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,
RightCurlyBrace, RightParen, RightSquareBracket, Set, SetComp, SimpleString, SimpleWhitespace,
RightCurlyBrace, RightParen, RightSquareBracket, SetComp, SimpleString, SimpleWhitespace,
TrailingWhitespace, Tuple,
};

Expand Down Expand Up @@ -145,95 +145,6 @@ pub(crate) fn fix_unnecessary_list_comprehension_dict(
))
}

/// Drop a trailing comma from a list of tuple elements.
fn drop_trailing_comma<'a>(
tuple: &Tuple<'a>,
) -> Result<(
Vec<Element<'a>>,
ParenthesizableWhitespace<'a>,
ParenthesizableWhitespace<'a>,
)> {
let whitespace_after = tuple
.lpar
.first()
.ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))?
.whitespace_after
.clone();
let whitespace_before = tuple
.rpar
.first()
.ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))?
.whitespace_before
.clone();

let mut elements = tuple.elements.clone();
if elements.len() == 1 {
if let Some(Element::Simple {
value,
comma: Some(..),
..
}) = elements.last()
{
if whitespace_before == ParenthesizableWhitespace::default()
&& whitespace_after == ParenthesizableWhitespace::default()
{
elements[0] = Element::Simple {
value: value.clone(),
comma: None,
};
}
}
}

Ok((elements, whitespace_after, whitespace_before))
}

/// (C405) Convert `set((1, 2))` to `{1, 2}`.
pub(crate) fn fix_unnecessary_literal_set(expr: &Expr, checker: &Checker) -> Result<Edit> {
let locator = checker.locator();
let stylist = checker.stylist();

// Expr(Call(List|Tuple)))) -> Expr(Set)))
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 (elements, whitespace_after, whitespace_before) = match &arg.value {
Expression::Tuple(inner) => drop_trailing_comma(inner)?,
Expression::List(inner) => (
inner.elements.clone(),
inner.lbracket.whitespace_after.clone(),
inner.rbracket.whitespace_before.clone(),
),
_ => {
bail!("Expected Expression::Tuple | Expression::List");
}
};

if elements.is_empty() {
call.args = vec![];
} else {
tree = Expression::Set(Box::new(Set {
elements,
lbrace: LeftCurlyBrace { whitespace_after },
rbrace: RightCurlyBrace { whitespace_before },
lpar: vec![],
rpar: vec![],
}));
}

Ok(Edit::range_replacement(
pad_expression(
tree.codegen_stylist(stylist),
expr.range(),
checker.locator(),
checker.semantic(),
),
expr.range(),
))
}

/// (C406) Convert `dict([(1, 2)])` to `{1: 2}`.
pub(crate) fn fix_unnecessary_literal_dict(expr: &Expr, checker: &Checker) -> Result<Edit> {
let locator = checker.locator();
Expand Down Expand Up @@ -511,56 +422,6 @@ pub(crate) fn pad_end(
}
}

/// (C409) Convert `tuple([1, 2])` to `tuple(1, 2)`
pub(crate) fn fix_unnecessary_literal_within_tuple_call(
expr: &Expr,
locator: &Locator,
stylist: &Stylist,
) -> Result<Edit> {
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 (elements, whitespace_after, whitespace_before) = match &arg.value {
Expression::Tuple(inner) => (
&inner.elements,
&inner
.lpar
.first()
.ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))?
.whitespace_after,
&inner
.rpar
.first()
.ok_or_else(|| anyhow::anyhow!("Expected at least one set of parentheses"))?
.whitespace_before,
),
Expression::List(inner) => (
&inner.elements,
&inner.lbracket.whitespace_after,
&inner.rbracket.whitespace_before,
),
_ => {
bail!("Expected Expression::Tuple | Expression::List");
}
};

tree = Expression::Tuple(Box::new(Tuple {
elements: elements.clone(),
lpar: vec![LeftParen {
whitespace_after: whitespace_after.clone(),
}],
rpar: vec![RightParen {
whitespace_before: whitespace_before.clone(),
}],
}));

Ok(Edit::range_replacement(
tree.codegen_stylist(stylist),
expr.range(),
))
}

/// (C411) Convert `list([i * i for i in x])` to `[i * i for i in x]`.
pub(crate) fn fix_unnecessary_list_call(
expr: &Expr,
Expand Down
89 changes: 70 additions & 19 deletions crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_literal_set.rs
View file
Original file line number Diff line number Diff line change
@@ -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::{self as ast, Expr};
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;

Expand Down Expand Up @@ -53,16 +51,13 @@ impl AlwaysFixableViolation for UnnecessaryLiteralSet {
}

/// C405 (`set([1, 2])`)
pub(crate) fn unnecessary_literal_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_literal_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") {
Expand All @@ -73,13 +68,69 @@ pub(crate) fn unnecessary_literal_set(
Expr::Tuple(_) => "tuple",
_ => return,
};

let mut diagnostic = Diagnostic::new(
UnnecessaryLiteralSet {
obj_type: kind.to_string(),
},
expr.range(),
call.range(),
);
diagnostic
.try_set_fix(|| fixes::fix_unnecessary_literal_set(expr, checker).map(Fix::unsafe_edit));

// Convert `set((1, 2))` to `{1, 2}`.
diagnostic.set_fix({
let elts = match &argument {
Expr::List(ast::ExprList { elts, .. }) => elts,
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts,
_ => unreachable!(),
};

match elts.as_slice() {
// If the list or tuple is empty, replace the entire call with `set()`.
[] => Fix::unsafe_edit(Edit::range_replacement("set()".to_string(), call.range())),
// If it's a single-element tuple (with no whitespace around it), remove the trailing
// comma.
[elt]
if argument.is_tuple_expr()
// The element must start right after the `(`.
&& elt.start() == argument.start() + TextSize::new(1)
// The element must be followed by exactly one comma and a closing `)`.
&& elt.end() + TextSize::new(2) == argument.end() =>
{
// Replace from the start of the call to the start of the inner element.
let call_start = Edit::replacement(
pad_start("{", call.range(), checker.locator(), checker.semantic()),
call.start(),
elt.start(),
);

// Replace from the end of the inner element to the end of the call with `}`.
let call_end = Edit::replacement(
pad_end("}", call.range(), checker.locator(), checker.semantic()),
elt.end(),
call.end(),
);

Fix::unsafe_edits(call_start, [call_end])
}
_ => {
// Replace from the start of the call to the start of the inner list or tuple with `{`.
let call_start = Edit::replacement(
pad_start("{", call.range(), checker.locator(), checker.semantic()),
call.start(),
argument.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(
pad_end("}", call.range(), checker.locator(), checker.semantic()),
argument.end() - TextSize::from(1),
call.end(),
);

Fix::unsafe_edits(call_start, [call_end])
}
}
});

checker.diagnostics.push(diagnostic);
}
Loading

0 comments on commit a6bc4b2

Please sign in to comment.