gh-119933 : Improve ``SyntaxError`` message for invalid type parameters expressions by picnixz · Pull Request #119976 · python/cpython · GitHub
Skip to content

gh-119933 : Improve SyntaxError message for invalid type parameters expressions - #119976

Merged
JelleZijlstra merged 27 commits into
python:mainfrom
picnixz:fix-119933
Jun 17, 2024
Merged

JelleZijlstra merged 27 commits into
python:mainfrom
picnixz:fix-119933

Conversation

@picnixz

@picnixz picnixz commented Jun 3, 2024

Copy link
Copy Markdown
Member

cc @JelleZijlstra

I added a bunch of tests but feel free to tell me if there are more cases to check. It took me a bit of time to observe that entering the context was handled inside symtable_visit_type_param_bound_or_default and not upon calling it.

Tell me if I need a What's New entry by the way or if the NEWS entry should be improved.

@AlexWaygood AlexWaygood changed the title gh-119933 : fix SyntaxError for invalid type parameters expressions gh-119933 : Improve SyntaxError message for invalid type parameters expressions Jun 3, 2024
Comment thread Include/internal/pycore_symtable.h Outdated
Comment thread Python/symtable.c Outdated
@JelleZijlstra
JelleZijlstra self-requested a review June 3, 2024 16:51
@picnixz

picnixz commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

@JelleZijlstra

Copy link
Copy Markdown
Member

Thanks! I agree with renaming TypeParamBlock to TypeParamsBlock.

For TypeVarBoundBlock, note that it is used for bounds and constraints as well as defaults, and the latter can appear on ParamSpecs and TypeVarTuples as well as TypeVars. Therefore, I think it's better to use the full name "TypeVariableBlock", using "type variable" as a general term for all three flavors.

My biggest issue with changing these names is backwards compatibility in the symtable module. We explicitly document a set of strings at https://docs.python.org/3/library/symtable.html#symtable.SymbolTable.get_type. We can add to the set of strings for 3.14 and probably for 3.13, but for 3.12 it should stay the same.

@picnixz

picnixz commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

Therefore, I think it's better to use the full name "TypeVariableBlock", using "type variable" as a general term for all three flavors.

Yes, that's what I had in mind (actually, I wrote TypeVarBlock but it should have been TypeVariableBlock, so my bad).

Since we are anyway using full names, I could also suggest using TypeParametersBlock, unless you think it's too verbose.

We can add to the set of strings for 3.14 and probably for 3.13, but for 3.12 it should stay the same.

3.13 should be fine (I guess) because it's still a pre-release so... but would it make sense to instead return enumeration constants that are publicly accessible instead of strings like that..? at least we wouldn't have the issue in the future if the grammar is extended/changed and we could probably make the attribute deprecated more easily ?

@JelleZijlstra

Copy link
Copy Markdown
Member

Yes, I think using an enum would make sense and it could be made backwards-compatible by using StrEnum.

picnixz added 3 commits June 4, 2024 18:00
- improve comments
- rename 'TypeVarBoundBlock' -> 'TypeVariableBlock'
- rename 'TypeParamBlock' -> 'TypeParametersBlock'
- rename 'ste_description' -> 'ste_context_info'
- only set 'ste_context_info' just before visiting the actual expression
- rename '_symtable.TYPE_TYPE_VAR_BOUND' to '_symtable.TYPE_TYPE_VARIABLE'
- rename '_symtable.TYPE_TYPE_PARAM' to '_symtable.TYPE_TYPE_PARAMETERS'
- add string enumeration for symbol table type
@picnixz

picnixz commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

@JelleZijlstra I've updated the proposal. By the way, I've only done that for what I've added but I'll likely re-order the case so that they match the enumeration. I put TypeAliasBlock, TypeParametersBlock and TypeVariableBlock in the enumeration (in that order) to reflect the "top-to-bottom" construction (TypeVariableBlock requires to first enter a TypeParametersBlock).

Comment thread Include/internal/pycore_symtable.h Outdated
Comment thread Lib/symtable.py Outdated
Comment thread Lib/symtable.py Outdated
Comment thread Misc/NEWS.d/next/Library/2024-06-03-13-48-44.gh-issue-119933.Kc0HG5.rst Outdated
Comment thread Python/symtable.c Outdated
@picnixz
picnixz requested a review from carljm as a code owner June 12, 2024 10:12
@JelleZijlstra
JelleZijlstra self-requested a review June 12, 2024 21:13
Comment thread Lib/symtable.py Outdated
Comment thread Lib/test/test_symtable.py

@carljm carljm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General direction looks fine to me. I won't have time soon to do a detailed review but if @JelleZijlstra is happy with it, that's good enough for me.

Comment thread Doc/library/symtable.rst Outdated
Comment thread Python/symtable.c Outdated
Comment thread Python/symtable.c Outdated
picnixz and others added 3 commits June 17, 2024 09:25
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra
JelleZijlstra merged commit 4bf17c3 into python:main Jun 17, 2024
@JelleZijlstra JelleZijlstra added the needs backport to 3.13 bugs and security fixes label Jun 17, 2024
@miss-islington-app

Copy link
Copy Markdown

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

@miss-islington-app

Copy link
Copy Markdown

Sorry, @picnixz and @JelleZijlstra, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 4bf17c381fb7b465f0f26aecb94a6c54cf9be2d3 3.13

@JelleZijlstra

Copy link
Copy Markdown
Member

I am doing the backport

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Jun 17, 2024
…ype parameters expressions (pythonGH-119976)

(cherry picked from commit 4bf17c3)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-app

bedevere-app Bot commented Jun 17, 2024

Copy link
Copy Markdown

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 17, 2024
JelleZijlstra added a commit that referenced this pull request Jun 17, 2024
…rameters expressions (GH-119976) (#120641)

(cherry picked from commit 4bf17c3)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@picnixz
picnixz deleted the fix-119933 branch June 18, 2024 12:17
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
…ameters expressions (python#119976)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
…ameters expressions (python#119976)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…ameters expressions (python#119976)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants