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 the NAME 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 of import a as a.b (it'll just parse import 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 the NAME 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 of import a as a.b (it'll just parse import a and be done with it).

Yeah, but we are adding now a lookahead (&(',' | ')') which also includes NEWLINE. I am a bit wary of introducing rules that deal explicitly with NEWLINE tokens because I have seen those backfire in the past. For example, it's not imediately clear why a file that just has import 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

>>> from a import b.c
  File "<stdin>", line 1
    from a import b.c
                   ^
SyntaxError: invalid syntax

this can also be improved, not only as. 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. Converting b=['as' z=NAME { z }] to its own rules import_alias with invalid_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 add invalid_dotted_as_name: dotted_name 'as' a=expression as the first rule to dotted_name, but this way it can accidentally match first on the second on the valid 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 the official 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. Converting b=['as' z=NAME { z }] to its own rules import_alias with invalid_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 add invalid_dotted_as_name: dotted_name 'as' a=expression as the first rule to dotted_name, but this way it can accidentally match first on the second on the valid 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.gram
index 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_grammar
Using random seed: 2322618595
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 5.06 Run 2 tests sequentially in a single process
0:00:00 load avg: 5.06 [1/2] test_syntax
0:00:00 load avg: 5.06 [2/2] test_grammar

== Tests result: SUCCESS ==

All 2 tests OK.

Total duration: 268 ms
Total tests: run=116
Total 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 👌🤘