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

Fix access to operators on metaclass through typevar #5009

Merged
merged 4 commits into from May 7, 2018

Conversation

Projects
None yet
2 participants
@elazarg
Contributor

elazarg commented May 6, 2018

Fix #4929: typevars were not handled correctly on has_member.

The test uses multiplication instead of addition since adding __radd__ to the stub reveals several inconsistencies in unrelated tests.

@ilevkivskyi

Thanks! Two comments here

return self.has_member(typ.item.type.metaclass_type, member)
if isinstance(typ.item, AnyType):
item = typ.item
if isinstance(item, TypeVarType):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 6, 2018

Collaborator

I wonder shouldn't we add the same fallback to upper bound at the very top of this function? For example if I have just

def f(x: T) -> T:
    x + 0
S = TypeVar("S", bound=Test)
def f(x: Type[Test]) -> str:
return x * 0

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 6, 2018

Collaborator

This will still pass if the inferred type will become Any. I would either add an explicit reveal_type or add --warn-return-any flag in this test.

This comment has been minimized.

@elazarg

elazarg May 6, 2018

Contributor

I've avoided reveal_type since it passes with the broken code too. But I agree it should be added just to know it's not Any.

@ilevkivskyi

OK, looks good now (but one more test will make it even better).

@@ -2431,6 +2431,10 @@ def has_member(self, typ: Type, member: str) -> bool:
"""Does type have member with the given name?"""
# TODO: refactor this to use checkmember.analyze_member_access, otherwise
# these two should be carefully kept in sync.
if isinstance(typ, TypeVarType):
typ = typ.upper_bound

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 7, 2018

Collaborator

Could you also add a test case for this branch? (Similar to the one you already have.)

@ilevkivskyi ilevkivskyi merged commit e85c78e into python:master May 7, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment