-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add support for overloading coroutines. #6821
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -430,6 +430,7 @@ def visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: | |
|
|
||
| def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: | ||
| num_abstract = 0 | ||
| num_awaitable_coroutine = 0 | ||
| if not defn.items: | ||
| # In this case we have already complained about none of these being | ||
| # valid overloads. | ||
|
|
@@ -445,8 +446,22 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: | |
| self.check_func_item(fdef.func, name=fdef.func.name()) | ||
| if fdef.func.is_abstract: | ||
| num_abstract += 1 | ||
| if fdef.func.is_awaitable_coroutine: | ||
| num_awaitable_coroutine += 1 | ||
| 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) | ||
| # If items contains coroutines and check_func_item fixed their type, | ||
| # also fix the overload type. | ||
| if num_awaitable_coroutine: | ||
| assert isinstance(defn.type, Overloaded) | ||
| types = [] | ||
| for fdef, typ in zip(defn.items, defn.type.items()): | ||
| assert isinstance(fdef, Decorator) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could get a crash here if one of the overloads doesn't have
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah -- maybe we could just skip this entire block if |
||
| types.append( | ||
| self.get_awaitable_coroutine_return_type(fdef.func, typ)) | ||
| defn.type = Overloaded(types) | ||
| if defn.impl: | ||
| defn.impl.accept(self) | ||
| if defn.info: | ||
|
|
@@ -853,17 +868,7 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: Optional[str]) | |
| if defn.is_awaitable_coroutine: | ||
| # Update the return type to AwaitableGenerator. | ||
| # (This doesn't exist in typing.py, only in typing.pyi.) | ||
| t = typ.ret_type | ||
| c = defn.is_coroutine | ||
| ty = self.get_generator_yield_type(t, c) | ||
| tc = self.get_generator_receive_type(t, c) | ||
| if c: | ||
| tr = self.get_coroutine_return_type(t) | ||
| else: | ||
| tr = self.get_generator_return_type(t, c) | ||
| ret_type = self.named_generic_type('typing.AwaitableGenerator', | ||
| [ty, tc, tr, t]) | ||
| typ = typ.copy_modified(ret_type=ret_type) | ||
| typ = self.get_awaitable_coroutine_return_type(defn, typ) | ||
| defn.type = typ | ||
|
|
||
| # Push return type. | ||
|
|
@@ -963,6 +968,21 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: Optional[str]) | |
|
|
||
| self.binder = old_binder | ||
|
|
||
| def get_awaitable_coroutine_return_type(self, | ||
| defn: FuncItem, | ||
| typ: CallableType) -> CallableType: | ||
| t = typ.ret_type | ||
| c = defn.is_coroutine | ||
| ty = self.get_generator_yield_type(t, c) | ||
| tc = self.get_generator_receive_type(t, c) | ||
| if c: | ||
| tr = self.get_coroutine_return_type(t) | ||
| else: | ||
| tr = self.get_generator_return_type(t, c) | ||
| ret_type = self.named_generic_type('typing.AwaitableGenerator', | ||
| [ty, tc, tr, t]) | ||
| return typ.copy_modified(ret_type=ret_type) | ||
|
|
||
| def is_forward_op_method(self, method_name: str) -> bool: | ||
| if self.options.python_version[0] == 2 and method_name == '__div__': | ||
| return True | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5001,3 +5001,18 @@ def f(x): | |
|
|
||
| reveal_type(f(g([]))) # E: Revealed type is 'builtins.list[builtins.int]' | ||
| [builtins fixtures/list.pyi] | ||
|
|
||
| [case testTypeCheckOverloadCoroutine] | ||
| from types import coroutine | ||
| from typing import overload | ||
| @overload | ||
| @coroutine | ||
| def f(x: int) -> None: ... | ||
| @overload | ||
| @coroutine | ||
| def f(x: str) -> None: ... | ||
| def f(x): pass | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should maybe also include a check that enforces that 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 The fact that we currently don't do this for 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? |
||
|
|
||
| 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])' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add test that show that the overload actually works as expected? For example, test that
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more request for more test cases: could you try checking to see what happens when you try using |
||
| [builtins fixtures/dict.pyi] | ||
| [typing fixtures/typing-full.pyi] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test that goes through this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a basic test:
Details
and I got a massive traceback:
Details
More work is definitely needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have custom nice error messages only for basic things like
listortuplemissing in the fixture. I think this purely a fixture problem.