pep612: more semantic analysis for paramspec #9422
Conversation
Creates a number of TODOs that I shall get to.
hauntsaninja
force-pushed
the
pep612clean
branch
from
deae77a
to
c7a80de
Compare
There was a problem hiding this comment.
Thanks for the PR! The overall approach looks good to me. Left a bunch of minor comments. Feel free to merge one you've addressed them, or you can ask for another review iteration.
A more meta comment (don't need to address right now): If a user tries to use ParamSpec they'll likely get failed assertions? Before we make a public release with this new code, if the implementation is still incomplete, it would be better to hide it behind a command line flag so that users aren't exposed to assertion failures.
| var_def.line | ||
| ) | ||
| else: | ||
| return var_def |
There was a problem hiding this comment.
Should we construct a new instance and set the full name, at least?
There was a problem hiding this comment.
In general, TypeAnalyser visit methods seem pretty happy to return their input, so not sure I see the point (unless we were to add upper_bound to ParamSpecDef or TypeVarLikeDef, in which case we'd need to visit it and reconstruct).
There was a problem hiding this comment.
I was confused -- no change is needed here.
| else: | ||
| vs.append(var.name) | ||
| # For other TypeVarLikeDefs, just use the repr | ||
| vs.append(repr(var)) |
There was a problem hiding this comment.
Add a TODO comment about actually implementing suitable repr? In the longer term, it would make sense to use a similar way of constructing the repr of both cases.
There was a problem hiding this comment.
TypeVarLikeDef already implements a suitable repr.
The code here is just a reimplementation of TypeVarDef's repr that additionally visits values / upper_bound. We'd only need to do something here if we added upper_bound to ParamSpecDef or TypeVarLikeDef.
|
Thanks! Fixed all the review points that have been marked as resolved, replied to the other two. Sure, adding a command line flag seems reasonable / easy. I don't think any of my changes are in 0.790, but I'll make sure users can't hit the assertions before the next release. And for now, ParamSpec doesn't yet exist in typing_extensions (or typing). |
FYI, it will be in the next alpha of 3.10 in typing.py, and likely in the next release of typing_extensions too. And it's already in typeshed (as is |
Linking #8645
Like #9339, much of this is in #9250. However, this PR is a little less self contained, in that it causes several future TODOs.
A lot of the changes here are necessitated by changing the type of
CallableType.variablestoSequence[TypeVarLikeDef]. This is nice, because it informs me where I need to make changes in the future / integrates a little bit better with existing type var stuff. But let me know if you think I should instead add another field to CallableType.The other decision here that I'm less sure of is not adding upper_bound and variance fields to ParamSpecDef. These fields exist on ParamSpecExpr, since PEP 612 reserves that. I figured we should add these to ParamSpecDef only once they have semantics so that it's easier to figure out where we need to define the new semantics, but that means TypeVarLikeDef abstracts over less and might lead to unnecessary work if we expect those semantics to be defined any time soon.