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

Merge TypeVarDef and TypeVarType #9951

Merged
merged 13 commits into from
Aug 4, 2021
Merged

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Jan 24, 2021

In the spirit of #4814 (comment)

I'm not sure what the distinction between TypeVarDef and TypeVarType was (note that there's also TypeVarExpr, whose purpose is clear to me).
In practice, it seems it boiled down to: a) TypeVarDef has a different __repr__, that's not user facing but was used in tests, b) in two places, we used the construction of a TypeVarType from a TypeVarDef as an opportunity to change the line number.

I ran into this because:
a) I was considering whether to create a corresponding ParamSpecDef and ParamSpecType
b) I was trying to fix an issue with applytype sometimes swallowing generics, which to fix I needed TypeVarType to have access to its TypeVarDef.
I was unable to find a reason for TypeVarType to not just be the TypeVarDef and here we are.

@hauntsaninja hauntsaninja requested a review from JukkaL January 24, 2021 00:55
@gvanrossum
Copy link
Member

Do we even need the separation between TypeVarDef and ParamSpecDef? There seems a lot of duplication. A single flag bit or maybe making ParamSpecDef a trivial subclass of the other might simplify things there.

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator Author

Yeah, it's conceivable it could be made a flag. But having it be its own type makes my development much easier, since it allows mypy to tell me where I need to add functionality. I already extracted what's common into TypeVarLikeDef / ParamSpecDef is basically a trivial subclass of TypeVarLikeDef.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@emmatyping
Copy link
Collaborator

We should probably give people advanced notice about this change in #6617 since I expect several plugins deal with TypeVars.

Copy link
Collaborator

@msullivan msullivan left a comment

Choose a reason for hiding this comment

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

I made a suggestion but ignore it if you don't agree.

And yeah, we should mention it in the plugin issue.

@@ -3277,13 +3277,12 @@ def check_lst_expr(self, items: List[Expression], fullname: str,
# Used for list and set expressions, as well as for tuples
# containing star expressions that don't refer to a
# Tuple. (Note: "lst" stands for list-set-tuple. :-)
tvdef = TypeVarDef('T', 'T', -1, [], self.object_type())
tv = TypeVarType(tvdef)
tvdef = TypeVarType('T', 'T', -1, [], self.object_type())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe keep the tv names (and similar below) instead of the tvdef ones?

@msullivan
Copy link
Collaborator

And this is great. Honestly I never understood the distinction distinction and am happy to learn that this confusion was warranted.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2021

Diff from mypy_primer, showing the effect of this PR on open source code:

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/mypy.py:41: error: Module "mypy.types" has no attribute "TypeVarDef"; maybe "TypeVar" or "TypeVarType"?  [attr-defined]
+ pydantic/mypy.py:337: error: Missing positional arguments "fullname", "id", "values", "upper_bound" in call to "TypeVarType"  [call-arg]

@hauntsaninja hauntsaninja merged commit 2736edb into python:master Aug 4, 2021
@hauntsaninja hauntsaninja deleted the typevar2 branch August 4, 2021 07:53
christianbundy added a commit to christianbundy/pydantic that referenced this pull request Sep 5, 2021
Problem: While using the Pydantic plugin with bleeding-edge Mypy I
realized that Pydantic is using a recently-removed type.

Solution: Find two instances of `TypeVarDef` and rename to
`TypeVarType`.

See-also: python/mypy#9951
christianbundy added a commit to christianbundy/pydantic that referenced this pull request Sep 5, 2021
Problem: While using the Pydantic plugin with bleeding-edge Mypy I
realized that Pydantic is using a recently-removed type.

Solution: Find two instances of `TypeVarDef` and rename to
`TypeVarType`.

See-also: python/mypy#9951
DetachHead pushed a commit to DetachHead/pydantic that referenced this pull request Mar 8, 2022
Problem: While using the Pydantic plugin with bleeding-edge Mypy I
realized that Pydantic is using a recently-removed type.

Solution: Find two instances of `TypeVarDef` and rename to
`TypeVarType`.

See-also: python/mypy#9951
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.

4 participants