bpo-43914: Highlight invalid ranges in SyntaxErrors by pablogsal · Pull Request #25525 · python/cpython

pablogsal · 2021-04-22T17:31:11Z

https://bugs.python.org/issue43914

pablogsal · 2021-04-22T17:36:07Z

@lysnikolaou Could you take a look so we can get this before feature freeze? Some notes:

Most of the noise is just propagating the end line and the end offset.
I will add new tests (existing tests have been adapted) in a different PR, because this is already gigantic.
The Pr is separated into logical commits to make the review easier. As you can see most of the commits is just propagating the end position information everywhere.

lysnikolaou · 2021-04-22T18:37:59Z

@pablogsal Yup, I'll have a look tomorrow morning.

lysnikolaou

Really really great work! 🚀

I've left a couple of comments/questions.

Python/compile.c

lysnikolaou · 2021-04-23T11:05:20Z

Grammar/python.gram

-        RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "Generator expression must be parenthesized") }
+    | a=args ',' '*' { RAISE_SYNTAX_ERROR_KNOWN_LOCATION(a, "iterable argument unpacking follows keyword argument unpacking") }
+    | a=expression b=for_if_clauses ',' [args | expression for_if_clauses] {
+        RAISE_SYNTAX_ERROR_KNOWN_RANGE(a, asdl_seq_GET(b, b->size-1)->target, "Generator expression must be parenthesized") }


Could we use _PyPegen_seq_last_item here?

Yeah, but it won't be shorter because _PyPegen_seq_last_item is generic and does type erasure so we need to recast to get the target

I think I can make it more readable with some macros

Check out commit 6f01d08

lysnikolaou · 2021-04-23T11:24:30Z

I also have some recommendations for the new error ranges. For example I'd prefer:

>>> f(a.b=b*3*4, c)
  File "<stdin>", line 1
    f(a.b=b*3*4, c)
      ^^^^^^^^^
SyntaxError: expression cannot contain assignment, perhaps you meant "=="?

over

>>> f(a.b=b*3*4, c)
  File "<stdin>", line 1
    f(a.b=b*3*4, c)
      ^^^^
SyntaxError: expression cannot contain assignment, perhaps you meant "=="?

I guess, it'd be better to merge this and I'll open another PR to discuss these there. Thoughts?

pablogsal · 2021-04-23T11:31:14Z

I guess, it'd be better to merge this and I'll open another PR to discuss these there. Thoughts?

Yeah, that one is especially tricky because it (not with what you suggest but I have seen it happen) can make the parser advance too much and the error will be reported too far away from the '=' sign.

lysnikolaou · 2021-04-23T11:37:27Z

I guess, it'd be better to merge this and I'll open another PR to discuss these there. Thoughts?

Yeah, that one is especially tricky because it (not with what you suggest but I have seen it happen) can make the parser advance too much and the error will be reported too far away from the '=' sign.

Yup, I can see that happening.

lysnikolaou

Let's go! 🎉

lysnikolaou

Only a nit.

Lib/test/test_exceptions.py

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>

bedevere-bot · 2021-04-23T12:48:54Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 4e8bb50 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2021-04-23T13:21:04Z

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 4e8bb50 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

bedevere-bot · 2021-04-23T13:27:09Z

@pablogsal: Please replace # with GH- in the commit message next time. Thanks!

the-knights-who-say-ni added the CLA signed label Apr 22, 2021

bedevere-bot added the awaiting core review label Apr 22, 2021

pablogsal requested a review from lysnikolaou April 22, 2021 17:35

pablogsal added 5 commits April 22, 2021 18:56

Add new APIs to SyntaxErrors to support end positions

f379dbb

Update the symbol table to use the new APIs

36343c9

Update the compiler to use the new APIs

501aa62

Add new APIs to the parser to support end positions

bdfd6e3

Update the grammar to use the new parser APIs

40322f8

pablogsal force-pushed the tok_better branch from fd73493 to ffd520f Compare April 22, 2021 17:56

Update tests

e4d29e8

pablogsal force-pushed the tok_better branch from ffd520f to e4d29e8 Compare April 22, 2021 18:06

pablogsal changed the title ~~Highlight invalid ranges in SyntaxErrors~~ bpo-43914: Highlight invalid ranges in SyntaxErrors Apr 22, 2021

Add NEWS entry

0add3aa

pablogsal marked this pull request as ready for review April 22, 2021 18:23

pablogsal requested a review from markshannon as a code owner April 22, 2021 18:23

Update documentation

ee7bc89

lysnikolaou reviewed Apr 23, 2021

View reviewed changes

pablogsal force-pushed the tok_better branch from 73baa5c to 9c93966 Compare April 23, 2021 11:40

pablogsal requested a review from lysnikolaou April 23, 2021 11:46

Address feedback from review

6f01d08

pablogsal force-pushed the tok_better branch from 9c93966 to 6f01d08 Compare April 23, 2021 11:46

lysnikolaou approved these changes Apr 23, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Apr 23, 2021

Add more tests and fix some edge cases

81b43d7

lysnikolaou reviewed Apr 23, 2021

View reviewed changes

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

Update Lib/test/test_exceptions.py

4e8bb50

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 23, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 23, 2021

pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 23, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 23, 2021

pablogsal merged commit a77aac4 into python:master Apr 23, 2021

bedevere-bot removed the awaiting merge label Apr 23, 2021

pablogsal deleted the tok_better branch April 23, 2021 13:27

ambv mentioned this pull request Nov 17, 2021

[3.9] bpo-45820: Fix a segfault when the parser fails without reading any input (GH-29580) #29584

Merged

nineteendo mentioned this pull request Dec 3, 2024

[3.12] gh-111609: end_offset is ignored in subclasses of SyntaxError #127554

Merged

Uh oh!

Conversation

pablogsal commented Apr 22, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lysnikolaou commented Apr 22, 2021

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lysnikolaou Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Apr 23, 2021 • 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 Apr 23, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Apr 23, 2021 • 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!

lysnikolaou commented Apr 23, 2021

Uh oh!

pablogsal commented Apr 23, 2021

Uh oh!

lysnikolaou commented Apr 23, 2021

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Apr 23, 2021

Uh oh!

bedevere-bot commented Apr 23, 2021

Uh oh!

bedevere-bot commented Apr 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pablogsal commented Apr 22, 2021 •

edited by bedevere-bot

Loading

pablogsal commented Apr 22, 2021 •

edited

Loading

pablogsal Apr 23, 2021 •

edited

Loading

pablogsal Apr 23, 2021 •

edited

Loading