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 involving explicit reexport, import cycle, wildcard #11632

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 28, 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:

self.add_imported_symbol(name, node, i,

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.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thank you for tracking this down! This was a tricky issue and was clearly causing a lot of pain for users. Hopefully we can include this in the next mypy release, which should go out soon.

@JukkaL JukkaL merged commit cee5d3e into python:master Dec 3, 2021
@JukkaL JukkaL mentioned this pull request Dec 3, 2021
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.
@hauntsaninja hauntsaninja deleted the fixbigcrash branch December 3, 2021 21:15
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
2 participants