From 6c4d779140e0e16b041029000a96e8317f76624f Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 18 Apr 2024 20:00:15 +0530 Subject: [PATCH] Consider `if` expression for parenthesized with items parsing (#11010) ## Summary This PR fixes the bug in parenthesized with items parsing where the `if` expression would result into a syntax error. The reason being that once we identify that the ambiguous left parenthesis belongs to the context expression, the parser converts the parsed with item into an equivalent expression. Then, the parser continuous to parse any postfix expressions. Now, attribute, subscript, and call are taken into account as they're grouped in `parse_postfix_expression` but `if` expression has it's own parsing function. Use `parse_if_expression` once all postfix expressions have been parsed. Ideally, I think that `if` could be included in postfix expression parsing as they can be chained as well (`x if True else y if True else z`). ## Test Plan Add test cases and verified the snapshots. --- .../ok/ambiguous_lpar_with_items_if_expr.py | 4 + .../src/parser/expression.rs | 2 +- .../src/parser/statement.rs | 15 +- ...@ambiguous_lpar_with_items_if_expr.py.snap | 279 ++++++++++++++++++ 4 files changed, 297 insertions(+), 3 deletions(-) create mode 100644 crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py create mode 100644 crates/ruff_python_parser/tests/snapshots/valid_syntax@ambiguous_lpar_with_items_if_expr.py.snap diff --git a/crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py b/crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py new file mode 100644 index 0000000000000..2e0b46ef27eb5 --- /dev/null +++ b/crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py @@ -0,0 +1,4 @@ +with (x) if True else y: ... +with (x for x in iter) if True else y: ... +with (x async for x in iter) if True else y: ... +with (x)[0] if True else y: ... diff --git a/crates/ruff_python_parser/src/parser/expression.rs b/crates/ruff_python_parser/src/parser/expression.rs index cac035541b2b3..0a7196b1750e4 100644 --- a/crates/ruff_python_parser/src/parser/expression.rs +++ b/crates/ruff_python_parser/src/parser/expression.rs @@ -2370,7 +2370,7 @@ impl<'src> Parser<'src> { /// If the parser isn't positioned at an `if` token. /// /// See: - fn parse_if_expression(&mut self, body: Expr, start: TextSize) -> ast::ExprIf { + pub(super) fn parse_if_expression(&mut self, body: Expr, start: TextSize) -> ast::ExprIf { self.bump(TokenKind::If); let test = self.parse_simple_expression(AllowStarredExpression::No); diff --git a/crates/ruff_python_parser/src/parser/statement.rs b/crates/ruff_python_parser/src/parser/statement.rs index eb490328fdd4b..e397321b9d595 100644 --- a/crates/ruff_python_parser/src/parser/statement.rs +++ b/crates/ruff_python_parser/src/parser/statement.rs @@ -2085,7 +2085,7 @@ impl<'src> Parser<'src> { self.expect(TokenKind::Rpar); } - let lhs = if parsed_with_items.len() == 1 && !has_trailing_comma { + let mut lhs = if parsed_with_items.len() == 1 && !has_trailing_comma { // SAFETY: We've checked that `items` has only one item. let expr = parsed_with_items.pop().unwrap().item.context_expr; @@ -2145,7 +2145,18 @@ impl<'src> Parser<'src> { // considered when parsing the with item in the case. So, the parser // stops when it sees the `)` token and doesn't check for any postfix // expressions. - let context_expr = self.parse_postfix_expression(lhs, start); + lhs = self.parse_postfix_expression(lhs, start); + + // test_ok ambiguous_lpar_with_items_if_expr + // with (x) if True else y: ... + // with (x for x in iter) if True else y: ... + // with (x async for x in iter) if True else y: ... + // with (x)[0] if True else y: ... + let context_expr = if self.at(TokenKind::If) { + Expr::If(self.parse_if_expression(lhs, start)) + } else { + lhs + }; let optional_vars = self .at(TokenKind::As) diff --git a/crates/ruff_python_parser/tests/snapshots/valid_syntax@ambiguous_lpar_with_items_if_expr.py.snap b/crates/ruff_python_parser/tests/snapshots/valid_syntax@ambiguous_lpar_with_items_if_expr.py.snap new file mode 100644 index 0000000000000..97ec50ffc2523 --- /dev/null +++ b/crates/ruff_python_parser/tests/snapshots/valid_syntax@ambiguous_lpar_with_items_if_expr.py.snap @@ -0,0 +1,279 @@ +--- +source: crates/ruff_python_parser/tests/fixtures.rs +input_file: crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py +--- +## AST + +``` +Module( + ModModule { + range: 0..153, + body: [ + With( + StmtWith { + range: 0..28, + is_async: false, + items: [ + WithItem { + range: 5..23, + context_expr: If( + ExprIf { + range: 5..23, + test: BooleanLiteral( + ExprBooleanLiteral { + range: 12..16, + value: true, + }, + ), + body: Name( + ExprName { + range: 6..7, + id: "x", + ctx: Load, + }, + ), + orelse: Name( + ExprName { + range: 22..23, + id: "y", + ctx: Load, + }, + ), + }, + ), + optional_vars: None, + }, + ], + body: [ + Expr( + StmtExpr { + range: 25..28, + value: EllipsisLiteral( + ExprEllipsisLiteral { + range: 25..28, + }, + ), + }, + ), + ], + }, + ), + With( + StmtWith { + range: 29..71, + is_async: false, + items: [ + WithItem { + range: 34..66, + context_expr: If( + ExprIf { + range: 34..66, + test: BooleanLiteral( + ExprBooleanLiteral { + range: 55..59, + value: true, + }, + ), + body: Generator( + ExprGenerator { + range: 34..51, + elt: Name( + ExprName { + range: 35..36, + id: "x", + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 37..50, + target: Name( + ExprName { + range: 41..42, + id: "x", + ctx: Store, + }, + ), + iter: Name( + ExprName { + range: 46..50, + id: "iter", + ctx: Load, + }, + ), + ifs: [], + is_async: false, + }, + ], + parenthesized: true, + }, + ), + orelse: Name( + ExprName { + range: 65..66, + id: "y", + ctx: Load, + }, + ), + }, + ), + optional_vars: None, + }, + ], + body: [ + Expr( + StmtExpr { + range: 68..71, + value: EllipsisLiteral( + ExprEllipsisLiteral { + range: 68..71, + }, + ), + }, + ), + ], + }, + ), + With( + StmtWith { + range: 72..120, + is_async: false, + items: [ + WithItem { + range: 77..115, + context_expr: If( + ExprIf { + range: 77..115, + test: BooleanLiteral( + ExprBooleanLiteral { + range: 104..108, + value: true, + }, + ), + body: Generator( + ExprGenerator { + range: 77..100, + elt: Name( + ExprName { + range: 78..79, + id: "x", + ctx: Load, + }, + ), + generators: [ + Comprehension { + range: 80..99, + target: Name( + ExprName { + range: 90..91, + id: "x", + ctx: Store, + }, + ), + iter: Name( + ExprName { + range: 95..99, + id: "iter", + ctx: Load, + }, + ), + ifs: [], + is_async: true, + }, + ], + parenthesized: true, + }, + ), + orelse: Name( + ExprName { + range: 114..115, + id: "y", + ctx: Load, + }, + ), + }, + ), + optional_vars: None, + }, + ], + body: [ + Expr( + StmtExpr { + range: 117..120, + value: EllipsisLiteral( + ExprEllipsisLiteral { + range: 117..120, + }, + ), + }, + ), + ], + }, + ), + With( + StmtWith { + range: 121..152, + is_async: false, + items: [ + WithItem { + range: 126..147, + context_expr: If( + ExprIf { + range: 126..147, + test: BooleanLiteral( + ExprBooleanLiteral { + range: 136..140, + value: true, + }, + ), + body: Subscript( + ExprSubscript { + range: 126..132, + value: Name( + ExprName { + range: 127..128, + id: "x", + ctx: Load, + }, + ), + slice: NumberLiteral( + ExprNumberLiteral { + range: 130..131, + value: Int( + 0, + ), + }, + ), + ctx: Load, + }, + ), + orelse: Name( + ExprName { + range: 146..147, + id: "y", + ctx: Load, + }, + ), + }, + ), + optional_vars: None, + }, + ], + body: [ + Expr( + StmtExpr { + range: 149..152, + value: EllipsisLiteral( + ExprEllipsisLiteral { + range: 149..152, + }, + ), + }, + ), + ], + }, + ), + ], + }, +) +```