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.

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

Should stubtest verify the presence of __all__ in a stub? #13300

Closed
hauntsaninja opened this issue Aug 1, 2022 · 5 comments · Fixed by #18005
Closed

Should stubtest verify the presence of __all__ in a stub? #13300

hauntsaninja opened this issue Aug 1, 2022 · 5 comments · Fixed by #18005

Comments

@hauntsaninja
Copy link
Collaborator

See #12214

Currently, stubtest will not ensure that __all__ is present in the stub if it is present at runtime.

We will always check whether the stub has the symbols we need, so ensuring that __all__ is in the stub may just cause additional busywork for maintainers of stubs.

However, users may do module.__all__ at runtime. If stubtest doesn't ensure that __all__ is present in the stub, this means such users could get false positives when using such stubs.

However, if __all__ isn't in __all__ is it a public API? stubtest will ensure that __all__ is present in the stub if __all__ is in __all__.

Anyway, busywork is my main concern here. The change itself is very easy: 02b4f14

@JelleZijlstra
Copy link
Member

Perhaps we can complain about the absence of __all__ only if it would make a difference to import * semantics: that is, if the set of symbols in __all__ at runtime is different from the set of non-underscored public symbols in the stub.

@Avasam
Copy link
Contributor

Avasam commented Jun 24, 2023

I'm less concerned about a user doing from module import __all__, but more about from module import * and not having the correct imported symbols because it was forgotten in the stub.

(also referencing this typeshed discussion: python/typeshed#6810 )

@Avasam
Copy link
Contributor

Avasam commented Sep 23, 2023

I now have an actual example where enforcing this would've helped a typeshed contributor other than myself: python/typeshed#10544 (comment)

@JelleZijlstra
Copy link
Member

I think we should just bite the bullet here and start warning when __all__ is missing. We already have __all__ in most of typeshed, and it's useful for stubtest to warn us when we forget it.

@Avasam
Copy link
Contributor

Avasam commented Oct 29, 2023

It shouldn't be too hard to add progressively with .*\.__all__ allowlist entries anyway right?
I imagine a lot of __all__ will be non-trivial due to missing symbols (or worst: invalid __all__ in source!)

srittau added a commit to srittau/mypy that referenced this issue Oct 21, 2024
hauntsaninja pushed a commit that referenced this issue Oct 23, 2024
Previously it wasn't an error if runtime included an
`__all__` declaration, but the stubs did not. This PR
changes this to reflect the consensus that it would
be a good idea to ensure consistency in this case.

Fixes #13300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants