Skip to content

stubtest: import submodules mentioned in __all__ - #9943

Merged
hauntsaninja merged 3 commits into
python:masterfrom
hauntsaninja:stubt
Jul 26, 2021
Merged

hauntsaninja merged 3 commits into
python:masterfrom
hauntsaninja:stubt

Conversation

@hauntsaninja

Copy link
Copy Markdown
Collaborator

This kind of helps resolve #9935

Comment thread mypy/stubtest.py
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))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://mail.python.org/archives/list/typing-sig@python.org/message/FPTLFORIBPKY36UO7PLO47HJDCLG2KD5/

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.

@hauntsaninja
hauntsaninja marked this pull request as ready for review January 23, 2021 20:18
@dalcinl

dalcinl commented Jan 24, 2021

Copy link
Copy Markdown

@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

hauntsaninja commented Jan 24, 2021 •

Copy link
Copy Markdown
Collaborator Author

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

dalcinl commented Jan 25, 2021

Copy link
Copy Markdown

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

@hauntsaninja
hauntsaninja merged commit 6fd2dc2 into python:master Jul 26, 2021
@hauntsaninja
hauntsaninja deleted the stubt branch July 26, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stubtest: Spurious warning related to symbols in '__all__'

3 participants