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

lysnikolaou

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.

…targets

lysnikolaou

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

gvanrossum

Almost there!

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.

pablogsal

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

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

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

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

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

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

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

Same here.

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

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

miss-islington

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

miss-islington

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

@lysnikolaou Could you do the manual backport?

lysnikolaou

@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

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

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

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

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

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

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 label Jun 18, 2020

Conversation

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

Uh oh!

Uh oh!

lysnikolaou commented May 15, 2020

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

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!

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