python / mypy

Optional static typing for Python
https://www.mypy-lang.org/
Other
18.5k stars 2.83k forks source link

mypy not flagging subclasses with incompatible constructors #6967

Open lbenezriravin opened 5 years ago

lbenezriravin commented 5 years ago

reproducer

from typing import Type
class Foo:
    def __init__(self, a):
        self.a = a

class Bar(Foo):
    def __init__(self):
        super().__init__("Bar")

def takes_foo(foo: Type[Foo]):
    x = foo("Foo")

takes_foo(Foo)
takes_foo(Bar)

observed behavior mypy raises no errors, but the code raises a TypeError at runtime.

expected behavior pep 484 states:

when new_user() calls user_class() this implies that all subclasses of User must support this in their constructor signature...A type checker ought to flag violations of such assumptions, but by default constructor calls that match the constructor signature in the indicated base class (User in the example above) should be allowed.

My understanding of the above is that mypy should be raising an error on the Bar definition because its signature is incompatible with that of its superclass. More generally, I tend to be surprised when code that passes mypy raises a TypeError at runtime.

environment master as of 2019-06-10

$ pipenv graph
mypy==0.710+dev.e2f31ed71bd1edd60bffc86d3fda9da15ba63b3d
  - mypy-extensions [required: >=0.4.0,<0.5.0, installed: 0.4.1]
  - typed-ast [required: >=1.4.0,<1.5.0, installed: 1.4.0]
$ pipenv run python --version
Python 3.7.3

Thank you all for an amazing tool!

refi64 commented 5 years ago

When a function has no type annotations at all, it's considered untyped, as in this case. If you add a -> None to the end, then it'll be typed and you'll get an error. There are also command line options to show untyped defs (--disallow-untyped-defs?).

lbenezriravin commented 5 years ago

Thanks for the response! Ah, that was silly of me to leave out the annotations on the __init__. However, adding type annotations hasn't helped in this case. The following still raises no errors:

from typing import Type

class Foo:
    def __init__(self, a: str) -> None:
        self.a = a

class Bar(Foo):
    def __init__(self) -> None:
        super().__init__('Bar')

def takes_foo(foo: Type[Foo]) -> None:
    foo('Foo')

takes_foo(Foo)
takes_foo(Bar)
ilevkivskyi commented 5 years ago

This is actually deliberate decision, mypy specifically whitelists __init__, __new__, and __init_subclass__ from LSP checks because incompatible constructor overrides is such a widespread idiom in Python (see however https://github.com/python/mypy/issues/3823).

Maybe we can add the stricter checks beyond flag, but OTOH we already have lots of flags. Maybe we can add a selection of stricter checks under the umbrella of --strict flag (which is currently just a selection of other flags)?

JukkaL commented 5 years ago

If we'd add an option for this, I think it would fit better under optional error codes that can enabled (this applies to some existing strictness related flags). This will be possible once we have support for error codes.

Since object() takes no arguments, we'd need to special case it somehow or no __init__ methods can accept any arguments.

gin-ahirsch commented 4 years ago

Maybe a decorator can be added to typing to allow a kind of "opt-in" or "opt-out" on a case-by-case basis. Something like typing.inherit_signature() and typing.different_signature(). I don't like these names, but I can't think of anything better right now.

typing.inherit_signature() would signal the requirement for a type's __init__() signature to be compatible with super().__init__(), such that def __init__() for Bar in the posted example would fail to type-check.

Using typing.different_signature() would instead make takes_foo(Bar) fail, because the signature for Bar.__init__() is annotated to differ from Foo.__init__(), but it would allow the method to exist without failing the type-check. In the example the signatures really do not match, but I think if this annotation was added type-checking should also fail if the signatures happened to match, because the annotation signals that this is not intended, meaning it should not be relied upon.

ilevkivskyi commented 4 years ago

A decorator is also possible, but IMO having an error code to enable this check using a unified mechanism of enabling error codes would be better in this case.

IIRC @JukkaL wanted to add such general mechanism, but it's not clear when we will time to work on this.

gin-ahirsch commented 4 years ago

I concur the flag is useful, but I think the decorators would still be a useful addition.