When you're done making the requested changes, leave the comment:I have made the requested changes; please review again.

I would prefer to not change or introduce new first-pass rules when introducing error messages. If you have problems with precedence, perhaps you can just move the invalid rule the first one

Well, I think in this case it's not possible to do it otherwise. You wanna to be checking theNAME variant first to be sure that there's no valid syntax there, then check the invalid rule for the error message, but before the no alias alternative is visited, since that will succeed in the case ofimport a as a.b (it'll just parseimport a and be done with it).

FWIW my guesstimate is that this might be frequent enough that changing existing rules is warranted.

Well, I think in this case it's not possible to do it otherwise. You wanna to be checking theNAME variant first to be sure that there's no valid syntax there, then check the invalid rule for the error message, but before the no alias alternative is visited, since that will succeed in the case ofimport a as a.b (it'll just parseimport a and be done with it).

Yeah, but we are adding now a lookahead (&(',' | ')') which also includesNEWLINE. I am a bit wary of introducing rules that deal explicitly withNEWLINE tokens because I have seen those backfire in the past. For example, it's notimediately clear why a file that just hasimport a as b (no newline at the end) is not now a syntax error (is because the implicit newline, but that's not obvious when you read the grammar file).

I will try some other approaches, I might come up with something simplier.

In the meantime I also noticed that

>>>fromaimportb.cFile"<stdin>",line1fromaimportb.c^SyntaxError:invalidsyntax

this can also be improved, not onlyas. But, I will defer this to another PR.

But, I will defer this to another PR.

Whatever you prefer, it's ok to do it all here as we are already discussing this rule :)

I've spent several hours trying something else:

1. Convertingb=['as' z=NAME { z }] to its own rulesimport_alias withinvalid_import_alias rule inside, but there's no clear way to tell apart'as' NAME and'as' NAME.NAME without some extra complexity and wierd lookaheads
2. I tried to just addinvalid_dotted_as_name: dotted_name 'as' a=expression as the first rule todotted_name, but this way it can accidentally match first on the second on thevalid code :(
3. I tried to simplify the current solution, but it is not easy as well. It requires look aheads.

Right now I think that it is better to close this PR, unfortunatelly.

Right now I think that it is better to close this PR, unfortunatelly.

If@lysnikolaou it's not too worried about the lookahead I am happy to consider landing it as is currently, but I am a bit worried that this also makes the grammar more difficult to reason about in theofficial docs because there it won't be apparent why the lookahead is needed (and other rules don't have it).

I've spent several hours trying something else:

1. Convertingb=['as' z=NAME { z }] to its own rulesimport_alias withinvalid_import_alias rule inside, but there's no clear way to tell apart'as' NAME and'as' NAME.NAME without some extra complexity and wierd lookaheads
2. I tried to just addinvalid_dotted_as_name: dotted_name 'as' a=expression as the first rule todotted_name, but this way it can accidentally match first on the second on thevalid code :(
3. I tried to simplify the current solution, but it is not easy as well. It requires look aheads.

Right now I think that it is better to close this PR, unfortunatelly.

This works:

diff --git a/Grammar/python.gram b/Grammar/python.gramindex c8415552b28..8eb9bb7473c 100644--- a/Grammar/python.gram+++ b/Grammar/python.gram@@ -218,17 +218,17 @@ import_from_targets[asdl_alias_seq*]: import_from_as_names[asdl_alias_seq*]:     | a[asdl_alias_seq*]=','.import_from_as_name+ { a } import_from_as_name[alias_ty]:-    | a=NAME 'as' b=NAME &(',' | ')' | NEWLINE) {-        _PyAST_alias(a->v.Name.id, b->v.Name.id, EXTRA) }     | invalid_import_from_as_name-    | a=NAME { _PyAST_alias(a->v.Name.id, NULL, EXTRA) }+    | a=NAME b=['as' z=NAME { z }] { _PyAST_alias(a->v.Name.id,+                                               (b) ? ((expr_ty) b)->v.Name.id : NULL,+                                               EXTRA) } dotted_as_names[asdl_alias_seq*]:     | a[asdl_alias_seq*]=','.dotted_as_name+ { a } dotted_as_name[alias_ty]:-    | a=dotted_name 'as' b=NAME &(',' | NEWLINE) {-        _PyAST_alias(a->v.Name.id, b->v.Name.id, EXTRA) }     | invalid_dotted_as_name-    | a=dotted_name { _PyAST_alias(a->v.Name.id, NULL, EXTRA) }+    | a=dotted_name b=['as' z=NAME { z }] { _PyAST_alias(a->v.Name.id,+                                                      (b) ? ((expr_ty) b)->v.Name.id : NULL,+                                                      EXTRA) } dotted_name[expr_ty]:     | a=dotted_name '.' b=NAME { _PyPegen_join_names_with_dot(p, a, b) }     | NAME@@ -1324,11 +1324,11 @@ invalid_import_from_targets:     | token=NEWLINE {         RAISE_SYNTAX_ERROR_STARTING_FROM(token, "Expected one or more names after 'import'") } invalid_dotted_as_name:-    | dotted_name 'as' a=expression {+    | dotted_name 'as' !(NAME (',' | ')' | NEWLINE)) a=expression {         RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,             "cannot use %s as import target", _PyPegen_get_expr_name(a)) } invalid_import_from_as_name:-    | NAME 'as' a=expression {+    | NAME 'as' !(NAME (',' | ')' | NEWLINE))   a=expression {         RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a,             "cannot use %s as import target", _PyPegen_get_expr_name(a)) }

./python.exe -m test test_syntax test_grammarUsing random seed: 2322618595Raised RLIMIT_NOFILE: 256 -> 10240:00:00 load avg: 5.06 Run 2 tests sequentially in a single process0:00:00 load avg: 5.06 [1/2] test_syntax0:00:00 load avg: 5.06 [2/2] test_grammar== Tests result: SUCCESS ==All 2 tests OK.Total duration: 268 msTotal tests: run=116Total test files: ru

Notice this doesn't modify the current working rules but just adds the invalid with higher precedence + filtering of the expected one. The filtering is needed otherwise it will match something valid if there is a syntax error afterwards.

@pablogsal thanks a lot for your help, great idea! Sorry, it took me so long to follow up on this PR.

Now it should be ready to review again 🫶

@pablogsal@lysnikolaou I would like to get this merged before the feature freeze, it would be amazing if you have some time to look at it :)

I'm merging this for now but could you create another PR adding the improvement to the what's new for 3.14 in the syntax error section?

Thanks a lot,@pablogsal! Without your help - it would be so much harder! 🫶

Sure, will do!

Fantastic job@sobolevn 👌🤘