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

Cannot instantiate subclasses (including newtypes) of floats #527

Open
jesboat opened this issue Nov 5, 2021 · 1 comment · May be fixed by #528
Open

Cannot instantiate subclasses (including newtypes) of floats #527

jesboat opened this issue Nov 5, 2021 · 1 comment · May be fixed by #528

Comments

@jesboat
Copy link

jesboat commented Nov 5, 2021

Consider the following example:

from typing import NewType
T = NewType('T', float)
T(1.0)

Pyre expands the NewType definition into:

class T(float):
    def __init__(self, input: float) -> None: pass

which fails with:

example.py:3:0 Invalid class instantiation [45]:
  Cannot instantiate abstract class `T` with abstract methods `__ceil__`, `__floor__`.

This is a very awkward case. Python's float is subclass of ABC numbers.Real (which is hardwired into Pyre The relevant declarations in typeshed (irrelevant methods elided, ...s in original) are:

class Real(Complex, SupportsFloat):
    @abstractmethod
    def __floor__(self) -> int: ...
    @abstractmethod
    def __ceil__(self) -> int: ...

class float:
    if sys.version_info >= (3, 9):
        def __ceil__(self) -> int: ...
        def __floor__(self) -> int: ...

(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 functions math.ceil and math.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 for int/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:

  1. Argue that float should properly be considered an abstract type (pre-3.9), since allowing instances would mean that someFloat.__ceil__() would pass the typechecker but fail at runtime. Take a hard-line approach and raise type errors. This is obviously insane.

  2. 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 of float itself but nothing else. (This is the status quo.)

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

  4. Remove __ceil__ and __floor__ from Real. This basically just moves the problem to math.floor and math.ceil by making them do an unchecked cast of their argument from Real to Union[float, subclass of Real which defines __ceil__ and __floor__].

  5. Modify typeshed to pretend float defines __ceil__ and __floor__ for all Python versions. This seems minimally objectionable. Pyre already thinks float.__ceil__ and float.__floor__ are ok because float <: Real. All it changes is extending the "I can be an instance of Real without actually implementing __ceil__/__floor__ as long as math.ceil and math.floor still work" privilege from float to float 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?

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.
@jesboat jesboat linked a pull request Nov 5, 2021 that will close this issue
@arthaud
Copy link
Contributor

arthaud commented Nov 5, 2021

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
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants