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

builtins: preparation for having the number types derive from the numbers ABCs #3108

Closed
wants to merge 3 commits into from

Conversation

bluetech
Copy link
Contributor

@bluetech bluetech commented Jul 8, 2019

This should make e.g. this work: (See comment #3108 (comment)).

from decimal import Decimal
from numbers import Real

x: Real = 1.2
x = Decimal('1.2')
if isinstance(x, Real):
    # Not dead to mypy

This PR has some preparation commits before making the change. Should be easier to review each one separately.

@bluetech bluetech force-pushed the builtins-numbers branch from cb4e876 to eec07e0 Compare July 8, 2019 09:36
@bluetech
Copy link
Contributor Author

bluetech commented Jul 8, 2019

Hmm, this causes mypy with the new semantic analyzer to crash:

RuntimeError: Internal error: unresolved placeholder type builtins.int

@srittau
Copy link
Collaborator

srittau commented Jul 8, 2019

I like the first three commits. I am a bit wary of deriving the built-in number types from the ABCs, though. The real classes are not derived from the ABCs and probably use ABCMeta.register() or a similar mechanism. While I'm not totally opposed to this for practicability reasons and since this about Python's built-in numeric tower, I'd prefer to see another, more general solution, either involving protocols or some kind of typing support to registering ABCs.

@bluetech
Copy link
Contributor Author

bluetech commented Jul 8, 2019

@srittau Thanks for taking a look, I'll remove the last commit from this PR and open an issue to discuss it.

@bluetech bluetech force-pushed the builtins-numbers branch from eec07e0 to 919609d Compare July 8, 2019 09:50
@bluetech bluetech changed the title builtins: have the number types derive from the numbers ABCs builtins: preparation for having the number types derive from the numbers ABCs Jul 8, 2019
@bluetech
Copy link
Contributor Author

bluetech commented Jul 8, 2019

@srittau Updated. Will open the issue a bit later.

@overload
def __round__(self, ndigits: int) -> float: ...
def __trunc__(self) -> int: ...
def __floor__(self) -> int: ...
def __ceil__(self) -> int: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last two don't seem to exist (which surprised me):

Python 3.7.3 (default, Jun  3 2019, 18:27:33) 
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> x = 3.0
>>> x.__trunc__()
3
>>> x.__floor__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'float' object has no attribute '__floor__'
>>> x.__ceil__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'float' object has no attribute '__ceil__'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, should have checked all of them.

I looked into cpython code, and what I understand is that math.floor (same for math.ceil) has a special case for floats, or rather types that can be converted to float (implement __float__()). In these cases, if __floor__ is not defined, it converts to float and does the operation directly.

import math

class X:
    def __float__(self):
        return 5.4

    #def __floor__(self):
    #    return 10

print(math.floor(X()))

Commented it prints 5, uncommented it prints 10.

So I think that, given numbers.Real already requires __float__, we can just diverge from the actual definition of numbers.Real and remove __floor__ and __ceil__ from it in typeshed. WDYT? The numbers.Real doc only requires math.floor to work.

@@ -70,7 +70,11 @@ class Real(Complex, SupportsFloat):
@abstractmethod
def __ceil__(self) -> int: ...
@abstractmethod
def __round__(self, ndigits: Optional[int] = ...): ...
@overload
def __round__(self, ndigits: None = ...): ...
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this needs overloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to have the same form as float above. Once this stub is filled the return types of the two overloads will be different, I think.

@@ -168,7 +168,7 @@ class int:
def __rtruediv__(self, x: int) -> float: ...
def __rmod__(self, x: int) -> int: ...
def __rdivmod__(self, x: int) -> Tuple[int, int]: ...
def __pow__(self, x: int) -> Any: ... # Return type can be int or float, depending on x.
def __pow__(self, x: int, modulo: Optional[int] = ...) -> Any: ... # Return type can be int or float, depending on x.
Copy link
Member

Choose a reason for hiding this comment

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

The arguments are actually positional-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there some special way to encode this?

Copy link
Member

Choose a reason for hiding this comment

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

Start the names with two underscores, like __modulo.

@bluetech
Copy link
Contributor Author

I stripped the useful parts of this PR to separate PRs and opened an issue about the __ceil__/__floor__ situation.

@bluetech bluetech closed this Aug 17, 2019
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.

3 participants