bpo-40334: Produce better error messages on invalid targets #20106

lysnikolaou · 2020-05-15T13:03:37Z

The following error messages get produced:

cannot delete ... for invalid del targets
cannot assign to ... for invalid targets in for and with
statements

Additionally a few cuts were added in various places before the
invocation of the invalid_* rule, in order to speed things
up.

https://bugs.python.org/issue40334

The following error messages get produced: - `cannot delete ...` for invalid `del` targets - `... is an illegal 'for' target` for invalid targets in for statements - `... is an illegal 'with' target` for invalid targets in with statements Additionally a few `cut`s were added in various places before the invocation of the `invalid_*` rule, in order to speed things up.

Lib/test/test_syntax.py

…targets

lysnikolaou · 2020-05-15T18:28:51Z

Since the error messages will not change I removed the NEWS blurb and added the skip news label. Also updated the initial PR description.

Grammar/python.gram

Parser/pegen/pegen.c

gvanrossum

Almost there!

Grammar/python.gram

gvanrossum · 2020-05-15T23:23:07Z

Parser/pegen/pegen.c

@@ -2058,7 +2058,7 @@ _PyPegen_make_module(Parser *p, asdl_seq *a) {
 // Error reporting helpers

 expr_ty
-_PyPegen_get_invalid_target(expr_ty e)
+_PyPegen_get_invalid_target(expr_ty e, int del_targets)


Can you please adopt a naming convention that makes it clear that this is effectively a bool? I was a bit baffled by this for a while. A venerable convention is to use names like is_foo or has_bar.

Not sure, if is_del is okay, because the questions "What's del, the target, which target?" comes to mind. What do you think?

Co-authored-by: Pablo Galindo <[email protected]>

Correctly identify invalid targets, when there are multiple context managers in a with statement, avoid SEGFAULT in invalid for targets and improve paremeter naming in _PyPegen_get_invalid_target.

Parser/pegen/pegen.h

pablogsal · 2020-05-17T03:24:31Z

I have updated the PR with the latest changes from #20003

gvanrossum

Hm... I poked another hole here. :-(

>>> for i, (f, g()) in 10: pass
Assertion failed: (e != NULL && e->kind == Compare_kind), function _PyPegen_get_invalid_for_target, file Parser/pegen/pegen.c, line 2110.
Abort trap: 6

gvanrossum · 2020-05-17T04:38:11Z

Simpler example that gets the same error:

>>> for f, g() in 10: pass

And this too:

>>> for f(), g in 10: pass

I recommend extreme caution here. If I can poke holes this quickly, I worry that we may be causing a regression if we rush this in before beta1. We obviously don't have the right testing strategy here.

There's nothing that says we have to get this level of detail right in beta1.

…PyPegen_get_invalid_target

lysnikolaou · 2020-05-17T10:34:16Z

Simpler example that gets the same error:
>>> for f, g() in 10: pass
And this too:
>>> for f(), g in 10: pass
I recommend extreme caution here. If I can poke holes this quickly, I worry that we may be causing a regression if we rush this in before beta1. We obviously don't have the right testing strategy here.

There's nothing that says we have to get this level of detail right in beta1.

I pushed a version that I think is pretty close, but I'm not 100% certain. I agree, that there's no rush to land this and that we should test a bit more.

lysnikolaou · 2020-06-10T17:17:26Z

Could you explain this a bit more? I don't get it.

What I meant is that; since this PR already generates 'can't use starred expression here' error messages for some cases, it isn't fully compatible with the old parser. del *a, b was the example I tested both on the old parser and pegen (with this PR applied), and they give different results.
For starred expressions, the message differs from the old parser;
(new parser)
>>> del *a, b
SyntaxError: cannot delete starred
(old parser)
>>> del *a, b
SyntaxError: can't use starred expression here

I think I understand now.

Indeed the old parser used to produce can't use starred expression here and the error message has changed since then. But this PR does not change it, it actually just adopts the changed error message which was introduced in #19911.

Also, I can remember @pablogsal mentioning somewhere that changing just the message is okay in terms of backwards-compatibility.

isidentical · 2020-06-10T17:55:35Z

Indeed the old parser used to produce can't use starred expression here and the error message has changed since then. But this PR does not change it, it actually just adopts the changed error message which was introduced in #19911.

I see, didn't notice that. Sorry for distraction.

Also, I can remember @pablogsal mentioning somewhere that changing just the message is okay in terms of backwards-compatibility.

So, is this error will be kept (cannot delete starred) or would it be cool to have a patch that customizes errors for starred context (can't use starred expression here)?

lysnikolaou · 2020-06-10T19:53:55Z

I like the cannot delete starred one better. When I see it, I immediately know without even looking at the line of the error that it's got to do with a del statement. On the contrary, can't use starred expression here does not give enough context, I feel. But really, I don't feel very strongly either way. What do the others think?

gvanrossum · 2020-06-10T21:02:00Z

Same here.

pablogsal · 2020-06-18T13:12:39Z

Parser/pegen.h

+    FOR_TARGETS
+} TARGETS_TYPE;
+expr_ty _PyPegen_get_invalid_target(expr_ty e, TARGETS_TYPE targets_type);
+#define GET_INVALID_TARGET(e) _PyPegen_get_invalid_target(e, STAR_TARGETS)


Maybe we should surround _PyPegen_get_invalid_target in a CHECK to make sure this does not return NULL (and not crash in that case).

pablogsal

After reviewing it again I have played with it a bit more (before the CHECK commit) and I was not able to break it in any way so I think we can land it now so more people interact with it and it this way we could detect any non-trivial problems if they exist.

lysnikolaou · 2020-06-18T22:42:11Z

FYI there is an unrelated CI failure on macOS in test_asyncio.

miss-islington · 2020-06-18T23:10:47Z

Thanks @lysnikolaou for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington · 2020-06-18T23:11:17Z

Sorry, @lysnikolaou and @pablogsal, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 01ece63d42b830df106948db0aefa6c1ba24416a 3.9

pablogsal · 2020-06-18T23:11:36Z

@lysnikolaou Could you do the manual backport?

lysnikolaou · 2020-06-18T23:12:16Z

@lysnikolaou Could you do the manual backport?

Of course, on it.

…-20106) The following error messages get produced: - `cannot delete ...` for invalid `del` targets - `... is an illegal 'for' target` for invalid targets in for statements - `... is an illegal 'with' target` for invalid targets in with statements Additionally, a few `cut`s were added in various places before the invocation of the `invalid_*` rule, in order to speed things up. Co-authored-by: Pablo Galindo <[email protected]> (cherry picked from commit 01ece63)

bedevere-bot · 2020-06-18T23:31:18Z

GH-20973 is a backport of this pull request to the 3.9 branch.

…-20106) (GH-20973) * bpo-40334: Produce better error messages on invalid targets (GH-20106) The following error messages get produced: - `cannot delete ...` for invalid `del` targets - `... is an illegal 'for' target` for invalid targets in for statements - `... is an illegal 'with' target` for invalid targets in with statements Additionally, a few `cut`s were added in various places before the invocation of the `invalid_*` rule, in order to speed things up. Co-authored-by: Pablo Galindo <[email protected]> (cherry picked from commit 01ece63)

stestagg · 2020-06-20T22:11:55Z

FYI, appears to have caused a regression: https://bugs.python.org/issue41060.
This causes with a as b to crash the interpreter

…-20106) The following error messages get produced: - `cannot delete ...` for invalid `del` targets - `... is an illegal 'for' target` for invalid targets in for statements - `... is an illegal 'with' target` for invalid targets in with statements Additionally, a few `cut`s were added in various places before the invocation of the `invalid_*` rule, in order to speed things up. Co-authored-by: Pablo Galindo <[email protected]>

lysnikolaou requested a review from pablogsal as a code owner May 15, 2020 13:03

the-knights-who-say-ni added the CLA signed label May 15, 2020

bedevere-bot added the awaiting review label May 15, 2020

lysnikolaou and others added 2 commits May 15, 2020 16:33

Fix test_exceptions

d4a9dd6

📜🤖 Added by blurb_it.

72a1339

pablogsal reviewed May 15, 2020

View reviewed changes

Lib/test/test_syntax.py Outdated Show resolved Hide resolved

Produce the same error messages with the old parser for with and for …

1b6cf0b

…targets

lysnikolaou added the skip news label May 15, 2020

lysnikolaou requested a review from pablogsal May 15, 2020 18:29

Add invalid_group rule

1ad3819

lysnikolaou requested a review from gvanrossum May 15, 2020 18:36

pablogsal reviewed May 15, 2020

View reviewed changes

Grammar/python.gram Show resolved Hide resolved

pablogsal reviewed May 15, 2020

