Support callback protocols #5463

ilevkivskyi · 2018-08-12T23:25:48Z

The issue proposed an interesting idea. Allow callables as subtypes of protocols with __call__. IMO this is not just reasonable and type safe, but is also more clear that the extended callable syntax in mypy_extensions. For example:

class Combiner(Protocol):
    def __call__(self, *vals: bytes, maxlen: Optional[int] = None) -> List[bytes]: ...

def batch_proc(data: Iterable[bytes], cb_results: Combiner) -> bytes:
    ...

The callback protocols:

Have cleaner familiar syntax in contrast to Callable[[VarArg(bytes), DefaultNamedArg(Optional[int], 'maxlen')], List[bytes]] (compare to above)
Allow to be more explicit/flexible about binding of type variables in generic callbacks (see tests for examples)
Support overloaded callbacks (this is simply impossible with the current extended callable syntax)
Are easy to implement

If this will get some traction, I would actually propose to deprecate extended callable syntax in favor of callback protocols.

emmatyping · 2018-08-13T00:55:52Z

If this will get some traction, I would actually propose to deprecate extended callable syntax in favor of callback protocols.

I really like the new syntax, and think we should go ahead with your proposal.

msullivan · 2018-08-13T06:08:32Z

One snag to replacing extended callables is that protocols don't seem to enforce argument name equivalence.

ilevkivskyi · 2018-08-13T09:09:53Z

@msullivan Could you please clarify what do you mean? I think they behave exactly the same way as "normal" callables (including the dunder argument name convention), see the test I added.

msullivan · 2018-08-13T18:45:17Z

I mean that mypy will reject the following:

def foo(f: Callable[[NamedArg(int, 'x')], None]) -> None:
    f(x = 10)

def bar(y: int) -> None:
    pass

foo(bar)

while the protocol version won't.

msullivan · 2018-08-13T18:45:31Z

Oh which maybe that is a bug in protocols more generally?

msullivan

My concerns about whether this as-is can be a drop in replacement for extended callables aside, this seems good to me.

ilevkivskyi · 2018-08-13T19:31:10Z

@msullivan maybe I am missing something, but isn't testCallableImplementsProtocolArgName exactly testing what you wrote?

ilevkivskyi · 2018-08-13T19:32:53Z

(the error message is cryptic in this case, because arg names are not shown, but we already have a high priority issue about this)

msullivan · 2018-08-13T20:24:06Z

Oh, oops, nevermind.

msullivan · 2018-08-13T20:31:18Z

Ah, my confusion was caused by testing that I did using regular protocols, where it doesn't seem to check argument names:

from typing import Protocol

class Caller(Protocol):
    def foo(self, x: str) -> None: ...

def func(caller: Caller) -> None:
    caller.foo(x='lol')

class Foo:
    def foo(self, y: str) -> None: ...

func(Foo())

ilevkivskyi · 2018-08-13T20:40:17Z

Yes, and this is intentional, see this code in is_protocol_implementation:

                # Nominal check currently ignores arg names
                is_compat = is_subtype(subtype, supertype, ignore_pos_arg_names=True)

the motivation for this is probably very old, I just wanted structural to match nominal as much as possible.

JukkaL

I liked this approach for representing extended callables! This also needs to be documented (it can happen in a follow-up PR). If some of the suggestions below are non-trivial to implement, they can also be implemented separately.

JukkaL · 2018-08-15T16:16:06Z

test-data/unit/check-protocols.test

+    pass
+
+func(call)
+func(bad)  # E: Argument 1 to "func" has incompatible type "Callable[[int, VarArg(str)], None]"; expected "Caller"


Once we deprecate the old syntax, we should come up with a different syntax for "rich" callables in error messages as well.

The format is already there, MessageBuilder.pretty_callable, but we need to update loads of test cases.

JukkaL · 2018-08-15T16:20:54Z

test-data/unit/check-protocols.test

+def func(caller: Caller[T, S]) -> Tuple[T, S]:
+    pass
+
+reveal_type(func(call))  # E: Revealed type is 'Tuple[builtins.int*, builtins.str*]'


Test also passing a generic function when a function accepts a non-generic protocol-based callable (where the generic function is valid).

OK, will add this test.

JukkaL · 2018-08-15T16:22:32Z

test-data/unit/check-protocols.test

+func(call)
+func(bad)  # E: Argument 1 to "func" has incompatible type "Callable[[str], None]"; expected "Caller"
+anon(bad)
+[out]


Idea for test cases:

Pass protocol callable X when function accepts protocol callable Y (both compatible and incompatible cases). Should these match argument names precisely or not?

Pass protocol callable when function accepts a normal callable. Should this be okay?

The second is already supported and has test cases, it was part of the original protocols PR. For the first however, the argument names are not checked, but I think they should be (even for nominal), because __call__ is special, unlike for __add__ arg names are "visible".

JukkaL · 2018-08-15T16:23:34Z

mypy/subtypes.py

@@ -214,6 +214,13 @@ def visit_callable_type(self, left: CallableType) -> bool:
                                  ignore_pos_arg_names=self.ignore_pos_arg_names)
                       for item in right.items())
        elif isinstance(right, Instance):
+            if right.type.is_protocol and right.type.protocol_members == ['__call__']:


Maybe we should also support mixing protocol callables and normal callables in joins and meets?

Hm, I will try this.

JukkaL · 2018-08-15T16:28:08Z

Also, there is a merge conflict.

ilevkivskyi · 2018-08-16T14:09:41Z

