Skip to content

Fix access to operators on metaclass through typevar #5009

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

Merged
merged 4 commits into from
May 7, 2018

Conversation

elazarg
Copy link
Contributor

@elazarg 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.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

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):
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

@ilevkivskyi ilevkivskyi May 7, 2018

Choose a reason for hiding this comment

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

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

2 participants