View reviewed changes

Grammar/python.gram Outdated Show resolved Hide resolved

pablogsal reviewed May 15, 2020

View reviewed changes

Parser/pegen/pegen.c Outdated Show resolved Hide resolved

gvanrossum reviewed May 15, 2020

View reviewed changes

lysnikolaou and others added 3 commits May 16, 2020 12:55

Update Grammar/python.gram

6f30315

Co-authored-by: Pablo Galindo <[email protected]>

Update Parser/pegen/pegen.c

f750aba

Co-authored-by: Pablo Galindo <[email protected]>

Address Guido's feedback

132178f

Correctly identify invalid targets, when there are multiple context managers in a with statement, avoid SEGFAULT in invalid for targets and improve paremeter naming in _PyPegen_get_invalid_target.

lysnikolaou requested a review from gvanrossum May 16, 2020 11:58

Inline _PyPegen_get_invalid_for_target

17cc4bc

pablogsal reviewed May 17, 2020

View reviewed changes

Parser/pegen/pegen.h Outdated Show resolved Hide resolved

lysnikolaou and others added 2 commits May 17, 2020 05:23

Revert inlining

def0f3d

Merge remote-tracking branch 'upstream/master' into targets-errors

ee3524b

gvanrossum reviewed May 17, 2020

View reviewed changes

Fix searching for invalid for targets, by visiting Compare nodes in _…

4959a12

…PyPegen_get_invalid_target

Merge branch 'master' into targets-errors

f0f934d

Merge branch 'master' into targets-errors

ede107e

pablogsal reviewed Jun 18, 2020

View reviewed changes

pablogsal approved these changes Jun 18, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 18, 2020

Add CHECK* checks to GET_INVALID_*TARGET macros

55b5327

lysnikolaou force-pushed the targets-errors branch from a00b611 to 55b5327 Compare June 18, 2020 22:41

pablogsal merged commit 01ece63 into python:master Jun 18, 2020

bedevere-bot removed the awaiting merge label Jun 18, 2020

miss-islington assigned pablogsal Jun 18, 2020

lysnikolaou deleted the targets-errors branch June 18, 2020 23:12

bedevere-bot removed the needs backport to 3.9 only security fixes label Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-40334: Produce better error messages on invalid targets #20106

bpo-40334: Produce better error messages on invalid targets #20106

lysnikolaou commented May 15, 2020 •

edited

Loading

lysnikolaou commented May 15, 2020

gvanrossum left a comment

gvanrossum May 15, 2020

lysnikolaou May 16, 2020

pablogsal commented May 17, 2020

gvanrossum left a comment

gvanrossum commented May 17, 2020

lysnikolaou commented May 17, 2020

lysnikolaou commented Jun 10, 2020

isidentical commented Jun 10, 2020

lysnikolaou commented Jun 10, 2020

gvanrossum commented Jun 10, 2020

pablogsal Jun 18, 2020 •

edited

Loading

pablogsal left a comment

lysnikolaou commented Jun 18, 2020

miss-islington commented Jun 18, 2020

miss-islington commented Jun 18, 2020

pablogsal commented Jun 18, 2020

lysnikolaou commented Jun 18, 2020

bedevere-bot commented Jun 18, 2020

stestagg commented Jun 20, 2020

bpo-40334: Produce better error messages on invalid targets #20106

bpo-40334: Produce better error messages on invalid targets #20106

Conversation

lysnikolaou commented May 15, 2020 • edited Loading

lysnikolaou commented May 15, 2020

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum May 15, 2020

Choose a reason for hiding this comment

lysnikolaou May 16, 2020

Choose a reason for hiding this comment

pablogsal commented May 17, 2020

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum commented May 17, 2020

lysnikolaou commented May 17, 2020

lysnikolaou commented Jun 10, 2020

isidentical commented Jun 10, 2020

lysnikolaou commented Jun 10, 2020

gvanrossum commented Jun 10, 2020

pablogsal Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

pablogsal left a comment

Choose a reason for hiding this comment

lysnikolaou commented Jun 18, 2020

miss-islington commented Jun 18, 2020

miss-islington commented Jun 18, 2020

pablogsal commented Jun 18, 2020

lysnikolaou commented Jun 18, 2020

bedevere-bot commented Jun 18, 2020

stestagg commented Jun 20, 2020

lysnikolaou commented May 15, 2020 •

edited

Loading

pablogsal Jun 18, 2020 •

edited

Loading