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

0.920: Calling function named "_" is not allowed #11774

Closed
jolaf opened this issue Dec 16, 2021 · 10 comments · Fixed by #11810
Closed

0.920: Calling function named "_" is not allowed #11774

jolaf opened this issue Dec 16, 2021 · 10 comments · Fixed by #11810
Labels
bug mypy got something wrong priority-0-high

Comments

@jolaf
Copy link
Contributor

jolaf commented Dec 16, 2021

The following code:

#from gettext import translation
#from typing import TYPE_CHECKING

#if TYPE_CHECKING:
def _(s: str) -> str: # stub for gettext string wrapper
    return s

#translation('Test').install()

print(_("test"))

produces the following output:

$ mypy test.py
test.py:10:7: error: Calling function named "_" is not allowed  [misc]
    print(_("test"))
          ^
Found 1 error in 1 file (checked 1 source file)

$ mypy --version
mypy 0.920

$ python3 --version
Python 3.8.10

$ cat /etc/issue
Ubuntu 20.04.3 LTS \n \l

$ uname -a
Linux ubuntu 5.4.0-91-generic #102-Ubuntu SMP Fri Nov 5 16:31:28 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Some lines of code in the test are commented as test would now work without proper gettext configuration.
Those lines show how a gettext-using code typically looks.

_ is a standard function name for gettext, created by translation().install(), it should be usable correctly.

If the error is really reasonable, at least there should be an option to turn it off.

@jolaf jolaf added the bug mypy got something wrong label Dec 16, 2021
@jolaf
Copy link
Contributor Author

jolaf commented Dec 16, 2021

Note: the problem didn't occur until v0.920.

@hashstat
Copy link

I'm using an _ function to attach metadata to attributes of classes (with metaclasses) and dataclasses for clarity and conciseness while providing proper type checking. While the function could be named field() or something else, it adds another name that the user must process when reading the code.

# scratch.py

from __future__ import annotations

import dataclasses
from typing import Any


def _(model_name: str | None, optional: bool = False) -> Any:
    return dataclasses.field(metadata=dict(model_name=model_name, optional=optional))


@dataclasses.dataclass
class Book:
    title: str = _('book_title')
    author: str = _('primary_author')
    publisher: str = _('publisher')
    quantity: int = _(None, True)
    price: float = _('msrp')

Running mypy on the above sample produces the following output:

$ mypy scratch.py
scratch.py:15: error: Calling function named "_" is not allowed  [misc]
scratch.py:16: error: Calling function named "_" is not allowed  [misc]
scratch.py:17: error: Calling function named "_" is not allowed  [misc]
scratch.py:18: error: Calling function named "_" is not allowed  [misc]
scratch.py:19: error: Calling function named "_" is not allowed  [misc]
Found 5 errors in 1 file (checked 1 source file)

$ python --version
Python 3.9.9

$ mypy --version
mypy 0.920

$ cat /etc/issue
Arch Linux \r (\l)

$ uname -a
Linux servo 5.15.5-arch1-1 #1 SMP PREEMPT Thu, 25 Nov 2021 22:09:33 +0000 x86_64 GNU/Linux

Now I can eliminate the error by renaming the function and then assign it to _:

# scratch.py
...
def field(model_name: str | None, optional: bool = False) -> Any:
    return dataclasses.field(metadata=dict(model_name=model_name, optional=optional))
_ = field
...
$ mypy scratch.py
Success: no issues found in 1 source file

But why should mypy prevent users from directly defining a function named _?

@theCapypara
Copy link

+1 on this issue, I read the original PR but to be honest, I don't understand why calling a function named _ is forbidden. It's great mypy now doesn't complain when redefining a function named _ but what use does this function have if now any function called _ is forbidden to be called?
This is a bug that pretty much broke type checks for my project, a setting to at least disable it would be nice.

@svenpanne
Copy link

Just for the record: 7edcead is the guilty commit which introduced the regression.

We have the same problem in our SW, we have more than 2k call sites for _, which we use as our i18n function. This totally rules out mypy 0.920 for our purposes (and the bug in typeshed for PathLike, but this is already fixed in the 0.930 branch).

