Mypy: Regression from 0.660 when importing from aiohttp

[msullivan]

mypy -c 'from aiohttp import ClientRequest' with aiohttp==3.4.4 fails on mypy 0.670 but passes on mypy 0.660.

Bisect shows the culprit to be https://github.com/python/mypy/pull/6256 and in particular I think to be https://github.com/python/mypy/pull/6256/files#diff-cc0809c072977e8bf33796c4b2c3a823R323.

Not totally sure what all the components involved in the issue are. PEP 561 was suspected but I now think that was a red herring. Star imports are definitely involved and I think __all__ as well

[msullivan]

OK this one is actually a pretty wild ride.

The tldr is that we (accidentally?) fixed a bug in which __all__ was treated as including every name that has ever appeared in an already processed module's __all__. This bug was allowing __all__s that were dynamically computed from their imports __all__s to appear to work, even though we can't parse them.

The minimized version of the regression is this:

[case testMultipleReexportsWithComputedAll1]
from c import *
Foo

[file c.py]
import d
from d import *
__all__ = d.__all__

[file d.py]
Foo = 10
__all__ = ['Foo']
[builtins fixtures/module_all.pyi]

This fails with a message that Foo isn't defined. The culprit turns out to be the addition of self.all_exports = []. Without that, we considered __all__ to always include anything that had been included in a __all__ in any previously processed module. That allowed the above to pass, even though we can't properly parse c's __all__.
(Though probably this is super fragile! Almost certainly there are cases where it will give an error in incremental or fine-grained modes because c is changed but d is not!)

And since this is obviously just a bug, there are cases that it allows and should not:

[case testMultipleReexportsWithComputedAll2]
from d import *
Bar  # E: Name 'Bar' is not defined

[file d.py]
import e
Foo = 10
Bar = 10
__all__ = ['Foo']

[file e.py]
Bar = 10
__all__ = ['Bar']

[builtins fixtures/module_all.pyi]

[msullivan]

So I think the possible paths forward are:

1. Say we don't support dynamic __all__s, this isn't a bug. Maybe even just give an error on dynamic __all__s so the problem shows up at its source.
2. Fail open instead of closed, and just ignore __all__s that we don't understand. This trades false negatives for false positives. I tend to lean towards this option?

It would also be possible to interpret aiohttp's dynamic __all__ computations but I think that isn't a road we want to go down.

Then the other question is whether this is worth a 0.671 release if we go with option 2.
(I kind of think that it is not by itself, but there is a dmypy windows crash report I'm about to investigate that seems likely to be grounds for a 0.671 on its own, so this is probably worth including.)

[ilevkivskyi]

I am also leaning towards option 2.

[gvanrossum]

Agreed on option 2.

[saulshanabrook]

I also created a minimal test case for this bug, because I didn't realize it had already been reported: https://github.com/saulshanabrook/minimal_mypy_import.

I verified MyPy errors on this example in 0.670 doesn't on 0.660.