[parser] Flag single unparenthesized generator expr with trailing comma in arguments. by abhijeetbodas2001 · Pull Request #17893 · astral-sh/ruff

abhijeetbodas2001 · 2025-05-06T17:31:30Z

Summary

The CPython parser does not allow generator expressions which are the sole arguments in an argument list to have a trailing comma.
With this change, we start flagging such instances.

Test Plan

Added new inline tests.

…ma in arguments.

abhijeetbodas2001 · 2025-05-06T17:33:17Z

crates/ruff_python_parser/src/parser/mod.rs


        self.recovery_context = saved_context;
+
+        trailing_comma_range


Instead of special-casing inside parse_comma_separated_list itself, I felt returning the trailing_comma_range to be simpler. But please let me know if this feels hacky.

This looks reasonable to me (and the change is quite small overall once I hid the whitespace diff), but I have two ideas here:

Can we just return a bool instead of an Option<TextRange>? It looks like we only ever use the Option to call is_some, so we could just call is_some in the return here.

Could we update the recovery_context_kind we pass in here so that we reuse the existing OtherError on line 657?

The second of these would prevent the need for the first and seems like a more robust approach, but I'm not really familiar with the available RecoveryContextKinds. If we can't get that to work, I think the current approach (with the bool modification) would be fine. @dhruvmanila would know better than me though!

The context is RecoveryContextKind::Arguments, which alows trailing commas.

By updating recovery_context_kind, do you mean making it so that OtherError is raised even for RecoveryContextKind::Arguments? And later on replace that OtherError with the more specialized UnparenthesizedGeneratorExpression (or remove it entirely, if generators are not involved)?

I addressed the first point and pushed here.
Happy to revert/change if we decide on the other way.

Sorry for the confusion, I pulled down the PR locally, and I think I understand what's going on better now. My suggestion was basically a worse wording of your idea to add special-casing inside of parse_comma_separated_list, but I see now that the proper error (UnparenthesizedGeneratorExpression) is emitted from validate_arguments, so I think your implementation totally makes sense.

abhijeetbodas2001 · 2025-05-06T17:33:44Z

@ntBre could you please review?

crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py

ntBre · 2025-05-06T18:25:50Z

crates/ruff_python_parser/src/parser/expression.rs

-                        }
-                    };
-
+        let trailing_comma_range =


This is just a note to other potential reviewers, but I highly recommend hiding whitespace from the diff here. The only change in this function is capturing the value trailing_comma_range, which is now returned by parse_comma_separated_list.

(I also did not know about this setting. Thank you!)

ntBre · 2025-05-06T18:33:33Z

crates/ruff_python_parser/src/parser/mod.rs


        self.recovery_context = saved_context;
+
+        trailing_comma_range


This looks reasonable to me (and the change is quite small overall once I hid the whitespace diff), but I have two ideas here:

Can we just return a bool instead of an Option<TextRange>? It looks like we only ever use the Option to call is_some, so we could just call is_some in the return here.

Could we update the recovery_context_kind we pass in here so that we reuse the existing OtherError on line 657?

The second of these would prevent the need for the first and seems like a more robust approach, but I'm not really familiar with the available RecoveryContextKinds. If we can't get that to work, I think the current approach (with the bool modification) would be fine. @dhruvmanila would know better than me though!

github-actions · 2025-05-06T18:53:51Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

mesonbuild/meson-python (error)

warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

mesonbuild/meson-python (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)

abhijeetbodas2001 · 2025-05-06T19:04:14Z

Thanks for reviewing, Brent. I addressed all but one comment.
Please see the question at #17893 (comment)

ntBre

Thanks, this looks good to me! And thanks for clearing up my confusion from earlier. I found a couple of minor nits, but otherwise I just want to give Dhruv a chance to look at this if he wants to.

crates/ruff_python_parser/src/parser/expression.rs

ntBre · 2025-05-06T20:02:50Z

crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py

@@ -1 +1,2 @@
 sum(x for x in range(10))
+sum((x for x in range(10)),)


I guess one more nit while I'm here, this is slightly confusing with the name of the test (unparenthesized_generator), but it's also annoying to have to rename snapshots and such, so I'm okay with leaving it as is.

Hmm, if the premise is that un-parenthesized expressions are bad, then I think it would be natural for the "ok" parts of the test to be good (ie, have parens)?

I actually added one more inline test here. Not related to the changes in this PR specifically, but I felt the case of multiple arguments was missing from the "ok" section.

abhijeetbodas2001 · 2025-05-07T06:09:05Z

Thanks Brent. I've addressed the review comments. For the test name -- IMO the name isn't exactly incorrect, so I'd prefer to keep it the same way.
(Although I did add a new inline test in the "ok" section which you might want to review.)

dhruvmanila

Thank you!

ntBre

Looks great, thank you! I think it's fine to leave the test name as is too.

…lass * origin/main: [`pylint`] add fix safety section (`PLC2801`) (#17825) Add instructions on how to upgrade to a newer Rust version (#17928) [parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893) [ty] Ensure that `T` is disjoint from `~T` even when `T` is a TypeVar (#17922) [ty] Sort collected diagnostics before snapshotting them in mdtest (#17926) [ty] Add basic file watching to server (#17912) Make completions an opt-in LSP feature (#17921) Add link to `ty` issue tracker (#17924) [ty] Add support for `__all__` (#17856) [ty] fix assigning a typevar to a union with itself (#17910) [ty] Improve UX for `[duplicate-base]` diagnostics (#17914) Clean up some Ruff references in the ty server (#17920)

[parser] Flag single unparenthesized generator expr with trailing com…

7fca896

…ma in arguments.

abhijeetbodas2001 requested review from MichaReiser and dhruvmanila as code owners May 6, 2025 17:31

abhijeetbodas2001 commented May 6, 2025

View reviewed changes

ntBre reviewed May 6, 2025

View reviewed changes

fix test

dfb734b

returns has_trailing_comma bool instead of Opt<TextRange>

12b874e

ntBre approved these changes May 6, 2025

View reviewed changes

ntBre added bug Something isn't working parser Related to the parser labels May 6, 2025

abhijeetbodas2001 added 2 commits May 7, 2025 11:29

address review feedback

25e921d

add one more inlint test

c984b73

MichaReiser approved these changes May 7, 2025

View reviewed changes

dhruvmanila approved these changes May 7, 2025

View reviewed changes

ntBre approved these changes May 7, 2025

View reviewed changes

ntBre merged commit f5096f2 into astral-sh:main May 7, 2025
34 checks passed

abhijeetbodas2001 deleted the syntax-error branch May 7, 2025 18:19

BrewTestBot mentioned this pull request May 9, 2025

ruff 0.11.9 Homebrew/homebrew-core#222996

Merged

		@@ -1 +1,2 @@
		sum(x for x in range(10))
		sum((x for x in range(10)),)

Comments

Conversation

abhijeetbodas2001 commented May 6, 2025 • edited by MichaReiser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 commented May 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

abhijeetbodas2001 commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijeetbodas2001 commented May 7, 2025

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

ntBre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abhijeetbodas2001 commented May 6, 2025 •

edited by MichaReiser

Loading

abhijeetbodas2001 May 6, 2025 •

edited

Loading

github-actions bot commented May 6, 2025 •

edited

Loading

`ruff-ecosystem` results

abhijeetbodas2001 commented May 6, 2025 •

edited

Loading

abhijeetbodas2001 May 7, 2025 •

edited

Loading