Fix AST generated from a dict literal containing dict unpacking by harupy · Pull Request #4449 · RustPython/RustPython

harupy · 2023-01-15T04:08:18Z

{"foo": 1, **{"bar": 1}} is parsed into the AST below in CPython and RustPython. In CPython, Dict.keys contains two keys (Constant and None), but in RustPython, Dict.keys only contains one key. This PR fixes it.

CPython:

Expand

Module(
  body=[
    Expr(
      value=Dict(
        👇👇👇 Contains two keys, but the second key is None
        keys=[
          Constant(
            value='foo',
            lineno=1,
            col_offset=1,
            end_lineno=1,
            end_col_offset=6),
          None],
        values=[
          Constant(
            value=1,
            lineno=1,
            col_offset=8,
            end_lineno=1,
            end_col_offset=9),
          Dict(
            keys=[
              Constant(
                value='bar',
                lineno=1,
                col_offset=14,
                end_lineno=1,
                end_col_offset=19)],
            values=[
              Constant(
                value=1,
                lineno=1,
                col_offset=21,
                end_lineno=1,
                end_col_offset=22)],
            lineno=1,
            col_offset=13,
            end_lineno=1,
            end_col_offset=23)],
        lineno=1,
        col_offset=0,
        end_lineno=1,
        end_col_offset=24),
      lineno=1,
      col_offset=0,
      end_lineno=1,
      end_col_offset=24)],
  type_ignores=[])

RustPython

Expand

[
    Located {
        location: Location {
            row: 1,
            column: 0,
        },
        end_location: Some(
            Location {
                row: 1,
                column: 24,
            },
        ),
        custom: (),
        node: Expr {
            value: Located {
                location: Location {
                    row: 1,
                    column: 0,
                },
                end_location: Some(
                    Location {
                        row: 1,
                        column: 24,
                    },
                ),
                custom: (),
                node: Dict {
                    👇👇👇 contains one key
                    keys: [
                        Located {
                            location: Location {
                                row: 1,
                                column: 1,
                            },
                            end_location: Some(
                                Location {
                                    row: 1,
                                    column: 6,
                                },
                            ),
                            custom: (),
                            node: Constant {
                                value: Str(
                                    "foo",
                                ),
                                kind: None,
                            },
                        },
                    ],
                    👇👇👇 values look correct?
                    values: [
                        Located {
                            location: Location {
                                row: 1,
                                column: 8,
                            },
                            end_location: Some(
                                Location {
                                    row: 1,
                                    column: 9,
                                },
                            ),
                            custom: (),
                            node: Constant {
                                value: Int(
                                    1,
                                ),
                                kind: None,
                            },
                        },
                        Located {
                            location: Location {
                                row: 1,
                                column: 13,
                            },
                            end_location: Some(
                                Location {
                                    row: 1,
                                    column: 23,
                                },
                            ),
                            custom: (),
                            node: Dict {
                                keys: [
                                    Located {
                                        location: Location {
                                            row: 1,
                                            column: 14,
                                        },
                                        end_location: Some(
                                            Location {
                                                row: 1,
                                                column: 19,
                                            },
                                        ),
                                        custom: (),
                                        node: Constant {
                                            value: Str(
                                                "bar",
                                            ),
                                            kind: None,
                                        },
                                    },
                                ],
                                values: [
                                    Located {
                                        location: Location {
                                            row: 1,
                                            column: 21,
                                        },
                                        end_location: Some(
                                            Location {
                                                row: 1,
                                                column: 22,
                                            },
                                        ),
                                        custom: (),
                                        node: Constant {
                                            value: Int(
                                                1,
                                            ),
                                            kind: None,
                                        },
                                    },
                                ],
                            },
                        },
                    ],
                },
            },
        },
    },
]

harupy · 2023-01-15T04:49:24Z

+        // let (keys, values) = match pairs.iter().position(|(k,_)| k.is_none()) {
+        //     Some(unpack_idx) => {
+        //         let mut pairs = pairs;
+        //         let (keys, mut values): (_, Vec<_>) = pairs.drain(..unpack_idx).map(|(k, v)| (*k.unwrap(), v)).unzip();
+        //
+        //         fn build_map(items: &mut Vec<(ast::Expr, ast::Expr)>) -> ast::Expr {
+        //             let location = items[0].0.location;
+        //             let end_location = items[0].0.end_location;
+        //             let (keys, values) = items.drain(..).unzip();
+        //             ast::Expr {
+        //                 location,
+        //                 end_location,
+        //                 custom: (),
+        //                 node: ast::ExprKind::Dict { keys, values }
+        //             }
+        //         }
+        //
+        //         let mut items = Vec::new();
+        //         for (key, value) in pairs.into_iter() {
+        //             if let Some(key) = key {
+        //                 items.push((*key, value));
+        //                 continue;
+        //             }
+        //             if !items.is_empty() {
+        //                 values.push(build_map(&mut items));
+        //             }
+        //             values.push(value);
+        //         }
+        //         if !items.is_empty() {
+        //             values.push(build_map(&mut items));
+        //         }
+        //         (keys, values)
+        //     },
+        //     None => pairs.into_iter().map(|(k, v)| (k, v)).unzip()
+        // };


