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

Incompatible singledispatch signature not detected #11734

Open
JukkaL opened this issue Dec 14, 2021 · 2 comments
Open

Incompatible singledispatch signature not detected #11734

JukkaL opened this issue Dec 14, 2021 · 2 comments
Labels
bug mypy got something wrong

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 14, 2021

This generates no error from mypy but fails at runtime:

from functools import singledispatch

@singledispatch
def f(x: object) -> int:
    return 1

@f.register
def _(x: int, y: str) -> int:
    return 2

print(f(1))  # No error reported, but fails at runtime

Another example:

from functools import singledispatch

@singledispatch
def f(x: object, y: str) -> None:
    y + ''

@f.register
def _(x: int, y: int) -> None:
    y + 1

print(f(1, 'x'))  # No error reported, but fails at runtime

Third example:

from functools import singledispatch

@singledispatch
def f(x: object, y: str) -> None:
    pass

@f.register
def _(x: int, yy: str) -> None:
    pass

print(f(1, y='x'))  # No error reported, but fails at runtime

We should check that the signatures of registered items are compatible with the generic one, except for the first argument.

@JukkaL JukkaL added the bug mypy got something wrong label Dec 14, 2021
@pranavrajpal
Copy link
Contributor

Example 1 should be relatively easy to support, but examples 2 and 3 have some subtlety involved.

For example 2, I don't know if we should allow arguments of registered implementations to be subtypes of the corresponding main singledispatch function's arguments, or whether we should require that they be the exact same types.

Example:

from functools import singledispatch

class A: pass
class B(A): pass

@singledispatch
def f(x: object, y: A) -> None:
    y + ''

@f.register
def g(x: int, y: B) -> None:
    y + 1

Allowing subtypes for the arguments could allow directly calling g to correctly error if you have something like g(5, A()), which wouldn't have been caught if the second parameter had been required to be A.

On the other hand, allowing subtypes could mask errors when calling f because we ignore the registered functions entirely when type checking calls to f. That means that something like f(5, A()) would be an error but wouldn't be caught when type checking. Requiring that the type of y be A in g would at least make it more obvious that mypy won't be able to help in that case and that g should handle the case that y isn't an instance of B but is an instance of A.

Example 3 seems like a singledispatch version of #6709, because I wouldn't be surprised if there is a lot of code that uses different argument names between implementations and then just never uses keyword arguments in calls.

I still think it's worth trying to show an error for, but we might have to abandon it if there are too many false positives. Alternatively, we could try putting this error behind a flag that gets enabled by --strict similar to what #6709 (comment) suggests.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Dec 15, 2021

I don't know if we should allow arguments of registered implementations to be subtypes of the corresponding main singledispatch function's arguments, or whether we should require that they be the exact same types.

I think that they should be allowed to vary contravariantly (i.e. registered implementations can use a more general type, but not a narrower subtype). This would be consistent with how method overriding is type checked.

Example 3 seems like a singledispatch version of #6709, because I wouldn't be surprised if there is a lot of code that uses different argument names between implementations and then just never uses keyword arguments in calls.

Yeah, it would be reasonable to not require argument names to be consistent, since we don't check them in other contexts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

2 participants