Simplify the Grammar #116988

Rexicon226 · 2024-03-19T09:53:08Z

Feature or enhancement

Proposal:

I've been implementing a Python runtime for a couple of months and I've been meaning to ask about something I noticed in the grammar.

Why do we use:

assignment:
    | NAME ':' expression ['=' annotated_rhs ] 
    | ('(' single_target ')' 
         | single_subscript_attribute_target) ':' expression ['=' annotated_rhs ] 
    | (star_targets '=' )+ (yield_expr | star_expressions) !'=' [TYPE_COMMENT] 
    | single_target augassign ~ (yield_expr | star_expressions) 

annotated_rhs: yield_expr | star_expressions

Instead of:

assignment:
    | NAME ':' expression ['=' annotated_rhs ] 
    | ('(' single_target ')' 
         | single_subscript_attribute_target) ':' expression ['=' annotated_rhs ] 
    | (star_targets '=' )+ (annotated_rhs) !'=' [TYPE_COMMENT] 
    | single_target augassign ~ (annotated_rhs) 

annotated_rhs: yield_expr | star_expressions

Forgive my ignorance when it comes to grammars but are these not the same thing?

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

gh-116988: Remove duplicates of annotated_rhs in the Grammar #117004

The text was updated successfully, but these errors were encountered:

lysnikolaou · 2024-03-19T10:11:06Z

This appears to be a simple oversight, since annotated_rhs and the expression within the parantheses are equivalent. @Rexicon226 Would you like to open a PR to fix this?

Rexicon226 · 2024-03-19T10:16:47Z

This appears to be a simple oversight, since annotated_rhs and the expression within the parantheses are equivalent. @Rexicon226 Would you like to open a PR to fix this?

Sure! Thank you for checking my sanity in understanding the grammar. Other than the grammar file itself, what else needs to be done for a PR? I am not familiar with contributing to CPython.

lysnikolaou · 2024-03-19T10:20:37Z

Don't feel pressured to fix this. You can let me know and I can push a quick PR as well.

If you want to open a PR for it though, start by going through the Developer's Guide. After having changed the grammar file (python.gram) with the changes you proposed above, you'll need to run make regen-pegen so that the C file containing the parser code gets regenerated.

Rexicon226 · 2024-03-19T10:24:12Z

Sounds good, I can do it now.

Rexicon226 · 2024-03-19T12:41:19Z

8000 I'm having an issue with something I'm trying. I've looked into other instances of this rule being duplicated and successfully swapped all other oversights, except this one:

cpython/Grammar/python.gram

Line 1402 in 408e127

    
           | '{' !(yield_expr | star_expressions) { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting a valid expression after '{'")}

Because annotated_rhs_rule returns a expr_ty instead of a void pointer like tmp functions do, I get a warning when building:

Parser/parser.c:24788:35: warning: passing argument 2 of ‘_PyPegen_lookahead’ from incompatible pointer type [-Wincompatible-pointer-types]
24788 |             _PyPegen_lookahead(0, annotated_rhs_rule, p)
      |                                   ^~~~~~~~~~~~~~~~~~
      |                                   |
      |                                   struct _expr * (*)(Parser *)

What is the correct solution here?

lysnikolaou · 2024-03-19T12:49:11Z

Did you add a parenthesis around annotated_rhs as in | '{' !(annotated_rhs)? If so, you can remove it and it will probably work. If not, I'll have to spend some more time to figure out why this may be happening.

EDIT: Can you maybe post the diff here?

Rexicon226 · 2024-03-19T12:52:06Z

Did you add a parenthesis around annotated_rhs as in | '{' !(annotated_rhs)? If so, you can remove it and it will probably work.

Both versions generate the same problem.

Here's the diff:

diff --git a/Grammar/python.gram b/Grammar/python.gram
index 5153b02..69f2ed4 100644
--- a/Grammar/python.gram
+++ b/Grammar/python.gram
@@ -913,7 +913,7 @@ fstring_middle[expr_ty]:
     | fstring_replacement_field
     | t=FSTRING_MIDDLE { _PyPegen_constant_from_token(p, t) }
 fstring_replacement_field[expr_ty]:
-    | '{' a=(yield_expr | star_expressions) debug_expr='='? conversion=[fstring_conversion] format=[fstring_full_format_spec] rbrace='}' {
+    | '{' a=annotated_rhs debug_expr='='? conversion=[fstring_conversion] format=[fstring_full_format_spec] rbrace='}' {
         _PyPegen_formatted_value(p, a, debug_expr, conversion, format, rbrace, EXTRA) }
     | invalid_replacement_field
 fstring_conversion[ResultTokenWithMetadata*]:
@@ -1198,7 +1198,7 @@ invalid_assignment:
     | (star_targets '=')* a=star_expressions '=' {
         RAISE_SYNTAX_ERROR_INVALID_TARGET(STAR_TARGETS, a) }
     | (star_targets '=')* a=yield_expr '=' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "assignment to yield expression not possible") }