What this block does is:

{ "a": "b", **c, "d": "e", # <- treat this as Dict so we can process this item with Instruction::DictUpdate **f, }

We can remove this.

youknowone · 2023-01-15T16:07:33Z

+        keys: &[Option<ast::Expr>],
+        values: &[ast::Expr],
+    ) -> CompileResult<()> {
+        emit!(self, Instruction::BuildMap { size: 0 });


This implementation looks like to make performance regression

Got it. Reverted.

This reverts commit b31b08a.

harupy · 2023-01-17T10:21:04Z

@youknowone @coolreader18 Would you mind taking a look?

youknowone · 2023-01-17T10:59:16Z

    },
    Dict {
-        keys: Vec<Expr<U>>,
+        keys: Vec<Option<Expr<U>>>,


I found I changed it to the previous form to keep cpython compatibility in #4042.
I'd like to ask Option<Expr<U>> is fit in cpython way.
I thought this is the new cpython way when i was working on #4042 which is changed in 3.10

@youknowone Why did you change it to the previous form?

I can't remember well so looking in it again.
I thought CPython's Python.asdl actually changed it to not optional here.
https://github.com/RustPython/RustPython/pull/4042/files#diff-242a44b9d53b6c6212a752e0c97d6cf2f87794f579700aa8015bb99cef63925cR63

But I still can see ast.parse generate None for empty keys in Python 3.10 as you described. I am confused.
Do you know what the '?' means in Python.asdl?

I think ? means optional.

Yes, that was the reason I thought CPython changed it from optional key to non-optional one in 3.10. How they still have None in parse.dump?

harupy · 2023-01-20T15:08:45Z

    },
    Dict {
-        keys: Vec<Expr<U>>,
+        keys: Vec<Option<Box<Expr<U>>>>,


Suggested change

keys: Vec<Option<Box<Expr<U>>>>,

keys: Vec<Option<Expr<U>>>,

@youknowone Is Box necessary?

youknowone · 2023-01-21T02:16:30Z

         | Lambda(arguments args, expr body)
         | IfExp(expr test, expr body, expr orelse)
-         | Dict(expr* keys, expr* values)
+         | Dict(expr?* keys, expr* values)


CPython doesn't have ? in keys anymore.

https://github.com/python/cpython/blob/main/Parser/Python.asdl#L64

@youknowone True, but dict.keys does contain None.

https://github.com/python/cpython/blob/main/Parser/Python.asdl#L64 used to look like | Dict(expr?* keys, expr* values)?

It looks like this line hasn't changed for 11 years: https://github.com/python/cpython/blame/3325f054e33b318aa56b74472f76a56b8afc0510/Parser/Python.asdl#L64

https://github.com/python/cpython/blob/3325f054e33b318aa56b74472f76a56b8afc0510/Grammar/python.gram#L904

double_starred_kvpair[KeyValuePair*]: | '**' a=bitwise_or { _PyPegen_key_value_pair(p, NULL, a) } ^^^^ | kvpair

@youknowone Can we move forward with expr?* keys or should we fix compiler/ast/asdl_rs.py as I did in
ea9db0e?

I don't think expr?* is the right thing here. Not only is it probably not valid ASDL syntax (see here, the modifiers are in brackets as opposed to braces, though this might not really be an issue), but it is also kinda ambiguous, is it an Optional list or a list of optionals?

Can't we get the same behavior by modifying asdl_rs and keeping Python.asdl the same as CPythons?

Not only is it probably not valid ASDL syntax (see here, the modifiers are in brackets as opposed to braces, though this might not really be an issue), but it is also kinda ambiguous, is it an Optional list or a list of optional?

Agreed!

Can't we get the same behavior by modifying asdl_rs and keeping Python.asdl the same as CPythons?

We can. I'm happy to revert the last commit.

@youknowone What do you think?

If we could do, I prefer to keep Python.asdl as same as CPython. It was your original approach, right?

Yes, updated the code.

youknowone · 2023-01-21T04:01:42Z

Sure, i am sorry for that.I will add a comment that part is our local patch not to break it again in future2023. 1. 21. 12:16, Harutaka Kawamura ***@***.***> 작성: @harupy commented on this pull request. In compiler/ast/Python.asdl:

@@ -60,7 +60,7 @@ module Python

| UnaryOp(unaryop op, expr operand) | Lambda(arguments args, expr body) | IfExp(expr test, expr body, expr orelse) - | Dict(expr* keys, expr* values) + | Dict(expr?* keys, expr* values) @youknowone Can we move forward with expr?* keys? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

harupy · 2023-01-21T04:43:35Z

I will add a comment that part is our local patch not to break it again in future

Makes sense! Feel free to push a commit to this PR if necessary :)

