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

False positive: signature incompatible with supertype #13106

Closed
agronholm opened this issue Jul 12, 2022 · 18 comments
Closed

False positive: signature incompatible with supertype #13106

agronholm opened this issue Jul 12, 2022 · 18 comments
Labels
bug mypy got something wrong topic-inheritance Inheritance and incompatible overrides topic-overloads

Comments

@agronholm
Copy link

agronholm commented Jul 12, 2022

Bug Report

Mypy falsely reports an overridden method to be incompatible with the supertype.

To Reproduce

Create the following file:

from __future__ import annotations

from abc import ABCMeta, abstractmethod
from typing import overload


class Interface(metaclass=ABCMeta):
    @abstractmethod
    @overload
    def foo(self, a: str) -> str:
        ...

    @abstractmethod
    @overload
    def foo(self, a: int) -> int:
        ...

    @abstractmethod
    def foo(self, a: str | int) -> str | int:
        pass


class Implementation(Interface):
    def foo(self, a: str | int) -> str | int:
        return a

Actual Behavior

bug.py:24: error: Signature of "foo" incompatible with supertype "Interface"
bug.py:24: note:      Superclass:
bug.py:24: note:          @overload
bug.py:24: note:          def foo(self, a: str) -> str
bug.py:24: note:          @overload
bug.py:24: note:          def foo(self, a: int) -> int
bug.py:24: note:      Subclass:
bug.py:24: note:          def foo(self, a: Union[str, int]) -> Union[str, int]

If I replace the union with str (and force the last line to return str), the error goes away. Pyright does not report any problems with this code either way. This isn't about ABCMeta or abstract methods either – if I remove all that, the error is still reported (but then pyright would complain that Interface.foo() does not return anything – which strangely mypy does not).

Your Environment

  • Mypy version used: 0.961
  • Mypy command-line flags: (none)
  • Mypy configuration options from mypy.ini (and other config files): (none)
  • Python version used: 3.7.13
  • Operating system and version: Fedora Linux 36
@agronholm agronholm added the bug mypy got something wrong label Jul 12, 2022
@AlexWaygood AlexWaygood added topic-overloads topic-inheritance Inheritance and incompatible overrides labels Jul 12, 2022
@aiudirog
Copy link

I came across this issue while looking for a different thing entirely, but IMO, I don't think this is actually a bug in mypy at all. At least not from a technical standpoint.

You see, in Interface you define the foo() method to have two overloads: one which takes a string and returns a string and another which takes an int and returns an int. However in Implementation, you define foo() with only one implementation that takes a string or int and returns either a string or int. Therefore, by typing alone, it is entirely possible for Implementation.foo() to take a string and return an int and vice versa. This is definitely not compatible typing wise with Interface.foo(). The proper solution would be to duplicate the overloads in the subclass.

To make this pattern more convenient, I would suggest some sort of @inherit_overloads or @copy_overloads(other_func) decorator could be added to the typing library.

As an alternative to overloads, you could probably make use of a TypeVar instead:

StrOrInt = TypeVar('StrOrInt', str, int)


class Interface(metaclass=ABCMeta):

    @abstractmethod
    def foo(self, a: StrOrInt) -> StrOrInt:
        pass


class Implementation(Interface):
    def foo(self, a: StrOrInt) -> StrOrInt:
        return a

It's not the most elegant thing, especially if Implementation is defined in another file entirely, but it should pass.

@agronholm
Copy link
Author

You see, in Interface you define the foo() method to have two overloads: one which takes a string and returns a string and another which takes an int and returns an int. However in Implementation, you define foo() with only one implementation that takes a string or int and returns either a string or int. Therefore, by typing alone, it is entirely possible for Implementation.foo() to take a string and return an int and vice versa. This is definitely not compatible typing wise with Interface.foo(). The proper solution would be to duplicate the overloads in the subclass.

I thought I had tried that early on in my original use case which is why I didn't try it here again. Thanks for pointing this out. Still, mypy doesn't make it easy to figure out what is not compatible 😞

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

What about this case?:

from __future__ import annotations

from typing import overload, Generic, TypeVar

T = TypeVar("T")

class Interface(Generic[T]):
    @overload
    def foo(self: Interface[None]) -> int:
        ...

    @overload
    def foo(self, a: T) -> int:
        ...

    def foo(self, a: T | None = None) -> str | int:
        return 1


class Implementation(Interface[int]):
    
    def foo(self, a: int) -> int:
        return 2

