Skip to content

Overloads for property getter (instance vs. through owner) #5093

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

Closed
ikonst opened this issue May 22, 2018 · 6 comments
Closed

Overloads for property getter (instance vs. through owner) #5093

ikonst opened this issue May 22, 2018 · 6 comments
Assignees

Comments

@ikonst
Copy link
Contributor

ikonst commented May 22, 2018

The docs for object.__get__ specify that:

object.__get__(self, instance, owner)

... owner is always the owner class, while instance is the instance that the attribute was accessed through, or None when the attribute is accessed through the owner.

The typeshed for pynamodb has:

class NumberAttribute(Attribute[Number]):
    def __get__(self, instance: Any, owner: Any) -> Number: ...

This allowed us to use PynamoDB idiomatically so that MyModel().my_number is considered a Union[int, float] rather than an instance of NumberAttribute. The underlying implementation might not use properties, but superficially it works ;)

Recent versions of PynamoDB added a conditional query syntax like this:

for my_model in MyModel.query(range_key_condition=MyModel.my_number > 42):
   ...

I've tried representing it like this:

class NumberAttribute(Attribute[Number]):
    @overload
    def __get__(self, instance: object, owner: Any) -> Number: ...
    @overload
    def __get__(self, instance: None, owner: Any) -> NumberAttribute: ...

but mypy seems to ignore the latter override.

Test code:

from pynamodb.attributes import NumberAttribute, ListAttribute
from pynamodb.models import Model


class MyModel(Model):
    my_number = NumberAttribute(hash_key=True)


reveal_type(MyModel.my_number)

m1 = MyModel()
reveal_type(m1.my_number)

Result:

pynamodb.py:9: error: Revealed type is 'Union[builtins.int, builtins.float]'
pynamodb.py:12: error: Revealed type is 'Union[builtins.int, builtins.float]'
@JelleZijlstra
Copy link
Member

Does it work if you reverse the order of the overloads?

@ikonst
Copy link
Contributor Author

ikonst commented May 22, 2018

class NumberAttribute(Attribute[Number]):
    @overload
    def __get__(self, instance: None, owner: Any) -> 'NumberAttribute': ...
    @overload
    def __get__(self, instance: object, owner: Any) -> Number: ...

results in:

# reveal_type(MyModel.my_number)
Revealed type is 'Any'
# reveal_type(m1.my_number)
Revealed type is 'Union[builtins.int, builtins.float]'

@Michael0x2a
Copy link
Collaborator

This should hopefully be fixed sometime in the near future: we're currently in the process of overhauling how mypy handles overloads in general, and the plan is to fix this along the way.

(Well, it's not so much "fix" as it is "special-case overloading descriptors to allow this", but whatever.)

There's also some related discussion here: #3757.

@Michael0x2a
Copy link
Collaborator

I'm pretty certain this ought to be fixed on master now, though I'm not very familiar with the pynamodb stubs so can't say for certain.

@ikonst -- can you confirm?

@ikonst
Copy link
Contributor Author

ikonst commented Jun 9, 2018

This definition works:

class NumberAttribute(Attribute[float]):
    @overload
    def __get__(self, instance: Model, owner: Any) -> float: ...
    @overload
    def __get__(self, instance: None, owner: Any) -> NumberAttribute: ...

This one also works:

class NumberAttribute(Attribute[float]):
    @overload
    def __get__(self, instance: None, owner: Any) -> NumberAttribute: ...
    @overload
    def __get__(self, instance: object, owner: Any) -> float: ...

(note the None variant being first)

@ilevkivskyi
Copy link
Member

OK, so as I understand this one can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants