Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-43914: Highlight invalid ranges in SyntaxErrors #25525

Merged
merged 11 commits into from Apr 23, 2021
Merged

bpo-43914: Highlight invalid ranges in SyntaxErrors #25525

merged 11 commits into from Apr 23, 2021

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 22, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue43914
@pablogsal pablogsal requested a review from lysnikolaou Apr 22, 2021
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Apr 22, 2021 •
edited

@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.
pablogsal added 5 commits Apr 22, 2021
@pablogsal
Add new APIs to SyntaxErrors to support end positions
f379dbb
@pablogsal
Update the symbol table to use the new APIs
36343c9
@pablogsal
Update the compiler to use the new APIs
501aa62
@pablogsal
Add new APIs to the parser to support end positions
bdfd6e3
@pablogsal
Update the grammar to use the new parser APIs
40322f8
@pablogsal pablogsal force-pushed the pablogsal:tok_better branch from fd73493 to ffd520f Apr 22, 2021
@pablogsal
Update tests
Loading status checks…
e4d29e8
@pablogsal pablogsal force-pushed the pablogsal:tok_better branch from ffd520f to e4d29e8 Apr 22, 2021
@pablogsal pablogsal changed the title Highlight invalid ranges in SyntaxErrors bpo-43914: Highlight invalid ranges in SyntaxErrors Apr 22, 2021
@pablogsal
Add NEWS entry
Loading status checks…
0add3aa
@pablogsal pablogsal marked this pull request as ready for review Apr 22, 2021
@pablogsal pablogsal requested a review from markshannon as a code owner Apr 22, 2021
@lysnikolaou
Copy link
Contributor

@lysnikolaou lysnikolaou commented Apr 22, 2021

@pablogsal Yup, I'll have a look tomorrow morning.
@pablogsal
Update documentation
Loading status checks…
ee7bc89
@lysnikolaou
lysnikolaou reviewed Apr 23, 2021
View changes
Copy link
Contributor

@lysnikolaou lysnikolaou left a comment

Really really great work! 🚀

I've left a couple of comments/questions.
Python/compile.c Outdated Show resolved Hide resolved
Grammar/python.gram Outdated
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") }

This comment has been minimized.

Sign in to view
@lysnikolaou

lysnikolaou Apr 23, 2021
Contributor

Could we use _PyPegen_seq_last_item here?

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Apr 23, 2021 •
edited
Author Member

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

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Apr 23, 2021
Author Member

I think I can make it more readable with some macros

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Apr 23, 2021 •
edited
Author Member

Check out commit 6f01d08
@lysnikolaou
Copy link
Contributor

@lysnikolaou lysnikolaou commented Apr 23, 2021

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
Copy link
Member Author

@pablogsal pablogsal commented Apr 23, 2021

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
Copy link
Contributor

@lysnikolaou lysnikolaou commented Apr 23, 2021

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.
@pablogsal pablogsal force-pushed the pablogsal:tok_better branch from 73baa5c to 9c93966 Apr 23, 2021
@pablogsal pablogsal requested a review from lysnikolaou Apr 23, 2021
@pablogsal
Address feedback from review
Loading status checks…
6f01d08
@pablogsal pablogsal force-pushed the pablogsal:tok_better branch from 9c93966 to 6f01d08 Apr 23, 2021
@lysnikolaou
lysnikolaou approved these changes Apr 23, 2021
View changes
Copy link
Contributor

@lysnikolaou lysnikolaou left a comment

Let's go! 🎉
@pablogsal
Add more tests and fix some edge cases
Loading status checks…
81b43d7
@lysnikolaou
lysnikolaou reviewed Apr 23, 2021
View changes
Copy link
Contributor

@lysnikolaou lysnikolaou left a comment

Only a nit.
Lib/test/test_exceptions.py Outdated Show resolved Hide resolved
@pablogsal @lysnikolaou
Update Lib/test/test_exceptions.py
Verified
This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
4e8bb50 
Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 23, 2021

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

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 23, 2021

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

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.
@pablogsal pablogsal merged commit a77aac4 into python:master Apr 23, 2021
12 of 21 checks passed
12 of 21 checks passed
Docs
Details
Check for source changes
Details
Check if generated files are up to date
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Ubuntu SSL tests with OpenSSL ${{ matrix.openssl_ver }}
Details
buildbot/AMD64 Windows10 PR Build done.
Details
buildbot/AMD64 Debian PGO PR Build started.
Details
buildbot/AMD64 FreeBSD Non-Debug PR Build started.
Details
buildbot/AMD64 FreeBSD Shared PR Build started.
Details
buildbot/AMD64 Windows10 Pro PR Build started.
Details
buildbot/AMD64 Windows8.1 Non-Debug PR Build started.
Details
buildbot/AMD64 Windows8.1 Refleaks PR Build started.
Details
buildbot/s390x Debian PR Build started.
Details
buildbot/s390x SLES PR Build started.
Details
Azure Pipelines PR #20210423.27 succeeded
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 43914 found
Details
bedevere/news News entry found in Misc/NEWS.d
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Apr 23, 2021

@pablogsal: Please replace # with GH- in the commit message next time. Thanks!
@pablogsal pablogsal deleted the pablogsal:tok_better branch Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysnikolaou lysnikolaou

@markshannon markshannon

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
4 participants
@pablogsal @lysnikolaou @bedevere-bot @the-knights-who-say-ni