Add support for overloading coroutines. #6821

aaugustin · 2019-05-11T19:02:55Z

When applying @overload to @coroutine, update the return type of the
overload to AwaitableGenerator like the underlying coroutines.

Fix #6802.

@overload

When applying @overload to @coroutine, update the return type of the overload to AwaitableGenerator like the underlying coroutines. Fix #6802.

aaugustin · 2019-05-11T19:04:53Z

Disclaimer: since mypy isn't exactly a trivial project, I don't really know what I'm doing here.

This PR aims at providing a starting point for writing a proper patch.

JelleZijlstra

This generally looks good, but I have a few comments.

JelleZijlstra · 2019-06-02T00:55:35Z

mypy/checker.py

        if num_abstract not in (0, len(defn.items)):
            self.fail(message_registry.INCONSISTENT_ABSTRACT_OVERLOAD, defn)
+        if num_awaitable_coroutine not in (0, len(defn.items)):
+            self.fail(message_registry.INCONSISTENT_COROUTINE_OVERLOAD, defn)


Could you add a test that goes through this branch?

I added a basic test:

Details

[case testOverloadedCoroutineVariantMissingDecorator1] from foo import * [file foo.pyi] from types import coroutine from typing import overload @coroutine \ # E: Overloaded method has both coroutine and non-coroutine variants @overload def f(x: int) -> int: pass @overload def f(x: str) -> str: pass [out]

and I got a massive traceback:

Details

