Better error recovery for match patterns #10477

dhruvmanila · 2024-03-19T17:51:33Z

Summary

This PR updates the match statement's pattern parsing logic to better facilitate the error recovery. The following changes are made:

Disallow star pattern (`*<name>`) outside of sequence pattern

This is done by passing an enum stating whether to allow star pattern or not and is based on where the parser is in the stack.

Call stack for pattern parsing functions:

I took notes on where would I requires changing this, so sharing it for documenting purposes:

parse_match_patterns
- parse_match_pattern (AllowStarPattern::Yes)
  - Error if it’s a star pattern and it's not a sequence pattern
- parse_sequence_match_pattern
  - parse_match_pattern (AllowStarPattern::Yes)
parse_match_pattern (allow_star_pattern: AllowStarPattern)
- parse_match_pattern_lhs (allow_star_pattern: AllowStarPattern)
  - parse_match_pattern_mapping
    - parse_match_pattern_lhs (AllowStarPattern::No)
    - parse_match_pattern (AllowStarPattern::No)
  - parse_match_pattern_star
  - parse_delimited_match_pattern
    - parse_match_pattern (AllowStarPattern::Yes)
    - parse_sequence_match_pattern
  - parse_match_pattern_literal
    - parse_attr_expr_for_match_pattern
  - parse_match_pattern_class
    - parse_match_pattern (AllowStarPattern::No)
  - parse_complex_literal_pattern
    - parse_match_pattern_lhs (AllowStarPattern::No)

The mapping pattern FIRST set has been updated to include * to allow the parser to recover from this error in mapping pattern. Otherwise, the list parsing would completely skip parsing it.

Convert `Pattern` to `Expr`

In certain cases, the parser expects a specific type of pattern and uses that to extract information. For example, in complex literal, we parse the LHS and RHS as patterns but the LHS can only be a real number while the RHS can only be a complex number. This means that if the parser found any other pattern kind then we would convert the pattern into an equivalent expression and add an appropriate error. This will help any downstream tools.

Refer to recovery::pattern_to_expr for more details.

Extract complex number literal pattern parsing logic

Earlier, this was inside the parse_match_pattern_lhs which made the method a bit more complex than it needed to be. The logic has been extracted to a new parse_complex_pattern_literal and is called when the parser encounters a pattern followed by either + or -.

The parser only allows a valid complex literal pattern which is real_number +/- complex_number. For any other case, an appropriate error will be raised.

Review

Refer to the test cases to help in the review process.

Test Plan

Add test cases and update the snapshots.

github-actions · 2024-03-19T18:11:43Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

dhruvmanila · 2024-03-21T04:09:08Z

crates/ruff_python_parser/resources/valid/statement/match.py

-    case *a:
+    case *a,:


Star pattern is only allowed inside a sequence pattern as per the grammar.

crates/ruff_python_parser/src/parser/pattern.rs

dhruvmanila · 2024-03-21T04:38:34Z

crates/ruff_python_parser/src/parser/pattern.rs

+        let cls = match cls {
+            Pattern::MatchAs(ast::PatternMatchAs {
+                pattern: None,
+                name: Some(ident),
+                ..
+            }) => {
+                if ident.is_valid() {
+                    Box::new(Expr::Name(ast::ExprName {
+                        range: ident.range(),
+                        id: ident.id,
+                        ctx: ExprContext::Load,
+                    }))
+                } else {
+                    Box::new(Expr::Name(ast::ExprName {
+                        range: ident.range(),
+                        id: String::new(),
+                        ctx: ExprContext::Invalid,
+                    }))
+                }
+            }
+            Pattern::MatchValue(ast::PatternMatchValue { value, .. })
+                if matches!(&*value, Expr::Attribute(_)) =>
+            {
+                value
+            }
+            pattern => {
+                self.add_error(
+                    ParseErrorType::OtherError("invalid value for a class pattern".to_string()),
+                    &pattern,
+                );
+                Box::new(recovery::pattern_to_expr(pattern))
+            }
+        };
+


This is mainly moved from down below because class comes first and then the arguments. It's only to maintain the flow of parsing.

The change here is to use recovery::pattern_to_expr.

dhruvmanila · 2024-03-21T04:39:49Z

