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

Generic callback protocols break with kwargs #7311

Open
chadrik opened this issue Aug 9, 2019 · 10 comments
Open

Generic callback protocols break with kwargs #7311

chadrik opened this issue Aug 9, 2019 · 10 comments
Labels
needs discussion topic-calls Function calls, *args, **kwargs, defaults topic-protocols

Comments

@chadrik
Copy link
Contributor

chadrik commented Aug 9, 2019

I'm getting an unexpected error when using callback protocols with kwargs

Normal callback with no args or kwargs works fine:

from typing import *

T1 = TypeVar('T1')
T2 = TypeVar('T2')

T_contra = TypeVar('T_contra', contravariant=True)
T_co = TypeVar('T_co', covariant=True)

class Callback(Protocol[T_contra, T_co]):
  def __call__(self, __element: T_contra) -> T_co:
    pass

def applyit(func: Callback[T1, T2], y: T1) -> T2:
    return func(y)

def callback(x: str) -> int:
    pass

applyit(callback, 'foo')  # OK!

callback with args works fine:

from typing import *

T1 = TypeVar('T1')
T2 = TypeVar('T2')

T_contra = TypeVar('T_contra', contravariant=True)
T_co = TypeVar('T_co', covariant=True)

class CallbackWithArgs(Protocol[T_contra, T_co]):
  def __call__(self, __element: T_contra, *args: Any) -> T_co:
    pass

def applyit_with_args(func: CallbackWithArgs[T1, T2], y: T1) -> T2:
    return func(y)

def callback_with_args(x: str, *args: Any) -> int:
    pass

applyit_with_args(callback_with_args, 'foo')  # OK!

But the same thing with kwargs produces an error:

from typing import *

T1 = TypeVar('T1')
T2 = TypeVar('T2')

T_contra = TypeVar('T_contra', contravariant=True)
T_co = TypeVar('T_co', covariant=True)

class CallbackWithKwargs(Protocol[T_contra, T_co]):
  def __call__(self, __element: T_contra, **kwargs: Any) -> T_co:
    pass

def applyit_with_kwargs(func: CallbackWithKwargs[T1, T2], y: T1) -> T2:
    return func(y)

def callback_with_kwargs(x: str, **kwargs: Any) -> int:
    pass

# Argument 1 to "applyit_with_kwargs" has incompatible type "Callable[[str, KwArg(Any)], int]"; expected "CallbackWithKwargs[str, <nothing>]"
applyit_with_kwargs(callback_with_kwargs, 'foo')  # Error!

Perhaps this is expected behavior and I'm just overlooking some subtle interplay between the args in the last case, but it seems like a bug.

mypy 0.720

@msullivan
Copy link
Collaborator

So first, in any case, the error messages around this are very unhelpful.

It seems to be that the situation is that if there are kwargs, then a positional-only argument (with __) in the protocol will only be matched if the function argument is also positional-only.

There is an argument for the correctness of that, which is that something make a call like func(x=something) would be seen viewed as passing something as part of kwargs, when it is actually being passed to the first positional argument. (Though in this particular case there would still be an error, since the positional argument is unpassed; things would be different if the positional argument was optional.)

There are a lot of tradeoffs in function subtyping, though, so it might be worth doing it differently?

@ilevkivskyi
Copy link
Member

If juts making the first argument of callback_with_kwargs() positional-only fixes the problem, I would keep this as is. But I don't have any strong opinion here.

@chadrik
Copy link
Contributor Author

chadrik commented Aug 9, 2019

If just making the first argument of callback_with_kwargs() positional-only fixes the problem, I would keep this as is. But I don't have any strong opinion here.

Are you referring to the upcoming python 3.8 feature to make an argument positional-only (/) or something else? If so, since that is a syntactical feature and thus difficult to scope within code, and it could take quite some time for 3.8 to gain widespread adoption -- especially for authors of libraries that have to maintain support for older versions of python -- is there an adjustment that can be made to favor the case that I've presented here? What would be the collateral damage of doing so?

