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

regression with Generic/ParamSpec between 1.4.1 and 1.5.0 #15844

Open
jessemyers-lettuce opened this issue Aug 10, 2023 · 6 comments
Open

regression with Generic/ParamSpec between 1.4.1 and 1.5.0 #15844

jessemyers-lettuce opened this issue Aug 10, 2023 · 6 comments
Labels
bug mypy got something wrong topic-protocols

Comments

@jessemyers-lettuce
Copy link

jessemyers-lettuce commented Aug 10, 2023

Bug Report

With 1.5.0, some of our software has started to surface a var-annotated ("Need type annotation for") error that it did not surface with 1.4.1; I could not find anything in the 1.5.0 release notes that suggested that this was intentional.

To Reproduce

Our actual software is a bit domain-specific, but I've tried to extract the details; the concept involves a data descriptor that attaches a callable to a class and generates a new callable that inverts how the callable's constructor and runtime arguments are supplied. The solution depends on ParamSpec to preserve the argument signatures and uses Generic so that the same internals can be used for different functions.

#!/usr/bin/env python3
from dataclasses import dataclass
from typing import Generic, ParamSpec, Protocol, Self, TypeVar, overload


P = ParamSpec("P")
T = TypeVar("T")


class Transform(Protocol[P, T]):
    """Generic protocol to transform a value using inputs."""

    def __init__(self, *args: P.args, **kwargs: P.kwargs) -> None:
        raise NotImplementedError

    def __call__(self, value: T) -> T:
        raise NotImplementedError


class BoundTransform(Generic[P, T]):
    """Wrapper around a transform function that performs late-binding."""

    def __init__(
        self,
        value: T,
        transform_cls: type[Transform[P, T]],
    ) -> None:
        self.value: T = value
        self.transform_cls: type[Transform[P, T]] = transform_cls

    def __call__(
        self,
        *args: P.args,
        **kwargs: P.kwargs,
    ) -> T:
        return self.transform_cls(*args, **kwargs)(self.value)


class HasValue(Protocol[T]):
    value: T

class Transformer(Generic[P, T]):
    """Data descriptor that applies the bound transformer to something with a value."""

    def __init__(
        self,
        transform_cls: type[Transform[P, T]],
    ) -> None:
        self.transform_cls: type[Transform[P, T]] = transform_cls

    @overload
    def __get__(self, obj: None, objtype: type[HasValue[T]]) -> Self:
        ...

    @overload
    def __get__(self, obj: HasValue[T], objtype: type[HasValue[T]]) -> BoundTransform[P, T]:
        ...

    def __get__(self, obj: HasValue[T] | None, objtype: type[HasValue[T]]) -> Self | BoundTransform[P, T]:
        if obj is None:
            return self

        return BoundTransform(obj.value, self.transform_cls)


@dataclass(frozen=True)
class Add:
    """Transform implementation that adds two integers."""
    value: int

    def __call__(self, value: int) -> int:
        return value + self.value


@dataclass(frozen=True)
class HasNumber(HasValue[int]):
    """HasValue implementation that holds an integer."""
    value: int

    add = Transformer(Add)



if __name__ == "__main__":
    # Usage example
    foo = HasNumber(42)
    bar = foo.add(-42)
    assert bar == 0

Expected Behavior

No errors from mypy. This is true for 1.4.1.

Actual Behavior

Errors. This is true for 1.5.0

main.py:81: error: Need type annotation for "add"  [var-annotated]
Found 1 error in 1 file (checked 1 source file)

Your Environment

  • Mypy version used: 1.4.1 and 1.5.0
  • Mypy command-line flags: n/a
  • Mypy configuration options from mypy.ini (and other config files): n/a
  • Python version used: 3.11.4
@jessemyers-lettuce jessemyers-lettuce added the bug mypy got something wrong label Aug 10, 2023
@AlexWaygood AlexWaygood added the topic-paramspec PEP 612, ParamSpec, Concatenate label Aug 10, 2023
@hauntsaninja
Copy link
Collaborator

Hmm, this bisects to #15490

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Aug 12, 2023

Here's a repro with much less going on:

from typing import Generic, ParamSpec, Protocol, TypeVar

P = ParamSpec("P")
T = TypeVar("T", covariant=True)

class Transform(Protocol[P, T]):
    def __init__(self, *args: P.args, **kwargs: P.kwargs) -> None:
        raise NotImplementedError

class Transformer(Generic[P, T]):
    def __init__(self, transform_cls: type[Transform[P, T]]) -> None:
        raise NotImplementedError

