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

Make Type[X] compatible with metaclass of X #3346

Merged
merged 6 commits into from May 31, 2017

Conversation

Projects
None yet
4 participants
@elazarg
Contributor

elazarg commented May 9, 2017

Fix #3326.

PR #2837 seems completely broken; the checks were all backwards, so this PR reverses them.

I had to special-case EnumMeta since the bound for the self argument can't be lower than the current class. I fail to see how it can work without something like metaclass=M argument for TypeVar.

@ilevkivskyi

This mostly looks good. I have an idea about special-casing self-types of the form Type[T].

Show outdated Hide outdated mypy/subtypes.py
@@ -263,8 +257,7 @@ def visit_overloaded(self, left: Overloaded) -> bool:
elif isinstance(right, TypeType):
# All the items must have the same type object status, so
# it's sufficient to query only (any) one of them.
# This is unsound, we don't check the __init__ signature.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 13, 2017

Collaborator

I think this comment should stay here, since the __init__ signature should be checked for all items in an overload.

@ilevkivskyi

ilevkivskyi May 13, 2017

Collaborator

I think this comment should stay here, since the __init__ signature should be checked for all items in an overload.

Show outdated Hide outdated test-data/unit/check-classes.test
Show outdated Hide outdated test-data/unit/check-classes.test
Show outdated Hide outdated test-data/unit/check-classes.test
cv.x
x3: M = cv
[builtins fixtures/classmethod.pyi]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 13, 2017

Collaborator

I would add few reveal_types for methods in these tests, just in case.

@ilevkivskyi

ilevkivskyi May 13, 2017

Collaborator

I would add few reveal_types for methods in these tests, just in case.

This comment has been minimized.

@elazarg

elazarg May 13, 2017

Contributor

I tried, although I'm not sure what information is gained.

@elazarg

elazarg May 13, 2017

Contributor

I tried, although I'm not sure what information is gained.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 14, 2017

Collaborator

Actually I wanted something like reveal_type(A.g1) and reveal_type(A.g1()), in particular, where return types of g2 etc. depend on type variables (like List[TA]). Plus also the situations where name of staticmethod/classmethod in class coincides with a name of a method in metaclass.

@ilevkivskyi

ilevkivskyi May 14, 2017

Collaborator

Actually I wanted something like reveal_type(A.g1) and reveal_type(A.g1()), in particular, where return types of g2 etc. depend on type variables (like List[TA]). Plus also the situations where name of staticmethod/classmethod in class coincides with a name of a method in metaclass.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 30, 2017

Collaborator

Actually I wanted something like reveal_type(A.g1) and reveal_type(A.g1()), in particular, where return types of g2 etc. depend on type variables (like List[TA]). Plus also the situations where name of staticmethod/classmethod in class coincides with a name of a method in metaclass.

@elazarg Have you added these tests? Otherwise I think this PR looks good.
@JukkaL could you please try this PR with the Dropbox internal codebase before it is merged?

@ilevkivskyi

ilevkivskyi May 30, 2017

Collaborator

Actually I wanted something like reveal_type(A.g1) and reveal_type(A.g1()), in particular, where return types of g2 etc. depend on type variables (like List[TA]). Plus also the situations where name of staticmethod/classmethod in class coincides with a name of a method in metaclass.

@elazarg Have you added these tests? Otherwise I think this PR looks good.
@JukkaL could you please try this PR with the Dropbox internal codebase before it is merged?

This comment has been minimized.

@elazarg

elazarg May 30, 2017

Contributor

Hmm I'm pretty sure I did, but probably forgot to push... :\ I will check.

@elazarg

elazarg May 30, 2017

Contributor

Hmm I'm pretty sure I did, but probably forgot to push... :\ I will check.

This comment has been minimized.

@elazarg

elazarg May 30, 2017

Contributor

Ok, I don't have time at this moment, so I tried pushing kinda blindly. I might have mixed up something. This supposed to be the tests only, not the check-on-access you've asked for. I hope I did not mess up too bad; if I did, I can probably try to fix it later this week. In such case, it can still be useful to check earlier commit against the internal repo.

Sorry about the mess.

@elazarg

elazarg May 30, 2017

Contributor

Ok, I don't have time at this moment, so I tried pushing kinda blindly. I might have mixed up something. This supposed to be the tests only, not the check-on-access you've asked for. I hope I did not mess up too bad; if I did, I can probably try to fix it later this week. In such case, it can still be useful to check earlier commit against the internal repo.

Sorry about the mess.

This comment has been minimized.

@JukkaL

JukkaL May 30, 2017

Collaborator

@ilevkivskyi I can try this with Dropbox internal code tomorrow (need to go now).

@JukkaL

JukkaL May 30, 2017

Collaborator

@ilevkivskyi I can try this with Dropbox internal code tomorrow (need to go now).

This comment has been minimized.

@gvanrossum

gvanrossum May 30, 2017

Member

No worries, I have tried this with our internal code, there are no new errors this found (either in our codebase or in the PR). I haven't reviewed the PR though so I can't "Approve" it. But Ivan can.

@gvanrossum

gvanrossum May 30, 2017

Member

No worries, I have tried this with our internal code, there are no new errors this found (either in our codebase or in the PR). I haven't reviewed the PR though so I can't "Approve" it. But Ivan can.

@ilevkivskyi

OK, this looks good now. We can discuss whether we need additional errors on access, and whether we can allow cls: Type[A] first argument annotation in metaclass methods (this PR only allows this for EnumMeta) in a separate issue.

@ilevkivskyi ilevkivskyi merged commit f60d874 into python:master May 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

carljm added a commit to carljm/mypy that referenced this pull request May 31, 2017

Merge branch 'master' into module-alias
* master:
  Support accessing modules imported in class bodies within methods. (#3450)
  Make Type[X] compatible with metaclass of X (#3346)
  Sync typeshed (#3479)
  Handle flags in pytest passthrough (#3467)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment