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

stubtest: error if module level dunder is missing, housekeeping #12217

Merged
merged 4 commits into from
Feb 20, 2022

Conversation

hauntsaninja
Copy link
Collaborator

Basically a follow up to #12203

New errors in typeshed from this:

_decimal.__libmpdec_version__ is not present in stub
_heapq.__about__ is not present in stub
builtins.__build_class__ is not present in stub
cgitb.__UNDEF__ is not present in stub
decimal.__libmpdec_version__ is not present in stub
sys.__unraisablehook__ is not present in stub

Some general housekeeping, moving things around, renaming things, adding
some comments.

New errors in typeshed from this:
```
_decimal.__libmpdec_version__ is not present in stub
_heapq.__about__ is not present in stub
builtins.__build_class__ is not present in stub
cgitb.__UNDEF__ is not present in stub
decimal.__libmpdec_version__ is not present in stub
sys.__unraisablehook__ is not present in stub
```

Some general housekeeping, moving things around, renaming things, adding
some comments.
@hauntsaninja
Copy link
Collaborator Author

cc @AlexWaygood

"__new_member__", # If an enum defines __new__, the method is renamed as __new_member__
"__dataclass_fields__", # Generated by dataclasses
"__dataclass_params__", # Generated by dataclasses
"__doc__", # mypy's semanal for namedtuples assumes this is str, not Optional[str]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one should be a pretty easy fix

# TODO: remove the following from this list
"__author__",
"__version__",
"__copyright__",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"__copyright__",
"__copyright__",
"__about__",

Another one I noticed in my experiments recently!

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see you consider heap.__about__ a true positive?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we could just put __about__: str in the stub. Doesn't do any harm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's quite educational :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reasoning here is that it's only clearly a true negative if the Python runtime is what's creating the attribute. Everything else you might conceivably want to have stubbed. I'm especially eager to remove __version__ from this list, which is widely used in third party stubs.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that makes sense!

@@ -204,7 +209,7 @@ def verify_mypyfile(
to_check = set(
m
for m, o in stub.names.items()
if not o.module_hidden and (not m.startswith("_") or hasattr(runtime, m))
if not o.module_hidden and (not is_probably_private(m) or hasattr(runtime, m))
Copy link
Member

Choose a reason for hiding this comment

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

I was planning something very similar, which was why I factored it out into a helper function even though it was only used once in my patch 😆

@github-actions

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator Author

This causes us to error if __all__ is in stub, but not in runtime.
It's possible that's undesirable? But we'll be thinking about __all__ very soon (sorry for merge conflicts!)

@AlexWaygood
Copy link
Member

(sorry for merge conflicts!)

😡

@AlexWaygood
Copy link
Member

It's possible that's undesirable?

I think it should be fine? I can't think of any scenarios off the top of my head where that would be a problem 🙂

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Feb 20, 2022

I was thinking something like: you're adding stubs for a third party module and want to use __all__ as a way to tell type checkers what the public interface is meant to be. (The only alternative being not adding stubs for private stuff at all)

@AlexWaygood
Copy link
Member

Why don't we just add __all__ to IGNORED_MODULE_DUNDERS for now? It's no change from the status quo, and, as you say, we'll be thinking properly about __all__ very soon.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@hauntsaninja
Copy link
Collaborator Author

That would be a regression from the status quo, since we wouldn't check __all__ even if the stub had it. So the bare minimum tuple length errors we had go away. I'll merge this, this might be the kind of problem where we wait to see if anyone ever reports it.

@hauntsaninja hauntsaninja merged commit 58514a9 into python:master Feb 20, 2022
@hauntsaninja hauntsaninja deleted the stubtdund branch February 20, 2022 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants