Skip to content

Commit

Permalink
Avoid incorrect tuple transformation in single-element case (C409) (#…
Browse files Browse the repository at this point in the history
…10491)

# Summary
Fixed: incorrect rule transformation rule C409 with single element.

# Test Plan
Added examples from #10323 to test fixtures.
  • Loading branch information
WindowGenerator authored Mar 21, 2024
1 parent 9d705a4 commit 4045df4
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,11 @@
tuple([ # comment
1, 2
])

tuple((
1,
))

t6 = tuple([1])
t7 = tuple((1,))
t8 = tuple([1,])
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_text_size::{Ranged, TextSize};
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::{Ranged, TextRange, TextSize};

use crate::checkers::ast::Checker;

Expand Down Expand Up @@ -71,9 +72,12 @@ pub(crate) fn unnecessary_literal_within_tuple_call(checker: &mut Checker, call:
if !call.arguments.keywords.is_empty() {
return;
}
let Some(argument) =
helpers::first_argument_with_matching_function("tuple", &call.func, &call.arguments.args)
else {
let Some(argument) = helpers::exactly_one_argument_with_matching_function(
"tuple",
&call.func,
&call.arguments.args,
&call.arguments.keywords,
) else {
return;
};
if !checker.semantic().is_builtin("tuple") {
Expand All @@ -92,23 +96,41 @@ pub(crate) fn unnecessary_literal_within_tuple_call(checker: &mut Checker, call:
call.range(),
);

// Convert `tuple([1, 2])` to `tuple(1, 2)`
// Convert `tuple([1, 2])` to `(1, 2)`
diagnostic.set_fix({
// Replace from the start of the call to the start of the inner list or tuple with `(`.
let call_start = Edit::replacement(
"(".to_string(),
let elts = match argument {
Expr::List(ast::ExprList { elts, .. }) => elts.as_slice(),
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.as_slice(),
_ => return,
};

let needs_trailing_comma = if let [item] = elts {
SimpleTokenizer::new(
checker.locator().contents(),
TextRange::new(item.end(), call.end()),
)
.all(|token| token.kind != SimpleTokenKind::Comma)
} else {
false
};

// Replace `[` with `(`.
let elt_start = Edit::replacement(
"(".into(),
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(
")".to_string(),
// Replace `]` with `)` or `,)`.
let elt_end = Edit::replacement(
if needs_trailing_comma {
",)".into()
} else {
")".into()
},
argument.end() - TextSize::from(1),
call.end(),
);

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

checker.diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ C409.py:16:1: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite a
17 | | 1, 2
18 | | ])
| |__^ C409
19 |
20 | tuple((
|
= help: Rewrite as a `tuple` literal

Expand All @@ -155,5 +157,85 @@ C409.py:16:1: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite a
17 17 | 1, 2
18 |-])
18 |+)
19 19 |
20 20 | tuple((
21 21 | 1,

C409.py:20:1: C409 [*] Unnecessary `tuple` literal passed to `tuple()` (remove the outer call to `tuple()`)
|
18 | ])
19 |
20 | / tuple((
21 | | 1,
22 | | ))
| |__^ C409
23 |
24 | t6 = tuple([1])
|
= help: Remove outer `tuple` call

Unsafe fix
17 17 | 1, 2
18 18 | ])
19 19 |
20 |-tuple((
20 |+(
21 21 | 1,
22 |-))
22 |+)
23 23 |
24 24 | t6 = tuple([1])
25 25 | t7 = tuple((1,))

C409.py:24:6: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite as a `tuple` literal)
|
22 | ))
23 |
24 | t6 = tuple([1])
| ^^^^^^^^^^ C409
25 | t7 = tuple((1,))
26 | t8 = tuple([1,])
|
= help: Rewrite as a `tuple` literal

Unsafe fix
21 21 | 1,
22 22 | ))
23 23 |
24 |-t6 = tuple([1])
24 |+t6 = (1,)
25 25 | t7 = tuple((1,))
26 26 | t8 = tuple([1,])

C409.py:25:6: C409 [*] Unnecessary `tuple` literal passed to `tuple()` (remove the outer call to `tuple()`)
|
24 | t6 = tuple([1])
25 | t7 = tuple((1,))
| ^^^^^^^^^^^ C409
26 | t8 = tuple([1,])
|
= help: Remove outer `tuple` call

Unsafe fix
22 22 | ))
23 23 |
24 24 | t6 = tuple([1])
25 |-t7 = tuple((1,))
25 |+t7 = (1,)
26 26 | t8 = tuple([1,])

C409.py:26:6: C409 [*] Unnecessary `list` literal passed to `tuple()` (rewrite as a `tuple` literal)
|
24 | t6 = tuple([1])
25 | t7 = tuple((1,))
26 | t8 = tuple([1,])
| ^^^^^^^^^^^ C409
|
= help: Rewrite as a `tuple` literal

Unsafe fix
23 23 |
24 24 | t6 = tuple([1])
25 25 | t7 = tuple((1,))
26 |-t8 = tuple([1,])
26 |+t8 = (1,)

0 comments on commit 4045df4

Please sign in to comment.