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

Annotate TypedDict as _SpecialForm rather than object. #12985

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rchen152
Copy link
Collaborator

@rchen152 rchen152 commented Nov 8, 2024

For consistency, I think it would be slightly better to use _SpecialForm rather than object here. The comment suggests that object was used because TypedDict is non-subscriptable, but NoReturn is also non-subscriptable and is annotated as _SpecialForm.

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

If we take this approach, should we be removing __getitem__ from _SpecialForm?

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

If we take this approach, should we be removing __getitem__ from _SpecialForm?

Hmm, I didn't look closely enough at _SpecialForm. It does have a __getitem__, both in the stubs and at runtime.

It also looks like there are some weird mypy primer errors about a type mismatch between typing_extensions._SpecialForm and something called <typing special form>.

Let me think a bit more about this PR. Tbh I might just give up on it, since the motivation for it wasn't particularly strong in the first place. It just looked odd to me to have TypedDict described as a _SpecialForm but not annotated as such.

@rchen152 rchen152 marked this pull request as draft November 8, 2024 09:16

This comment has been minimized.

@rchen152
Copy link
Collaborator Author

rchen152 commented Nov 8, 2024

Ok, I've had a chance to think about and play around with this a bit more:

  • Conceptually, TypedDict is a special form (despite not being one at runtime), so I still think it would be better to mark it as such. We're already not hewing very close to the runtime definitions of many of these typing constructs anyway.
  • The mypy error occurs whenever a _SpecialForm is imported from both typing and typing_extensions in the same file. It's not specific to TypedDict. Making typing_extensions import the typing definition seems to fix it.
  • Removing __getitem__ from _SpecialForm generates stubtest errors. Easy enough to suppress with an addition to the allowlist, but I'm on the fence about whether to do it. NoReturn: _SpecialForm already exists, so it doesn't seem like the __getitem__ is causing any problems.

@rchen152 rchen152 marked this pull request as ready for review November 8, 2024 21:03
Copy link
Contributor

github-actions bot commented Nov 8, 2024

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

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