Fix variables in __all__ being flagged by --no-implicit-reexport

[bluetech]

The natural expectation is that when a variable is explicitly added to a
module's __all__, the module explicitly means to export it.

Previously, this was not the case, and a from [..] import [..] as [..]
reexport was required by --no-implicit-export even if the variable is
listed in __all__.

Fixes #7042

[bluetech]

Following @JukkaL's suggestion in #7042 (comment), I tried to look at the code for a couple of hours and this is what I came up with. I am not sure it is correct at all, or if it has any unexpected consequences, so please review carefully (once/if it passes CI) :)

[msullivan]

This looks solid except for the one subtlety I mentioned.

[msullivan]

Looking at the docs for module_public and module_hidden:

• public means that it will be picked up by an import *
• hidden means that it can't be imported at all

So I think we need to set public = True when the name does appear to prevent it from being spurious excluded from import *.

[bluetech]

@msullivan Yes, testing with *-imports I see some unexpected things.

If I understand module_public and module_hidden correctly, this is what they mean (with --no-implicit-reexport enabled):

module_public	module_hidden	meaning
false	false	can be imported directly, not included in import *
false	true	cannot be imported directly, not included in import *
true	false	can be imported directly, included in import *
true	true	cannot be imported directly, included in import *

But the "cannot be imported directly, included in import *" case, I think makes no sense? That is, I think that if a symbol is imported with import *, it should always be possible to import it directly.

If true, this suggests that instead of two booleans, an enum should be used, maybe something like this:

class SymbolModuleVisibility:
    """The visibility of the symbol in a module, i.e. whether and how another module can import the symbol from this module.

    Only relevant for module symbol tables.
    """
    # Another module cannot import this symbol.
    hidden = 1
    # Another module can import this symbol directly.
    exported = 2
    # Another module can import this symbol directly, and with `from <module> import *`.
    exported_public = 3

Things which affect visibility (AFAICS) are:

• Name starts with _.
• Name appears in __all__.
• Name imported from another module with as (from another_module import x as y).
• Name is a module import without as (import another_module).
• Is stub file (per rules in PEP 484 Stub Files section).
• Implicit reexports are enabled.

Do you think it makes sense to get into that, or just do the fix you suggested and be done with it?

[msullivan]

Removing the potential invalid state by using defined constants seems reasonable. (Unfortunately we can't use an Enum right now because of limitations in mypyc, a compiler we use to compile mypy)

[bluetech]

I applied @msullivan's suggestion and also adjusted the docs.

I also added a second commit which switches the code to use an enum as proposed in the comment above.

I want to add more test cases and probably adjust the terminology in the second commit, so marking as WIP.

[msullivan]

Make these names all caps

[bluetech]

Left a comment in the issue #7042 (comment) on what's blocking this.

[ilevkivskyi]

@bluetech
What is the status of this PR? It fixes a high-priority issue and looks mostly ready apart from a merge conflict caused by moving semantic analyzer files.

[bluetech]

@ilevkivskyi Sorry for not following up. This needs some work/research, in particular spec'ing out the rules for each of these:

• runtime imports
• type checking regular files with implicit exports
• type checking regular files without implicit exports
• type checking stub files

per each type of import and export (partial list?)

• import x
• import x as z
• import x.y (what happens to x)
• import x.y as z
• from x import y
• from x import y as z
• from x import *
• __all__ defined?
• symbol starts with _?

and I haven't had enough patience to do it. For now I'd like to close it, but instead I'll just try to go back to the initial commit and rebase which at least makes the __all__ exports explicit, and you can decide if you want to merge it or close it.

[bluetech]

Updated as described above.

[ilevkivskyi]

I think we can keep this open (after #7361 is merged) as a place to discuss possible follow-up refactoring/clean-up that will replace module_public/module_hidden with an enum and clarifies the corner cases.

[bluetech]

This was squashed in to e9cf4f2 (PR #7361).