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

Is it intentional that Callable[..., T] methods can be overridden arbitrarily #8795

Closed
ezyang opened this issue May 10, 2020 · 3 comments
Closed

Comments

@ezyang
Copy link

ezyang commented May 10, 2020

Imagine that you are trying to write a class to represent function-like objects (actual example at pytorch/pytorch#35566), which may have varying input and argument types. Because mypy doesn't support variadic generics (#193), we'll settle for some syntax that may not be very type safe as long as mypy doesn't complain about it. However, we'd like to avoid having subclasses to have to add # type: ignore themselves (since this is not very good UX).

The obvious spelling of this class does not work:

from typing import Any

class Function:
    def apply(self, *args: Any) -> Any:
        raise NotImplementedError

class AddTwo(Function):
    def apply(self, input: int) -> int:
        return input + 2 

fails with:

bar.py:8: error: Signature of "apply" incompatible with supertype "Function"  [override]
Found 1 error in 1 file (checked 1 source file)

Which makes sense, because the supertype declared that it would work for any arbitrary list of input arguments, and our override implementation only works for a single int argument.

It turns out (discovered by @suo at pytorch/pytorch#29099) that if you define apply to be a Callable[..., Any] mypy will accept the definition:

from typing import Any, Callable

class Function:
    def _apply(self, *args: Any) -> Any:
        raise NotImplementedError
    apply: Callable[..., Any] = _apply

class AddTwo(Function):
    def apply(self, input: int) -> int:
        return input + 2 

This is great for me (who wanted to figure out how to type this code without forcing AddTwo to type: ignore their definition), but the fact that this semantics-preserving program transformation caused the program to start typechecking again seems very questionable! So, is this a bug, or intentional behavior?

$ mypy --version
mypy 0.770
@ezyang
Copy link
Author

ezyang commented May 10, 2020

Here is another similar variant that also typechecks:

from typing import Any

class Function:
    def _apply(self, *args: Any) -> Any:
        raise NotImplementedError
    apply: Any = _apply

class AddTwo(Function):
    def apply(self, input: int) -> int:
        return input + 2

@Stannislav
Copy link

Here is another similar variant that also typechecks:

from typing import Any

class Function:
    def _apply(self, *args: Any) -> Any:
        raise NotImplementedError
    apply: Any = _apply

class AddTwo(Function):
    def apply(self, input: int) -> int:
        return input + 2

this solution breaks PyCharm's code inspection (haven't tested other IDEs...) for derived classes:

class SubFunction(Function):
    def apply(self): ...

PyCharms flags that not all abstract methos have been overridden (here: apply_ was not overridden).

I'm totally on PyCharm's side on this one.

I see that these semantics have now made it into the torch.nn.Module class - now all custom torch modules get flagged.

@gvanrossum
Copy link
Member

that this semantics-preserving program transformation caused the program to start typechecking again seems very questionable!

Your "solution" just introduces a more relaxed type. It's no more a surprise that it "works" than this:

x = 0
x + ""  # Error
y: Any = 0
y + ""  # No error

IMO the best solution for your original problem (an intentional Liskov violation) really is to use # type: ignore in the subclass.

I'm closing this, because I don't see a mypy bug or feature 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

No branches or pull requests

3 participants