I propose to simply revert the above commit, it seems fundamentally wrong to rule out some commonly used function name in an ad hoc manner. And to be honest: I don't fully understand what the commit is supposed to "fix".

@sobolevn
Copy link
Member

sobolevn commented Dec 20, 2021

I don't fully understand what the commit is supposed to "fix".

Some context: this commit was a part of our "better singledispatch" project.
When using singledispatch it is a common practice to write something as:

@some.register
def _(arg: int) -> ...

@some.register
def _(arg: str) -> ...

But, back then mypy did not allow to shadow top level definitions. So, it was decided to allow _ redefinition. I guess it is not allowed to call _, because in this case it is not clear which function you are calling (my guess).

I am +1 on allowing _ to be called, because I use it in several places, including https://github.com/dry-python/lambdas

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 20, 2021

Yes, we should allow calling _. Marking as high priority since this is a regression.

@pranavrajpal
Copy link
Contributor

it is not allowed to call _, because in this case it is not clear which function you are calling

Yeah, this is correct. Something like this:

def _(arg: str) -> int: ...

def _(arg: int) -> str: ...

_('a')

is probably a bug for the same reason that redefining a non-underscore function is a bug.

Maybe the fix for this can be allowing calling underscore functions only when they haven't being redefined. None of the examples in this PR redefine _ so we can try treating it as a regular function until we see a second definition. I'm not sure how hard this would be to implement, though.

7edcead is the guilty commit which introduced the regression.

I think the commit that introduced this was ed09f8d. The commit you're talking about added similar support for mypyc (which will probably have to be updated based on whatever the solution to this issue is).

I propose to simply revert the above commit

The problem with reverting that commit is that it would be a regression for projects that use _ as the name of a throwaway function, because it would lead to additional errors about functions named _ being redefined.

@JelleZijlstra
Copy link
Member

As a quick fix for the upcoming release, we should probably just allow calling _. The more advanced logic you cite (treating it as a regular function until a redefinition) can be implemented later.

@svenpanne
Copy link

I don't really care about the details of how this is going to be fixed, the main point is: The most straightforward use case, i.e. a plain old function/method (no redefinition, decoration, etc.) called _ should continue to work, i.e.

def _() -> None: pass
_()

should work.

What I still don't fully understand: Why should _ be treated specially under any circumstances? From the language POV it's just a name like foo or bar. Or did I miss something in the language spec? If I didn't miss anything, every special treatment of some identifier name, _ or any other, looks wrong to me.

JukkaL added a commit that referenced this issue Dec 21, 2021
Fixes #11774. The issue has relevant background information in the
discussion.

It may be worth it to eventually add back some restrictions, but we'd
only focus the restrictions on singledispatch functions. This is a
quick fix to work around a regression.
JukkaL added a commit that referenced this issue Dec 21, 2021
Fixes #11774. The issue has relevant background information in the
discussion.

It may be worth it to eventually add back some restrictions, but we'd
only focus the restrictions on singledispatch functions. This is a
quick fix to work around a regression.
ilevkivskyi pushed a commit that referenced this issue Dec 21, 2021
Fixes #11774. The issue has relevant background information in the
discussion.

It may be worth it to eventually add back some restrictions, but we'd
only focus the restrictions on singledispatch functions. This is a
quick fix to work around a regression.
JukkaL added a commit that referenced this issue Dec 22, 2021
Fixes #11774. The issue has relevant background information in the
discussion.

It may be worth it to eventually add back some restrictions, but we'd
only focus the restrictions on singledispatch functions. This is a
quick fix to work around a regression.
@jolaf
Copy link
Contributor Author

jolaf commented Dec 22, 2021

@JukkaL thanks a lot for prompt fixing!!

tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this issue Jan 20, 2022
Fixes python#11774. The issue has relevant background information in the
discussion.

It may be worth it to eventually add back some restrictions, but we'd
only focus the restrictions on singledispatch functions. This is a
quick fix to work around a regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong priority-0-high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants