Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

MAINT: Fix issue 77: Don't allow initializing generic #80

Closed
wants to merge 8 commits into from
Closed

MAINT: Fix issue 77: Don't allow initializing generic #80

wants to merge 8 commits into from

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented May 14, 2020

This pull request addresses the issues raised in #77: constructors were previously available for np.generic and a number of its subclasses, many of which could not actually be instantiated.

Furthermore np.generic (and its non-instantiatable subclasses) have been marked as abstract base classes; np.generic.__init__() being marked as the respective abstract method.
This ensures that mypy will start complaining whenever instantiating a np.generic subclass that does not define __init__().

Removed constructors from the following np.generic subclasses:

  • np.number
  • np.complexfloating

Added constructors for the following np.generic subclasses:

  • np.bool_
  • np.object_
  • np.float16
  • np.float32
  • np.float64
  • np.complex64
  • np.complex128
  • np.void
  • np.bytes
  • np.str_

@BvB93 BvB93 changed the title Fix issue https://github.com/numpy/numpy-stubs/issues/77: Don't allow initializing generic Fix issue https://github.com/numpy/numpy-stubs/issues/77 May 14, 2020
@BvB93 BvB93 changed the title Fix issue https://github.com/numpy/numpy-stubs/issues/77 Fix issue 77: Don't allow initializing generic May 14, 2020
@BvB93 BvB93 changed the title Fix issue 77: Don't allow initializing generic MAINT: Fix issue 77: Don't allow initializing generic May 14, 2020

class character(_real_generic, metaclass=ABCMeta): ...

class bytes_(character):
Copy link
Member Author

Choose a reason for hiding this comment

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

So contrary to its bytes.__init__() counterpart in typeshed (ref), it seems that np.bytes_ can truly take any object as argument.

>>> import numpy as np

>>> np.bytes_(0)
b''

>>> np.bytes_(None)
b'None'  # Equivalent to np.bytes_(str(None))

>>> class Test: 
...     pass

>>> np.bytes_(Test)
b"<class '__main__.Test'>"
>>> np.bytes_(Test())
b'<__main__.Test object at ...>'

@@ -388,22 +389,25 @@ class ndarray(_ArrayOrScalarCommon, Iterable, Sized, Container):
def __contains__(self, key) -> bool: ...
def __index__(self) -> int: ...

class generic(_ArrayOrScalarCommon):
def __init__(self, value: Any = ...) -> None: ...
class generic(_ArrayOrScalarCommon, metaclass=ABCMeta):
Copy link
Member

Choose a reason for hiding this comment

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

I think a problem here is that generic is not actually an ABC; e.g.

>>> inspect.isabstract(np.generic)
False
>>> np.generic.__abstractmethods__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: __abstractmethods__

Copy link
Member Author

Choose a reason for hiding this comment

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

What I'm proposing here is to cheat a little bit for the sake of typing, considering that the meta-class does (as far as mypy can tell) not affect the actual class anyway:

from abc import ABCMeta

class A: ...
class B(metaclass=ABCMeta): ...

A.__abstractmethods__
B.__abstractmethods__
>>> mypy test.py
test.py:6: error: "Type[A]" has no attribute "__abstractmethods__"
test.py:7: error: "Type[B]" has no attribute "__abstractmethods__"
Found 2 errors in 1 file (checked 1 source file)

Copy link
Member

Choose a reason for hiding this comment

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

I think it does though; e.g.

In [1]: import abc

In [2]: class B(metaclass=abc.ABCMeta):
   ...:     @abc.abstractmethod
   ...:     def foo(self): pass
   ...:

In [3]: B.__abstractmethods__
Out[3]: frozenset({'foo'})

OTOH I can't think of anything better to do... will keep pondering.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think i figured out why mypy isn't picking up on the __abstractmethods__ attribute. A closer look at the abc module in typeshed shows that it's rather incomplete (ref), only having the ABCMeta.register() method defined.

I initially thought mypy's lack of __abstractmethods__ recognition might have been a feature, but the relevant abc stub file just seems incomplete (and might thus be changed in the future). With this in mind I'm less confident in the use of ABCMeta, even though, as far as I'm aware, it is the only way to explicitly disallow the initialization of a class.

Copy link
Member Author

Choose a reason for hiding this comment

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

After a bit of testing I may have found a compromise: use the @abstractmethod decorator without the ABCMeta metaclass.
It does mean that mypy will warn about a lack of appropriate metaclass, but these errors can easily be silenced with # type: ignore.

A (slimmed down) piece of example code:

class generic(_ArrayOrScalarCommon):
    @abstractmethod
    def __init__(self, *args: Any, **kwargs: Any) -> None: ...

class _real_generic(generic): ...  # type: ignore

class bool_(_real_generic):
    def __init__(self, value: object = ...) -> None: ...

Copy link
Member

Choose a reason for hiding this comment

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

This seems like a reasonable compromise so long as we document it extensively (though it would be awkward if a third-party were subclassing the scalar hierarchy because then the ignores would leak into their code). A thing we should check on though is what other type checkers will do with this. We're pretty mypy heavy, but I don't want to use hacks that preclude people from using other type checkers.

It's also possible that it's better to allow the incorrect initialization for now and think about whether NumPy can be updated to make generic a true abc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just did a few tests with a number of other type checkers (pyright, pyre and pytype). With the sole exception of pyright they all seem to properly pick up the @abstractmethod decorator (just like mypy). Pyright, specifically, will remain silent unless the ABCMeta has been assigned as metaclass.

Used test code

from typing import Any
from abc import abstractmethod

class generic():
    @abstractmethod
    def __init__(self, *args: Any, **kwargs: Any) -> None: ...

class _real_generic(generic): ...  # type: ignore

class bool_(_real_generic):
    def __init__(self, value: object = ...) -> None: ...

a = generic()
b = _real_generic()
c = bool_()

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for checking that. Seems like this is a good solution then, just make sure to add comments about what's happening (and probably point to this discussion in the comment).

Bas van Beek added 7 commits June 5, 2020 22:15
This pull request addresses the issues raised in #77:
constructors were previously available for ``np.generic`` and a number of its subclasses, many of which could not actually be instantiated.

Furthermore ``np.generic`` (and its non-instantiatable subclasses) have been marked as abstract base classes, ``np.generic.__init__()`` being marked as an abstract method.
This ensures that mypy will start complaining whenever instantiating a ``np.generic`` subclass that does not define ``__init__()``.

Removed constructors from the following ``np.generic`` subclasses:
* ``np.number``
* ``np.complexfloating``

Added constructors for the following ``np.generic`` subclasses:
* ``np.bool_``
* ``np.object_``
* ``np.float16``
* ``np.float32``
* ``np.float64``
* ``np.complex64``
* ``np.complex128``
* ``np.void``
* ``np.bytes``
* ``np.str_``
Prevents mypy from complaining about too little or many arguments, as its exact signature depends on the respective subclass anyway.
Only the "Cannot instantiate abstract class" error should remains now.
`NamedTemporaryFile` is a bit wonky/broken (?) in Windows, disable this part of the test.
@@ -1,6 +1,7 @@
import builtins
import sys
import datetime as dt
from abc import abstractmethod, ABCMeta
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove the ABCMeta import I think?

person142 pushed a commit that referenced this pull request Jun 6, 2020
This pull request addresses the issues raised in
#77: constructors were
previously available for ``np.generic`` and a number of its
subclasses, many of which could not actually be instantiated.
@person142
Copy link
Member

Merged in 39984a4.

@person142 person142 closed this Jun 6, 2020
@BvB93 BvB93 deleted the generic-init branch June 6, 2020 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants