From f5096f205006c11ec12094cbd293cce0c4a1f730 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Bodas <55339528+abhijeetbodas2001@users.noreply.github.com> Date: Wed, 7 May 2025 23:41:35 +0530 Subject: [PATCH] [parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893) Fixes #17867 ## 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. --- .../err/args_unparenthesized_generator.py | 1 + .../ok/args_unparenthesized_generator.py | 2 + .../src/parser/expression.rs | 218 ++++++++-------- crates/ruff_python_parser/src/parser/mod.rs | 8 +- ...tax@args_unparenthesized_generator.py.snap | 89 ++++++- ...tax@args_unparenthesized_generator.py.snap | 232 ++++++++++++++++-- 6 files changed, 426 insertions(+), 124 deletions(-) diff --git a/crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py b/crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py index 45f01b2ea2..2252aae5db 100644 --- a/crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py +++ b/crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py @@ -1,2 +1,3 @@ sum(x for x in range(10), 5) total(1, 2, x for x in range(5), 6) +sum(x for x in range(10),) diff --git a/crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py b/crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py index ecadabd4e3..a42c0867fb 100644 --- a/crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py +++ b/crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py @@ -1 +1,3 @@ +zip((x for x in range(10)), (y for y in range(10))) sum(x for x in range(10)) +sum((x for x in range(10)),) diff --git a/crates/ruff_python_parser/src/parser/expression.rs b/crates/ruff_python_parser/src/parser/expression.rs index 3ccfb99351..3bc7072ce3 100644 --- a/crates/ruff_python_parser/src/parser/expression.rs +++ b/crates/ruff_python_parser/src/parser/expression.rs @@ -661,117 +661,120 @@ impl<'src> Parser<'src> { let mut seen_keyword_argument = false; // foo = 1 let mut seen_keyword_unpacking = false; // **foo - self.parse_comma_separated_list(RecoveryContextKind::Arguments, |parser| { - let argument_start = parser.node_start(); - if parser.eat(TokenKind::DoubleStar) { - let value = parser.parse_conditional_expression_or_higher(); - - keywords.push(ast::Keyword { - arg: None, - value: value.expr, - range: parser.node_range(argument_start), - }); - - seen_keyword_unpacking = true; - } else { - let start = parser.node_start(); - let mut parsed_expr = parser - .parse_named_expression_or_higher(ExpressionContext::starred_conditional()); - - match parser.current_token_kind() { - TokenKind::Async | TokenKind::For => { - if parsed_expr.is_unparenthesized_starred_expr() { - parser.add_error( - ParseErrorType::IterableUnpackingInComprehension, - &parsed_expr, - ); - } - - parsed_expr = Expr::Generator(parser.parse_generator_expression( - parsed_expr.expr, - start, - Parenthesized::No, - )) - .into(); - } - _ => { - if seen_keyword_unpacking && parsed_expr.is_unparenthesized_starred_expr() { - parser.add_error( - ParseErrorType::InvalidArgumentUnpackingOrder, - &parsed_expr, - ); - } - } - } - - let arg_range = parser.node_range(start); - if parser.eat(TokenKind::Equal) { - seen_keyword_argument = true; - let arg = if let ParsedExpr { - expr: Expr::Name(ident_expr), - is_parenthesized, - } = parsed_expr - { - // test_ok parenthesized_kwarg_py37 - // # parse_options: {"target-version": "3.7"} - // f((a)=1) - - // test_err parenthesized_kwarg_py38 - // # parse_options: {"target-version": "3.8"} - // f((a)=1) - // f((a) = 1) - // f( ( a ) = 1) - - if is_parenthesized { - parser.add_unsupported_syntax_error( - UnsupportedSyntaxErrorKind::ParenthesizedKeywordArgumentName, - arg_range, - ); - } - - ast::Identifier { - id: ident_expr.id, - range: ident_expr.range, - } - } else { - // TODO(dhruvmanila): Parser shouldn't drop the `parsed_expr` if it's - // not a name expression. We could add the expression into `args` but - // that means the error is a missing comma instead. - parser.add_error( - ParseErrorType::OtherError("Expected a parameter name".to_string()), - &parsed_expr, - ); - ast::Identifier { - id: Name::empty(), - range: parsed_expr.range(), - } - }; - + let has_trailing_comma = + self.parse_comma_separated_list(RecoveryContextKind::Arguments, |parser| { + let argument_start = parser.node_start(); + if parser.eat(TokenKind::DoubleStar) { let value = parser.parse_conditional_expression_or_higher(); keywords.push(ast::Keyword { - arg: Some(arg), + arg: None, value: value.expr, range: parser.node_range(argument_start), }); + + seen_keyword_unpacking = true; } else { - if !parsed_expr.is_unparenthesized_starred_expr() { - if seen_keyword_unpacking { - parser.add_error( - ParseErrorType::PositionalAfterKeywordUnpacking, - &parsed_expr, - ); - } else if seen_keyword_argument { - parser.add_error( - ParseErrorType::PositionalAfterKeywordArgument, - &parsed_expr, - ); + let start = parser.node_start(); + let mut parsed_expr = parser + .parse_named_expression_or_higher(ExpressionContext::starred_conditional()); + + match parser.current_token_kind() { + TokenKind::Async | TokenKind::For => { + if parsed_expr.is_unparenthesized_starred_expr() { + parser.add_error( + ParseErrorType::IterableUnpackingInComprehension, + &parsed_expr, + ); + } + + parsed_expr = Expr::Generator(parser.parse_generator_expression( + parsed_expr.expr, + start, + Parenthesized::No, + )) + .into(); + } + _ => { + if seen_keyword_unpacking + && parsed_expr.is_unparenthesized_starred_expr() + { + parser.add_error( + ParseErrorType::InvalidArgumentUnpackingOrder, + &parsed_expr, + ); + } } } - args.push(parsed_expr.expr); + + let arg_range = parser.node_range(start); + if parser.eat(TokenKind::Equal) { + seen_keyword_argument = true; + let arg = if let ParsedExpr { + expr: Expr::Name(ident_expr), + is_parenthesized, + } = parsed_expr + { + // test_ok parenthesized_kwarg_py37 + // # parse_options: {"target-version": "3.7"} + // f((a)=1) + + // test_err parenthesized_kwarg_py38 + // # parse_options: {"target-version": "3.8"} + // f((a)=1) + // f((a) = 1) + // f( ( a ) = 1) + + if is_parenthesized { + parser.add_unsupported_syntax_error( + UnsupportedSyntaxErrorKind::ParenthesizedKeywordArgumentName, + arg_range, + ); + } + + ast::Identifier { + id: ident_expr.id, + range: ident_expr.range, + } + } else { + // TODO(dhruvmanila): Parser shouldn't drop the `parsed_expr` if it's + // not a name expression. We could add the expression into `args` but + // that means the error is a missing comma instead. + parser.add_error( + ParseErrorType::OtherError("Expected a parameter name".to_string()), + &parsed_expr, + ); + ast::Identifier { + id: Name::empty(), + range: parsed_expr.range(), + } + }; + + let value = parser.parse_conditional_expression_or_higher(); + + keywords.push(ast::Keyword { + arg: Some(arg), + value: value.expr, + range: parser.node_range(argument_start), + }); + } else { + if !parsed_expr.is_unparenthesized_starred_expr() { + if seen_keyword_unpacking { + parser.add_error( + ParseErrorType::PositionalAfterKeywordUnpacking, + &parsed_expr, + ); + } else if seen_keyword_argument { + parser.add_error( + ParseErrorType::PositionalAfterKeywordArgument, + &parsed_expr, + ); + } + } + args.push(parsed_expr.expr); + } } - } - }); + }); self.expect(TokenKind::Rpar); @@ -781,7 +784,7 @@ impl<'src> Parser<'src> { keywords: keywords.into_boxed_slice(), }; - self.validate_arguments(&arguments); + self.validate_arguments(&arguments, has_trailing_comma); arguments } @@ -2521,9 +2524,9 @@ impl<'src> Parser<'src> { /// Performs the following validations on the function call arguments: /// 1. There aren't any duplicate keyword argument - /// 2. If there are more than one argument (positional or keyword), all generator expressions - /// present should be parenthesized. - fn validate_arguments(&mut self, arguments: &ast::Arguments) { + /// 2. If there are more than one argument (positional or keyword) or a single argument with a + /// trailing comma, all generator expressions present should be parenthesized. + fn validate_arguments(&mut self, arguments: &ast::Arguments, has_trailing_comma: bool) { let mut all_arg_names = FxHashSet::with_capacity_and_hasher(arguments.keywords.len(), FxBuildHasher); @@ -2541,7 +2544,7 @@ impl<'src> Parser<'src> { } } - if arguments.len() > 1 { + if has_trailing_comma || arguments.len() > 1 { for arg in &*arguments.args { if let Some(ast::ExprGenerator { range, @@ -2550,11 +2553,14 @@ impl<'src> Parser<'src> { }) = arg.as_generator_expr() { // test_ok args_unparenthesized_generator + // zip((x for x in range(10)), (y for y in range(10))) // sum(x for x in range(10)) + // sum((x for x in range(10)),) // test_err args_unparenthesized_generator // sum(x for x in range(10), 5) // total(1, 2, x for x in range(5), 6) + // sum(x for x in range(10),) self.add_error(ParseErrorType::UnparenthesizedGeneratorExpression, range); } } diff --git a/crates/ruff_python_parser/src/parser/mod.rs b/crates/ruff_python_parser/src/parser/mod.rs index c602ba7c75..19596364a8 100644 --- a/crates/ruff_python_parser/src/parser/mod.rs +++ b/crates/ruff_python_parser/src/parser/mod.rs @@ -539,17 +539,19 @@ impl<'src> Parser<'src> { } /// Parses a comma separated list of elements where each element is parsed - /// sing the given `parse_element` function. + /// using the given `parse_element` function. /// /// The difference between this function and `parse_comma_separated_list_into_vec` /// is that this function does not return the parsed elements. Instead, it is the /// caller's responsibility to handle the parsed elements. This is the reason /// that the `parse_element` parameter is bound to [`FnMut`] instead of [`Fn`]. + /// + /// Returns `true` if there is a trailing comma present. fn parse_comma_separated_list( &mut self, recovery_context_kind: RecoveryContextKind, mut parse_element: impl FnMut(&mut Parser<'src>), - ) { + ) -> bool { let mut progress = ParserProgress::default(); let saved_context = self.recovery_context; @@ -659,6 +661,8 @@ impl<'src> Parser<'src> { } self.recovery_context = saved_context; + + trailing_comma_range.is_some() } #[cold] diff --git a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@args_unparenthesized_generator.py.snap b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@args_unparenthesized_generator.py.snap index 47aa86d8a0..b4050ccf13 100644 --- a/crates/ruff_python_parser/tests/snapshots/invalid_syntax@args_unparenthesized_generator.py.snap +++ b/crates/ruff_python_parser/tests/snapshots/invalid_syntax@args_unparenthesized_generator.py.snap @@ -1,14 +1,13 @@ --- source: crates/ruff_python_parser/tests/fixtures.rs input_file: crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py -snapshot_kind: text --- ## AST ``` Module( ModModule { - range: 0..65, + range: 0..92, body: [ Expr( StmtExpr { @@ -194,6 +193,82 @@ Module( ), }, ), + Expr( + StmtExpr { + range: 65..91, + value: Call( + ExprCall { + range: 65..91, + func: Name( + ExprName { + range: 65..68, + id: Name("sum"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 68..91, + args: [ + Generator( + ExprGenerator { + range: 69..89, + elt: Name( + ExprName { + range: 69..70, + id: Name("x"), + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 71..89, + target: Name( + ExprName { + range: 75..76, + id: Name("x"), + ctx: Store, + }, + ), + iter: Call( + ExprCall { + range: 80..89, + func: Name( + ExprName { + range: 80..85, + id: Name("range"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 85..89, + args: [ + NumberLiteral( + ExprNumberLiteral { + range: 86..88, + value: Int( + 10, + ), + }, + ), + ], + keywords: [], + }, + }, + ), + ifs: [], + is_async: false, + }, + ], + parenthesized: false, + }, + ), + ], + keywords: [], + }, + }, + ), + }, + ), ], }, ) @@ -204,6 +279,7 @@ Module( 1 | sum(x for x in range(10), 5) | ^^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here 2 | total(1, 2, x for x in range(5), 6) +3 | sum(x for x in range(10),) | @@ -211,4 +287,13 @@ Module( 1 | sum(x for x in range(10), 5) 2 | total(1, 2, x for x in range(5), 6) | ^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here +3 | sum(x for x in range(10),) + | + + + | +1 | sum(x for x in range(10), 5) +2 | total(1, 2, x for x in range(5), 6) +3 | sum(x for x in range(10),) + | ^^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here | diff --git a/crates/ruff_python_parser/tests/snapshots/valid_syntax@args_unparenthesized_generator.py.snap b/crates/ruff_python_parser/tests/snapshots/valid_syntax@args_unparenthesized_generator.py.snap index 961e761f30..043e187b9d 100644 --- a/crates/ruff_python_parser/tests/snapshots/valid_syntax@args_unparenthesized_generator.py.snap +++ b/crates/ruff_python_parser/tests/snapshots/valid_syntax@args_unparenthesized_generator.py.snap @@ -1,67 +1,195 @@ --- source: crates/ruff_python_parser/tests/fixtures.rs input_file: crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py -snapshot_kind: text --- ## AST ``` Module( ModModule { - range: 0..26, + range: 0..107, body: [ Expr( StmtExpr { - range: 0..25, + range: 0..51, value: Call( ExprCall { - range: 0..25, + range: 0..51, func: Name( ExprName { range: 0..3, - id: Name("sum"), + id: Name("zip"), ctx: Load, }, ), arguments: Arguments { - range: 3..25, + range: 3..51, args: [ Generator( ExprGenerator { - range: 4..24, + range: 4..26, elt: Name( ExprName { - range: 4..5, + range: 5..6, id: Name("x"), ctx: Load, }, ), generators: [ Comprehension { - range: 6..24, + range: 7..25, target: Name( ExprName { - range: 10..11, + range: 11..12, id: Name("x"), ctx: Store, }, ), iter: Call( ExprCall { - range: 15..24, + range: 16..25, func: Name( ExprName { - range: 15..20, + range: 16..21, id: Name("range"), ctx: Load, }, ), arguments: Arguments { - range: 20..24, + range: 21..25, args: [ NumberLiteral( ExprNumberLiteral { - range: 21..23, + range: 22..24, + value: Int( + 10, + ), + }, + ), + ], + keywords: [], + }, + }, + ), + ifs: [], + is_async: false, + }, + ], + parenthesized: true, + }, + ), + Generator( + ExprGenerator { + range: 28..50, + elt: Name( + ExprName { + range: 29..30, + id: Name("y"), + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 31..49, + target: Name( + ExprName { + range: 35..36, + id: Name("y"), + ctx: Store, + }, + ), + iter: Call( + ExprCall { + range: 40..49, + func: Name( + ExprName { + range: 40..45, + id: Name("range"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 45..49, + args: [ + NumberLiteral( + ExprNumberLiteral { + range: 46..48, + value: Int( + 10, + ), + }, + ), + ], + keywords: [], + }, + }, + ), + ifs: [], + is_async: false, + }, + ], + parenthesized: true, + }, + ), + ], + keywords: [], + }, + }, + ), + }, + ), + Expr( + StmtExpr { + range: 52..77, + value: Call( + ExprCall { + range: 52..77, + func: Name( + ExprName { + range: 52..55, + id: Name("sum"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 55..77, + args: [ + Generator( + ExprGenerator { + range: 56..76, + elt: Name( + ExprName { + range: 56..57, + id: Name("x"), + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 58..76, + target: Name( + ExprName { + range: 62..63, + id: Name("x"), + ctx: Store, + }, + ), + iter: Call( + ExprCall { + range: 67..76, + func: Name( + ExprName { + range: 67..72, + id: Name("range"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 72..76, + args: [ + NumberLiteral( + ExprNumberLiteral { + range: 73..75, value: Int( 10, ), @@ -86,6 +214,82 @@ Module( ), }, ), + Expr( + StmtExpr { + range: 78..106, + value: Call( + ExprCall { + range: 78..106, + func: Name( + ExprName { + range: 78..81, + id: Name("sum"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 81..106, + args: [ + Generator( + ExprGenerator { + range: 82..104, + elt: Name( + ExprName { + range: 83..84, + id: Name("x"), + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 85..103, + target: Name( + ExprName { + range: 89..90, + id: Name("x"), + ctx: Store, + }, + ), + iter: Call( + ExprCall { + range: 94..103, + func: Name( + ExprName { + range: 94..99, + id: Name("range"), + ctx: Load, + }, + ), + arguments: Arguments { + range: 99..103, + args: [ + NumberLiteral( + ExprNumberLiteral { + range: 100..102, + value: Int( + 10, + ), + }, + ), + ], + keywords: [], + }, + }, + ), + ifs: [], + is_async: false, + }, + ], + parenthesized: true, + }, + ), + ], + keywords: [], + }, + }, + ), + }, + ), ], }, )