bpo-40334: Produce better error messages on invalid targets

Grammar/python.gram

@@ -92,9 +92,9 @@ assignment[stmt_ty]:
     | a=('(' b=single_target ')' { b }
          | single_subscript_attribute_target) ':' b=expression c=['=' d=annotated_rhs { d }] {
         CHECK_VERSION(6, "Variable annotations syntax is", _Py_AnnAssign(a, b, c, 0, EXTRA)) }
-    | a=(z=star_targets '=' { z })+ b=(yield_expr | star_expressions) tc=[TYPE_COMMENT] {
+    | a=(z=star_targets '=' { z })+ ~ b=(yield_expr | star_expressions) tc=[TYPE_COMMENT] {
          _Py_Assign(a, b, NEW_TYPE_COMMENT(p, tc), EXTRA) }
-    | a=single_target b=augassign c=(yield_expr | star_expressions) {
+    | a=single_target b=augassign ~ c=(yield_expr | star_expressions) {
          _Py_AugAssign(a, b->kind, c, EXTRA) }
     | invalid_assignment
 
@@ -122,7 +122,9 @@ yield_stmt[stmt_ty]: y=yield_expr { _Py_Expr(y, EXTRA) }
 
 assert_stmt[stmt_ty]: 'assert' a=expression b=[',' z=expression { z }] { _Py_Assert(a, b, EXTRA) }
 
-del_stmt[stmt_ty]: 'del' a=del_targets { _Py_Delete(a, EXTRA) }
+del_stmt[stmt_ty]:
+    | 'del' a=del_targets &(';' | NEWLINE) { _Py_Delete(a, EXTRA) }
+    | invalid_del_stmt
 
 import_stmt[stmt_ty]: import_name | import_from
 import_name[stmt_ty]: 'import' a=dotted_as_names { _Py_Import(a, EXTRA) }
@@ -164,10 +166,11 @@ while_stmt[stmt_ty]:
     | 'while' a=named_expression ':' b=block c=[else_block] { _Py_While(a, b, c, EXTRA) }
 
 for_stmt[stmt_ty]:
-    | 'for' t=star_targets 'in' ex=star_expressions ':' tc=[TYPE_COMMENT] b=block el=[else_block] {
+    | 'for' t=star_targets 'in' ~ ex=star_expressions ':' tc=[TYPE_COMMENT] b=block el=[else_block] {
         _Py_For(t, ex, b, el, NEW_TYPE_COMMENT(p, tc), EXTRA) }
-    | ASYNC 'for' t=star_targets 'in' ex=star_expressions ':' tc=[TYPE_COMMENT] b=block el=[else_block] {
+    | ASYNC 'for' t=star_targets 'in' ~ ex=star_expressions ':' tc=[TYPE_COMMENT] b=block el=[else_block] {
         CHECK_VERSION(5, "Async for loops are", _Py_AsyncFor(t, ex, b, el, NEW_TYPE_COMMENT(p, tc), EXTRA)) }
+    | invalid_for_target
 
 with_stmt[stmt_ty]:
     | 'with' '(' a=','.with_item+ ','? ')' ':' b=block {
@@ -178,6 +181,7 @@ with_stmt[stmt_ty]:
        CHECK_VERSION(5, "Async with statements are", _Py_AsyncWith(a, b, NULL, EXTRA)) }
     | ASYNC 'with' a=','.with_item+ ':' tc=[TYPE_COMMENT] b=block {
        CHECK_VERSION(5, "Async with statements are", _Py_AsyncWith(a, b, NEW_TYPE_COMMENT(p, tc), EXTRA)) }
+    | invalid_with_target
 with_item[withitem_ty]:
     | e=expression o=['as' t=target { t }] { _Py_withitem(e, o, p->arena) }
 
@@ -311,7 +315,7 @@ star_named_expression[expr_ty]:
     | '*' a=bitwise_or { _Py_Starred(a, Load, EXTRA) }
     | named_expression
 named_expression[expr_ty]:
-    | a=NAME ':=' b=expression { _Py_NamedExpr(CHECK(_PyPegen_set_expr_context(p, a, Store)), b, EXTRA) }
+    | a=NAME ':=' ~ b=expression { _Py_NamedExpr(CHECK(_PyPegen_set_expr_context(p, a, Store)), b, EXTRA) }
     | expression !':='
     | invalid_named_expression
 
@@ -484,18 +488,18 @@ strings[expr_ty] (memo): a=STRING+ { _PyPegen_concatenate_strings(p, a) }
 list[expr_ty]:
     | '[' a=[star_named_expressions] ']' { _Py_List(a, Load, EXTRA) }
 listcomp[expr_ty]:
-    | '[' a=named_expression b=for_if_clauses ']' { _Py_ListComp(a, b, EXTRA) }
+    | '[' a=named_expression ~ b=for_if_clauses ']' { _Py_ListComp(a, b, EXTRA) }
     | invalid_comprehension
 tuple[expr_ty]:
     | '(' a=[y=star_named_expression ',' z=[star_named_expressions] { _PyPegen_seq_insert_in_front(p, y, z) } ] ')' {
         _Py_Tuple(a, Load, EXTRA) }
 group[expr_ty]: '(' a=(yield_expr | named_expression) ')' { a }
 genexp[expr_ty]:
-    | '(' a=expression b=for_if_clauses ')' { _Py_GeneratorExp(a, b, EXTRA) }
+    | '(' a=expression ~ b=for_if_clauses ')' { _Py_GeneratorExp(a, b, EXTRA) }
     | invalid_comprehension
 set[expr_ty]: '{' a=expressions_list '}' { _Py_Set(a, EXTRA) }
 setcomp[expr_ty]:
-    | '{' a=expression b=for_if_clauses '}' { _Py_SetComp(a, b, EXTRA) }
+    | '{' a=expression ~ b=for_if_clauses '}' { _Py_SetComp(a, b, EXTRA) }
     | invalid_comprehension
 dict[expr_ty]:
     | '{' a=[kvpairs] '}' { _Py_Dict(CHECK(_PyPegen_get_keys(p, a)),
@@ -509,10 +513,11 @@ kvpair[KeyValuePair*]:
 for_if_clauses[asdl_seq*]:
     | for_if_clause+
 for_if_clause[comprehension_ty]:
-    | ASYNC 'for' a=star_targets 'in' b=disjunction c=('if' z=disjunction { z })* {
+    | ASYNC 'for' a=star_targets 'in' ~ b=disjunction c=('if' z=disjunction { z })* {
         CHECK_VERSION(6, "Async comprehensions are", _Py_comprehension(a, b, c, 1, p->arena)) }
-    | 'for' a=star_targets 'in' b=disjunction c=('if' z=disjunction { z })* {
+    | 'for' a=star_targets 'in' ~ b=disjunction c=('if' z=disjunction { z })* {
         _Py_comprehension(a, b, c, 0, p->arena) }
+    | invalid_for_target
 
 yield_expr[expr_ty]:
     | 'yield' 'from' a=expression { _Py_YieldFrom(a, EXTRA) }
@@ -582,19 +587,15 @@ single_subscript_attribute_target[expr_ty]:
     | a=t_primary '[' b=slices ']' !t_lookahead { _Py_Subscript(a, b, Store, EXTRA) }
 
 del_targets[asdl_seq*]: a=','.del_target+ [','] { a }
-# The lookaheads to del_target_end ensure that we don't match expressions where a prefix of the
-# expression matches our rule, thereby letting these cases fall through to invalid_del_target.
 del_target[expr_ty] (memo):
-    | a=t_primary '.' b=NAME &del_target_end { _Py_Attribute(a, b->v.Name.id, Del, EXTRA) }
-    | a=t_primary '[' b=slices ']' &del_target_end { _Py_Subscript(a, b, Del, EXTRA) }
+    | a=t_primary '.' b=NAME !t_lookahead { _Py_Attribute(a, b->v.Name.id, Del, EXTRA) }
+    | a=t_primary '[' b=slices ']' !t_lookahead { _Py_Subscript(a, b, Del, EXTRA) }
     | del_t_atom
 del_t_atom[expr_ty]:
-    | a=NAME &del_target_end { _PyPegen_set_expr_context(p, a, Del) }
+    | a=NAME { _PyPegen_set_expr_context(p, a, Del) }
     | '(' a=del_target ')' { _PyPegen_set_expr_context(p, a, Del) }
     | '(' a=[del_targets] ')' { _Py_Tuple(a, Del, EXTRA) }
     | '[' a=[del_targets] ']' { _Py_List(a, Del, EXTRA) }
-    | invalid_del_target
-del_target_end: ')' | ']' | ',' | ';' | NEWLINE
 
 targets[asdl_seq*]: a=','.target+ [','] { a }
 target[expr_ty] (memo):
@@ -642,15 +643,28 @@ invalid_assignment:
         RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "illegal target for annotation") }
     | a=star_expressions '=' (yield_expr | star_expressions) {
         RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
-            _PyPegen_get_invalid_target(a),
-            "cannot assign to %s", _PyPegen_get_expr_name(_PyPegen_get_invalid_target(a))) }
+            GET_INVALID_TARGET(a),
+            "cannot assign to %s", _PyPegen_get_expr_name(GET_INVALID_TARGET(a))) }
     | a=star_expressions augassign (yield_expr | star_expressions) {
         RAISE_SYNTAX_ERROR_KNOWN_LOCATION( 
             a, 
             "'%s' is an illegal expression for augmented assignment",
             _PyPegen_get_expr_name(a)
         )}
-
+invalid_del_stmt:
+    | 'del' a=star_expressions {
+        GET_INVALID_DEL_TARGET(a) != NULL ?
+        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
+            GET_INVALID_DEL_TARGET(a),
+            "cannot delete %s", _PyPegen_get_expr_name(GET_INVALID_DEL_TARGET(a))
+        ) :
+        RAISE_SYNTAX_ERROR("invalid syntax") }
+    # This additional alternative is necessary, because (*a) is not a valid star_expression
+    # and will not be caught by the previous one.
+    | 'del' '(' a=starred_expression ')' {
+        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
+            a,
+            "cannot delete starred") }
 invalid_block:
     | NEWLINE !INDENT { RAISE_INDENTATION_ERROR("expected an indented block") }
 invalid_comprehension:
@@ -666,6 +680,19 @@ invalid_lambda_star_etc:
 invalid_double_type_comments:
     | TYPE_COMMENT NEWLINE TYPE_COMMENT NEWLINE INDENT {
         RAISE_SYNTAX_ERROR("Cannot have two type comments on def") }
-invalid_del_target:
-    | a=star_expression &del_target_end {
-        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "cannot delete %s", _PyPegen_get_expr_name(a)) }
+invalid_with_target:
+    | ASYNC? 'with' expression 'as' a=expressions ':' {
+        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
+            GET_INVALID_TARGET(a),
+            "'%s' is an illegal 'with' target", _PyPegen_get_expr_name(GET_INVALID_TARGET(a))
+        ) }
+
+# Comparison is used here, because the `a() in b` in `for a() in b: pass` is parsed
+# as a comparison. Thus, we only need to search the left side of the comparison
+# for invalid targets.
+invalid_for_target:
+    | ASYNC? 'for' a=comparison {
+        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(
+            GET_INVALID_TARGET(a->v.Compare.left),
+            "'%s' is an illegal 'for' target", _PyPegen_get_expr_name(GET_INVALID_TARGET(a->v.Compare.left))
+        ) }

