-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-125326: Improve SyntaxError for invalid type params definiton
#125353
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
Conversation
Misc/NEWS.d/next/Core_and_Builtins/2024-10-12-11-04-52.gh-issue-125326.LCH9st.rst
Outdated
Show resolved
Hide resolved
…e-125326.LCH9st.rst Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Misc/NEWS.d/next/Core_and_Builtins/2024-10-12-11-04-52.gh-issue-125326.LCH9st.rst
Outdated
Show resolved
Hide resolved
…e-125326.LCH9st.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct but I'm not sure the special error messages are worth it; I haven't personally seen evidence from users who saw these errors and were confused. Adding more special-case error messages to the grammar increases the complexity of the system. I'll leave that to the parser maintainers to decide.
| | '[' token=']' { | ||
| RAISE_SYNTAX_ERROR_STARTING_FROM( | ||
| token, | ||
| "Type parameter list cannot be empty")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Type parameter list cannot be empty")} | |
| "type parameter list cannot be empty")} |
We're not completely consistent but most syntax error messages are lowercase.
|
Yeah, I kinda agree that this PR is not ideal, as I said in the first post. |
This does not fix 100% of cases, but makes the situation better. What is not covered? For example:
This happens because of how rules detect top-level
type_params. I don't think that there's a clean way to handle this.However, my patch also fixes a lot of possible corner cases.