10000 gh-119933 : Improve ``SyntaxError`` message for invalid type parameters expressions by picnixz · Pull Request #119976 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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
Merged
merged 27 commits into from
Jun 17, 2024
Merged

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

merged 27 commits into from
Jun 17, 2024

Conversation

picnixz
Copy link
Member
@picnixz picnixz commented Jun 3, 2024 •
edited by bedevere-app bot
Loading

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.

@bedevere-app bedevere-app bot mentioned this pull request Jun 3, 2024
Closed
@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
@picnixz picnixz mentioned this pull request Jun 3, 2024
Closed
JelleZijlstra
JelleZijlstra reviewed Jun 3, 2024
View reviewed changes
@JelleZijlstra JelleZijlstra self-requested a review June 3, 2024 16:51
@picnixz
Copy link
Member Author
picnixz commented Jun 4, 2024

After some sleep, I suggest renaming TypeVarBoundBlock and TypeParamBlock but keep the three distinctions, at least for compatibility purposes. This change would affect the symtable module, especially symtable.SymbolTable.get_type but there are various issues with the current naming IMO.

  1. FWIU, those blocks describe "annotation scopes". As mentioned here, class Bag[T] is equivalent to:

    annotation-def TYPE_PARAMS_OF_Bag():
  T = typing.TypeVar("T")
  class Bag(typing.Generic[T]):
      __type_params__ = (T,)
      ...
  return Bag
Bag = TYPE_PARAMS_OF_Bag()

    In other words, upon encountering a generic class definition, we actually enter a TypeParamBlock. It is however very confusing to use that terminology because we are not processing a single type parameter, but possibly one or more type parameters.

  2. Once we are in a TypeParamBlock, we then move to process the actual type parameters. Type parameters without bound/constraints/default have no symbol table and the TypeVarBoundBlock is used otherwise. However, since this kind of block is also used for non-typing.TypeVar objects but also for those typing.TypeVarTuple and typing.ParamSpec ones, this might be confusing. From what I remember on my compiler's & co lectures, in the following definition:

def foo[T: int, *Ts = *tuple[int], **P = [int, str]](): pass

I would say that T, *Ts and **P are "type variables" and that "foo" has 3 "type parameters". In particular, I would like to rename TypeParamBlock into TypeParamsBlock and TypeVarBoundBlock into TypeVarBlock even though it is implicitly assumed that there is a bound/constraints or default for that one. The rationale behind this choice is also motivated by the following test in test_symtable.py:

class GenericMine[T: int]:
  pass

The symbol table's type for GenericMine is "type parameter" (because it's the annotation scope); the symbol table's type for the inner's GenericMine (i.e., the concrete class being created) is "class" (this is fine) and the symbol table's type for "T" is "TypeVar bound", but this would also be its type if I had "[T = int]"...

The renaming would affect the current symtable module as follows:

  • The corresponding constants would be renamed but we could perhaps keep the previous ones for compatibility purposes and deprecate them for 3.14.

  • _symtable.TYPE_TYPE_VAR_BOUND should either refer to "type variable bound" only OR we add some additional information to know whether it is a type bound, a type constraint or a type default value. This information can also be used when considering a type alias but not when considering the type parameter. However, this requires to add three distinct booleans (two of them mutually exclusive but can be set both to 0) instead of the flags that I originally introduce. Otherwise, I don't know how to easily expose the different type of tables to symtable.

We could also emit a symbol table for type parameters without any bound, constraint or default and the renaming would make better sense. By the way, DEF_TYPE_PARAM is never exported to the Python module and we have something like:

>>> import symtable
>>> s = symtable.symtable("class A[T]: pass", "?", "exec")
>>> s.get_children()[0].lookup('T')
<symbol 'T': LOCAL, DEF_LOCAL>

where I would have expected T to be have the DEF_TYPE_PARAM flag. I'll open a separate issue for that one.

@JelleZ 8000 ijlstra
Copy link
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
Copy link
Member Author
picnixz commented Jun 4, 2024

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
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
@picnixz
address review
7482db6 
- 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
@picnixz
address review
2edf665 
- 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
use the same declaration order across files
845c7a6
@picnixz
Copy link
Member Author
picnixz commented Jun 4, 2024

@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).

JelleZijlstra
JelleZijlstra reviewed Jun 5, 2024
View reviewed changes
@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
JelleZijlstra
JelleZijlstra reviewed Jun 13, 2024
View reviewed changes
carljm
carljm reviewed Jun 16, 2024
View reviewed changes
Copy link
Member
@carljm carljm left a comment

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.

JelleZijlstra
JelleZijlstra reviewed Jun 16, 2024
View reviewed changes
picnixz and others added 3 commits June 17, 2024 09:25
@picnixz @JelleZijlstra
Update Python/symtable.c
e96f6b4 
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@picnixz @JelleZijlstra
Update Doc/library/symtable.rst
705a76f 
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@picnixz
address review
abad81b
@JelleZijlstra JelleZijlstra merged commit 4bf17c3 into python:main Jun 17, 2024
36 checks passed
@JelleZijlstra JelleZijlstra added the needs backport to 3.13 bugs and security fixes label Jun 17, 2024
@miss-islington-app
Copy link

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

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
Member

I am doing the backport

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Jun 17, 2024
@picnixz @JelleZijlstra
[3.13] pythongh-119933: Improve SyntaxError message for invalid t…
60a8b89 
…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
Copy link
bedevere-app bot commented Jun 17, 2024

GH-120641 is a backport of this pull request to the 3.13 branch.

@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
@JelleZijlstra @picnixz
[3.13] gh-119933: Improve SyntaxError message for invalid type pa…
7c47f93 
…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
@picnixz picnixz mentioned this pull request Jun 22, 2024
Merged
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@picnixz @JelleZijlstra @mrahtz
pythongh-119933: Improve SyntaxError message for invalid type par…
8028287 
…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
@picnixz @JelleZijlstra @noahbkim
pythongh-119933: Improve SyntaxError message for invalid type par…
02e6c4c 
…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
@picnixz @JelleZijlstra @estyxx
pythongh-119933: Improve SyntaxError message for invalid type par…
0116d5d 
…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
Reviewers

@carljm carljm carljm left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Assignees

@JelleZijlstra JelleZijlstra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@picnixz @JelleZijlstra @carljm
0