Fix `WithItem` ranges for parenthesized, non-`as` items #6782

charliermarsh · 2023-08-22T16:58:30Z

Summary

This PR attempts to address a problem in the parser related to the range's of WithItem nodes in certain contexts -- specifically, WithItem nodes in parentheses that do not have an as token after them.

For example, here:

with (a, b):
    pass

The range of the WithItem a is set to the range of (a, b), as is the range of the WithItem b. In other words, when we have this kind of sequence, we use the range of the entire parenthesized context, rather than the ranges of the items themselves.

Note that this also applies to cases like:

with (a, b, c as d):
    pass

You can see the issue in the parser here:

#[inline]
WithItemsNoAs: Vec<ast::WithItem> = {
    <location:@L> <all:OneOrMore<Test<"all">>> <end_location:@R> => {
        all.into_iter().map(|context_expr| ast::WithItem { context_expr, optional_vars: None, range: (location..end_location).into() }).collect()
    },
}

Fixing this issue is... very tricky. The naive approach is to use the range of the context_expr as the range for the WithItem, but that range will be incorrect when the context_expr is itself parenthesized. For example, that solution would fail here, since the range of the first WithItem would be that of a, rather than (a):

with ((a), b):
    pass

The with parsing in general is highly precarious due to ambiguities in the grammar. Changing it in any way seems to lead to an ambiguous grammar that LALRPOP fails to translate. Consensus seems to be that we don't really understand why the current grammar works (i.e., how it avoids these ambiguities as-is).

The solution implemented here is to avoid changing the grammar itself, and instead change the shape of the nodes returned by various rules in the grammar. Specifically, everywhere that we return Expr, we instead return ParenthesizedExpr, which includes a parenthesized range and the underlying Expr itself. (If an Expr isn't parenthesized, the ranges will be equivalent.) In WithItemsNoAs, we can then use the parenthesized range as the range for the WithItem.

charliermarsh · 2023-08-22T17:00:20Z

crates/ruff_python_parser/src/python.lalrpop

+            ast::ParenthesizedExpr {
+                expr: elts.into_iter().next().unwrap(),
+                range: (location..end_location).into(),
+            }


This is where the parser deals with parenthesize expressions. We return the full range, as well as the underlying expression, rather than discarding the range here as we are today.

charliermarsh · 2023-08-22T17:00:41Z

crates/ruff_python_parser/src/python.lalrpop

    <location:@L> "[" <e:ListLiteralValues?> "]"<end_location:@R>  => {
-        let elts = e.unwrap_or_default();
+        let elts = e.into_iter().flatten().map(ast::Expr::from).collect();


I guess this requires another allocation unfortunately, since we have to translate Vec<ParenthesizedExpr> to Vec<Expr>?

charliermarsh · 2023-08-22T17:01:02Z

crates/ruff_python_parser/src/python.lalrpop

-            orelse: Box::new(orelse),
+            test: Box::new(test.into()),
+            body: Box::new(body.into()),
+            orelse: Box::new(orelse.into()),


We mostly need to add these From/Into conversions everywhere to translate between ParenthesizedExpr and Expr.

charliermarsh · 2023-08-22T17:01:24Z

I am looking for feedback on the approach prior to fixing the remaining ~150 compiler errors.

charliermarsh · 2023-08-22T22:27:58Z

Okay, this now compiles and does the right thing. The ranges are fixed.

charliermarsh · 2023-08-22T22:28:42Z

crates/ruff_python_parser/src/snapshots/ruff_python_parser__parser__tests__with_statement.snap

+            is_async: false,
+            items: [
+                WithItem {
+                    range: 566..569,


This is key: the length of the WithItem is 3, while the length of the inner item is 1.

github-actions · 2023-08-22T23:39:08Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

charliermarsh · 2023-08-23T13:45:50Z

Note that, on main, given:

with ((1), (2), 3 as x, (5)): pass

The last WithItem does use the full (5) range.

charliermarsh · 2023-08-23T13:47:00Z

AFAICT in the CPython AST the withitems don't have ranges at all so not sure it can guide us:

Module(
    body=[
        With(
            items=[
                withitem(
                    context_expr=Constant(
                        value=1,
                        lineno=1,
                        col_offset=7,
                        end_lineno=1,
                        end_col_offset=8)),
                withitem(
                    context_expr=Constant(
                        value=2,
                        lineno=1,
                        col_offset=12,
                        end_lineno=1,
                        end_col_offset=13)),
                withitem(
                    context_expr=Constant(
                        value=3,
                        lineno=1,
                        col_offset=16,
                        end_lineno=1,
                        end_col_offset=17),
                    optional_vars=Name(
                        id='x',
                        ctx=Store(),
                        lineno=1,
                        col_offset=21,
                        end_lineno=1,
                        end_col_offset=22)),
                withitem(
                    context_expr=Constant(
                        value=5,
                        lineno=1,
                        col_offset=25,
                        end_lineno=1,
                        end_col_offset=26))],
            body=[
                Pass(
                    lineno=1,
                    col_offset=30,
                    end_lineno=1,
                    end_col_offset=34)],
            lineno=1,
            col_offset=0,
            end_lineno=1,
            end_col_offset=34)],
    type_ignores=[])

MichaReiser · 2023-08-24T06:37:24Z

crates/ruff_python_ast/src/nodes.rs

+#[derive(Clone, Debug)]
+pub struct ParenthesizedExpr {


Can we make this type private (or pub(super) at most) and move it to parser.rs because it is mainly an internal workaround, but by no means public api

Unfortunately no -- it somehow becomes part of the public API in the generated LALRPOP code.

MichaReiser

I'm torn on this, mainly because of

Consensus seems to be that we don't really understand why the current grammar works (i.e., how it avoids these ambiguities as-is).

and because I believe that LR(1) or LALR(1) grammars should be able to support with statements without the need for hacks (It's LL(1) grammars that the Python PEP points out are unsupported). That's why I'm convinced that the issue is somewhere else in our grammar (but yeah, LALRPOP ambiguity messages are impossible to understand).

I'm okayish with merging this when we open an issue to follow up, ideally as part of the formatter Beta. Maybe I can get some time to play around with this.

charliermarsh · 2023-08-24T12:50:14Z

@MichaReiser - do you have an opinion on whether the ranges should include or exclude the parentheses? I’m torn on that myself. (Either outcome requires a code change — the PR here includes the parentheses, although that’s inconsistent with some other AST nodes, like the parenthesized pattern match case.)

charliermarsh · 2023-08-24T13:32:20Z

I'm honestly not sure what's correct here.

Given:

with (a): pass

We currently omit the parentheses for the range (so the withitem has the range of a).

Given:

with (a) as b: pass

We use the full range of (a) as b as the withitem range.

konstin · 2023-08-24T16:10:58Z

Those ranges look inconvenient but correct to me. It's the same if you'd see as as a bin op:

with (a):
    pass

with (a) + b:
    pass

MichaReiser · 2023-08-24T16:16:30Z

@MichaReiser - do you have an opinion on whether the ranges should include or exclude the parentheses? I’m torn on that myself. (Either outcome requires a code change — the PR here includes the parentheses, although that’s inconsistent with some other AST nodes, like the parenthesized pattern match case.)

I'm leaning towards including the range similar to tuples, where the parentheses are optional. With items are different from parenthesized expressions (and patterns) in that they don't allow nesting:

with (((a) as b)):
    pass

Isn't valid syntax

However, this does raises the question of what the expected range should be for:

with (a): pass

Is this a with item with a parenthesized expression or is the with item parenthesized, containing an expression? We would need to take a look at the grammar to understand the precedence.

Edit:
I studied the grammar and the precedence rules described in PEP617.

with_stmt:
    | 'with' '(' ','.with_item+ ','? ')' ':' block 
    | 'with' ','.with_item+ ':' [TYPE_COMMENT] block 
    | ASYNC 'with' '(' ','.with_item+ ','? ')' ':' block 
    | ASYNC 'with' ','.with_item+ ':' [TYPE_COMMENT] block 

with_item:
    | expression 'as' star_target &(',' | ')' | ':') 
    | expression

...while a PEG parser will check if the first alternative succeeds and only if it fails, will it continue with the second or the third one in the order in which they are written. This makes the choice operator not commutative.

My understanding of this is that the a in with (a) is not a parenthesized expression, the parentheses belong to the with_stmt because the parentheses rule is the first and it succeeds parsing.

MichaReiser · 2023-08-24T16:31:19Z

I guess the with (a) is more involved than I thought because the following is valid as well:

with (
	a,
	b
): pass

Meaning, it's not the with items that are parenthesized, but all of them.

MichaReiser · 2023-08-24T16:46:04Z

Oh, I did read the spec incorrectly. With items that cannot be parenthesized. It's either parenthesizing all with items, or the expression in the with item.

I would still expect the WithItem range to span the full range for parenthesized expressions. This to align it with an expression statement

(a)

The range of the expression statement is 0..3, the range of the expression is 1...2

charliermarsh · 2023-08-24T18:15:45Z

I might be misunderstanding your comment but with (a, b): pass is valid and it's two separate WithItems. Here are a few cases to consider:

with (a, b): pass

with (a, b) as c: pass

with ((a, b) as c): pass

with (a as b): pass

In the first case, that's two WithItem's (a and b), so both ranges should exclude the parentheses IIUC.

In the second case, it's a single WithItem that should include the parentheses.

In the third and fourth cases, that's a single WithItem. I guess it should include the outer parentheses?

MichaReiser · 2023-08-24T18:29:42Z

In the third and fourth cases, that's a single WithItem. I guess it should include the outer parentheses?

No, it should not include the ranges in my view because WithItems can't have parentheses. It's the With statement that allows parentheses or the expression, but not the with item.

From ny comment aboce

I studied the grammar and the precedence rules described in PEP617.

with_stmt:
    | 'with' '(' ','.with_item+ ','? ')' ':' block 
    | 'with' ','.with_item+ ':' [TYPE_COMMENT] block 
    | ASYNC 'with' '(' ','.with_item+ ','? ')' ':' block 
    | ASYNC 'with' ','.with_item+ ':' [TYPE_COMMENT] block 

with_item:
    | expression 'as' star_target &(',' | ')' | ':') 
    | expression

...while a PEG parser will check if the first alternative succeeds and only if it fails, will it continue with the second or the third one in the order in which they are written. This makes the choice operator not commutative.

My understanding of this is that the a in with (a) is not a parenthesized expression, the parentheses belong to the with_stmt because the parentheses rule is the first and it succeeds parsing.

What I meant that isn't valid is match (a as b), c: pass, proving that with items can't have their own parentheses.

charliermarsh · 2023-08-24T18:38:47Z

Ah, I see, I think I was just confused by the message you posted after that, saying that it should include the parentheses. But I assume there, when you say (a), you're referring to something like:

with ((a), b):
  pass

Since in this case, the outer parentheses belong to the with statement, but the inner parentheses on (a) belong to the item?

MichaReiser · 2023-08-24T18:44:12Z

Since in this case, the outer parentheses belong to the with statement, but the inner parentheses on (a) belong to the item?

yes, but we need to be careful with terminology. The parentheses belong to the expression a. But the range of the enclosing with item includes the whole expression range (including parentheses).

MichaReiser

It hurts, but I think that's the best we can do without rewriting our parser 😭

I also don't fully understand why this is working. It could be poor luck, or even a bug, but we take it...

MichaReiser · 2023-08-25T12:01:20Z

crates/ruff_python_parser/src/python.lalrpop

+    <location:@L> <context_expr:Test<Goal>> <end_location:@R> if Goal != "as" => ast::WithItem {
+        context_expr: context_expr.into(),
+        optional_vars: None,
+        range: (location..end_location).into(),
+    },
+    <location:@L> <context_expr:Test<"all">> "as" <optional_vars:Expression<"all">> <end_location:@R> => {


Nit: We could probably use the range from the context_expr directly rather than collecting another range.

This is leading me down some confusing path around ranges for parenthesized named expressions in with-items 😭

MichaReiser · 2023-08-25T12:02:57Z

crates/ruff_python_parser/src/python.lalrpop

    },
    <location:@L> "(" <elts:OneOrMore<Test<"all">>> <trailing_comma:","?> ")" <end_location:@R> if Goal != "no-withitems" => {
        if elts.len() == 1 && trailing_comma.is_none() {
-            elts.into_iter().next().unwrap()
+            ast::ParenthesizedExpr {
+                expr: elts.into_iter().next().unwrap().into(),


into...unwrap...into what 🤯

charliermarsh · 2023-08-28T00:45:42Z

This is still getting on case wrong (sigh):

with (a := 1):
    pass

The expression rules require that the named expression is parenthesized, so the ( withitem ) case doesn't match, and it ends up being difficult to treat the parentheses as part of the WithItem.

charliermarsh · 2023-08-30T17:34:04Z

Okay, this is now working in all (?) cases. I had to special-case parenthesized expressions and other expressions that "require" parentheses.

charliermarsh · 2023-08-30T17:34:14Z

I now need to look into the formatter changes.

charliermarsh · 2023-08-30T17:37:17Z

I also want to see the CodSpeed benchmarks.

MichaReiser · 2023-08-31T06:28:18Z

I also want to see the CodSpeed benchmarks.

Parse time regresses by 1-2%. Can you change the codespeed threshold to 1% to see how it goes?

MichaReiser · 2023-08-31T06:28:51Z

I now need to look into the formatter changes.

Does this mean the PR is ready to review or not? If not, can you put it back in draft state?

charliermarsh · 2023-08-31T06:40:08Z

No, the PR is ready to review. Or rather, to merge? I actually didn’t expect it to be re-reviewed given that it had already been reviewed and accepted.

charliermarsh · 2023-08-31T06:42:43Z

(The formatter changes are downstream of this, for example, #3711.)

codspeed-hq · 2023-08-31T13:41:45Z

CodSpeed Performance Report

Merging #6782 will degrade performances by 1.73%

_{Comparing charlie/parse (9e2aec4) with main (f4ba0ea)}

Summary

🔥 1 improvements
❌ 9 regressions
✅ 10 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`charlie/parse`	Change
🔥	`linter/default-rules[pydantic/types.py]`	39.2 ms	38.2 ms	+2.49%
❌	`linter/default-rules[numpy/ctypeslib.py]`	18.2 ms	18.5 ms	-1.41%
❌	`linter/default-rules[large/dataset.py]`	91.5 ms	92.9 ms	-1.47%
❌	`linter/all-rules[large/dataset.py]`	156.9 ms	159.1 ms	-1.37%
❌	`parser[numpy/globals.py]`	1.4 ms	1.4 ms	-1.23%
❌	`parser[numpy/ctypeslib.py]`	12.5 ms	12.7 ms	-1.52%
❌	`parser[unicode/pypinyin.py]`	4.3 ms	4.3 ms	-1.47%
❌	`parser[large/dataset.py]`	68.8 ms	70 ms	-1.73%
❌	`parser[pydantic/types.py]`	26.8 ms	27.2 ms	-1.34%
❌	`linter/default-rules[unicode/pypinyin.py]`	6.3 ms	6.4 ms	-1%

Finish Trying to figure this out Handle named expressions

charliermarsh commented Aug 22, 2023

View reviewed changes

charliermarsh requested review from MichaReiser and zanieb August 22, 2023 17:01

charliermarsh force-pushed the charlie/parse branch from a2a2070 to f893965 Compare August 22, 2023 22:27

charliermarsh marked this pull request as ready for review August 22, 2023 22:27

charliermarsh commented Aug 22, 2023

View reviewed changes

charliermarsh added the bug Something isn't working label Aug 22, 2023

charliermarsh force-pushed the charlie/parse branch 2 times, most recently from 5fea24a to bdfc5a7 Compare August 22, 2023 23:26

MichaReiser reviewed Aug 24, 2023

View reviewed changes

MichaReiser approved these changes Aug 24, 2023

View reviewed changes

MichaReiser approved these changes Aug 25, 2023

View reviewed changes

MichaReiser added the parser Related to the parser label Aug 25, 2023

MichaReiser removed the bug Something isn't working label Aug 25, 2023

charliermarsh force-pushed the charlie/parse branch 2 times, most recently from db4d356 to a5ed1ff Compare August 30, 2023 17:25

charliermarsh force-pushed the charlie/parse branch from a5ed1ff to f188605 Compare August 30, 2023 17:36

charliermarsh force-pushed the charlie/parse branch from f188605 to 9e2aec4 Compare August 31, 2023 13:32

Add parse

4e20c35

Finish Trying to figure this out Handle named expressions

charliermarsh force-pushed the charlie/parse branch from 9e2aec4 to 4e20c35 Compare August 31, 2023 13:59

charliermarsh merged commit 68f605e into main Aug 31, 2023
16 checks passed

charliermarsh deleted the charlie/parse branch August 31, 2023 15:21

chenrui333 mentioned this pull request Sep 1, 2023

ruff 0.0.287 Homebrew/homebrew-core#141134

Merged

Fix WithItem ranges for parenthesized, non-as items #6782

Fix WithItem ranges for parenthesized, non-as items #6782

Conversation

charliermarsh commented Aug 22, 2023 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 22, 2023

charliermarsh commented Aug 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2023 • edited Loading

PR Check Results

Ecosystem

charliermarsh commented Aug 23, 2023 • edited Loading

charliermarsh commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

charliermarsh commented Aug 24, 2023

charliermarsh commented Aug 24, 2023

konstin commented Aug 24, 2023

MichaReiser commented Aug 24, 2023 • edited Loading

MichaReiser commented Aug 24, 2023

MichaReiser commented Aug 24, 2023

charliermarsh commented Aug 24, 2023

MichaReiser commented Aug 24, 2023 • edited Loading

charliermarsh commented Aug 24, 2023 • edited Loading

MichaReiser commented Aug 24, 2023 • edited Loading

MichaReiser left a comment • edited Loading

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

charliermarsh commented Aug 28, 2023

charliermarsh commented Aug 30, 2023

charliermarsh commented Aug 30, 2023

charliermarsh commented Aug 30, 2023

MichaReiser commented Aug 31, 2023

MichaReiser commented Aug 31, 2023

charliermarsh commented Aug 31, 2023

charliermarsh commented Aug 31, 2023

codspeed-hq bot commented Aug 31, 2023

Merging #6782 will degrade performances by 1.73%

Summary

Benchmarks breakdown

Fix `WithItem` ranges for parenthesized, non-`as` items #6782

Fix `WithItem` ranges for parenthesized, non-`as` items #6782

charliermarsh commented Aug 22, 2023 •

edited

Loading

github-actions bot commented Aug 22, 2023 •

edited

Loading

charliermarsh commented Aug 23, 2023 •

edited

Loading

MichaReiser commented Aug 24, 2023 •

edited

Loading

MichaReiser commented Aug 24, 2023 •

edited

Loading

charliermarsh commented Aug 24, 2023 •

edited

Loading

MichaReiser commented Aug 24, 2023 •

edited

Loading

MichaReiser left a comment •

edited

Loading