@JukkaL Thanks for review! Could you please take a look again at this?

JukkaL

Thanks for the updates! Left various minor comments.

JukkaL · 2018-08-16T15:24:31Z

mypy/checker.py

+        # __call__ is special among dunders, because arguments can be passed
+        # as keyword args (unlike other dunders).
+        ignore_names = name != '__call__'
+        if not is_subtype(override, original, ignore_pos_arg_names=ignore_names):


Hmm, this could break some existing code which assumes that __call__ won't be called with keyword arguments. Maybe there is a way to restrict this to only protocol classes, where __call__ is special among dunder methods?

JukkaL · 2018-08-16T15:26:59Z

test-data/unit/check-classes.test

+class Base:
+    def __call__(self, arg: int) -> None: ...
+class Sub(Base):
+    def __call__(self, zzz: int) -> None: ...  # E: Signature of "__call__" incompatible with supertype "Base"


As discussed above, this might break existing code, and the override doesn't seem special since non-protocols aren't compatible with callable types.

JukkaL · 2018-08-16T15:28:29Z

test-data/unit/check-protocols.test

+a: Call
+b: Normal
+
+reveal_type([a, b])  # E: Revealed type is 'builtins.list[def (__main__.B) -> __main__.B]'


Try also [b, a]?

JukkaL · 2018-08-16T15:28:46Z

test-data/unit/check-protocols.test

+def a(x: Call) -> None: ...
+def b(x: Normal) -> None: ...
+
+reveal_type([a, b])  # E: Revealed type is 'builtins.list[def (x: def (__main__.B) -> __main__.B)]'


Try also [b, a]?

JukkaL · 2018-08-16T15:30:31Z

docs/source/protocols.rst

+Callback protocols
+******************
+
+Protocols can be used to define flexible callback types that are impossible


Nit: they are currently possible with the legacy extended callable syntax.

Some of them are really impossible, like overloaded callbacks. I will try to clarify here.

JukkaL · 2018-08-16T15:31:27Z

docs/source/protocols.rst

+******************
+
+Protocols can be used to define flexible callback types that are impossible
+to express using the ``Callable[...]`` syntax, for example variadic, overloaded,


Style nit: ", for example" -> ", such as" would perhaps read a bit better

JukkaL · 2018-08-16T15:33:50Z

docs/source/protocols.rst

+                            # different name and kind in the callback
+
+Callback protocols and ``Callable[...]`` types can be used interchangeably.
+To indicate an "anonymous" positional-only argument use double underscore,


Maybe explicitly mention that keyword argument names in __call__ methods must be identical, unless a double underscore prefix is used.

ilevkivskyi · 2018-08-16T18:29:12Z

Just not to wait for @JukkaL to return from vacation I am merging this now. All major points have been addressed, if anything minor left, we can fix it later.

Ivan Levkivskyi added 2 commits August 12, 2018 23:49

Support callback protocols

9247a65

Add an extra note for bad assignments

c37e333

ilevkivskyi requested a review from JukkaL August 12, 2018 23:25

Test arg names

29e71bf

msullivan approved these changes Aug 13, 2018

View reviewed changes

JukkaL reviewed Aug 15, 2018

View reviewed changes

ilevkivskyi and others added 4 commits August 15, 2018 23:18

Merge branch 'master' into add-proto-call

1b2c98e

Address CR

192cb31

Fix self-check

2822b48

Add docs

7939613

ilevkivskyi mentioned this pull request Aug 16, 2018

Use MessageBuilder.pretty_callable in more places #5490

Open

JukkaL reviewed Aug 16, 2018

View reviewed changes

Ivan Levkivskyi added 3 commits August 16, 2018 18:06

Address some comments

8ebcf90

Skip arg name check in nominal case; only keep it for structural

7ef0e19

Remove test

9219cc0

ilevkivskyi merged commit 5c73e6a into python:master Aug 16, 2018

ilevkivskyi deleted the add-proto-call branch August 16, 2018 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support callback protocols #5463

Support callback protocols #5463

ilevkivskyi commented Aug 12, 2018 •

edited

Loading

emmatyping commented Aug 13, 2018

msullivan commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan left a comment

ilevkivskyi commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

JukkaL left a comment

JukkaL Aug 15, 2018

ilevkivskyi Aug 15, 2018

JukkaL Aug 15, 2018

ilevkivskyi Aug 15, 2018

JukkaL Aug 15, 2018

ilevkivskyi Aug 15, 2018

JukkaL Aug 15, 2018

ilevkivskyi Aug 15, 2018

JukkaL commented Aug 15, 2018

ilevkivskyi commented Aug 16, 2018

JukkaL left a comment

JukkaL Aug 16, 2018

JukkaL Aug 16, 2018

JukkaL Aug 16, 2018

JukkaL Aug 16, 2018

JukkaL Aug 16, 2018

ilevkivskyi Aug 16, 2018

JukkaL Aug 16, 2018

JukkaL Aug 16, 2018

ilevkivskyi commented Aug 16, 2018

Support callback protocols #5463

Support callback protocols #5463

Conversation

ilevkivskyi commented Aug 12, 2018 • edited Loading

emmatyping commented Aug 13, 2018

msullivan commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

msullivan commented Aug 13, 2018

msullivan commented Aug 13, 2018

ilevkivskyi commented Aug 13, 2018

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL commented Aug 15, 2018

ilevkivskyi commented Aug 16, 2018

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Aug 16, 2018

ilevkivskyi commented Aug 12, 2018 •

edited

Loading