gh-131421: fix ASDL grammar for `Dict` to have an `expr?*` keys field #131419

Demonstrandum · 2025-03-18T13:48:08Z

In the ast documentation for Python:

https://docs.python.org/3/library/ast.html#ast.Dict it is made clear that:

When doing dictionary unpacking using dictionary literals the expression to be expanded goes in the values list, with a None at the corresponding position in keys.

Hence, keys is really a expr?* and not a expr*.

Issue: Wrong Python.asdl type for keys in Dict literals #131421

In the `ast` documentation for Python: * https://docs.python.org/3/library/ast.html#ast.Dict it is made clear that: > When doing dictionary unpacking using dictionary literals the expression to be expanded goes in the values list, with a `None` at the corresponding position in `keys`. Hence, `keys` is really a `expr?*` and *not* a `expr*`.

cpython-cla-bot · 2025-03-18T13:48:11Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2025-03-18T13:48:12Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

picnixz · 2025-03-18T14:47:20Z

Parser/Python.asdl

@@ -63,7 +63,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)


This kind of change makes me wonder whether we should also add '+' to indicate 1 or more items. For instance nonlocal requires at least one name, same for import, global and others.

Similarly, there is no explanation of what * and ? actually mean. While they can be inferred, I would expect a caption for them. WDYT @JelleZijlstra?

JelleZijlstra · 2025-03-18T14:53:58Z

Python.asdl is used to generate the AST code, and this change doesn't compile (to reproduce run make regen-ast locally; ideally we'd fix CI to do that).

I don't think it's really worth changing the grammar here.

picnixz · 2025-03-18T14:56:53Z

I don't think it's really worth changing the grammar here.

Can we perhaps add a comment instead? because now that I think about it, for static analysis tools, we could be surprised (like, if we only focus on the ASDL and not on the documented entry itself). [I could be surprised in Sphinx though I don't think I've encountered an issue with this]

Demonstrandum · 2025-03-18T16:38:01Z

It represents a point where it completely breaks expectation, where up until then, the ASDL (as far as I have observed) was being completely respected by the ast module. Since * and ? are modifiers on types, and expr? is still a type, morally a star should be able to follow any type, including expr? to get a expr?*, for example.

Would this be very difficult to parse / break a lot of handling?

JelleZijlstra · 2025-03-18T16:50:29Z

It's been like this for a long time and I don't recall any other reports of confusion about it, so there's a good argument to stick with the working code.

That said, if you can provide a patch that parses this new syntax correctly and that compiles correctly, we can consider it.

Demonstrandum · 2025-03-18T18:17:47Z

@JelleZijlstra I updated the Parser/asdl.py ASDL parser to support chained quantifiers. I made sure the opt and seq interface it exposes remains the same (based of the last quantifier supplied). This means the generated Python/Python-ast.c is unchanged.

Naturally, make regen-ast works now.

tomasr8 · 2025-03-18T18:23:33Z

Parser/asdl.py

+class Quantifier:
+    class _Optional: pass
+    class _Sequence: pass
+    OPTIONAL = _Optional()
+    SEQUENCE = _Sequence()


Could this just be an enum?

Sure, I just prefer these to have unique/singleton values (as opposed to ints or whatnot).

I can change it if its outside the expected coding style.

If you wanted singletons you could also just do OPTIONAL = object() but to be honest I think an Enum/StrEnum is a better choice here since it's more explicit.