crates/ruff_python_parser/src/parser/pattern.rs

                    if let Pattern::MatchAs(ast::PatternMatchAs {
-                        name: Some(attr), ..
+                        pattern: None,
+                        name: Some(attr),
+                        ..


To disallow:

match subject: case Foo(a as b = 1): pass

MichaReiser · 2024-03-21T08:38:01Z

crates/ruff_python_parser/resources/invalid/statements/match/invalid_class_pattern.py

+    # Positional pattern cannot follow keyword pattern
+    # case Foo(x, y=1, z):
+    #     pass


Is this intentionally commented out?

Yes. So, positional pattern cannot follow keyword pattern in a class pattern (case Foo(x=1, y): ... is invalid) but this means the pre-order assert in the invalid code fails.

I'll probably need to have a mathod on PatternArguments similar to how's one for Arguments (arguments_source_order) to return an argument in the order defined in source.

I need to think about this because it's only required while testing.

...ython_parser/tests/snapshots/invalid_syntax@statements__match__invalid_class_pattern.py.snap

..._parser/tests/snapshots/invalid_syntax@statements__match__invalid_lhs_or_rhs_pattern.py.snap

...hon_parser/tests/snapshots/invalid_syntax@statements__match__invalid_mapping_pattern.py.snap

crates/ruff_python_parser/src/parser/pattern.rs

MichaReiser · 2024-03-21T09:11:42Z

crates/ruff_python_parser/src/parser/pattern.rs

+                // It's not possible to retain multiple double starred patterns because
+                // of the way the mapping node is represented in the grammar. The last
+                // value will always win.
+                rest = Some(identifier);
            } else {


Hm, this is a problem because we now drop "nodes", meaning the MatchPatternMapping range covers nodes that aren't represented in the AST.

Any chance we could convert the star to a starred expression instead and push a pattern at the right position? If not, then we may need to bail out early here, even if it means that the error recovery isn't as good as it could be (or we have to change the AST representation)

Any chance we could convert the star to a starred expression instead and push a pattern at the right position?

Not really because there's no way to represent a double starred expression in the AST, there's only a ExprStarred which is for *foo.

If not, then we may need to bail out early here, even if it means that the error recovery isn't as good as it could be (or we have to change the AST representation)

I could see what bailing would look like but I'd prefer to change the AST representation even it means a bit more work to be done.

Thinking quickly about the AST representation, there are two solutions which comes to my mind:

Add a new Pattern variant for **data and store it in MatchPatternMapping::patterns field. This is probably not a good idea

Replace the keys, patterns and rest fields with a single entries field which is an enum with two kind (MappingPattern::DoubleStar and MappingPattern::KeyValue)

Also, this is happening in a list parsing so might involve using the parser context flags.

I'm also leaning on changing the AST (e.g. similar to Pyright's AST). What's important to me is that we don't spend too much time now working on error recovery. I'm also okay marking this as a todo (creating an issue) and following up on this later because the focus now is on replacing the parser, not on perfect error recovery.

Yeah, I'd prefer to not to do it now as well. I'll open an issue for it.

crates/ruff_python_parser/src/parser/pattern.rs

crates/ruff_python_parser/src/parser/recovery.rs

crates/ruff_python_parser/src/error.rs

crates/ruff_python_parser/resources/invalid/statements/match/invalid_mapping_pattern.py

## Summary This PR is a follow-up to #10477 which does the following: 1. Improve error message around complex literal pattern. Remove usage of "lhs" and "rhs" term from user facing messages. 2. Better error recovery for unary plus. This isn't allowed in the literal pattern so we'll just parse it and throw an error directly. 3. Fix a bug where we disallowed a unary sub in sequence pattern 4. Reduce error range for invalid key value for a mapping pattern ## Test Plan Add new test cases, update and review the snapshots

## Summary This PR addresses the feedback given here: #10477 (comment) **Problem:** The `as` pattern containing both the pattern and name (`foo as bar`) cannot be converted to an expression as there's no AST node to represent that. The solution for now is to not drop any nodes, panic in the conversion if the parser is doing so, and make sure the calling code is avoiding the panic. There are 3 places where the conversion happens: 1. Mapping pattern 2. Class pattern 3. Complex literal pattern Another thing to keep in mind is that the `parse_match_pattern_lhs` method only parses the `as` pattern if it's parenthesized. The reason being that the `(` is ambiguous as to whether it's a parenthesized pattern or the start of a sequence pattern. Now, the mapping items are parsed using list parsing and it's only called if the start token is in the FIRST set which the `(` token isn't. This avoids the panic in mapping pattern. For the class and complex literal pattern, the starting point is `parse_match_pattern_lhs` method. It then checks if the parser is at `(` (as in `Foo(x, y)`) or `+`/`-` (as in `1 + 1j`) and continues with the respective parsing methods. We want to avoid going further in case we have an `as` pattern to avoid the panic. This means we'll exit early in that case. ## Test Plan Added a few test cases, updated the snapshots. Note that this PR isn't focused on error recovery, it's to avoid dropping any potential node which is the case when converting from an `as` pattern to an expression. Another thing to note is that the following test case fails (not added) because the error recovery is bad and the node ranges aren't in order: ```python match subject: case {x as y: 1}: pass ``` <details><summary>Click here to view the AST and errors for the above code:</summary> <p> ```rs Module( ModModule { range: 0..50, body: [ Match( StmtMatch { range: 0..49, subject: Name( ExprName { range: 6..13, id: "subject", ctx: Load, }, ), cases: [ MatchCase { range: 19..49, pattern: MatchMapping( PatternMatchMapping { range: 24..35, keys: [ Name( ExprName { range: 25..26, id: "x", ctx: Store, }, ), Name( ExprName { range: 30..31, id: "y", ctx: Store, }, ), ], patterns: [ MatchValue( PatternMatchValue { range: 29..29, value: Name( ExprName { range: 27..29, id: "as", ctx: Load, }, ), }, ), MatchValue( PatternMatchValue { range: 33..34, value: NumberLiteral( ExprNumberLiteral { range: 33..34, value: Int( 1, ), }, ), }, ), ], rest: None, }, ), guard: None, body: [ Pass( StmtPass { range: 45..49, }, ), ], }, ], }, ), ], }, ) ``` Errors: ``` Syntax Error: invalid mapping pattern key at byte range 25..26 | 1 | match subject: 2 | case {x as y: 1}: | ^ Syntax Error: invalid mapping pattern key 3 | pass | Syntax Error: expected Colon, found As at byte range 27..29 | 1 | match subject: 2 | case {x as y: 1}: | ^^ Syntax Error: expected Colon, found As 3 | pass | Syntax Error: expected Comma, found Name at byte range 30..31 | 1 | match subject: 2 | case {x as y: 1}: | ^ Syntax Error: expected Comma, found Name 3 | pass | ``` </p> </details>

## Summary This fixes the bug which I encountered in #10523 (comment) ## Test Plan Add the failing test case and update the snapshot.

## Summary This PR updates the match statement's pattern parsing logic to better facilitate the error recovery. The following changes are made: ### Disallow star pattern (`*<name>`) outside of sequence pattern This is done by passing an enum stating whether to allow star pattern or not and is based on where the parser is in the stack. <details><summary>Call stack for pattern parsing functions:</summary> <p> I took notes on where would I requires changing this, so sharing it for documenting purposes: * `parse_match_patterns` * `parse_match_pattern` (`AllowStarPattern::Yes`) * Error if it’s a star pattern and it's not a sequence pattern * `parse_sequence_match_pattern` * `parse_match_pattern` (`AllowStarPattern::Yes`) * `parse_match_pattern` (`allow_star_pattern: AllowStarPattern`) * `parse_match_pattern_lhs` (`allow_star_pattern: AllowStarPattern`) * `parse_match_pattern_mapping` * `parse_match_pattern_lhs` (`AllowStarPattern::No`) * `parse_match_pattern` (`AllowStarPattern::No`) * `parse_match_pattern_star` * `parse_delimited_match_pattern` * `parse_match_pattern` (`AllowStarPattern::Yes`) * `parse_sequence_match_pattern` * `parse_match_pattern_literal` * `parse_attr_expr_for_match_pattern` * `parse_match_pattern_class` * `parse_match_pattern` (`AllowStarPattern::No`) * `parse_complex_literal_pattern` * `parse_match_pattern_lhs` (`AllowStarPattern::No`) </p> </details> The mapping pattern `FIRST` set has been updated to include `*` to allow the parser to recover from this error in mapping pattern. Otherwise, the list parsing would completely skip parsing it. ### Convert `Pattern` to `Expr` In certain cases, the parser expects a specific type of pattern and uses that to extract information. For example, in complex literal, we parse the LHS and RHS as patterns but the LHS can only be a real number while the RHS can only be a complex number. This means that if the parser found any other pattern kind then we would convert the pattern into an equivalent expression and add an appropriate error. This will help any downstream tools. Refer to `recovery::pattern_to_expr` for more details. ### Extract complex number literal pattern parsing logic Earlier, this was inside the `parse_match_pattern_lhs` which made the method a bit more complex than it needed to be. The logic has been extracted to a new `parse_complex_pattern_literal` and is called when the parser encounters a pattern followed by either `+` or `-`. The parser only allows a valid complex literal pattern which is `real_number +/- complex_number`. For any other case, an appropriate error will be raised. ## Review Refer to the test cases to help in the review process. ## Test Plan Add test cases and update the snapshots.

dhruvmanila added the parser Related to the parser label Mar 19, 2024

dhruvmanila force-pushed the dhruv/pattern-recovery branch 2 times, most recently from b7242be to e1f1cc8 Compare March 20, 2024 11:14

dhruvmanila force-pushed the dhruv/parser branch from 1cf7954 to 707d62a Compare March 20, 2024 16:00

dhruvmanila force-pushed the dhruv/pattern-recovery branch from e1f1cc8 to 801539c Compare March 20, 2024 18:57

Better error recovery for match patterns

ebeea51

dhruvmanila force-pushed the dhruv/pattern-recovery branch from 801539c to ebeea51 Compare March 21, 2024 04:07

dhruvmanila commented Mar 21, 2024

View reviewed changes

crates/ruff_python_parser/src/parser/pattern.rs Outdated Show resolved Hide resolved

dhruvmanila commented Mar 21, 2024

View reviewed changes

dhruvmanila marked this pull request as ready for review March 21, 2024 04:40

dhruvmanila requested a review from MichaReiser as a code owner March 21, 2024 04:40

MichaReiser reviewed Mar 21, 2024

View reviewed changes

dhruvmanila added 2 commits March 21, 2024 15:08

Make parsing class and complex non-exclusive

80b091a

Ensure LHS is real number for complex literal pattern

acbaff2

MichaReiser reviewed Mar 21, 2024

View reviewed changes

snowsignal reviewed Mar 22, 2024

View reviewed changes

crates/ruff_python_parser/src/error.rs Show resolved Hide resolved

crates/ruff_python_parser/resources/invalid/statements/match/invalid_mapping_pattern.py Outdated Show resolved Hide resolved

dhruvmanila mentioned this pull request Mar 22, 2024

Improve error message, recover from unary add #10518

Merged

dhruvmanila mentioned this pull request Mar 22, 2024

Avoid dropping node from as pattern #10523

Merged

dhruvmanila added 2 commits March 22, 2024 20:23

Use expr range for invalid node (#10568)

94f0e8f

## Summary This fixes the bug which I encountered in #10523 (comment) ## Test Plan Add the failing test case and update the snapshot.

dhruvmanila mentioned this pull request Mar 25, 2024

Update PatternMatchMapping AST node #10570

Open

Consistently add newline at the end of test files

bacbd95

dhruvmanila requested a review from MichaReiser March 25, 2024 13:14

MichaReiser approved these changes Mar 25, 2024

View reviewed changes

dhruvmanila merged commit 236aa4a into dhruv/parser Mar 25, 2024
17 checks passed

dhruvmanila deleted the dhruv/pattern-recovery branch March 25, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error recovery for match patterns #10477

Better error recovery for match patterns #10477

dhruvmanila commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

dhruvmanila Mar 21, 2024

dhruvmanila Mar 21, 2024

dhruvmanila Mar 21, 2024

MichaReiser Mar 21, 2024

dhruvmanila Mar 21, 2024

MichaReiser Mar 21, 2024

dhruvmanila Mar 22, 2024

dhruvmanila Mar 22, 2024

MichaReiser Mar 22, 2024

dhruvmanila Mar 22, 2024 •

edited

Loading

dhruvmanila Mar 25, 2024

Better error recovery for match patterns #10477

Better error recovery for match patterns #10477

Conversation

dhruvmanila commented Mar 19, 2024 • edited Loading

Summary

Disallow star pattern (*<name>) outside of sequence pattern

Convert Pattern to Expr

Extract complex number literal pattern parsing logic

Review

Test Plan

github-actions bot commented Mar 19, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvmanila Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruvmanila commented Mar 19, 2024 •

edited

Loading

Disallow star pattern (`*<name>`) outside of sequence pattern

Convert `Pattern` to `Expr`

github-actions bot commented Mar 19, 2024 •

edited

Loading

`ruff-ecosystem` results

dhruvmanila Mar 22, 2024 •

edited

Loading