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

Fix crash with overload and callable object decorators #11630

Merged
merged 3 commits into from Nov 28, 2021

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 27, 2021

Fixes #8356, as identified by @pranavrajpal
Fixes #9112
Fixes #9967

Note that we still don't fully support the singledispatch pattern in #8356,
since we get 'overloaded function has no attribute "register"', but that's
much easier to work around than a crash.

hauntsaninja added 2 commits November 27, 2021 01:11
Fixes the crash in python#8356, as identified by @pranavrajpal

Note that we still don't support the singledispatch in that issue, since
we get 'overloaded function has no attribute "register"', but that's
much easier to work around than a crash.
@pranavrajpal
Copy link
Contributor

Do you know how hard it would be to fix #9967 as well? The underlying issue seems like the same as the other 2 issues.

I put a simplified example in #9967 (comment).

@hauntsaninja
Copy link
Collaborator Author

Thanks for pointing that one out, pushed a fix for it as well!

impl_type = inner_type
elif isinstance(inner_type, Instance):
inner_call = get_proper_type(
find_member('__call__', inner_type, inner_type, is_operator=True)
Copy link
Member

@sobolevn sobolevn Nov 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_member won't call any plugins. And might not be as accurate as analyze_member_access.

Any specific reason to use find_member here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have any specific reason, feel free to change! Just make sure that the getattr handling for the call operator matches runtime handling, e.g. with code like:

class X:
    def __getattr__(self, attr):
        if attr == "__call__":
            return lambda *a, **kw: print(a, kw)
        raise AttributeError

@X()
def f(): ...

In general, I'd love for member access to get cleaned up. I last tried a little bit in #8438 but that was swiftly reverted in #8500

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related #3832

I will send a PR in a moment 🙂

Copy link
Member

@sobolevn sobolevn Nov 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make sure that the getattr handling for the call operator matches runtime handling

Right now it does not work this way with both find_member and analyze_member_access. I will have to add this feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think find_member does the right thing when passed is_operator=True? This is what I meant: #11637 (the current code does what I'd expect)

inner_call = get_proper_type(
find_member('__call__', inner_type, inner_type, is_operator=True)
)
if isinstance(inner_call, CallableType):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more problem:

from typing import Callable, Union, Any, overload, Protocol

class DCall(Protocol):
    def __call__(self, arg: Union[int, str]) -> None:
        ...

class D:
    def __getattr__(self, attr: str) -> DCall:
        ...   # Will return `__call__` in runtime

def dec_d(f: Callable[..., Any]) -> D:
    return D()

@overload
def f_d(arg: int) -> None: ...
@overload
def f_d(arg: str) -> None: ...
@dec_d
def f_d(arg: Union[int, str]) -> None: ...

This now reports: out/ex.py:18: error: "D" not callable, because the return type of __getattr__ is not CallableType, but Instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to create a new issue out of it. Sadly, I don't have the time right now to fix it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it's late for me, so I might be missing something, but that seems to be the correct behaviour?

λ cat ex.py
from typing import Callable, Union, Any, overload, Protocol

class DCall(Protocol):
    def __call__(self, arg: Union[int, str]) -> None:
        ...

class D:
    def __getattr__(self, attr: str) -> DCall:
        if attr == "__call__":
            return lambda *a, **kw: print(a, kw)
        raise AttributeError

def dec_d(f: Callable[..., Any]) -> D:
    return D()

@overload
def f_d(arg: int) -> None: ...
@overload
def f_d(arg: str) -> None: ...
@dec_d
def f_d(arg: Union[int, str]) -> None: ...

import pytest

D().__call__(1)
with pytest.raises(TypeError):
    print(D()(1))
with pytest.raises(TypeError):
    print(f_d(1))
print("correct")

λ python3 ex.py
(1,) {}
correct

λ mypy ex.py
ex.py:20: error: "D" not callable
ex.py:27: error: "D" not callable
Found 2 errors in 1 file (checked 1 source file)

JukkaL pushed a commit that referenced this pull request Dec 3, 2021
Fixes #8481
Fixes #9941
Fixes #11025
Fixes #11038

This is the unresolved crash that's been reported the most times, e.g., it's as easy 
to repro as `mypy --no-implicit-reexport -c 'import pytorch_lightning'` (or even 
`import torch`, with the right PyTorch version). I know of multiple popular Python 
packages that have made changes just to work around this crash. I would love to 
see this released, potentially along with #11630.

Thanks to @rraval for making a minimal repro!

The fix ended up being a one-liner, but it took me a bit to track down :-)

