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

Use TypeIs for is_typeddict #12039

Closed
wants to merge 2 commits into from

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented May 26, 2024

Use TypeIs for is_typeddict, which will allow code like this to be type-checked properly, similarly to: #11929:

def f(X: type[Any]) -> None:
    if is_typeddict(X):
        X.__required_keys__
        reveal_type(X.__required_keys__)  # E: Revealed type is 'frozenset[str]'

@max-muoto max-muoto marked this pull request as ready for review May 26, 2024 23:19
@max-muoto max-muoto changed the title Use TypeIs for TypedDict Use TypeIs for is_typeddict May 26, 2024

This comment has been minimized.

@max-muoto
Copy link
Contributor Author

max-muoto commented May 26, 2024

The only change from mypy primer fixes a false positive in Pydantic:

    if is_typeddict(tp):
        return tp.__orig_bases__  # type: ignore

(This is valid, and will type check correctly now, on 3.12+)

@AlexWaygood
Copy link
Member

The only change from mypy primer fixes a false positive in Pydantic:

The mypy primer hit is saying that the type: ignore was previously unused but, with this change, would now be used. That seems like it's introducing a false positive rather than taking one away

@max-muoto
Copy link
Contributor Author

max-muoto commented May 26, 2024

The only change from mypy primer fixes a false positive in Pydantic:

The mypy primer hit is saying that the type: ignore was previously unused but, with this change, would now be used. That seems like it's introducing a false positive rather than taking one away

I'm fairly certain it's saying it's no longer necessary? From using Pyright, at least, it'll throw a warning if one isn't being used, but perhaps I'm misinterpreting.

Edit: One reason why that might be is because __orig_bases__ isn't present on TypeDict before 3.12, and before Any was getting used. So I don't think it's problematic.

@max-muoto
Copy link
Contributor Author

    # __orig_bases__ sometimes exists on <3.12, but not consistently,
    # so we only add it to the stub on 3.12+
    if sys.version_info >= (3, 12):
        __orig_bases__: ClassVar[tuple[Any, ...]]

@AlexWaygood
Copy link
Member

Mypy primer results are a diff. If a line is coloured in red with a minus sign at the beginning, it means that mypy emitted that message with the typeshed main branch, but no longer emits that message with the PR branch.

@max-muoto
Copy link
Contributor Author

Mypy primer results are a diff. If a line is coloured in red with a minus sign at the beginning, it means that mypy emitted that message with the typeshed main branch, but no longer emits that message with the PR branch.

I see, makes sense!

@AlexWaygood
Copy link
Member

But yeah, as you say, the new error seems like a true positive rather than a false positive — it shows mypy applying type narrowing correctly!

@AlexWaygood
Copy link
Member

Could you apply the same change to typing_extensions.is_typeddict? The two need to be kept in sync

@max-muoto
Copy link
Contributor Author

Could you apply the same change to typing_extensions.is_typeddict? The two need to be kept in sync

Will do!

@max-muoto
Copy link
Contributor Author

Could you apply the same change to typing_extensions.is_typeddict? The two need to be kept in sync

dd309b5

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pydantic (https://github.com/samuelcolvin/pydantic)
- pydantic/_internal/_decorators.py:285: error: Unused "type: ignore" comment  [unused-ignore]

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Advantages of doing this:

  • It allows type checkers to apply type narrowing when they see is_typeddict being used, which means that users can safely access class-level attributes such as __required_keys__ and __optional_keys__ on TypedDict classes.

Disadvantages of doing this:

  • It's a bit weird for type checkers to narrow to a fictional type. typing._TypedDict doesn't actually ever appear in a TypedDict class's mro; it's a class factory rather than a base class:
    >>> from typing import TypedDict
    >>> class Foo(TypedDict): pass
    ... 
    >>> Foo.__mro__
    (<class '__main__.Foo'>, <class 'dict'>, <class 'object'>)
  • The only real-world case where mypy_primer highlights this making a difference regards __orig_bases__. We already have a type-safe helper function for regarding that attribute (types.get_original_bases, backported as typing_extensions.get_original_bases), so I'm not sure I see that as a convincing use case.

Overall, I think I'm -0 on this. But curious about what other maintainers think.

@JelleZijlstra
Copy link
Member

I would oppose adding this. _TypedDict is not a real type and type checkers do not have to use it; type checkers that don't would likely get confused.

@max-muoto max-muoto deleted the TypeIs-for-TypedDict branch May 28, 2024 02:59
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.

None yet

3 participants