(one reason to do the class trick is to have a bit of a nicer repr(), but OTOH, I don't think we need to worry about importing enum)

Is it worth importing enum? If you want to add your suggested change I can commit it.

picnixz

I'm not sure if this is already the case or not, but can you quickly check that the tests for ast.parse('{}') handle this case?

Parser/asdl.py

picnixz · 2025-03-18T18:30:20Z

Parser/asdl.py

-        elif self.cur_token.kind == TokenKind.Question:
-            is_opt = True
+        quantifiers = []
+        while self.cur_token.kind in (TokenKind.Asterisk, TokenKind.Question):


Shouldn't we check that we don't have ** for instance or ???

Maybe. But both ?? and ** are legitimate types, ** perhaps more legitimate than ?? is.

Ah yes, actually the current parser doesn't care. I thought that it would have warned the user. And should T** actually mean a list of list of T? is the obtained C code designed to handle this kind of grammar?

I have no strong opinion now on whether we should warn ??.

There is essentially no change to the C code at all at this point. I don't know how this was handled before, since expr?* was always there implicitly, but this didn't seem to matter.

picnixz · 2025-03-18T18:32:44Z

Parser/asdl.py

 class Field(AST):
-    def __init__(self, type, name=None, seq=False, opt=False):
+    def __init__(self, type, name=None, quantifiers=None):


Is there a need to support more than two qualifiers? and/or shouldn't we check the consistency of those qualifiers as well?

Currently there are no more than two quantifiers on any type, as per my change to Python.asdl.

I'm just implementing the EBNF grammar described at the top of the asdl.py file. And I don't think this files does any amount of extra validation so far anyway.

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Demonstrandum requested review from pablogsal and lysnikolaou as code owners March 18, 2025 13:48

bedevere-app bot added the awaiting review label Mar 18, 2025

Demonstrandum changed the title ~~Python.asdl: Fix Dict variant to have an expr?* keys field.~~ gh-131421: Python.asdl: Fix Dict variant to have an expr?* keys field. Mar 18, 2025

bedevere-app bot mentioned this pull request Mar 18, 2025

Wrong Python.asdl type for keys in Dict literals #131421

Open

picnixz added the skip news label Mar 18, 2025

picnixz reviewed Mar 18, 2025

View reviewed changes

picnixz changed the title ~~gh-131421: Python.asdl: Fix Dict variant to have an expr?* keys field.~~ gh-131421: fix ASDL grammar for Dict to have an expr?* keys field Mar 18, 2025

Update Parser/asdl.py to parse multiple chained quantifiers.

2a19020

Demonstrandum requested review from isidentical, JelleZijlstra and Eclips4 as code owners March 18, 2025 18:14

tomasr8 reviewed Mar 18, 2025

View reviewed changes

picnixz reviewed Mar 18, 2025

View reviewed changes

Remove newline before return.

cbccd92

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

gh-131421: fix ASDL grammar for `Dict` to have an `expr?*` keys field #131419

gh-131421: fix ASDL grammar for `Dict` to have an `expr?*` keys field #131419

Demonstrandum commented Mar 18, 2025 •

edited by bedevere-app bot

Loading

cpython-cla-bot bot commented Mar 18, 2025 •

edited

Loading

bedevere-app bot commented Mar 18, 2025

picnixz Mar 18, 2025

JelleZijlstra commented Mar 18, 2025

picnixz commented Mar 18, 2025 •

edited

Loading

Demonstrandum commented Mar 18, 2025

JelleZijlstra commented Mar 18, 2025

Demonstrandum commented Mar 18, 2025

tomasr8 Mar 18, 2025

Demonstrandum Mar 18, 2025

tomasr8 Mar 18, 2025

picnixz Mar 18, 2025

Demonstrandum Mar 19, 2025

picnixz left a comment

picnixz Mar 18, 2025

Demonstrandum Mar 18, 2025

picnixz Mar 18, 2025

Demonstrandum Mar 18, 2025

picnixz Mar 18, 2025

Demonstrandum Mar 18, 2025

gh-131421: fix ASDL grammar for Dict to have an expr?* keys field #131419

Are you sure you want to change the base?

gh-131421: fix ASDL grammar for Dict to have an expr?* keys field #131419

Conversation

Demonstrandum commented Mar 18, 2025 • edited by bedevere-app bot Loading

cpython-cla-bot bot commented Mar 18, 2025 • edited Loading

bedevere-app bot commented Mar 18, 2025

Choose a reason for hiding this comment

JelleZijlstra commented Mar 18, 2025

picnixz commented Mar 18, 2025 • edited Loading

Demonstrandum commented Mar 18, 2025

JelleZijlstra commented Mar 18, 2025

Demonstrandum commented Mar 18, 2025

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

picnixz left a 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

gh-131421: fix ASDL grammar for `Dict` to have an `expr?*` keys field #131419

gh-131421: fix ASDL grammar for `Dict` to have an `expr?*` keys field #131419

Demonstrandum commented Mar 18, 2025 •

edited by bedevere-app bot

Loading

cpython-cla-bot bot commented Mar 18, 2025 •

edited

Loading

picnixz commented Mar 18, 2025 •

edited

Loading