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

__ceil__, __floor__ and numbers.Real #3195

Closed
bluetech opened this issue Aug 17, 2019 · 13 comments · Fixed by #4274
Closed

__ceil__, __floor__ and numbers.Real #3195

bluetech opened this issue Aug 17, 2019 · 13 comments · Fixed by #4274

Comments

@bluetech
Copy link
Contributor

PR #3108 (originally) tried to make float a subclass of numbers.Real. Two method implementations were missing, __floor__ and __ceil__ (cpython). These do exist for int and Decimal etc., but @srittau noticed that they actually do not exist for float itself, therefore they cannot be added to float's stubs, and float cannot be made to conform to numbers.Real.


I did some digging and found that abstractly numbers.Real really just wants math.floor() and math.ceil() to work. And in cpython, these two functions work something like this (using floor ):

  1. If the argument is a float, do the operation directly in C code.
  2. Else, if the argument has a __floor__ method, use it.
  3. Else, if the argument has a __float__ method, use it and go to (1).

As it happen, numbers.Real does require __float__() to be implemented. So as a workaround for this issue, I proposed removing __floor__ and __ceil__ from typeshed's numbers.Real stub, since the __float__ fallback can be relied on instead.


Another view is that this is a cpython issue and should be fixed there.


And another view is that numbers is a lost cause for static typing and should just be ignored or replaced with something better (like protocols).

@srittau
Copy link
Collaborator

srittau commented Oct 11, 2019

If I understand correctly, the issue boils down to the problem that isinstance(3.3, numbers.Real) returns True, although float is missing __ceil__ and __floor__, which is required by Real. I assume the isinstance works by some special magic in cpython. If that is the case, I think the best approach is to copy that special-casing in type checkers than try to work around it in the stubs. Also it might be worth to bring the question up with the cpython developers. I find it strange that float is missing those methods when int has them.

@JelleZijlstra
Copy link
Member

The reason it works at runtime is that numbers.py has a line Real.register(float).

@gvanrossum
Copy link
Member

I guess math.ceil() and math.floor() special-case float.

Can you file a bug in bugs.python.org about the missing methods? They should be added for completeness.

@bluetech
Copy link
Contributor Author

Can you file a bug in bugs.python.org about the missing methods? They should be added for completeness.

https://bugs.python.org/issue38629

@staticdev
Copy link

@bluetech this bug is considered fixed, but I am still having error.

@srittau
Copy link
Collaborator

srittau commented Jun 26, 2020

These methods need to be added to typeshed for Python 3.9+.

@staticdev
Copy link

@srittau I tried with 3.6, 3.7 and 3.8 using floor. It is not accepting float as return.

@srittau
Copy link
Collaborator

srittau commented Jun 26, 2020

For versions <3.9, either type checkers needs to implement special logic (unlikely to happen, though), or you need to # type: ignore this. Unfortunately there is no good solution on the typeshed side.

srittau added a commit to srittau/typeshed that referenced this issue Jun 26, 2020
JelleZijlstra pushed a commit that referenced this issue Jun 27, 2020
@bluetech
Copy link
Contributor Author

@srittau

You added __ceil__ and __floor__ to float on >=3.9 only, which reflects the real situation correctly.

Will you consider "lying" and change the condition to >=3? The rationale for doing this is explained in the original issue:

  1. float.__ceil__/float.__floor__ are usually not called directly, but using math.ceil/math.floor, which do work for float.
  2. __ceil__ and __floor__ are required by numbers.Real on Python 3.
  3. numbers.Real is implemented manually (with register) on float on Python 3.
  4. We want to numbers.Real to be implemented on float also in typeshed, however this is only possible when __ceil__/__floor__ exist.

@srittau
Copy link
Collaborator

srittau commented Jun 28, 2020

I still maintain that it's better to special-case this is type checkers. In my experience, lying in the stubs has always proved problematic. While it solves one problem, it usually creates others. This is especially the case, since nowadays type stubs are used by more than type checkers.

@bluetech
Copy link
Contributor Author

That's understandable - thanks for considering!

@hauntsaninja
Copy link
Collaborator

I'm curious if there are specific problems you anticipate from the lie?

All I can see for now are false negatives if someone does 3.0 .__floor__(). Calling __floor__ directly is orders of magnitude less common than using numbers.Real (which float is generally the archetypal example of):
https://grep.app/search?q=.__floor__%28%29&filter[lang][0]=Python
https://grep.app/search?q=numbers.Real&case=true&filter[lang][0]=Python
Besides, if we expect type checkers to special case this, users will likely hit this false negative anyway.

The other major users of type stubs (afaik) are IDEs. I guess suggesting __floor__ in autocomplete would be a false positive, so a worse kind of mistake, but as above, it seems like calling __floor__ directly is very rare.

This problem will go away by itself in another couple years, so not the biggest deal. And I will definitely acknowledge that my last attempt to lie in typeshed crashed and burned :-) But still curious what you foresee!

@srittau
Copy link
Collaborator

srittau commented Jun 29, 2020

To be honest, I can't make a strong case in this particular instance. But I remember an argument before to include overridden methods in sub-classes, even if the signature is not changed, if the sub-class implements a particular method. I don't remember the specifics. I also remember several cases where we deviated from the implementation just to revert the changes later. These are the things that make me wary. And I find it more appropriate for type checkers to work around the quirks of the Python language than trying to work around those quirks in stubs.

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 a pull request may close this issue.

6 participants