harupy · 2023-01-22T02:21:35Z

@youknowone @DimitrisJim Updated the code! Would you mind taking another look?

youknowone

Thank you! And I am really sorry for confusing review for this PR 😅

harupy · 2023-01-22T11:23:50Z

Not a problem at all!

This PR upgrades RustPython to fix the type of `Dict.keys` to `Vec<Option<Expr>>` (see RustPython/RustPython#4449 for why this change was needed) and unblock #1884.

Fix dict spreading in dict literal

e1f41e8

harupy changed the title ~~Fix AST when parsing a dict literal containing dict spreading~~ Fix AST generated from a dict literal containing dict spreading Jan 15, 2023

harupy added 2 commits January 15, 2023 13:11

Fix unparse

49b682e

Fix scan_expression and compile_dict

cdb1323

This was referenced Jan 15, 2023

wip: Add flake8-pie PIE804: no-unnecessary-dict-kwargs sbdchd/ruff#1

Closed

Add flake8-pie PIE804: no-unnecessary-dict-kwargs astral-sh/ruff#1884

Merged

harupy commented Jan 15, 2023

View reviewed changes

Comment thread compiler/parser/src/snapshots/rustpython_parser__parser__tests__dict_containing_spread.snap Resolved

harupy commented Jan 15, 2023

View reviewed changes

harupy added 2 commits January 15, 2023 14:54

Fix scan_expression and compile_dict

ae77099

Fix clippy errors

fdd3bea

harupy marked this pull request as ready for review January 15, 2023 06:47

harupy added 3 commits January 15, 2023 16:10

Remove commented-out code

82892f3

Add Option to Dict.keys field

ea9db0e

Improve test

290736a

harupy commented Jan 15, 2023

View reviewed changes

Comment thread compiler/codegen/src/compile.rs Resolved

harupy added 3 commits January 15, 2023 22:08

Simplify scan_expression

15228f4

Simplify compile_dict

b31b08a

Rename test

964cc1b

youknowone requested a review from coolreader18 January 15, 2023 16:04

youknowone reviewed Jan 15, 2023

View reviewed changes

Revert "Simplify compile_dict"

f4363c8

This reverts commit b31b08a.

harupy requested review from youknowone and removed request for coolreader18 January 16, 2023 00:20

harupy changed the title ~~Fix AST generated from a dict literal containing dict spreading~~ Fix AST generated from a dict literal containing dict unpacking Jan 16, 2023

youknowone reviewed Jan 17, 2023

View reviewed changes

harupy requested a review from youknowone January 20, 2023 03:39

harupy commented Jan 20, 2023

View reviewed changes

youknowone reviewed Jan 21, 2023

View reviewed changes

Fix comment

88e3c83

harupy force-pushed the fix-dict-spread-in-dict branch from b0df453 to 88e3c83 Compare January 21, 2023 15:13

harupy requested review from DimitrisJim and youknowone and removed request for DimitrisJim and youknowone January 21, 2023 16:50

youknowone approved these changes Jan 22, 2023

View reviewed changes

youknowone merged commit 4f38cb6 into RustPython:main Jan 22, 2023

harupy mentioned this pull request Jan 22, 2023

Update RustPython to fix Dict.keys type astral-sh/ruff#2086

Merged

Conversation

harupy commented Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CPython:

RustPython

Uh oh!

Uh oh!

harupy Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy commented Jan 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harupy Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Jan 21, 2023 via email

Uh oh!

harupy commented Jan 21, 2023

Uh oh!

harupy commented Jan 22, 2023

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

harupy commented Jan 22, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

harupy commented Jan 15, 2023 •

edited

Loading

harupy Jan 15, 2023 •

edited

Loading

harupy Jan 15, 2023 •

edited

Loading

youknowone Jan 18, 2023 •

edited

Loading

harupy Jan 20, 2023 •

edited

Loading

harupy Jan 21, 2023 •

edited

Loading

harupy Jan 21, 2023 •

edited

Loading

youknowone Jan 21, 2023 •

edited

Loading