class Add: ...

class HasNumber:
    add = Transformer(Add)

where reveal_type(HasNumber.add) used to Revealed type is "test.Transformer[[], Any]" but now is just "test.Transformer[Any, Any]"

@hauntsaninja
Copy link
Collaborator

For what it's worth, I'm not sure that an __init__ really makes sense on a Protocol. This is sort of the root cause of trouble here. Like I'm genuinely unsure why the Protocol is generic over P and what that really means. Maybe @AlexWaygood has more thoughts

@AlexWaygood AlexWaygood added topic-protocols and removed topic-paramspec PEP 612, ParamSpec, Concatenate labels Aug 12, 2023
@AlexWaygood
Copy link
Member

Heh, this is a fun/annoying corner case.

I agree it doesn't make much sense to ever treat __init__ as a protocol member. In general, mypy doesn't check __init__ or __new__ methods in subclasses to see if they violate the Liskov Substitution Principle -- and that's correct behaviour; constructor and initialiser methods are generally seen to be exempt from LSP considerations. It therefore follows that it doesn't make much sense to try to create an interface whereby objects can be seen to implement that interface iff they have a particular __init__ method.

I'm actually surprised mypy isn't emitting a type-abstract error for Transformer.__init__ in your smaller snippet above @hauntsaninja -- I feel like Transformer should be seen as an abstract class here. A safer annotation for Transformer.__init__ would probably be the following, though that doesn't solve OP's problem (mypy still emits the var-annotated error):

class Transformer(Generic[P, T]):
    def __init__(self, transform_cls: Callable[P, Transform[P, T]]) -> None:
        raise NotImplementedError

The complication, however, is that there are other reasons you might want to define __init__ on a protocol class, even if you don't want it to be seen as part of the interface you're defining. The runtime implementation of protocols changed in Python 3.11 in order to allow you to use __init__ methods on protocol classes as "mixin methods" that can be called from instances nominal subclasses of that protocol class. E.g.:

>>> from typing import Protocol
>>> class Foo(Protocol):
...     x: int
...     def __init__(self, x: int) -> None:
...         self.x = x * 2
...
>>> class Bar(Foo):
...     def __init__(self, x: int, y: int) -> None:
...         super().__init__(x)
...         self.y = y
...
>>> b = Bar(2, 3)
>>> vars(b)
{'x': 4, 'y': 3}

PEP-544 explicitly states that it's okay to call methods defined on protocol classes from instances of nominal subclasses of those protocols. The difference here is that __init__ is pretty unique in that it can only be a mixin method on a protocol class; it's invalid to treat it as a protocol member.

I don't think that necessarily means that you shouldn't be able to define generic __init__ methods on a protocol class; you might want to do something like this, which seems reasonable. This seems to work fine with the latest mypy, however (not sure if we can find an example of something that is reasonable and still breaks with the latest mypy):

from typing import Any, Callable, ParamSpec, TypeVar, Protocol

P = ParamSpec("P")
R = TypeVar("R")

class Interface(Protocol[P, R]):
    f: Callable[P, R]
    args: tuple[Any, ...]
    kwargs: dict[str, Any]
    def __init__(self, f: Callable[P, R], *args: P.args, **kwargs: P.kwargs) -> None:
        self.f = f
        self.args = args
        self.kwargs = kwargs

class Concrete(Interface[[int], int]): pass

@jessemyers-lettuce
Copy link
Author

I'm not sure that an init really makes sense on a Protocol.

You all clearly have a lot more depth in how Protocol is supposed to be used. The first-order documentation (e.g. here), doesn't really give developers (e.g. me) this kind of detail.

I grabbed onto Protocol (in lieu of Callable) at least, in part, because our actual implementation goes a bit deeper than a pure Add function. We actually have a sort of SQL query builder where the callable type uses the parameters (P) to return a Sequence[T] and where there are multiple functions that work together to build the query (e.g. one for the where clause, one for the order-by, etc). It really is important to capture the parameters as members and using __init__ seemed like the obvious solution.

@ilevkivskyi
Copy link
Member

My 5 cents here (as the author of the PEP):

Indeed until recently we didn't check __init__ etc. during subtyping, but we did use it for type inference. FWIW I think it is fine to use special methods excluded from subtyping just for inference (if there are no other ways we can infer types). It should be easy to add this back as a fallback with few lines in constraints.py.

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-protocols
Projects
None yet
Development

No branches or pull requests

4 participants