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


 expr_ty
-_PyPegen_get_invalid_target(expr_tye)
+_PyPegen_get_invalid_target(expr_tye, intdel_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 <Pablogsal@gmail.com>

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.

isidentical · 2020-06-10T16:34:44Z

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 

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_tye, TARGETS_TYPEtargets_type);
+#defineGET_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 <Pablogsal@gmail.com> (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 <Pablogsal@gmail.com> (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 <Pablogsal@gmail.com>

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

lysnikolaouand 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.pyShow resolvedHide 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.gramShow resolvedHide resolved

pablogsal reviewed May 15, 2020
View reviewed changes

Grammar/python.gram Outdated Show resolvedHide resolved

pablogsal reviewed May 15, 2020
View reviewed changes

Parser/pegen/pegen.c Outdated Show resolvedHide resolved

gvanrossum reviewed May 15, 2020
View reviewed changes

lysnikolaouand others added 3 commits May 16, 2020 12:55

Update Grammar/python.gram
6f30315
Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

Update Parser/pegen/pegen.c
f750aba
Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

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 resolvedHide resolved

lysnikolaouand 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 55b5327Compare June 18, 2020 22:41

pablogsal merged commit 01ece63 into python:masterJun 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 label Jun 18, 2020

Uh oh!

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

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

Uh oh!

Conversation

lysnikolaou commented May 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lysnikolaou commented May 15, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gvanrossumMay 15, 2020

Choose a reason for hiding this comment

Uh oh!

lysnikolaouMay 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablogsal commented May 17, 2020

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented May 17, 2020

Uh oh!

lysnikolaou commented May 17, 2020

Uh oh!

isidentical commented Jun 10, 2020

Uh oh!

lysnikolaou commented Jun 10, 2020

Uh oh!

isidentical commented Jun 10, 2020

Uh oh!

lysnikolaou commented Jun 10, 2020

Uh oh!

gvanrossum commented Jun 10, 2020

Uh oh!

pablogsalJun 18, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

lysnikolaou commented Jun 18, 2020

Uh oh!

miss-islington commented Jun 18, 2020

Uh oh!

miss-islington commented Jun 18, 2020

Uh oh!

pablogsal commented Jun 18, 2020

Uh oh!

lysnikolaou commented Jun 18, 2020

Uh oh!

bedevere-bot commented Jun 18, 2020

Uh oh!

stestagg commented Jun 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lysnikolaou commented May 15, 2020•
edited
Loading

pablogsalJun 18, 2020•
edited
Loading