stubgen: include all in output #16356

JelleZijlstra · 2023-10-28T18:13:32Z

AlexWaygood · 2023-10-28T18:17:22Z

test-data/unit/stubgen.test

@@ -2963,6 +2999,8 @@ __version__ = ''
 [out]
 from m import __version__ as __version__


This was a problem before your PR, but I'm not sure what the justification is for not importing __about__ and __author__ in the generated stub here, given that they're included in __all__ at runtime

Yes, they're in IGNORED_DUNDERS in stubutil.py. It generally makes sense to exclude these names as they're not useful in stubs, but we shouldn't ignore them if they're in __all__.

Pushed a change so that IGNORED_DUNDERS aren't ignored if they're in __all__.

AlexWaygood · 2023-10-28T18:21:25Z

test-data/unit/stubgen.test

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


Hmm. Maybe it's problematic if stubgen adds __all__ to the generated stub, even though a symbol in __all__ at runtime (in this case, g) isn't actually defined at runtime. On the other hand, maybe it's outside of stubgen's purview to worry about such things; maybe it should just copy the runtime __all__ faithfully into the stub even when __all__ is weird at runtime, as you're doing now.

It's probably better to omit the undefined names so the generated stub won't create errors for type checkers, so I pushed that change. Could go either way though.

(For future readers of this thread: we decided to go back on this; see the discussion in #16356 (comment) for why)

AlexWaygood · 2023-10-28T23:10:39Z

mypy/stubutil.py

+        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


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

If the runtime has __all__:
a. If a name is included in __all__, treat it as public
b. Else, treat the name as private

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

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!

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.

AlexWaygood · 2023-10-28T23:17:21Z

test-data/unit/stubgen.test

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

 [out]
 import urllib as urllib

+__all__ = ['urllib']


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

AlexWaygood · 2023-10-28T23:18:37Z

test-data/unit/stubgen.test

@@ -741,6 +769,8 @@ x = 1
 class C:
    def g(self): ...
 [out]
+__all__ = ['f', 'x', 'C']


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

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.

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:

If __all__ is different in the stub to what it is at runtime, then stubtest will complain.

If __all__ in the stub contains members that don't exist in the stub, pyright will complain.

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

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).

AlexWaygood

github-actions · 2023-10-28T23:55:54Z

stubgen: include __all__ in output

f03f12c

Fixes python#10314

AlexWaygood reviewed Oct 28, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

JelleZijlstra added 2 commits October 28, 2023 11:41

Re-export ignored names in __all__

b545d45

omit undefined names from __all__

59e4415

This comment has been minimized.

Sign in to view

fix test

cc37d9e

This comment has been minimized.

Sign in to view

AlexWaygood reviewed Oct 28, 2023

View reviewed changes

Believe the runtime __all__

f8d5c55

AlexWaygood approved these changes Oct 28, 2023

View reviewed changes

JelleZijlstra merged commit 0ff7a29 into python:master Oct 30, 2023

JelleZijlstra deleted the stubgenall branch October 30, 2023 18:48

QuLogic mentioned this pull request Apr 1, 2024

[pre-commit.ci] pre-commit autoupdate matplotlib/matplotlib#28003

Merged

JelleZijlstra restored the stubgenall branch September 10, 2024 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

stubgen: include all in output #16356

stubgen: include all in output #16356

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stubgen: include __all__ in output #16356

stubgen: include __all__ in output #16356

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stubgen: include all in output #16356

stubgen: include all in output #16356