Since things worked with implicit reexport, I differentially patched in explicit reexport 
logic to narrow things down. This let me observe that if I hardcoded pkg.a.B to get 
reexported, we hit this branch, which clobbers the PlaceholderNode for pkg.c.B, 
which fixes things: 

https://github.com/python/mypy/blob/f79e7afec2c863c34d7a9b41ebb732dc26128fff/mypy/semanal.py#L2028

Which is a little weird — we shouldn't have a PlaceholderNode for pkg.c.B at all. But 
with a breakpoint in that branch, it was easy to notice that with `--no-implicit-reexport` 
pkg.a.B was first created with `module_public=True` (resulting in creation of a 
PlaceholderNode in pkg.c.B) and only on a later pass acquired `module_public=False`. 
So tracking down where pkg.a.B symbol was first created with `module_public=True` 
led me to this "obvious" bug.
ilevkivskyi pushed a commit that referenced this pull request Dec 3, 2021
Fixes #8356, as identified by @pranavrajpal
Fixes #9112
Fixes #9967

Note that we still don't fully support the singledispatch pattern in #8356,
since we get 'overloaded function has no attribute "register"', but that's
much easier to work around than a crash.

Co-authored-by: hauntsaninja <>
ilevkivskyi pushed a commit that referenced this pull request Dec 3, 2021
Fixes #8481
Fixes #9941
Fixes #11025
Fixes #11038

This is the unresolved crash that's been reported the most times, e.g., it's as easy 
to repro as `mypy --no-implicit-reexport -c 'import pytorch_lightning'` (or even 
`import torch`, with the right PyTorch version). I know of multiple popular Python 
packages that have made changes just to work around this crash. I would love to 
see this released, potentially along with #11630.

Thanks to @rraval for making a minimal repro!

The fix ended up being a one-liner, but it took me a bit to track down :-)

Since things worked with implicit reexport, I differentially patched in explicit reexport 
logic to narrow things down. This let me observe that if I hardcoded pkg.a.B to get 
reexported, we hit this branch, which clobbers the PlaceholderNode for pkg.c.B, 
which fixes things: 

https://github.com/python/mypy/blob/f79e7afec2c863c34d7a9b41ebb732dc26128fff/mypy/semanal.py#L2028

Which is a little weird — we shouldn't have a PlaceholderNode for pkg.c.B at all. But 
with a breakpoint in that branch, it was easy to notice that with `--no-implicit-reexport` 
pkg.a.B was first created with `module_public=True` (resulting in creation of a 
PlaceholderNode in pkg.c.B) and only on a later pass acquired `module_public=False`. 
So tracking down where pkg.a.B symbol was first created with `module_public=True` 
led me to this "obvious" bug.
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
Fixes python#8356, as identified by @pranavrajpal
Fixes python#9112
Fixes python#9967

Note that we still don't fully support the singledispatch pattern in python#8356,
since we get 'overloaded function has no attribute "register"', but that's
much easier to work around than a crash.

Co-authored-by: hauntsaninja <>
tushar-deepsource pushed a commit to DeepSourceCorp/mypy that referenced this pull request Jan 20, 2022
…#11632)

Fixes python#8481
Fixes python#9941
Fixes python#11025
Fixes python#11038

This is the unresolved crash that's been reported the most times, e.g., it's as easy 
to repro as `mypy --no-implicit-reexport -c 'import pytorch_lightning'` (or even 
`import torch`, with the right PyTorch version). I know of multiple popular Python 
packages that have made changes just to work around this crash. I would love to 
see this released, potentially along with python#11630.

Thanks to @rraval for making a minimal repro!

The fix ended up being a one-liner, but it took me a bit to track down :-)

Since things worked with implicit reexport, I differentially patched in explicit reexport 
logic to narrow things down. This let me observe that if I hardcoded pkg.a.B to get 
reexported, we hit this branch, which clobbers the PlaceholderNode for pkg.c.B, 
which fixes things: 

https://github.com/python/mypy/blob/f79e7afec2c863c34d7a9b41ebb732dc26128fff/mypy/semanal.py#L2028

Which is a little weird — we shouldn't have a PlaceholderNode for pkg.c.B at all. But 
with a breakpoint in that branch, it was easy to notice that with `--no-implicit-reexport` 
pkg.a.B was first created with `module_public=True` (resulting in creation of a 
PlaceholderNode in pkg.c.B) and only on a later pass acquired `module_public=False`. 
So tracking down where pkg.a.B symbol was first created with `module_public=True` 
led me to this "obvious" bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants