stubtest: import submodules mentioned in __all__

[hauntsaninja]

This kind of helps resolve #9935

[dalcinl]

I believe this is incorrect, you just have to updated with both __all__ and runtime_public_contents.

__all__ is not about things being public or private, it is just about from pgk import *. For example, you could have tons of public stuff in a package, but just list just the most useful things in __all__.

However, consider that __all__ can list names starting with understcore (_), not sure if those should be considered public or private in mypy, but from pgk import * do import them.

[hauntsaninja]

Type checkers use __all__ as a way to determine the public API:

https://mail.python.org/archives/list/[email protected]/message/FPTLFORIBPKY36UO7PLO47HJDCLG2KD5/

mypy/mypy/semanal.py

Line 483 in 5aa707d

def adjust_public_exports(self) -> None:

Note that due to the logic on L218 (on the right), there's a class of stuff that stubtest will check if it's present in the stub and runtime, but won't complain about if it's missing from the stub.

[dalcinl]

@hauntsaninja I'm certainly not an expert in this field. From your words, I understand that you are trying to explain to me that typecheckers have to (or decided to) use different rules that the usual, documented, years-old runtime Python rules. The situation with stubtest is unfortunate. To be useful, stubtest has to reconcile potentially different set of rules between typecheckers and runtime, and surely apply some compromises.

I tested your branch a bit, and because of the way you handle __all__ in the runtime module, together with a TODO you left in a comment, I'm afraid things do not work so well. In a extreme case, imagine that a module defines tons of classes and functions, and sets __all__ = [], then with your current code, all the other stuff in the module is not going to be checked.

Please consider the following patch, it passes all the tests (with tox -e py37), and IMHO it is closer to Python's runtime behavior regarding __all__.

diff --git a/mypy/stubtest.py b/mypy/stubtest.py
index 3e52eb8be..0c554df0a 100644
--- a/mypy/stubtest.py
+++ b/mypy/stubtest.py
@@ -214,9 +214,9 @@ def verify_mypyfile(
     to_check = set(
         m
         for m, o in stub.names.items()
-        # TODO: change `o.module_public` to `not o.module_hidden`
-        if o.module_public and (not m.startswith("_") or hasattr(runtime, m))
+        if not o.module_hidden and (not m.startswith("_") or hasattr(runtime, m))
     )
+    # Check things in the module that are public
     runtime_public_contents = [
         m
         for m in dir(runtime)
@@ -225,8 +225,9 @@ def verify_mypyfile(
         # have a good way to detect re-exports at runtime.
         and getattr(getattr(runtime, m), "__module__", None) == runtime.__name__
     ]
-    # Check all things declared in module's __all__, falling back to runtime_public_contents
-    to_check.update(getattr(runtime, "__all__", runtime_public_contents))
+    to_check.update(runtime_public_contents)
+    # Check all things declared in module's __all__
+    to_check.update(getattr(runtime, "__all__", []))
     to_check.difference_update({"__file__", "__doc__", "__name__", "__builtins__", "__package__"})
 
     for entry in sorted(to_check):

[hauntsaninja]

Yeah, stubtest does have to strike a tricky balance, so thank you for acknowledging that!

Note that the hasattr(runtime, m) check means that if you have the entries in the stub, even if your runtime has __all__ = [], they will be checked (the TODO makes this more consistent; I'll make that change in a week or two, once the typeshed refactor has taken place)

Still, that doesn't help with identifying missing entries from the stub if you use __all__ for a purpose other than defining your public symbols. I'm not going to change the default behaviour here, but I would be open to adding a flag (name suggestions?)

[dalcinl]

Still, that doesn't help with identifying missing entries from the stub if you use __all__ for a purpose other than defining your public symbols. I'm not going to change the default behaviour here,

I guess you are afraid of false positives? Well, IMHO false positives are better than missing stuff in the to_check list. You have allowlists to silence false positives, errors should never pass silently, Anyway, it seems we will not fully agree on this one, but I understand your compromises, and you have the bragging rights.

but I would be open to adding a flag (name suggestions?)

I'm not really good at naming, but let me try:

1. --exhaustive
2. --check-exhaustive
3. --check-extra-entries (my favorite so far)
4. --check-extra-runtime-entries
5. --deep-introspection
6. --deep-runtime-introspection