-
Notifications
You must be signed in to change notification settings - Fork 237
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
Should we adopt mypy's @overload? #14
Comments
For example, consider the signature of @overload
def __init__(self) -> None: pass
@overload
def __init__(self, o: object) -> None: pass
@overload
def __init__(self, o: _byte_types, encoding: str = None, errors: str = 'strict') -> None: pass Union types are not sufficient to represent the fact that
@overload
def __getitem__(self, i: int) -> int: pass
@overload
def __getitem__(self, s: slice) -> bytes: pass Again, union types (or type variables) are not sufficient.
@overload
def map(func: Function[[_T1], _S], iter1: Iterable[_T1]) -> Iterator[_S]: pass
@overload
def map(func: Function[[_T1, _T2], _S], iter1: Iterable[_T1],
iter2: Iterable[_T2]) -> Iterator[_S]: pass
# ... and we could add more items to support more than two iterables I think we need at least a stub-level feature for representing the above cases. A potential approach is to use multiple signatures for a function in a stub file, without a decorator. For example: def __getitem__(self, i: int) -> int: pass
def __getitem__(self, s: slice) -> bytes: pass |
The examples that @JukkaL listed here are one of the reasons pytypedecl went with a separate file for type definitions. In general I saw some minor interest in having multiple dispatch implemented in functools after we introduced singledispatch. I'd be happy to work on that separate PEP as a follow-up. Can we leave it out for now to not risk introducing a confusing feature that's impossible to remove later? |
We need something for stubs; there are a fair number of stdlib things Okay to disallow it for non-stub files and doing a separate PEP for multi-dispatch. |
Closing .The decision is made. We'll support |
Summary: The typechecker needs to ignore the existence of a signature if that signature is overloaded with typing.overload decorated defines. In other words, when selecting signatures, the overloaded define (containing the actual implementation of the function) should never be selected. typing.overload designates a definition that is purely for the typechecker's benefit, and is ignored at runtime because it is immediately overwritten with a definition that contains the actual implementation. Calling a function decorated with typing.overload throws at runtime: ``` >>> import typing >>> typing.overload ... def foo(x): ... return 1 ... >>> foo(1) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/local/fbcode/gcc-5-glibc-2.23/lib/python3.6/typing.py", line 1591, in _overload_dummy "You should not call an overloaded function. " NotImplementedError: You should not call an overloaded function. A series of The controller you requested could not be found. functions outside a stub module should always be followed by an implementation that is not The controller you requested could not be found.. ``` Therefore, the purpose of typing.overload defines is simply to clarify the type signature when the return annotation should vary depending on the parameter annotations (but maybe not in a way specifiable with type vars): ``` typing.overload def foo(x: int) -> str: ... typing.overload def foo(x:str) -> int:... def foo(x: Union[int, str]) -> Union[int, str] ``` Currently, when pyre sees the above set of signatures and then tries to resolve a call to `foo(x)` where `x` is an `int`, we look at all overloads equally and can select the `Union[int, str] -> Union[int, str]` signature and end up assuming the return type of `foo(x)` is `Union[int, str]` rather than the correct and more specific `str`. If `typing.overload` definitions exist, we should be selecting only from those, and not from the implementation signature. **NodeAPI Bug (T35334236)** ``` overload staticmethod def get(roots): # type: (TRoot) -> NodeGetCity[NodeCity] pass overload # noqa staticmethod def get(roots): # type: (TRoots) -> NodeGetCity[List[NodeCity]] pass staticmethod # noqa def get(roots): # type: (Union[TRoot, TRoots]) -> NodeGetCity """ Get the node object/s corresponding to the roots. param roots: Node|fbid, or iterable<Node|fbid>. return A Query object that will return a Node if a Node or NodeId was passed as roots otherwise will return a list of Nodes. """ return NodeGetCity(roots, nodeapi_ext.NodeErrorControllerType.ENFORCE) ``` Pyre selects the third method, which has a return type of NodeGetCity with an unknown type parameter (which inherits from awaitable), so awaiting on this returns an unknown type. Pyre *should* realize that the overloaded methods are there to refine the type signature of get depending on the input type. Merging with a pre-existing task to handle overloads this way. **More context:** Pep: https://docs.python.org/3/library/typing.html#typing.overload Discussion: python/typing#253 python/mypy#3160 python/typing#14 **Next steps - not urgent (T35517446):** 1. When typechecking a define, if it is decorated with an typing.overload and there does not exist a matching definition *not* decorated with typing.overload. (overload functions throw when called at runtime) 2. When typechecking a define, if there exist other same name defines decorated with typing.overload, throw if overloading functions do not comprehensively cover the type signature of this overloaded function Reviewed By: dkgi Differential Revision: D10494018 fbshipit-source-id: 48973008d94cc539198ec13552b8108412413f2a
In mypy there's an @overload decorator. However, it was introduced before Union, and the latter is usually sufficient (or using type variables).
Is it still needed? If we need it, should it be part of this PEP or should it be a separate proposal for a runtime feature to be added to functools (like @singledispatch)? But if separate, how would it interact with generic types?
The text was updated successfully, but these errors were encountered: