Skip to content
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

pep612: more semantic analysis for paramspec #9422

Merged
merged 13 commits into from
Sep 8, 2020

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 6, 2020

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.variables to Sequence[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.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

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.

mypy/applytype.py Outdated Show resolved Hide resolved
mypy/tvar_scope.py Outdated Show resolved Hide resolved
mypy/typeanal.py Outdated Show resolved Hide resolved
var_def.line
)
else:
return var_def
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we construct a new instance and set the full name, at least?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was confused -- no change is needed here.

else:
vs.append(var.name)
# For other TypeVarLikeDefs, just use the repr
vs.append(repr(var))
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

test-data/unit/check-parameter-specification.test Outdated Show resolved Hide resolved
@hauntsaninja
Copy link
Collaborator Author

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

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good.

@JukkaL JukkaL merged commit ec76cca into python:master Sep 8, 2020
@hauntsaninja hauntsaninja deleted the pep612clean branch September 8, 2020 17:22
@gvanrossum
Copy link
Member

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

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