Regression from 0.660 when importing from aiohttp #6364

Activity

1. msullivan commented on Feb 9, 2019

   

   msullivan

   on Feb 9, 2019

   CollaboratorAuthor

   More actions

   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]
   
2. msullivan commented on Feb 9, 2019

   

   msullivan

   on Feb 9, 2019

   CollaboratorAuthor

   More actions

   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.)

   👍React with 👍1Reacted by Bryan Forbes
3. ilevkivskyi commented on Feb 9, 2019

   

   ilevkivskyi

   on Feb 9, 2019

   Member

   More actions

   I am also leaning towards option 2.

   👍React with 👍2Reacted by Emma Smith and Bryan Forbes
4. gvanrossum commented on Feb 9, 2019

   

   gvanrossum

   on Feb 9, 2019

   Member

   More actions

   Agreed on option 2.

   👍React with 👍2Reacted by Emma Smith and Bryan Forbes
5. saulshanabrook commented on Feb 9, 2019

   

   saulshanabrook

   on Feb 9, 2019

   Contributor

   More actions

   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.
6. JelleZijlstra

   added

   topic-__all__Dealing with __all__Dealing with __all__

   on Mar 19, 2022