-    | a=star_expressions augassign (yield_expr | star_expressions) {
+    | a=star_expressions augassign annotated_rhs {
         RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
             a,
             "'%s' is an illegal expression for augmented assignment",
@@ -1399,17 +1399,17 @@ invalid_replacement_field:
     | '{' a='!' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "f-string: valid expression required before '!'") }
     | '{' a=':' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "f-string: valid expression required before ':'") }
     | '{' a='}' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "f-string: valid expression required before '}'") }
-    | '{' !(yield_expr | star_expressions) { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting a valid expression after '{'")}
-    | '{' (yield_expr | star_expressions) !('=' | '!' | ':' | '}') {
+    | '{' !(annotated_rhs) { RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting a valid expression after '{'")}
+    | '{' annotated_rhs !('=' | '!' | ':' | '}') {
         PyErr_Occurred() ? NULL : RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting '=', or '!', or ':', or '}'") }
-    | '{' (yield_expr | star_expressions) '=' !('!' | ':' | '}') {
+    | '{' annotated_rhs '=' !('!' | ':' | '}') {
         PyErr_Occurred() ? NULL : RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting '!', or ':', or '}'") }
-    | '{' (yield_expr | star_expressions) '='? invalid_conversion_character
-    | '{' (yield_expr | star_expressions) '='? ['!' NAME] !(':' | '}') {
+    | '{' annotated_rhs '='? invalid_conversion_character
+    | '{' annotated_rhs '='? ['!' NAME] !(':' | '}') {
         PyErr_Occurred() ? NULL : RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting ':' or '}'") }
-    | '{' (yield_expr | star_expressions) '='? ['!' NAME] ':' fstring_format_spec* !'}' {
+    | '{' annotated_rhs '='? ['!' NAME] ':' fstring_format_spec* !'}' {
         PyErr_Occurred() ? NULL : RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting '}', or format specs") }
-    | '{' (yield_expr | star_expressions) '='? ['!' NAME] !'}' {
+    | '{' annotated_rhs '='? ['!' NAME] !'}' {
         PyErr_Occurred() ? NULL : RAISE_SYNTAX_ERROR_ON_NEXT_TOKEN("f-string: expecting '}'") }
 
 invalid_conversion_character:

lysnikolaou · 2024-03-19T13:05:47Z

Right, this is the first time we're using a negative lookahead on a rule that's explicitly typed to something other than void * then. We should probably explicitly cast the function we pass to _PyPegen_lookahead, so a diff like this one should probably work:

diff --git a/Tools/peg_generator/pegen/c_generator.py b/Tools/peg_generator/pegen/c_generator.py
index 7cdd5debe9..84ed183c76 100644
--- a/Tools/peg_generator/pegen/c_generator.py
+++ b/Tools/peg_generator/pegen/c_generator.py
@@ -253,7 +253,7 @@ def lookahead_call_helper(self, node: Lookahead, positive: int) -> FunctionCall:
         else:
             return FunctionCall(
                 function=f"_PyPegen_lookahead",
-                arguments=[positive, call.function, *call.arguments],
+                arguments=[positive, f"(void *(*)(Parser *)) {call.function}", *call.arguments],
                 return_type="int",
             )

Rexicon226 · 2024-03-19T13:11:03Z

Funny how we came to the same conclusion at the same time. That is what I just did.
Will open a PR is a moment. Any advice on writing the News entry? Should I mention this change we've made to the pegen?

lysnikolaou · 2024-03-19T13:16:37Z

This is a small enough change (with no visible behavior changes) that it probably doesn't need a NEWS blurb.

dongrixinyu · 2024-09-05T09:23:01Z

Excuse me.
I am reading the source code of parse.c ane notice the function assignment_rule.
I am confused about the annotation of

// assignment:
//     | NAME ':' expression ['=' annotated_rhs]
//     | ('(' single_target ')' | single_subscript_attribute_target) ':' expression ['=' annotated_rhs]
//     | ((star_targets '='))+ (yield_expr | star_expressions) !'=' TYPE_COMMENT?
//     | single_target augassign ~ (yield_expr | star_expressions)
//     | invalid_assignment

What's the meaning of these patterns, for example, ((star_targets '='))+ (yield_expr | star_expressions) !'=' TYPE_COMMENT?

What is star_targets and star_expressions? Could you provide some help to understand it?

Rexicon226 added the type-feature A feature request or enhancement label Mar 19, 2024

lysnikolaou added type-bug An unexpected behavior, bug, or error easy topic-parser and removed type-feature A feature request or enhancement labels Mar 19, 2024

Rexicon226 mentioned this issue Mar 19, 2024

gh-116988: Remove duplicates of annotated_rhs in the Grammar #117004

Merged

pablogsal closed this as completed in #117004 Apr 24, 2024

pablogsal pushed a commit that referenced this issue Apr 24, 2024

gh-116988: Remove duplicates of annotated_rhs in the Grammar (#117004)

9b280ab

tomasr8 mentioned this issue Aug 12, 2024

Simplify the grammar of the assignment rule #122951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Simplify the Grammar #116988

Simplify the Grammar #116988

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Simplify the Grammar #116988

Simplify the Grammar #116988

Comments

Uh oh!

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Linked PRs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!