__________________________________________________________________ testOverloadedCoroutineVariantMissingDecorator1 __________________________________________________________________ [gw4] darwin -- Python 3.7.3 /Users/myk/.virtualenvs/mypy/bin/python3.7 data: /Users/myk/dev/mypy/test-data/unit/check-overloading.test:5046: SystemExit: 2 ------------------------------------------------------------------------------- Captured stdout call -------------------------------------------------------------------------------- Traceback (most recent call last): File "<string>", line 1, in <module> File "<string>", line 8, in <module> File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 1525, in serve SlaveGateway(io=io, id=id, _startcount=2).serve() File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 1038, in serve self._execpool.integrate_as_primary_thread() File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 250, in integrate_as_primary_thread self._perform_spawn(reply) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 268, in _perform_spawn reply.run() File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 204, in run self._result = func(*args, **kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/execnet/gateway_base.py", line 1063, in executetask do_exec(co, loc) # noqa File "<string>", line 1, in do_exec File "<remote exec>", line 249, in <module> File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__ return self._hookexec(self, self.get_hookimpls(), kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 68, in _hookexec return self._inner_hookexec(hook, methods, kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 62, in <lambda> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall res = hook_impl.function(*args) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/main.py", line 242, in pytest_cmdline_main return wrap_session(config, _main) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/main.py", line 209, in wrap_session session.exitstatus = doit(config, session) or 0 File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/main.py", line 249, in _main config.hook.pytest_runtestloop(session=session) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__ return self._hookexec(self, self.get_hookimpls(), kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 68, in _hookexec return self._inner_hookexec(hook, methods, kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 62, in <lambda> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall res = hook_impl.function(*args) File "<remote exec>", line 67, in pytest_runtestloop File "<remote exec>", line 84, in run_one_test File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__ return self._hookexec(self, self.get_hookimpls(), kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 68, in _hookexec return self._inner_hookexec(hook, methods, kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 62, in <lambda> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall res = hook_impl.function(*args) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 78, in pytest_runtest_protocol runtestprotocol(item, nextitem=nextitem) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 93, in runtestprotocol reports.append(call_and_report(item, "call", log)) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 173, in call_and_report call = call_runtest_hook(item, when, **kwds) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 198, in call_runtest_hook lambda: ihook(item=item, **kwds), when=when, reraise=reraise File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 226, in from_call result = func() File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 198, in <lambda> lambda: ihook(item=item, **kwds), when=when, reraise=reraise File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__ return self._hookexec(self, self.get_hookimpls(), kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 68, in _hookexec return self._inner_hookexec(hook, methods, kwargs) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/manager.py", line 62, in <lambda> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall res = hook_impl.function(*args) File "/Users/myk/.virtualenvs/mypy/lib/python3.7/site-packages/_pytest/runner.py", line 123, in pytest_runtest_call item.runtest() File "/Users/myk/dev/mypy/mypy/test/data.py", line 223, in runtest suite.run_case(self) File "/Users/myk/dev/mypy/mypy/test/testcheck.py", line 124, in run_case self.run_case_once(testcase) File "/Users/myk/dev/mypy/mypy/test/testcheck.py", line 186, in run_case_once alt_lib_path=test_temp_dir) File "/Users/myk/dev/mypy/mypy/build.py", line 166, in build result = _build(sources, options, alt_lib_path, flush_errors, fscache, stdout, stderr) File "/Users/myk/dev/mypy/mypy/build.py", line 226, in _build graph = dispatch(sources, manager, stdout) File "/Users/myk/dev/mypy/mypy/build.py", line 2572, in dispatch process_graph(graph, manager) File "/Users/myk/dev/mypy/mypy/build.py", line 2885, in process_graph process_stale_scc(graph, scc, manager) File "/Users/myk/dev/mypy/mypy/build.py", line 2992, in process_stale_scc graph[id].type_check_first_pass() File "/Users/myk/dev/mypy/mypy/build.py", line 2101, in type_check_first_pass self.type_checker().check_first_pass() File "/Users/myk/dev/mypy/mypy/checker.py", line 285, in check_first_pass self.accept(d) File "/Users/myk/dev/mypy/mypy/checker.py", line 396, in accept stmt.accept(self) File "/Users/myk/dev/mypy/mypy/nodes.py", line 497, in accept return visitor.visit_overloaded_func_def(self) File "/Users/myk/dev/mypy/mypy/checker.py", line 429, in visit_overloaded_func_def self._visit_overloaded_func_def(defn) File "/Users/myk/dev/mypy/mypy/checker.py", line 446, in _visit_overloaded_func_def self.check_func_item(fdef.func, name=fdef.func.name()) File "/Users/myk/dev/mypy/mypy/checker.py", line 788, in check_func_item self.check_func_def(defn, typ, name) File "/Users/myk/dev/mypy/mypy/checker.py", line 871, in check_func_def typ = self.get_awaitable_coroutine_return_type(defn, typ) File "/Users/myk/dev/mypy/mypy/checker.py", line 983, in get_awaitable_coroutine_return_type [ty, tc, tr, t]) File "/Users/myk/dev/mypy/mypy/checker.py", line 3669, in named_generic_type info = self.lookup_typeinfo(name) File "/Users/myk/dev/mypy/mypy/checker.py", line 3675, in lookup_typeinfo sym = self.lookup_qualified(fullname) File "/Users/myk/dev/mypy/mypy/checker.py", line 3737, in lookup_qualified raise KeyError(msg.format(last, name)) KeyError: "Failed qualified lookup: 'AwaitableGenerator' (fullname = 'typing.AwaitableGenerator')." ------------------------------------------------------------------------------- Captured stderr call -------------------------------------------------------------------------------- tmp/foo.pyi:4: error: INTERNAL ERROR -- please report a bug at https://github.com/python/mypy/issues version: 0.710+dev.1980f6a8af1f1fd556e42dcb7cc108071b7167ac.dirty tmp/foo.pyi:4: : note: use --pdb to drop into pdb

More work is definitely needed here.

Part of this might just be an issue with the text fixtures (tests don't always get run with full typing). I thought we showed a special error message in that case though.

I thought we showed a special error message in that case though.

We have custom nice error messages only for basic things like list or tuple missing in the fixture. I think this purely a fixture problem.

JelleZijlstra · 2019-06-02T00:56:00Z

mypy/checker.py

+            assert isinstance(defn.type, Overloaded)
+            types = []
+            for fdef, typ in zip(defn.items, defn.type.items()):
+                assert isinstance(fdef, Decorator)


I wonder if we could get a crash here if one of the overloads doesn't have @coroutine.

Yeah -- maybe we could just skip this entire block if @coroutine is applied inconsistently? It's probably not worth trying to infer more precise types once we know the decorators are wonky.

JelleZijlstra · 2019-06-02T00:56:48Z

test-data/unit/check-overloading.test

+def f(x: str) -> None: ...
+def f(x): pass
+
+reveal_type(f) # E: Revealed type is 'Overload(def (x: builtins.int) -> typing.AwaitableGenerator[Any, Any, Any, None], def (x: builtins.str) -> typing.AwaitableGenerator[Any, Any, Any, None])'


Could you add test that show that the overload actually works as expected? For example, test that await f(1) works, but await f([]) does not.

One more request for more test cases: could you try checking to see what happens when you try using @abstractmethod with @coroutine? (E.g. make sure we still get the correct types, make sure the result makes sense no matter which decorator you apply first)

JelleZijlstra · 2019-06-02T00:57:19Z

Also, there is a merge conflict.

Michael0x2a

The overall idea looks sound to me. It's a little ad-hoc, but I think that's fine: decorators in general are pretty ad-hoc in mypy (and overloads + decorators doubly so).

The main change I'd like to see is a one or two more test cases (in addition to the suggestions Jelle had) -- I think we can land this once those changes are in (and the merge conflict fixed).

Michael0x2a · 2019-06-02T20:04:00Z

test-data/unit/check-overloading.test

+@overload
+@coroutine
+def f(x: str) -> None: ...
+def f(x): pass


I wonder if we should maybe also include a check that enforces that @coroutine is included in the implementation as well? Or more generally, mandate that decorators used consistently across all overload signatures + the implementation.

Normally, we don't check whether the implementation signature actually matches the given overload variants because it's too difficult to do so. But it might be worth the extra effort to make sure at least decorators in general are applied consistently: it's not as hard to check, and not including the @coroutine decorator on the implementation can result in some potentially confusing errors at runtime.

The fact that we currently don't do this for @abstractmethod is an oversight, I think -- it doesn't really make a major difference at runtime whether f actually is abstract or not (especially since mypy's abstract checks largely supersede the same ones done at runtime by the abc module), so probably nobody bothered reporting this.

This is probably out-of-scope for this PR though. Up to you if you want to try tackling this or not -- but if you don't have time, could you file an issue so we can keep track of this?

Michael0x2a · 2019-06-02T20:34:23Z

mypy/checker.py

+            assert isinstance(defn.type, Overloaded)
+            types = []
+            for fdef, typ in zip(defn.items, defn.type.items()):
+                assert isinstance(fdef, Decorator)


Yeah -- maybe we could just skip this entire block if @coroutine is applied inconsistently? It's probably not worth trying to infer more precise types once we know the decorators are wonky.

Michael0x2a · 2019-06-02T20:36:09Z

test-data/unit/check-overloading.test

+def f(x: str) -> None: ...
+def f(x): pass
+
+reveal_type(f) # E: Revealed type is 'Overload(def (x: builtins.int) -> typing.AwaitableGenerator[Any, Any, Any, None], def (x: builtins.str) -> typing.AwaitableGenerator[Any, Any, Any, None])'


One more request for more test cases: could you try checking to see what happens when you try using @abstractmethod with @coroutine? (E.g. make sure we still get the correct types, make sure the result makes sense no matter which decorator you apply first)

aaugustin · 2019-06-03T07:46:31Z

Thanks for the review! I'll update the PR.

aaugustin · 2019-10-05T13:48:10Z

This PR attempted to fix a bug that no longer exists in mypy 0.730.

Add support for overloading coroutines.

a8fbc10

When applying @overload to @coroutine, update the return type of the overload to AwaitableGenerator like the underlying coroutines. Fix #6802.

aaugustin added 3 commits May 11, 2019 21:10

Fix typing.

75bf1a1

Use the same imports as other tests.

f780fe6

Fix refactoring error.

9a3790e

ilevkivskyi mentioned this pull request Jun 1, 2019

False positive on await for AbstractEventLoop.create_connection (regression in 0.700) #6909

Closed

ilevkivskyi requested a review from Michael0x2a June 1, 2019 23:44

JelleZijlstra requested changes Jun 2, 2019

View reviewed changes

Michael0x2a requested changes Jun 2, 2019

View reviewed changes

aaugustin closed this Oct 5, 2019

Uh oh!

Add support for overloading coroutines. #6821

Add support for overloading coroutines. #6821

Uh oh!

Conversation

aaugustin commented May 11, 2019

Uh oh!

aaugustin commented May 11, 2019

Uh oh!

JelleZijlstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra commented Jun 2, 2019

Uh oh!

Michael0x2a left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaugustin commented Jun 3, 2019

Uh oh!

aaugustin commented Oct 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants