reportUnsupportedDunderAll does not consider submodule names in __all__

[waweber]

Describe the bug

The reportUnsupportedDunderAll rule triggers on __all__ definitions in a package's __init__.py that refer to the name of a submodule even when the submodule exists. The rule should only report names that are neither present in __init__.py nor are the name of a submodule.

This is the same issue reported in #3304, but the docs have recently been updated with more information on the semantics of __all__.

Code or Screenshots

In a package with the structure:

example/
    __init__.py
    submodule.py

# example/__init__.py
__all__ = [
    "submodule"  # <-- "submodule" is specified in __all__ but is not present in module
]

VS Code extension or command-line

Command line pyright 1.1.333

Details from the Python docs

Refer to the docs for import. It defines how __all__ works for modules:

The public names defined by a module are determined by checking the module’s namespace for a variable named __all__; if defined, it must be a sequence of strings which are names defined or imported by that module.

This lines up with everything so far. But there is additional behavior for packages which is detailed in an entirely different section of the documentation for some reason:

The import statement uses the following convention: if a package’s __init__.py code defines a list named __all__, it is taken to be the list of module names that should be imported when from package import * is encountered.

An example for a package sound/effects/__init__.py is given:

__all__ = ["echo", "surround", "reverse"]

This would mean that from sound.effects import * would import the three named submodules of the sound.effects package.

It also goes on to clarify that __all__ can contain both names defined in __init__.py and names of submodules:

Be aware that submodules might become shadowed by locally defined names. For example, if you added a reverse function to the sound/effects/init.py file, the from sound.effects import * would only import the two submodules echo and surround, but not the reverse submodule, because it is shadowed by the locally defined reverse function:

__all__ = [
    "echo",      # refers to the 'echo.py' file
    "surround",  # refers to the 'surround.py' file
    "reverse",   # !!! refers to the 'reverse' function now !!!
]

def reverse(msg: str):  # <-- this name shadows the 'reverse.py' submodule
    return msg[::-1]    #     in the case of a 'from sound.effects import *'

Summary:

• __all__ should contain a list of names defined in the module/package
• The names are what is imported by a * import
• The list defines the package/module's public API
• If the file is a module:
  • __all__ should contain a list of names defined in the module.
• If the file is a package's __init__.py:
  • If a name in __all__ refers to a name defined in the __init__.py, that object is what will be imported, both by from package import * and from package import name.
  • If the name is not defined in __init__.py, it should refer to a submodule with that name, and that submodule will be imported by from package import * and from package import name

I think the rule needs to be enhanced to check for submodule names, as this appears to be correct behavior according to the docs, and as seen in the standard library packages defining __all__.

[erictraut]

The reportUnsupportedDunderAll diagnostic check is targeted at library maintainers who are interested in type checking their library code. In particular, it ensures that the use of __all__ complies with the rules that static type checkers use for "py.typed" libraries. You can read about these rules here.

The original purpose of __all__ was to control the behavior of a wildcard import from that module. For "py.typed" libraries, it has been extended to also indicate which symbols should be considered "public" for a library when it is imported using a normal (non-wildcard) import statement.

With that in mind, let's look at the example you've provided in your bug report. It involves a module example with a submodule submodule. Let's create another module that imports this library. In addition to your example, let's add a "py.typed" file in the example directory to indicate that the library should be considered "py.typed".

# test.py
import example

print(example.submodule)  # Crash!

This code will generate a runtime exception on the last line because submodule is not imported and then re-exported by the top-level example module.

If the code uses a wildcard import (which is an ill-advised practice when importing from a library), then the code can access submodule.

from example import *

print(submodule) # Does not crash

I think it's therefore useful for the reportUnsupportedDunderAll rule to warn the library author that the __all__ definition includes a symbol that is not going to work as expected for users of the library. At least that's the intent behind the current behavior.

If you don't like how reportUnsupportedDunderAll works, you can disable it in your pyright configuration. Or you can suppress a specific diagnostic by adding a # pyright: ignore[reportUnsupportedDunderAll] to the end of that line.

[waweber]

@erictraut I agree that is correct in the case of a module's __all__.

But for a package's __init__.py, the meaning of __all__ is special-cased: "if a package’s __init__.py code defines a list named __all__, it is taken to be the list of module names that should be imported when from package import * is encountered."

In other words, although "submodule" is included in __all__, a user cannot assume that example.submodule will work, as it may not have been imported yet. Again, this is different than the behavior of a normal module's __all__

The docs also state "It is up to the package author to keep this list up-to-date when a new version of the package is released." So I think it would be useful if this rule properly handled __all__ for packages, since it is easy for a library author to move/rename a module and forget to update the list: the rule could identify that an entry no longer refers to a submodule and print the warning, letting the library author know something was missed.

I also agree that "*" imports are ill-advised, but if the rule suggests the submodule should be imported and re-exported, it causes one of the same problems that the "*" import does: if the package contains many modules, import example.submodule will result in the interpreter importing all of them even though only a single module was requested. The unique behavior of the __init__.py's __all__ allows the library author to specify an index of modules without incurring any overhead from importing the modules.

[erictraut]

I think you're arguing that there are legitimate cases where the author of a module may want to include a submodule in an __all__, thus making the submodule available when a wildcard import is used, but not importing and re-exporting it from the parent module for non-wildcard imports. I'm not convinced. I think this situation is almost always indicative of a bug — an unintended and undesirable case that most module authors would want to be informed of. For that reason, I think pyright's current behavior makes sense.

If your use case differs from the norm, you can disable this diagnostic check.

[waweber]

It is the intended and correct behavior as written in the official Python documentation and practiced by the standard library.

I disagree, but I understand and I will simply select this non-default setting going forward.

For completeness, and for those curious, here is an issue and PR to the official Python repository which gives us a clear definition of the behavior of a package's __all__: It is a list of module names that will be imported by * if requesting that name raises AttributeError! Hence, it is by design that a module name in __all__ may be undefined.

[Andrej730]

Met the same issue and it is very confusing that Pyright doesn't support __all__ in the way it is defined in the official Python documentation, even with user would try to adjust Pyright settings for this.

Using # pyright: reportUnsupportedDunderAll=false doesn't help as then user won't be able to see the symbols that are actually missing and present in __all__ - only specifying # pyright: ignore[reportUnsupportedDunderAll] for each of the imported symbol would work but then it could still lead to this:

since it is easy for a library author to move/rename a module and forget to update the list: the rule could identify that an entry no longer refers to a submodule and print the warning, letting the library author know something was missed.

Also, just noticed that even though Pyright doesn't support sumodules in __all__ but it still kind of supports it when you do a wildcard import and it's figuring the imported symbols.
E.g. if testlib has __all__ = () then in the snippet from test_lib import *; print(sublib) Pyright will complain that "sublib" is not defined.
But if testlib will contain __all__ = ("sublib",) then Pyright also will recognize sublib import by a wildcard just fine.

[xixixao]

Hey @erictraut, I am debating the current Pyright behavior on this pdoc thread: mitmproxy/pdoc#849, and I wonder, have things changed in Pyright, since this original conversation?

Because from my testing

# test.py
import example

print(example.submodule)  # Crash!

Pyright warns on this, irregardless of what's in __all__, and for:

from example import *

print(submodule) # Does not crash

Pyright doesn't error.

So irregardless of __all__, Pyright is handling things in a way that matches runtime, and so the only thing I'd like from Pyright is to check that when I put "foo" into __all__, that it is either indeed either locally defined or a submodule, and leave the "callsite" behavior as is.

Would you be open to changing this now?