Lib/test/test_exceptions.py

@@ -248,9 +248,9 @@ def baz():
         check('def f():\n  x, y: int', 2, 3)
         check('[*x for x in xs]', 1, 2)
         check('foo(x for x in range(10), 100)', 1, 5)
+        check('for 1 in []: pass', 1, 5)
         check('(yield i) = 2', 1, 1 if support.use_old_parser() else 2)
         check('def f(*):\n  pass', 1, 7 if support.use_old_parser() else 8)
-        check('for 1 in []: pass', 1, 5 if support.use_old_parser() else 7)
 
     @cpython_only
     def testSettingException(self):

Lib/test/test_syntax.py

@@ -148,6 +148,49 @@
 Traceback (most recent call last):
 SyntaxError: 'list' is an illegal expression for augmented assignment
 
+Invalid targets in `for` loops and `with` statements should also
+produce a specialized error message
+
+>>> for a() in b: pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'for' target
+
+>>> for (a, b()) in b: pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'for' target
+
+>>> for [a, b()] in b: pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'for' target
+
+>>> for (*a, b, c+1) in b: pass
+Traceback (most recent call last):
+SyntaxError: 'operator' is an illegal 'for' target
+
+>>> for (x, *(y, z.d())) in b: pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'for' target
+
+>>> with a as b(): pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'with' target
+
+>>> with a as (b, c()): pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'with' target
+
+>>> with a as [b, c()]: pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'with' target
+
+>>> with a as (*b, c, d+1): pass
+Traceback (most recent call last):
+SyntaxError: 'operator' is an illegal 'with' target
+
+>>> with a as (x, *(y, z.d())): pass
+Traceback (most recent call last):
+SyntaxError: 'function call' is an illegal 'with' target
+
 From compiler_complex_args():
 
 >>> def f(None=1):

Misc/NEWS.d/next/Core and Builtins/2020-05-15-13-38-54.bpo-40334.eQqslk.rst

@@ -0,0 +1 @@
+Produce better error messages for invalid targets. The error message remains the same for invalid `del` targets (`cannot delete <expr_type>`) and changes for invalid targets in `for` and `with` statements. In `for` statements the new error message is `'<expr_type>' is an illegal 'for' target`, while in `with` statements it is `'<expr_type>' is an illegal 'with' target`.