Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove CST-based fixer for C408 #9822

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Remove CST-based fixer for C408 #9822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C408.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
5 changes: 1 addition & 4 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}
Expand Down
170 changes: 63 additions & 107 deletions crates/ruff_linter/src/rules/flake8_comprehensions/fixes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
},
};

Expand Down Expand Up @@ -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<Edit> {
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<Edit> {
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<String> = 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(),
))
}
Expand Down
84 changes: 65 additions & 19 deletions crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_collection_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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,
}
Loading
Loading