Skip to content

Patch check_op_reversible to support tuple subclasses. #19046

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

randolf-scholz
Copy link
Contributor

Fixes #19006

  • Added unit test TypeCheckSuite::check-expressions.test::testReverseBinaryOperator4

This patch drops the is_subtype(right_type, left_type) check in favor of the weaker covers_at_runtime(right_type, left_type).


Open Issues

  • Some tests were still failing because isinstance(left_type, Instance) fails when left_type is tuple[()]. As a workaround, I changed the unconditional check

    and isinstance(left_type, Instance)
    and isinstance(right_type, Instance)
    and not (
        left_type.type.alt_promote is not None
        and left_type.type.alt_promote.type is right_type.type
    )
    and lookup_definer(left_type, op_name) != lookup_definer(right_type, rev_op_name)

    into the conditional check

    # Checking (A implies B) using the logically equivalent (not A or B), where
    #    A: left and right are both `Instance` objects
    #    B: right's __rop__ method is different from left's __op__ method
    not (isinstance(left_type, Instance) and isinstance(right_type, Instance))
    or (
        lookup_definer(left_type, op_name) != lookup_definer(right_type, rev_op_name)
        and (
            left_type.type.alt_promote is None
            or left_type.type.alt_promote.type is not right_type.type
        )
    )

    But I am not sure this is the correct thing to do.

  • I get different results when running tests versus checking a file manually:

    $ pytest mypy/test/testcheck.py::TypeCheckSuite::check-expressions.test
    
    Expected:
    Actual:
    main:15: error: Expression is of type "Tuple[int, ...]", not "Size" (diff)
    main:16: error: Expression is of type "Tuple[int, ...]", not "Size" (diff)
    

    versus

    $ mypy example.py
    example.py:4: error: Signature of "__add__" incompatible with supertype "tuple"  [override]
    example.py:4: note:      Superclass:
    example.py:4: note:          @overload
    example.py:4: note:          def __add__(self, tuple[int, ...], /) -> tuple[int, ...]
    example.py:4: note:          @overload
    example.py:4: note:          def [_T] __add__(self, tuple[_T, ...], /) -> tuple[int | _T, ...]
    example.py:4: note:      Subclass:
    example.py:4: note:          def __add__(self, tuple[int, ...], /) -> Size
    example.py:14: error: Expression is of type "tuple[int, ...]", not "Size"  [assert-type]
    example.py:15: error: Expression is of type "tuple[int, ...]", not "Size"  [assert-type]
    example.py:16: error: Expression is of type "tuple[int, ...]", not "Size"  [assert-type]
    example.py:18: error: Expression is of type "tuple[int, ...]", not "Size"  [assert-type]
    Found 5 errors in 1 file (checked 1 source file)
    

@randolf-scholz randolf-scholz changed the title Reverse op for subclass Patch check_op_reversible to support tuple subclasses. May 6, 2025
Copy link
Contributor

github-actions bot commented May 6, 2025

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

xarray (https://github.com/pydata/xarray)
+ xarray/computation/computation.py: note: In function "_cov_corr":
+ xarray/computation/computation.py:304: error: Redundant cast to "T_DataArray"  [redundant-cast]

@sterliakov
Copy link
Collaborator

I get different results when running tests versus checking a file manually:

Likely typeshed definition of tuple.__{r,}add__ is more complex than what's in our fixture ([buitlins fixtures/tuple.pyi]). That fixture is used, khm, everywhere, so consider copying it and adding real __add__ and __radd__ to tuple from typeshed (mypy/typeshed/stdlib/builtins.pyi) in the new fixture.

Conditional check

Your new version looks reasonable: we want to exclude some kinds of Instances that shouldn't be treated that way, but for any other combination runtime coverage is sufficient. This sounds good, though perhaps more tests are needed (typevar in LHS/RHS? TypeVarTuple? TypedDict/dict?) to make sure we don't actually add more than necessary.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect type inference with __radd__ with subclass of tuple[int, ...]
2 participants