Skip to content

Commit

Permalink
Consider if expression for parenthesized with items parsing (#11010)
Browse files Browse the repository at this point in the history 
## 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.
  • Loading branch information
@dhruvmanila
dhruvmanila authored Apr 18, 2024
1 parent 8020d48 commit 6c4d779
Show file tree
Hide file tree
Showing 4 changed files with 297 additions and 3 deletions.
4 changes: 4 additions & 0 deletions crates/ruff_python_parser/resources/inline/ok/ambiguous_lpar_with_items_if_expr.py
View file
Original file line number Diff line number Diff line change
@@ -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: ...
2 changes: 1 addition & 1 deletion crates/ruff_python_parser/src/parser/expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2370,7 +2370,7 @@ impl<'src> Parser<'src> {
/// If the parser isn't positioned at an `if` token.
///
/// See: <https://docs.python.org/3/reference/expressions.html#conditional-expressions>
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);
Expand Down
15 changes: 13 additions & 2 deletions crates/ruff_python_parser/src/parser/statement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)
Expand Down
279 changes: 279 additions & 0 deletions ...ruff_python_parser/tests/snapshots/valid_syntax@ambiguous_lpar_with_items_if_expr.py.snap
View file
Original file line number Diff line number Diff line change
@@ -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,
},
),
},
),
],
},
),
],
},
)
```

0 comments on commit 6c4d779

Please sign in to comment.