Here the other branch of the overload don't apply to the subclass, but Mypy still complains.

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

Implementation can't handle the case where None nothing is passed, which Interface promised it could.

Edit: To be clear, the fact that you are ignoring the input param and returning a static integer doesn't matter. MyPy only cares about your type signatures, the same as your users who are (hopefully) reading your documentation.

Edit2: I misread the code, see: #13106 (comment)

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

Implementation can't handle the case where None is passed, which Interface promised it could.

But that is not true! Interface just promised that a T could be handled, and that if T is None you can omit it in the call.

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

Can I though?

>>> Implementation().foo()
TypeError                                 Traceback (most recent call last)
Input In [2], in <cell line: 24>()
     22     def foo(self, a: int) -> int:
     23         return 2
---> 24 Implementation().foo()

TypeError: Implementation.foo() missing 1 required positional argument: 'a'

You need to actually make it optional:

class Implementation(Interface[int]):

    def foo(self, a: int | None = None) -> int:
        return 2

This passes MyPy no problem.


I do realize I misread your original code as having def foo(self, a: None = None) -> int: instead of def foo(self: Interface[None]) -> int: but that doesn't matter in this circumstance anyway. My apologies for the confusion.

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

Can I though?

Not for Implementation, as it uses T=int. Other subclasses with T=None can.

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

Ah, but here's the trick:

class Implementation(Interface[None]):

    def foo(self) -> int:
        return 2

will still fail with:

a.py:22: error: Signature of "foo" incompatible with supertype "Interface"
a.py:22: note:      Superclass:
a.py:22: note:          @overload
a.py:22: note:          def foo(self) -> int
a.py:22: note:          @overload
a.py:22: note:          def foo(self, a: None) -> int
a.py:22: note:      Subclass:
a.py:22: note:          def foo(self) -> int
Found 1 error in 1 file (checked 1 source file)

Why? Because Interface declares that .foo() can be called with an argument for parameter a, even if it is still None. Which means Implementation().foo(None) needs to continue to work.

This passes:

class Implementation(Interface[None]):

    def foo(self, a: None = None) -> int:
        return 2

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

Maybe I am not explaining myself: the problem is that Mypy complains even when I inherit from Interface[int], and thus I should not need to provide the first overload (because it does not apply in that case).

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

Now I'm seeing it. I would say the problem there is you have an overload that only applies to one specific type of subclass, which smells like an anti-pattern to me. Personally I wouldn't see this as a bug but rather an unsupported feature.

Do you have an example of this idea in another language with strong typing?

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

No, I don't have an example.

I had this problem in the context of typing scikit-learn TransformerMixin as a Generic over the target. Some transformers do not require a target and thus you can call the fit method without passing one. However, for transformations that require a target I didn't want Mypy to allow it.

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

I see, that makes sense. Just to make sure we're on the same page, you're trying to make TransformerMixin generic? So in the scenario where y is optional, you can leave it off but when it is not, you can't?

@vnmabus
Copy link

vnmabus commented Sep 1, 2022

Yes, I am the maintainer of scikit-fda, a library that depends on scikit-learn. As scikit-learn is not typed, I am subclassing some classes in order to provide typing in my own library:

class TransformerMixin(  # noqa: D101
    ABC,
    Generic[Input, Output, Target],
    sklearn.base.TransformerMixin,  # type: ignore[misc]
):

    @overload
    def fit(
        self: TransformerNoTarget,
        X: Input,
    ) -> TransformerNoTarget:
        pass

    @overload
    def fit(
        self: SelfType,
        X: Input,
        y: Target,
    ) -> SelfType:
        pass

    def fit(  # noqa: D102
        self: SelfType,
        X: Input,
        y: Target | None = None,
    ) -> SelfType:
        return self

    @overload
    def fit_transform(
        self: TransformerNoTarget,
        X: Input,
    ) -> Output:
        pass

    @overload
    def fit_transform(
        self,
        X: Input,
        y: Target,
    ) -> Output:
        pass

    def fit_transform(  # noqa: D102
        self,
        X: Input,
        y: Target | None = None,
        **fit_params: Any,
    ) -> Output:
        if y is None:
            return self.fit(  # type: ignore[no-any-return]
                X,
                **fit_params,
            ).transform(X)

        return self.fit(  # type: ignore[no-any-return]
            X,
            y,
            **fit_params,
        ).transform(X)

@aiudirog
Copy link

