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

Fix TypeIs for types with type params in Unions #17232

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

kreathon
Copy link
Contributor

@kreathon kreathon commented May 10, 2024

Fix TypeIs narrowing for types with type parameters in Union types.

Addresses: #17181

Example:

bar: list[int] | list[str]

if is_list_int(bar):  <- here both Union members are currently erased to list[Any] (which is subtype of the erased TypeIs argument: list[Any])
    reveal_type(bar)
else:                 <- here is nothing left in the Union for this branch  and we get the type Never
    reveal_type(bar)  <- currently this is marked as unreachable

Implementation

My goal was to not split the implementation up (see first commit) into handling of the isinstance and TypeIs, but to use a common implementation.

Before, the code was using is_proper_subtype with erased types:

    supertype = erase_type(supertype)
    if is_proper_subtype(
        erase_type(item), supertype, ignore_promotions=True, erase_instances=True

However, this does no longer work (see example above). The idea is to use is_subtype (which should also be able to handle the isinstance implementation), because e.g. list[int] is subtype of list[Any].

The only problem with this implementation are "trivial" Any cases that should not result in any narrowing. The code tries to manually handle these (I could not find any existing method that would do something similar).

Test Plan

  • add testTypeIsUnionWithTypeParams (test case of the bug report)
  • add testTypeIsAwaitableAny test case, because it is also an example in the PEP. Note that the behavior of this test did not change with this implementation see.
  • add testTypeIsTypeAnytest case, because of pandera mypy_primer output (see below)

Primer Output

pandera

New behavior should be an improvement compared to before:

def is_subtype(
    arg1: Union[A, Type[A]],
    arg2: Union[B, Type[B]],
) -> bool:
    """Returns True if first argument is lower/equal in DataType hierarchy."""
    if inspect.isclass(arg1):
        reveal_type(arg1)
        arg1_cls = arg1
    else:
        reveal_type(arg1)
        arg1_cls = arg1.__class__

    arg2_cls = arg2 if inspect.isclass(arg2) else arg2.__class__
    return issubclass(arg1_cls, arg2_cls)

Old:

main.py:17: note: Revealed type is "type[Any]"
main.py:20: note: Revealed type is "Union[__main__.A, type[__main__.A]]"
main.py:21: error: Incompatible types in assignment (expression has type "type[A] | overloaded function", variable has type "type[Any]")  [assignment]
main.py:24: error: Argument 2 to "issubclass" has incompatible type "type[Any] | type[B] | overloaded function"; expected "_ClassInfo"  [arg-type]

New (see added test case):

main.py:19: note: Revealed type is "type[Any]"
main.py:22: note: Revealed type is "__main__.A"

Note

It seems that the narrowing still does not work as excepted in some cases (see the pandera example). The type should be type[__main__.A] instead of type[Any]. However, I think that this is a different issue.

This comment has been minimized.

@kreathon kreathon marked this pull request as draft May 10, 2024 10:56
@JelleZijlstra JelleZijlstra self-assigned this May 10, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

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

pandera (https://github.com/pandera-dev/pandera)
+ pandera/dtypes.py:555: error: Unused "type: ignore" comment  [unused-ignore]

@kreathon kreathon marked this pull request as ready for review May 25, 2024 15:55
@kreathon
Copy link
Contributor Author

@JelleZijlstra I am ready for feedback 🙂

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

2 participants