Skip to content
This repository has been archived by the owner. It is now read-only.

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

Closed
wants to merge 8 commits into from

Conversation

@BvB93
Copy link
Contributor

@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):

This comment has been minimized.

@BvB93

BvB93 May 15, 2020
Author Contributor

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):

This comment has been minimized.

@person142

person142 May 17, 2020
Collaborator

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__

This comment has been minimized.

@BvB93

BvB93 May 18, 2020
Author Contributor

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)

This comment has been minimized.

@person142

person142 May 19, 2020
Collaborator

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.

This comment has been minimized.

@BvB93

BvB93 May 19, 2020
Author Contributor

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.

This comment has been minimized.

@BvB93

BvB93 May 26, 2020
Author Contributor

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

This comment has been minimized.

@person142

person142 Jun 5, 2020
Collaborator

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.

This comment has been minimized.

@BvB93

BvB93 Jun 5, 2020
Author Contributor

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_()

This comment has been minimized.

@person142

person142 Jun 5, 2020
Collaborator

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

BvB93 added 7 commits 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 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

This comment has been minimized.

@person142

person142 Jun 6, 2020
Collaborator

Forgot to remove the ABCMeta import I think?

person142 added 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
Collaborator

@person142 person142 commented Jun 6, 2020

Merged in 39984a4.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.