-
Notifications
You must be signed in to change notification settings - Fork 442
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
Cannot instantiate subclasses (including newtypes) of floats #527
Comments
jesboat
added a commit
to jesboat/pyre-check
that referenced
this issue
Nov 5, 2021
Summary: On python before 3.9, `float` is a subclass of the ABC numbers.Real which defines abstract methods `__floor__` and `__ceil__`, but doesn't provide an actual implementation of those methods. This is arguably a type error in Python, because the following code is well-typed: def foo(x: numbers.Real): return x.__floor() def bar(x: float): return foo(x) bar(some float) but fail at runtime. This doesn't create a user-visible problem because `__floor__` and `__ceil__` aren't meant to be called directly; instead, users are supposed to call `math.floor(x)` and `math.ceil(x)` which are fine without the dunder methods as long as the object is a primitive number (or a subclass of one, or can be converted to one in various ways.) This creates the unfortunate situation that Pyre won't let people instantiate subclasses of `float` without definitions of `__floor__` and `__ceil__`, even though no typical code actually requires those methods. (Pyre currently has a special case to allow the instantiation of `float` itself.) The ideal behavior would be if Pyre treated `numbers.Real` as "in order to be a subtype of `numbers.Real`, you need to implement the floor/ceil dunder methods, or be a subtype of int/float/etc, or implement `__float__` or `__index__`, etc." and "numbers.Real are not guaranteed to define `__floor__` and `__ceil__`. This would be insane to implement. Pretending that `float` defines `__floor__` and `__ceil__` is a minimally objectionable workaround. It means that float will continue to work, subclasses of float will start to work, and the only well-typed code which can error at runtime is `val.__floor__()` when `type(val) == T <: float`... But that problem already exists when `T == float` and so I don't find extending it to subclasses of float to be particularly concerning. Resolves facebook#527 (github) Test plan: These two examples fail to typecheck before: # example 1 import typing T = typing.NewType('T', float) T(4.0) # example 2 class C(float): def __init__(self, v: float) -> None: pass C(4.0) with errors: example.py:3:0 Invalid class instantiation [45]: Cannot instantiate abstract class `T` with abstract methods `__ceil__`, `__floor__`. example.py:9:14 Invalid class instantiation [45]: Cannot instantiate abstract class `C` with abstract methods `__ceil__`, `__floor__`. but both pass after.
cc @grievejia or @pradeep90 might have an opinion here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Consider the following example:
Pyre expands the NewType definition into:
which fails with:
This is a very awkward case. Python's
float
is subclass of ABCnumbers.Real
(which is hardwired into Pyre The relevant declarations in typeshed (irrelevant methods elided,...
s in original) are:(from numbers.pyi and builtins.pyi)
In fact,
float
on CPython 3.7 and 3.8 does appear to be missing__ceil__
and__float__
methods. This doesn't break users because the exposed API for ceil/floor is through the functionsmath.ceil
andmath.floor
which handle floats internally and only delegate to__ceil__
and__floor__
when called on a non-float
.This doesn't cause Pyre to explode normally presumably because
AbstractClassInstantiation
failures are internally suppressed forint
/float
/bool
. But, that suppression trick fails for subclasses of builtins.This is a kinda gross situation, and there are no obvious good fixes. Some ideas:
Argue that
float
should properly be considered an abstract type (pre-3.9), since allowing instances would mean thatsomeFloat.__ceil__()
would pass the typechecker but fail at runtime. Take a hard-line approach and raise type errors. This is obviously insane.Argue that
float
should properly be considered an abstract type (pre-3.9). Admit that it's impractical to prevent people from instantiating floats, and say that the minimum breakage would be to allow instantiation offloat
itself but nothing else. (This is the status quo.)Attempt to have Pyre capture the actual Python behavior, which is something like "requiring instantiation of subclasses of Real (except
float
or its subclasses) to define__ceil__
and__floor__
, but treat attempts to use__ceil__
or__floor__
as type errors. This seems like a lot of implementation hacks for relatively little benefit.Remove
__ceil__
and__floor__
from Real. This basically just moves the problem tomath.floor
andmath.ceil
by making them do an unchecked cast of their argument fromReal
toUnion[float, subclass of Real which defines __ceil__ and __floor__]
.Modify typeshed to pretend
float
defines__ceil__
and__floor__
for all Python versions. This seems minimally objectionable. Pyre already thinksfloat.__ceil__
andfloat.__floor__
are ok becausefloat <: Real
. All it changes is extending the "I can be an instance of Real without actually implementing__ceil__
/__floor__
as long asmath.ceil
andmath.floor
still work" privilege fromfloat
tofloat
subclasses.Of these, I'd lean towards option 5, and I'll put a diff up for that. There might be some better idea I didn't think of.
Thoughts?
The text was updated successfully, but these errors were encountered: