Fix AST generated from a dict literal containing dict unpacking #4449

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,
                                        },
                                    },
                                ],
                            },
                        },
                    ],
                },
            },
        },
    },
]

compiler/parser/src/snapshots/rustpython_parser__parser__tests__dict_containing_spread.snap

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

compiler/parser/python.lalrpop

+        // 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.

compiler/codegen/src/compile.rs

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

compiler/codegen/src/compile.rs

+        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

compiler/ast/src/ast_gen.rs

@@ -195,7 +195,7 @@ pub enum ExprKind<U = ()> {
        orelse: Box<Expr<U>>,
    },
    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

compiler/ast/src/ast_gen.rs

@@ -195,7 +195,7 @@ pub enum ExprKind<U = ()> {
        orelse: Box<Expr<U>>,
    },
    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

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)


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

compiler/parser/src/snapshots/rustpython_parser__parser__tests__dict_containing_spread.snap Outdated Show resolved Hide 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

compiler/codegen/src/compile.rs Show resolved Hide 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 youknowone and DimitrisJim and removed request for youknowone and DimitrisJim 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AST generated from a dict literal containing dict unpacking #4449

Fix AST generated from a dict literal containing dict unpacking #4449

harupy commented Jan 15, 2023 •

edited

Loading

harupy Jan 15, 2023 •

edited

Loading

youknowone Jan 15, 2023

harupy Jan 15, 2023 •

edited

Loading

harupy commented Jan 17, 2023

youknowone Jan 17, 2023

harupy Jan 17, 2023

youknowone Jan 18, 2023 •

edited

Loading

harupy Jan 18, 2023

youknowone Jan 21, 2023

harupy Jan 20, 2023 •

edited

Loading

youknowone Jan 21, 2023

harupy Jan 21, 2023

harupy Jan 21, 2023 •

edited

Loading

harupy Jan 21, 2023

harupy Jan 21, 2023

harupy Jan 21, 2023 •

edited

Loading

DimitrisJim Jan 21, 2023

harupy Jan 21, 2023

youknowone Jan 21, 2023 •

edited

Loading

harupy Jan 21, 2023

youknowone commented Jan 21, 2023 via email

harupy commented Jan 21, 2023

harupy commented Jan 22, 2023

youknowone left a comment

harupy commented Jan 22, 2023

Fix AST generated from a dict literal containing dict unpacking #4449

Fix AST generated from a dict literal containing dict unpacking #4449

Conversation

harupy commented Jan 15, 2023 • edited Loading

CPython:

RustPython

harupy Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

harupy commented Jan 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harupy Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone Jan 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone commented Jan 21, 2023 via email

harupy commented Jan 21, 2023

harupy commented Jan 22, 2023

youknowone left a comment

Choose a reason for hiding this comment

harupy commented Jan 22, 2023

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