aiudirog commented Sep 1, 2022

While I see how your approach would be more convenient to your users with only a single mixin class, I think having TransformerNoTarget as dedicated subclass makes this more explicit and easier to read/maintain:

class TransformerMixin(  # noqa: D101
    ABC,
    Generic[Input, Output, Target],
    sklearn.base.TransformerMixin,  # type: ignore[misc]
):

    def fit(
        self: SelfType,
        X: Input,
        y: Target,
    ) -> SelfType:
        return self

    def fit_transform(  # noqa: D102
        self,
        X: Input,
        y: Target,
        **fit_params: Any,
    ) -> Output:
        return self.fit(  # type: ignore[no-any-return]
            X,
            **fit_params,
        ).transform(X)


class TransformerNoTarget(TransformerMixin[Input, Output, None]):

    @overload
    def fit(
        self: SelfType,
        X: Input,
    ) -> SelfType:
        pass

    @overload
    def fit(
        self: SelfType,
        X: Input,
        y: None = None,
    ) -> SelfType:
        pass

    def fit(  # noqa: D102
        self: SelfType,
        X: Input,
        y: None = None,
    ) -> SelfType:
        return self

    @overload
    def fit_transform(
        self: SelfType,
        X: Input,
        **fit_params: Any,
    ) -> Output:
        pass

    @overload
    def fit_transform(
        self,
        X: Input,
        y: None = None,
        **fit_params: Any,
    ) -> Output:
        pass

    def fit_transform(  # noqa: D102
        self,
        X: Input,
        y: None = None,
        **fit_params: Any,
    ) -> Output:
        return super().fit(X, y, **fit_params)

The above code passes MyPy and allows a TransformerNoTarget to be used wherever a TransformerMixin is acceptable while giving it the expected convenience of not having to provide y when it isn't used while requiring it in all other scenarios.

@vnmabus
Copy link

vnmabus commented Sep 2, 2022

Well, that introduces additional problems:

  • What about transformers that allow either None or another target?
  • What about subclasses? If I want to add an InductiveTransformer for typing transform, then I now need TWO subclasses (honestly, this would be better with non-required methods as proposed in Optional class and protocol fields and methods typing#601, as then we can type transform and inverse_transform without having a subclass explosion).

@aiudirog
Copy link

aiudirog commented Sep 2, 2022

In order to explore this more, I tried to code up the same example in Java and immediately ran into an issue:

// Interface.java
public interface Interface<T> {
    public Integer foo();
    public Integer foo(T a);
}

// Implementation.java
public class Implementation implements Interface<Integer> {
    public Integer foo(Integer a) { return a; }
}

There's no way to provide a type for this in the method definition to declare that the method only exists in a specific implementation!

As expected, this throws an error because we didn't implement the version that doesn't take any parameters:

$ javac Implementation.java
Implementation.java:1: error: Implementation is not abstract and does not override abstract method foo() in Interface
public class Implementation implements Interface<Integer> {
       ^
1 error

I could be missing something as my Java isn't the best, but I'm pretty sure there is no physical way to specify this kind of behavior at the pure typing level. The fact that we can provide a type for self in Python kind of puts us in only vaguely charted territory. I personally don't know of any strictly typed language that allows you to provide a type declaration for the self/this parameter that we can use to draw inspiration from. Most languages don't even take it as an argument, it's typically implied from the scope.

I would say that @overload being used in generic interfaces to declare methods that would only exist in a specific set of subclasses is a niche and specific functionality that probably needs to be a dedicated feature. Maybe a MyPy plugin could solve this as a proof of concept?


Also, just to be clear, I'm not a MyPy dev. I've just been using MyPy for a few years and just happened to have the answer to the original question and misread yours thinking it was a lot simpler than it is haha.

@vnmabus
Copy link

vnmabus commented Sep 2, 2022

There's no way to provide a type for this in the method definition to declare that the method only exists in a specific implementation!

That is an accepted and documented way to do that: https://mypy.readthedocs.io/en/stable/more_types.html?highlight=Advanced%20uses#restricted-methods-in-generic-classes

@aiudirog
Copy link

aiudirog commented Sep 2, 2022

Well then, looks like I missed some parts of the docs. In that case, I'm pretty sure we're dealing with a bug and you should open a dedicated ticket. Specifically on inheriting overloaded, restricted methods from generic classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-inheritance Inheritance and incompatible overrides topic-overloads
Projects
None yet
Development

No branches or pull requests

4 participants