Repository: python/mypy
Pull request: Stubtest: verify the contents of `__all__` in a stub
### Description

Fixes https://github.com/python/typeshed/issues/7284.

Stubtest currently does not check for the presence of `__all__` in a stub. Moreover, even if `__all__` is defined in the stub, stubtest does not currently check that the stub's `__all__` matches the runtime's `__all__`. This PR fixes that!

## Test Plan

I've modified an existing test case and added a new one.

## New errors reported by stubtest when checking typeshed, with this patch applied.

There are... 344 new errors reported by stubtest, with this PR applied, so it may not yet be mergeable. I've attached the full list of errors as a `.txt` file.

[all_discrepancies.txt](https://github.com/python/mypy/files/8102842/all_discrepancies.txt)



Problem statement:
`__all__` in a stub will easily go out of date
Stubtest does not check that the elements in a stub file's `__all__` match the elements in the runtime's `__all__`. It's also not obvious (to me, at least) how stubtest could be patched so that it would check that the elements match.

If `__all__` is a tuple, stubtest _will_ raise an error if the stub's `__all__` is a different length to the runtime's `__all__`, but it will not raise an equivalent error if `__all__` is a list.

flake8 will raise an error if a name in `__all__` is not defined in the file. flake8-pyi on master (not yet released) will also raise an error if `__all__` is merely annotated as `list[str]` without being defined. But flake8 will not raise an error if a name is _missing_ from `__all__`.

All this means that the `__all__` definitions in typeshed are liable to become outdated very easily if new elements are added to `__all__` in future Python versions. I'm not sure exactly what the solution is.

Hints from issue discussion:
Suggested solution here: https://github.com/python/mypy/pull/12214

But it's too noisy at the moment, due to the number of modules that have `__all__` at runtime, but not in the stub. So, it might have to wait a while.

Commit message: Blacken

Patch to review:
diff --git a/mypy/stubtest.py b/mypy/stubtest.py
index d2f9cfcca974..b1f523477a0a 100644
--- a/mypy/stubtest.py
+++ b/mypy/stubtest.py
@@ -20,7 +20,7 @@
 from contextlib import redirect_stderr, redirect_stdout
 from functools import singledispatch
 from pathlib import Path
-from typing import Any, Dict, Generic, Iterator, List, Optional, Tuple, TypeVar, Union, cast
+from typing import Any, Dict, Generic, Iterator, List, Optional, Set, Tuple, TypeVar, Union, cast
 
 import typing_extensions
 from typing_extensions import Type
@@ -242,6 +242,38 @@ def verify(
     yield Error(object_path, "is an unknown mypy node", stub, runtime)
 
 
+def _verify_exported_names(
+    object_path: List[str], stub: nodes.MypyFile, runtime_all_as_set: Set[str]
+) -> Iterator[Error]:
+    public_names_in_stub = {m for m, o in stub.names.items() if o.module_public}
+    names_in_stub_not_runtime = sorted(public_names_in_stub - runtime_all_as_set)
+    names_in_runtime_not_stub = sorted(runtime_all_as_set - public_names_in_stub)
+    if not (names_in_runtime_not_stub or names_in_stub_not_runtime):
+        return
+    yield Error(
+        object_path,
+        (
+            "module: names exported from the stub "
+            "do not correspond to the names exported at runtime.\n"
+            "(Note: This is probably either due to an inaccurate "
+            "`__all__` in the stub, "
+            "or due to a name being declared in `__all__` "
+            "but not actually defined in the stub.)"
+        ),
+        # pass in MISSING instead of the stub and runtime objects,
+        # as the line numbers aren't very relevant here,
+        # and it makes for a prettier error message.
+        stub_object=MISSING,
+        runtime_object=MISSING,
+        stub_desc=(
+            f"Names exported in the stub but not at runtime: " f"{names_in_stub_not_runtime}"
+        ),
+        runtime_desc=(
+            f"Names exported at runtime but not in the stub: " f"{names_in_runtime_not_stub}"
+        ),
+    )
+
+
 @verify.register(nodes.MypyFile)
 def verify_mypyfile(
     stub: nodes.MypyFile, runtime: MaybeMissing[types.ModuleType], object_path: List[str]
@@ -253,6 +285,18 @@ def verify_mypyfile(
         yield Error(object_path, "is not a module", stub, runtime)
         return
 
+    runtime_all_as_set: Optional[Set[str]]
+
+    if hasattr(runtime, "__all__"):
+        runtime_all_as_set = set(runtime.__all__)
+        if "__all__" in stub.names:
+            # Only verify the contents of the stub's __all__
+            # if the stub actually defines __all__
+            # Otherwise we end up with duplicate errors when __all__ is missing
+            yield from _verify_exported_names(object_path, stub, runtime_all_as_set)
+    else:
+        runtime_all_as_set = None
+
     # Check things in the stub
     to_check = {
         m
@@ -271,16 +315,16 @@ def _belongs_to_runtime(r: types.ModuleType, attr: str) -> bool:
         return not isinstance(obj, types.ModuleType)
 
     runtime_public_contents = (
-        runtime.__all__
-        if hasattr(runtime, "__all__")
-        else [
+        runtime_all_as_set | {"__all__"}
+        if runtime_all_as_set is not None
+        else {
             m
             for m in dir(runtime)
             if not is_probably_private(m)
             # Ensure that the object's module is `runtime`, since in the absence of __all__ we
             # don't have a good way to detect re-exports at runtime.
             and _belongs_to_runtime(runtime, m)
-        ]
+        }
     )
     # Check all things declared in module's __all__, falling back to our best guess
     to_check.update(runtime_public_contents)
diff --git a/mypy/test/teststubtest.py b/mypy/test/teststubtest.py
index ef06608a9c1b..f8b870744ad9 100644
--- a/mypy/test/teststubtest.py
+++ b/mypy/test/teststubtest.py
@@ -741,6 +741,10 @@ def f(): return 3
             error=None,
         )
 
+    @collect_cases
+    def test_missing_all(self) -> Iterator[Case]:
+        yield Case(stub="", runtime="__all__ = []", error="__all__")
+
     @collect_cases
     def test_missing(self) -> Iterator[Case]:
         yield Case(stub="x = 5", runtime="", error="x")
@@ -757,7 +761,14 @@ def h(x: str): ...
             runtime="",
             error="h",
         )
-        yield Case(stub="", runtime="__all__ = []", error=None)  # dummy case
+        # If runtime has __all__ but stub does not,
+        # we'll already raise an error for a missing __all__,
+        # so we shouldn't raise another error for inconsistent exported names
+        yield Case(stub="_Z = int", runtime="__all__ = []", error=None)
+        # But we *should* raise an error with the module name itself,
+        # if the stub *does* define __all__,
+        # but the stub's __all__ is inconsistent with the runtime's __all__
+        yield Case(stub="__all__ = ['foo']", runtime="", error="")
         yield Case(stub="", runtime="__all__ += ['y']\ny = 5", error="y")
         yield Case(stub="", runtime="__all__ += ['g']\ndef g(): pass", error="g")
         # Here we should only check that runtime has B, since the stub explicitly re-exports it


Code review comments:
diff_hunk: @@ -757,7 +761,14 @@ def h(x: str): ...
             runtime="",
             error="h",
         )
-        yield Case(stub="", runtime="__all__ = []", error=None)  # dummy case
+        # If runtime has __all__ but stub does not,
+        # we'll already raise an error for a missing __all__,
+        # so we shouldn't raise another error for inconsistent exported names
+        yield Case(stub="_Z = int", runtime="__all__ = []", error=None)
path: mypy/test/teststubtest.py
text: @user1:
Unfortunately, test cases within a `collect_cases` aren't that independent, since they end up in the same file. For all of these, the stub has an `__all__`. Big gotcha about how stubtest does tests, but it does make them much faster :-)

@author:
Argh, of course!

diff_hunk: @@ -271,16 +315,16 @@ def _belongs_to_runtime(r: types.ModuleType, attr: str) -> bool:
         return not isinstance(obj, types.ModuleType)
 
     runtime_public_contents = (
-        runtime.__all__
-        if hasattr(runtime, "__all__")
-        else [
+        runtime_all_as_set | {"__all__"}
path: mypy/stubtest.py
text: @user1:
I'm not sure that I like always checking for `__all__` (if the stub doesn't have `__all__` and `__all__` isn't itself in runtime's `__all__`). Either way we'll check whether we have the symbols we want, so in my mind this mostly becomes busy work.

@author:
I'd counter that `__all__` is itself a valid variable in a Python module, though, and leaving it out of the stub can lead to false positives! 

E.g. this line in the `sphinx` codebase used to cause mypy to emit an error, before we added `__all__` to the stub: https://github.com/sphinx-doc/sphinx/blob/3d6d501a43d2e5942153baa9414a4de34323c56b/sphinx/domains/python.py#L1438

See https://github.com/python/typeshed/pull/7373#issuecomment-1049430887

@user1:
But `__all__` isn't part of the public API if `__all__` doesn't contain `__all__` ;-)

Proposal:
This change has been lying around for far too long, since 90% of the change is awesome and necessary and uncontroversial, let's merge it minus the verify the presence of `__all__` bit. I'll open an issue to track verifying presence of `__all__` and I can assess whether this causes too much busywork for other users of stubtest.
```
diff --git a/mypy/stubtest.py b/mypy/stubtest.py
index 9595fa612..9a30c4be6 100644
--- a/mypy/stubtest.py
+++ b/mypy/stubtest.py
@@ -313,7 +313,7 @@ def verify_mypyfile(
         return not isinstance(obj, types.ModuleType)
 
     runtime_public_contents = (
-        runtime_all_as_set | {"__all__"}
+        runtime_all_as_set
         if runtime_all_as_set is not None
         else {
             m
```

@author:
Alrighty, let's do it!