Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stubgen: include __all__ in output #16356

Merged
merged 5 commits into from
Oct 30, 2023
Merged

stubgen: include __all__ in output #16356

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f03f12c
stubgen: include __all__ in output
JelleZijlstra Oct 28, 2023
b545d45
Re-export ignored names in __all__
JelleZijlstra Oct 28, 2023
59e4415
omit undefined names from __all__
JelleZijlstra Oct 28, 2023
cc37d9e
fix test
JelleZijlstra Oct 28, 2023
f8d5c55
Believe the runtime __all__
JelleZijlstra Oct 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

## Unreleased

...
Stubgen will now include `__all__` in its output if it is in the input file (PR [16356](https://github.com/python/mypy/pull/16356)).

#### Other Notable Changes and Fixes
...
Expand Down
53 changes: 36 additions & 17 deletions mypy/stubutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,10 +614,21 @@ def get_imports(self) -> str:

def output(self) -> str:
"""Return the text for the stub."""
imports = self.get_imports()
if imports and self._output:
imports += "\n"
return imports + "".join(self._output)
pieces: list[str] = []
if imports := self.get_imports():
pieces.append(imports)
if dunder_all := self.get_dunder_all():
pieces.append(dunder_all)
if self._output:
pieces.append("".join(self._output))
return "\n".join(pieces)

def get_dunder_all(self) -> str:
"""Return the __all__ list for the stub."""
if self._all_:
defined_names = [name for name in self._all_ if name in self._toplevel_names]
return f"__all__ = {defined_names!r}\n"
return ""

def add(self, string: str) -> None:
"""Add text to generated stub."""
Expand Down Expand Up @@ -651,8 +662,7 @@ def set_defined_names(self, defined_names: set[str]) -> None:
self.defined_names = defined_names
# Names in __all__ are required
for name in self._all_ or ():
if name not in self.IGNORED_DUNDERS:
self.import_tracker.reexport(name)
self.import_tracker.reexport(name)

# These are "soft" imports for objects which might appear in annotations but not have
# a corresponding import statement.
Expand Down Expand Up @@ -751,7 +761,13 @@ def is_private_name(self, name: str, fullname: str | None = None) -> bool:
return False
if name == "_":
return False
return name.startswith("_") and (not name.endswith("__") or name in self.IGNORED_DUNDERS)
if not name.startswith("_"):
return False
if self._all_ and name in self._all_:
return False
if name.startswith("__") and name.endswith("__"):
return name in self.IGNORED_DUNDERS
return True
Comment on lines +767 to +773
Copy link
Member

Choose a reason for hiding this comment

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

I think I prefer stubtest's heuristic here, which IIRC is something like:

  1. If the runtime has __all__:
    a. If a name is included in __all__, treat it as public
    b. Else, treat the name as private
  2. If the runtime doesn't have __all__:
    a. If the name doesn't start with _, treat it as public
    b. Else, if the name is a dunder, treat it as public unless it's one of the ones in the "excluded dunders" list
    c. Else, treat it as private

The key difference is that (IIUC) the current algorithm you have here would treat bar as a public name in this situation, whereas stubtest would treat it as private:

__all__ = ["foo"]

foo = 2
bar = 3

Copy link
Member

@AlexWaygood AlexWaygood Oct 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we changed this algorithm in the way I'm suggesting, though, it would have an impact on existing tests such as testIncludeClassNotInAll_inspect. It would be a fairly big behavioural change. Probably best to leave it to another PR!

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly we should change this, but that feels out of scope for this PR; the logic is mostly as it was before, except that names in __all__ are now not treated as private. Your suggested change might be right, but it's better handled as an independent change.


def should_reexport(self, name: str, full_module: str, name_is_alias: bool) -> bool:
if (
Expand All @@ -761,18 +777,21 @@ def should_reexport(self, name: str, full_module: str, name_is_alias: bool) -> b
):
# Special case certain names that should be exported, against our general rules.
return True
if name_is_alias:
return False
if self.export_less:
return False
if not self.module_name:
return False
is_private = self.is_private_name(name, full_module + "." + name)
if is_private:
return False
top_level = full_module.split(".")[0]
self_top_level = self.module_name.split(".", 1)[0]
if (
not name_is_alias
and not self.export_less
and (not self._all_ or name in self.IGNORED_DUNDERS)
and self.module_name
and not is_private
and top_level in (self_top_level, "_" + self_top_level)
):
if top_level not in (self_top_level, "_" + self_top_level):
# Export imports from the same package, since we can't reliably tell whether they
# are part of the public API.
return True
return False
return False
if self._all_:
return name in self._all_
return True
40 changes: 39 additions & 1 deletion test-data/unit/stubgen.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,20 +587,26 @@ __all__ = [] + ['f']
def f(): ...
def g(): ...
[out]
__all__ = ['f']

def f() -> None: ...

[case testOmitDefsNotInAll_semanal]
__all__ = ['f']
def f(): ...
def g(): ...
[out]
__all__ = ['f']

def f() -> None: ...

[case testOmitDefsNotInAll_inspect]
__all__ = [] + ['f']
def f(): ...
def g(): ...
[out]
__all__ = ['f']

def f(): ...

[case testVarDefsNotInAll_import]
Expand All @@ -610,6 +616,8 @@ x = 1
y = 1
def g(): ...
[out]
__all__ = ['f', 'g']

def f() -> None: ...
def g() -> None: ...

Expand All @@ -620,6 +628,8 @@ x = 1
y = 1
def g(): ...
[out]
__all__ = ['f', 'g']

def f(): ...
def g(): ...

Expand All @@ -628,6 +638,8 @@ __all__ = [] + ['f']
def f(): ...
class A: ...
[out]
__all__ = ['f']

def f() -> None: ...

class A: ...
Expand All @@ -637,6 +649,8 @@ __all__ = [] + ['f']
def f(): ...
class A: ...
[out]
__all__ = ['f']

def f(): ...

class A: ...
Expand All @@ -647,6 +661,8 @@ class A:
x = 1
def f(self): ...
[out]
__all__ = ['A']

class A:
x: int
def f(self) -> None: ...
Expand Down Expand Up @@ -684,6 +700,8 @@ x = 1
[out]
from re import match as match, sub as sub

__all__ = ['match', 'sub', 'x']

x: int

[case testExportModule_import]
Expand All @@ -694,6 +712,8 @@ y = 2
[out]
import re as re

__all__ = ['re', 'x']

x: int

[case testExportModule2_import]
Expand All @@ -704,6 +724,8 @@ y = 2
[out]
import re as re

__all__ = ['re', 'x']

x: int

[case testExportModuleAs_import]
Expand All @@ -714,6 +736,8 @@ y = 2
[out]
import re as rex

__all__ = ['rex', 'x']

x: int

[case testExportModuleInPackage_import]
Expand All @@ -722,13 +746,17 @@ __all__ = ['p']
[out]
import urllib.parse as p

__all__ = ['p']

[case testExportPackageOfAModule_import]
import urllib.parse
__all__ = ['urllib']

[out]
import urllib as urllib

__all__ = ['urllib']
Comment on lines 751 to +758
Copy link
Member

@AlexWaygood AlexWaygood Oct 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

kinda cool that stubgen has sophisticated enough inference to do this. Didn't realise that it had this capability!


[case testRelativeImportAll]
from .x import *
[out]
Expand All @@ -741,6 +769,8 @@ x = 1
class C:
def g(self): ...
[out]
__all__ = ['f', 'x', 'C']
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this. Stubtest would emit an error here if it encountered this, since __all__ is different in the stub to what it is at runtime. Rather than trying to synthesise a "corrected version of __all__", I think stubgen should just omit it from the generated stub in this situation

Copy link
Member Author

Choose a reason for hiding this comment

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

Omitting it also feels wrong though, and should generate a stubtest error (not sure if it currently does). Maybe the best option is to just emit __all__ as it is at runtime, even if it includes some undefined names.

Copy link
Member

Choose a reason for hiding this comment

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

Stubtest doesn't complain if __all__ is omitted at runtime, no (though I think it should -- see #13300).

We're in a bit of a catch-22 here if the runtime __all__ has members that don't exist at runtime. In terms of typeshed's CI:

  1. If __all__ is different in the stub to what it is at runtime, then stubtest will complain.
  2. If __all__ in the stub contains members that don't exist in the stub, pyright will complain.
  3. If __all__ is present at runtime but omitted in the stub, then neither will complain but, as you say, maybe stubtest should complain...

I think you're right that emitting __all__ as it is at runtime is probably the least opinionated thing to do. Maybe we should just add a comment in the test cases to say that we know this is weird, but there's no obviously correct solution for this situation

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, and both stubtest and pyright are correct in those errors. Ultimately if __all__ contains a name that doesn't exist, that's a bug in the runtime library, and it seems right that to work around it in typeshed we'll have to ignore an error (either through the stubtest allowlist or a type ignore).


def f() -> None: ...

x: int
Expand All @@ -758,6 +788,8 @@ x = 1
class C:
def g(self): ...
[out]
__all__ = ['f', 'x', 'C']

def f(): ...

x: int
Expand Down Expand Up @@ -2343,6 +2375,8 @@ else:
[out]
import cookielib as cookielib

__all__ = ['cookielib']

[case testCannotCalculateMRO_semanal]
class X: pass

Expand Down Expand Up @@ -2788,6 +2822,8 @@ class A: pass
# p/__init__.pyi
from p.a import A

__all__ = ['a']

a: A
# p/a.pyi
class A: ...
Expand Down Expand Up @@ -2961,7 +2997,9 @@ __uri__ = ''
__version__ = ''

[out]
from m import __version__ as __version__
from m import __about__ as __about__, __author__ as __author__, __version__ as __version__

__all__ = ['__about__', '__author__', '__version__']

[case testAttrsClass_semanal]
import attrs
Expand Down