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

Create a plugin to handle singledispatch #2904

Closed
gbritton1 opened this issue Feb 24, 2017 · 28 comments
Closed

Create a plugin to handle singledispatch #2904

gbritton1 opened this issue Feb 24, 2017 · 28 comments
Labels
false-positive mypy gave an error on correct code feature priority-1-normal

Comments

@gbritton1
Copy link

gbritton1 commented Feb 24, 2017

For me, this code:

from functools import singledispatch
@singledispatch
def cmp(other, match):
    return match in (..., other)

@cmp.register(type)
def _(other, match):
    return isinstance(match, other)`

results in the mypy error:

severity: 'Error'
message: 'error:'overload' decorator expected'

on the line @cmp.register(type)

mypy version:

PS C:\Program Files (x86)\Python36-32> .\scripts\mypy.bat --version
mypy 0.471
PS C:\Program Files (x86)\Python36-32>

What should I do to resolve the error?

@ambv
Copy link
Contributor

ambv commented Feb 26, 2017

I cannot reproduce your error. I installed mypy 0.471, copied your example code and ran it against mypy, no errors. Can you provide an exact minimal example where running against mypy 0.471 produces the output you're seeing?

I tried to no avail with various combinations of options:

mypy --warn-incomplete-stub --warn-redundant-casts --warn-no-return --warn-unused-ignores --show-error-context --strict-optional --follow-imports=normal --python=3.6 --fast-parser

@gbritton1
Copy link
Author

I attached a file with the exact code
testmypyerror.zip

also, here's the console log from the mypy run:

D:\Python>"C:\Program Files (x86)\Python36-32\Scripts\mypy.bat" d:\python\testmypyerror.py
d:\python\testmypyerror.py:8: error: 'overload' decorator expected
d:\python\testmypyerror.py:12: error: 'overload' decorator expected

D:\Python>

@gbritton1
Copy link
Author

FWIW:
D:\Python>"C:\Program Files (x86)\Python36-32\Scripts\mypy.bat" --version
mypy 0.471

@miedzinski
Copy link
Contributor

Mypy will produce this error if you have more than one named function _. In code sample attached to PR there is only one _, so mypy is fine with this. @gbritton1 is this correct?

@gbritton1
Copy link
Author

In the code sample, there are actually two functions named _. But those names are not ever used. Since the functions are decorated using the singledispatch decorators, those names are merely placeholders.

mypy is not fine with this! That's why I filed this PR. I get the errors I posted above. Note, if I rename the second function to e.g, "_42" the error goes away.

Perhaps this is outside the scope of what mypy can do? I mean, how is mypy to know that the decorator actually results in no function named "_" to be created?

OTOH the error messages are misleading, at least to me.

FWIW I simply followed the docs at https://docs.python.org/3/library/functools.html which give the example:

@singledispatch
def fun(arg, verbose=False):
    if verbose:
        print("Let me just say,", end=" ")
    print(arg)
@fun.register(int)
def _(arg, verbose=False):
    if verbose:
        print("Strength in numbers, eh?", end=" ")
    print(arg)
@fun.register(list)
def _(arg, verbose=False):
    if verbose:
        print("Enumerate this:")
    for i, elem in enumerate(arg):
        print(i, elem)

which also yields the same mypy error messages

@miedzinski
Copy link
Contributor

miedzinski commented Feb 28, 2017

Perhaps this is outside the scope of what mypy can do? I mean, how is mypy to know that the decorator actually results in no function named "_" to be created?

_ actually is created (and redefined if you declare it more than once) - try using it. singledispatch registers decorated function, but doesn't somehow hide it from scope. It is still there. I agree that the error message could be better though.

This is related to #1332. Until this gets "fixed", you can define functions with names like _int, _list. Depending on your use case you will probably need @overload as well.

@gbritton1
Copy link
Author

gbritton1 commented Feb 28, 2017

ah I see, now, you are quite correct. FWIW singledispatch could throw away the "_" function, I think that is, if I add

del _

then

print(fun(42))

it all works just fine.

Perhaps i should file a bug report against singledispatch. My gut tells me it should delete "_"

In fact, following your suggestion (_int, _list, etc), the namespace gets polluted with these functions

@miedzinski
Copy link
Contributor

You shouldn't add del statements and certainly singledispatch won't do neither. Names prefixed with _ aren't exported anyway. Just use different names. I wouldn't be surprised if mypy despite allowing redefining _ variables would still prohibit such functions without @overload.

@gbritton1
Copy link
Author

I added del to prove the concept. but what is your reasoning for not insisting singledispatch clean up after itself? IOW what is the value of the decorated functions with their original names? Especially when you follow the docs and use the same name for each one, so that at the end only the last one is actually defined?

I realize this is now OT. Apologies! Just trying to learn.

@miedzinski
Copy link
Contributor

Because one may want to use both @singledispatch function and the exact implementation. It wouldn't be possible if singledispatch silently deleted your functions. If you really insist on "cleaning up", then del these functions yourself, but I don't understand why you would want that. Prefix them with _ and they won't be exported.

And yes, mypy issue tracker isn't the best place to discuss stuff like that, so let's end this. :)

@gvanrossum
Copy link
Member

OK, so the full example (copied from the zip) is:

from functools import singledispatch
from typing import Sequence

@singledispatch
def cmp(other, match):
    return other in (..., match)

@cmp.register(type)
def _(other, match):
    return isinstance(match, other)

@cmp.register(Sequence)
def _(other, match):
    return all(cmp(m, o) for m, o in zip(match, other))

The reason mypy complains about this is the duplicate use of _. The reason is that to mypy, there is nothing special about @cmp.register it's just some decorator. I can reproduce the same behavior simpler:

def deco(f): return f
@deco
def foo(a): return 0
@deco
def foo(a): return 1

TBH I think the error message could be clearer. The error is much clearer when the redefined function is undecorated, e.g. this gives b.py:2: error: Name 'foo' already defined:

def foo(a): return 0
def foo(a): return 1

But either way mypy is just complaining about a redefinition, I'm not sure how we should address this. Should we special-case single-dispatch? Or should we special-case _?

@ilevkivskyi
Copy link
Member

I think _ should be special-cased. It is often used as a "throw away" variable.

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 3, 2017

Special casing _ in assignment statements has an existing issue: #465

@ethanhs
Copy link
Collaborator

ethanhs commented May 27, 2018

Hm, the current rules for special casing _ doesn't really solve this issue. I propose that we special case def _():... so that we don't report name already defined, since that is the idiomatic usage of singledispatch. I'm not familiar with any other idioms that use this. I looked at cpython and the usage is always for throw away defs, or singledispatch.

@ilevkivskyi
Copy link
Member

Another use case for functions with is name _ is i18n. So we should not blindly give _ an Any type. We can try suppressing the error, but it can fire at the call site. An ideal solution may be to write a plugin producing a reasonable signature (I think plugin API will need to be extended for this). Or we can special-case also @singledispatch to keep the first signature, which is typically the most wide one.

A possible temporary workaround could be using if typing.TYPE_CHECKING.

@ethanhs
Copy link
Collaborator

ethanhs commented May 29, 2018

So we should not blindly give _ an Any type.

I agree. A plugin seems the best way forward.

@ethanhs ethanhs added feature priority-1-normal false-positive mypy gave an error on correct code labels Nov 7, 2018
@ethanhs ethanhs changed the title singledispatch producing mypy error Create a plugin to handle singledispatch Nov 7, 2018
@ethanhs
Copy link
Collaborator

ethanhs commented Nov 7, 2018

While the original issue was a bug report, I am marking this a feature issue for adding a plugin, since that seems to be the current best method of dealing with singledispatch.

@T-Skinner
Copy link

I'm curious what the update on this is. Was a plugin ever made?

@JukkaL
Copy link
Collaborator

JukkaL commented Feb 4, 2020

There has no progress with a plugin, as far as I know.

@sitnarf
Copy link

sitnarf commented Nov 26, 2020

I currently use # type: ignore[no-redef]:

@json_serialize_types.register(int)  # type: ignore[no-redef]
@json_serialize_types.register(float)
def _(value):
    return value

@simon-liebehenschel
Copy link

I currently use # type: ignore[no-redef]:

@json_serialize_types.register(int)  # type: ignore[no-redef]
@json_serialize_types.register(float)
def _(value):
    return value

Wow! So we can specify multiple decorators when we need multiple types. Your code example must go to the official documentation.

@graingert
Copy link
Contributor

It would be useful to handle singledispatchmethod also

from __future__ import annotations

import functools

class Foo:
    @functools.singledispatchmethod
    def __getitem__(self, item: int) -> int:
        return 10

    @__getitem__.register
    def _(self, item: slice) -> list[int]:
        return [10, 20, 30]

See https://twitter.com/raymondh/status/1433555551133343746?s=19

@oscarbenjamin
Copy link

I think that a decorated def _() should be treated as a throwaway name by mypy if the bound name _ is not actually being used anywhere. That is of course the reason the name is _. It seems that flake8 can now special case def _() but mypy still complains.

I have once again needed to add hundreds of type: ignore to sympy because of this issue in relation to sympy's own internal multipledispatch module: sympy/sympy@ff0044c

@pranavrajpal
Copy link
Contributor

@oscarbenjamin That should be fixed on master by #10811 (and the mypy_primer run on that PR does show a lot of Name "_" already defined errors disappearing for sympy).

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Oct 19, 2021

This issue has mostly talked about the redefinition error. But, we should also have singledispatch typing working. e.g. With this example:

@singledispatch
def foo(data: object) -> NoReturn:
    raise ValueError(f"Unhandled type {type(data)}")
@foo.register
def _(data: str) -> str:
    return data
@foo.register
def _(data: int) -> str:
    return str(data)

a = foo(7)
reveal_type(a)

You also get errors due to not recognising the overloads:

src/service/transaction_status_service.py:72: error: Need type annotation for "a" [var-annotated]
src/service/transaction_status_service.py:73: note: Revealed type is "<nothing>"

It would be good for mypy to treat the @foo.register like an @overload.

I was also getting unreachable code errors after every call to the defined function. However, I was unable to reproduce it with a minimal example, so not sure exactly what is causing that.

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 13, 2021

Since we now have basic support for singledispatch, let's file any further issues or limitations related to singledispatch as separate issues and close this issue. Feel free to copy comments from this issue that are still unresolved into new issues.

@JukkaL JukkaL closed this as completed Dec 13, 2021
@NeilGirdhar
Copy link
Contributor

@JukkaL Would you happen to know which issue interested observers can follow for this problem? #2904 (comment)

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 13, 2021

@NeilGirdhar I created #11727 to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
false-positive mypy gave an error on correct code feature priority-1-normal
Projects
None yet
Development

No branches or pull requests