From c69b19fe1ddcd3cc57a5fa7f0541037465c277b7 Mon Sep 17 00:00:00 2001 From: Dylan <53534755+dylwil3@users.noreply.github.com> Date: Wed, 5 Feb 2025 07:38:03 -0600 Subject: [PATCH] [`flake8-comprehensions`] Handle trailing comma in fixes for `unnecessary-generator-list/set` (`C400`,`C401`) (#15929) The unsafe fixes for the rules [unnecessary-generator-list (C400)](https://docs.astral.sh/ruff/rules/unnecessary-generator-list/#unnecessary-generator-list-c400) and [unnecessary-generator-set (C401)](https://docs.astral.sh/ruff/rules/unnecessary-generator-set/#unnecessary-generator-set-c401) used to introduce syntax errors if the argument to `list` or `set` had a trailing comma, because the fix would retain the comma after transforming the function call to a comprehension. This PR accounts for the trailing comma when replacing the end of the call with a `]` or `}`. Closes #15852 --- .../fixtures/flake8_comprehensions/C400.py | 11 ++++ .../fixtures/flake8_comprehensions/C401.py | 10 +++ .../rules/unnecessary_generator_list.rs | 16 +++-- .../rules/unnecessary_generator_set.rs | 12 +++- ...8_comprehensions__tests__C400_C400.py.snap | 60 ++++++++++++++++-- ...8_comprehensions__tests__C401_C401.py.snap | 62 +++++++++++++++++-- 6 files changed, 155 insertions(+), 16 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py index 5c347f72ad..465e1f32d0 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C400.py @@ -16,6 +16,17 @@ list((2 * x for x in range(3))) list(((2 * x for x in range(3)))) list((((2 * x for x in range(3))))) +# Account for trailing comma in fix +# See https://github.com/astral-sh/ruff/issues/15852 +list((0 for _ in []),) +list( + (0 for _ in []) + # some comments + , + # some more + ) + + # Not built-in list. def list(*args, **kwargs): return None diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C401.py b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C401.py index e6e488312d..7d7ee4c556 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C401.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_comprehensions/C401.py @@ -26,6 +26,16 @@ set((2 * x for x in range(3))) set(((2 * x for x in range(3)))) set((((2 * x for x in range(3))))) +# Account for trailing comma in fix +# See https://github.com/astral-sh/ruff/issues/15852 +set((0 for _ in []),) +set( + (0 for _ in []) + # some comments + , + # some more +) + # Not built-in set. def set(*args, **kwargs): return None diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs index cf3d0cb2ad..c242f241d1 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_list.rs @@ -4,7 +4,8 @@ use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::ExprGenerator; -use ruff_text_size::{Ranged, TextSize}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::checkers::ast::Checker; @@ -123,11 +124,14 @@ pub(crate) fn unnecessary_generator_list(checker: &mut Checker, call: &ast::Expr ); // Replace `)` with `]`. - let call_end = Edit::replacement( - "]".to_string(), - call.arguments.end() - TextSize::from(1), - call.end(), - ); + // Place `]` at argument's end or at trailing comma if present + let mut tokenizer = + SimpleTokenizer::new(checker.source(), TextRange::new(argument.end(), call.end())); + let right_bracket_loc = tokenizer + .find(|token| token.kind == SimpleTokenKind::Comma) + .map_or(call.arguments.end(), |comma| comma.end()) + - TextSize::from(1); + let call_end = Edit::replacement("]".to_string(), right_bracket_loc, call.end()); // Remove the inner parentheses, if the expression is a generator. The easiest way to do // this reliably is to use the printer. diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs index 59803b0c03..5711b2696f 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/rules/unnecessary_generator_set.rs @@ -4,7 +4,8 @@ use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableExpr; use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::ExprGenerator; -use ruff_text_size::{Ranged, TextSize}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; +use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::checkers::ast::Checker; use crate::rules::flake8_comprehensions::fixes::{pad_end, pad_start}; @@ -126,9 +127,16 @@ pub(crate) fn unnecessary_generator_set(checker: &mut Checker, call: &ast::ExprC ); // Replace `)` with `}`. + // Place `}` at argument's end or at trailing comma if present + let mut tokenizer = + SimpleTokenizer::new(checker.source(), TextRange::new(argument.end(), call.end())); + let right_brace_loc = tokenizer + .find(|token| token.kind == SimpleTokenKind::Comma) + .map_or(call.arguments.end(), |comma| comma.end()) + - TextSize::from(1); let call_end = Edit::replacement( pad_end("}", call.range(), checker.locator(), checker.semantic()), - call.arguments.end() - TextSize::from(1), + right_brace_loc, call.end(), ); diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap index f5a75e8d17..fac96417f0 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C400_C400.py.snap @@ -127,7 +127,7 @@ C400.py:16:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) 16 |+[2 * x for x in range(3)] 17 17 | list((((2 * x for x in range(3))))) 18 18 | -19 19 | # Not built-in list. +19 19 | # Account for trailing comma in fix C400.py:17:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) | @@ -136,7 +136,7 @@ C400.py:17:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) 17 | list((((2 * x for x in range(3))))) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C400 18 | -19 | # Not built-in list. +19 | # Account for trailing comma in fix | = help: Rewrite as a list comprehension @@ -147,5 +147,57 @@ C400.py:17:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) 17 |-list((((2 * x for x in range(3))))) 17 |+[2 * x for x in range(3)] 18 18 | -19 19 | # Not built-in list. -20 20 | def list(*args, **kwargs): +19 19 | # Account for trailing comma in fix +20 20 | # See https://github.com/astral-sh/ruff/issues/15852 + +C400.py:21:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) + | +19 | # Account for trailing comma in fix +20 | # See https://github.com/astral-sh/ruff/issues/15852 +21 | list((0 for _ in []),) + | ^^^^^^^^^^^^^^^^^^^^^^ C400 +22 | list( +23 | (0 for _ in []) + | + = help: Rewrite as a list comprehension + +ℹ Unsafe fix +18 18 | +19 19 | # Account for trailing comma in fix +20 20 | # See https://github.com/astral-sh/ruff/issues/15852 +21 |-list((0 for _ in []),) + 21 |+[0 for _ in []] +22 22 | list( +23 23 | (0 for _ in []) +24 24 | # some comments + +C400.py:22:1: C400 [*] Unnecessary generator (rewrite as a list comprehension) + | +20 | # See https://github.com/astral-sh/ruff/issues/15852 +21 | list((0 for _ in []),) +22 | / list( +23 | | (0 for _ in []) +24 | | # some comments +25 | | , +26 | | # some more +27 | | ) + | |__^ C400 + | + = help: Rewrite as a list comprehension + +ℹ Unsafe fix +19 19 | # Account for trailing comma in fix +20 20 | # See https://github.com/astral-sh/ruff/issues/15852 +21 21 | list((0 for _ in []),) +22 |-list( +23 |- (0 for _ in []) + 22 |+[ + 23 |+ 0 for _ in [] +24 24 | # some comments +25 |- , +26 |- # some more +27 |- ) + 25 |+ ] +28 26 | +29 27 | +30 28 | # Not built-in list. diff --git a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C401_C401.py.snap b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C401_C401.py.snap index 87579feb4b..7ca2ce2725 100644 --- a/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C401_C401.py.snap +++ b/crates/ruff_linter/src/rules/flake8_comprehensions/snapshots/ruff_linter__rules__flake8_comprehensions__tests__C401_C401.py.snap @@ -290,7 +290,7 @@ C401.py:26:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) 26 |+{2 * x for x in range(3)} 27 27 | set((((2 * x for x in range(3))))) 28 28 | -29 29 | # Not built-in set. +29 29 | # Account for trailing comma in fix C401.py:27:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) | @@ -299,7 +299,7 @@ C401.py:27:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) 27 | set((((2 * x for x in range(3))))) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ C401 28 | -29 | # Not built-in set. +29 | # Account for trailing comma in fix | = help: Rewrite as a set comprehension @@ -310,5 +310,59 @@ C401.py:27:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) 27 |-set((((2 * x for x in range(3))))) 27 |+{2 * x for x in range(3)} 28 28 | -29 29 | # Not built-in set. -30 30 | def set(*args, **kwargs): +29 29 | # Account for trailing comma in fix +30 30 | # See https://github.com/astral-sh/ruff/issues/15852 + +C401.py:31:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) + | +29 | # Account for trailing comma in fix +30 | # See https://github.com/astral-sh/ruff/issues/15852 +31 | set((0 for _ in []),) + | ^^^^^^^^^^^^^^^^^^^^^ C401 +32 | set( +33 | (0 for _ in []) + | + = help: Rewrite as a set comprehension + +ℹ Unsafe fix +28 28 | +29 29 | # Account for trailing comma in fix +30 30 | # See https://github.com/astral-sh/ruff/issues/15852 +31 |-set((0 for _ in []),) + 31 |+{0 for _ in []} +32 32 | set( +33 33 | (0 for _ in []) +34 34 | # some comments + +C401.py:32:1: C401 [*] Unnecessary generator (rewrite as a set comprehension) + | +30 | # See https://github.com/astral-sh/ruff/issues/15852 +31 | set((0 for _ in []),) +32 | / set( +33 | | (0 for _ in []) +34 | | # some comments +35 | | , +36 | | # some more +37 | | ) + | |_^ C401 +38 | +39 | # Not built-in set. + | + = help: Rewrite as a set comprehension + +ℹ Unsafe fix +29 29 | # Account for trailing comma in fix +30 30 | # See https://github.com/astral-sh/ruff/issues/15852 +31 31 | set((0 for _ in []),) +32 |-set( +33 |- (0 for _ in []) + 32 |+{ + 33 |+ 0 for _ in [] +34 34 | # some comments +35 |- , +36 |- # some more +37 |-) + 35 |+ } +38 36 | +39 37 | # Not built-in set. +40 38 | def set(*args, **kwargs):