@ilevkivskyi
Copy link
Member

Are you referring to the upcoming python 3.8 feature to make an argument positional-only (/) or something else?

No, I am referring to __x notation you are already using above.

@chadrik
Copy link
Contributor Author

chadrik commented Aug 10, 2019

No, I am referring to __x notation you are already using above.

Oh, wow, I did not realize that you could use that notation in a callable implementation. The docs only describe this in the context of a protocol, and only for the purpose of relaxing name requirements.

To wrap up, I confirmed that this works:

def callback_with_kwargs(__x: str, **kwargs: Any) -> int:
    pass

And I assume that in python 3.8, this would work:

def callback_with_kwargs(x: str, /, **kwargs: Any) -> int:
    pass

The name prefix is a bit ugly, and I'm not sure if the maintainers of the project that I'm annotating will agree that it's an acceptable trade-off, especially considering it exposes this ugliness to end users who would be providing the callbacks. My counter argument to this is that static type checking is opt-in, and early adopters are already aware that it takes a bit of adjustment to normal patterns to fully utilize it, plus there's a proper solution coming in 3.8.

Given that there's a viable solution I agree it's not worth your time to make any adjustments, unless you think it would be very easy. Documentation would be the main area of improvement here.

I'll leave this open until we figure out what to do about docs.

Thanks again!

@chadrik
Copy link
Contributor Author

chadrik commented Aug 11, 2019

Sorry, one more followup question.

How do you make a callback protocol that accepts any number of keyword args? I thought that this would work, but it doesn't:

class CallbackWithKwargs(Protocol[T_contra, T_co]):
    def __call__(self, __element: T_contra, **kwargs: Any) -> T_co:
        pass

def applyit_with_kwargs(func: CallbackWithKwargs[T1, T2], y: T1) -> T2:
    return func(y)

def callback_with_kwargs2(__x: str, arg1=None, arg2=None) -> int:
    pass

# Error : Argument 1 to "applyit_with_kwargs" has incompatible type "Callable[[str, Any], int]"; expected "CallbackWithKwargs[str, <nothing>]"
applyit_with_kwargs(callback_with_kwargs, 'foo')

Is this one of those cases where I have to make a protocol for each number of args?

class CallbackWithKwargs1(Protocol[T_contra, T_co]):
    def __call__(self, __element: T_contra, __arg1=...) -> T_co:
        pass

class CallbackWithKwargs2(Protocol[T_contra, T_co]):
    def __call__(self, __element: T_contra, __arg1=..., __arg2=...) -> T_co:
        pass

...

CallbackWithKwargs=Union[
    CallbackWithKwargs1, CallbackWithKwargs2, ...]

@ilevkivskyi
Copy link
Member

Is this one of those cases where I have to make a protocol for each number of args?

Technically yes, otherwise the override is not type-safe. We have an issue to special case *args: Any, **kwargs: Any as a synonym for "don't care", see #5876

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 29, 2020

Since the behavior is technically correct (though arguably confusing) and there is a work-around, I will close this. This can be revisited if somebody has a concrete proposal about what should be changed.

@JukkaL JukkaL closed this as completed Jan 29, 2020
@chadrik
Copy link
Contributor Author

chadrik commented Jan 29, 2020

Note that the workaround in my case is to make dozens of overloads per function with this property, so I don't think the maintainers of the project I am working with will accept it as a valid solution. I've delayed even suggesting it since we're still in the early days of adoption and I don't want them to lose confidence in type annotation based on this one wart.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 29, 2020

Okay, reopening since this this seems to have a significant impact. It still isn't clear what's the best way to fix this.

@JukkaL JukkaL reopened this Jan 29, 2020
@AlexWaygood AlexWaygood added topic-protocols topic-calls Function calls, *args, **kwargs, defaults labels Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion topic-calls Function calls, *args, **kwargs, defaults topic-protocols
Projects
None yet
Development

No branches or pull requests

5 participants