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

Check that B.__radd__ is a subtype of A.__add__ for B <: A #1034

Open
jorenham opened this issue Feb 1, 2025 · 26 comments
Open

Check that B.__radd__ is a subtype of A.__add__ for B <: A #1034

jorenham opened this issue Feb 1, 2025 · 26 comments
Labels
rejected upstream also a bug in pyright/pylance or feature that isn't in pyright/pylance - they refused to address it type checking / linting issues relating to existing diagnostic rules or proposals for new diagnostic rules

Comments

@jorenham
Copy link
Contributor

jorenham commented Feb 1, 2025

Rejected upstream: microsoft/pyright#9684

we shouldn't support __radd__ priority like mypy, because it's not safe, we can't ensure that RHS is a subtype of LHS

class A:
    def __add__(self, other: object) -> int: ...
class B(A):
    def __radd__(self, other: object) -> str: ...  # expect error


def f(a1: A, a2: A):
    a1 + a2  # A.__add__ -> int, but at runtime, this would produce str
f(A(), B())
@DetachHead
Copy link
Owner

same issue as #1018?

@DetachHead DetachHead added type checking / linting issues relating to existing diagnostic rules or proposals for new diagnostic rules awaiting response waiting for more info from the author - will eventually close if they don't respond labels Feb 2, 2025
@jorenham
Copy link
Contributor Author

jorenham commented Feb 2, 2025

same issue as #1018?

no, this has to do with that special __radd__ behavior of subclasses:

>>> from typing import Self, override
... 
>>> class Float64(float):
...     @override
...     def __add__(self, x: float, /) -> Self:
...         return type(self)(x)
...     
...     @override
...     def __radd__(self, x: float, /) -> Self:
...         return self.__add__(x)
...         
>>> type(1.0 + Float64(2.2))
<class '__main__.Float64'>

but

from typing import Self, override


class Float64(float):
    @override
    def __add__(self, x: float, /) -> Self:
        return type(self)(x)
    
    @override
    def __radd__(self, x: float, /) -> Self:
        return self.__add__(x)


reveal_type(1.0 + Float64(2.0))  # pyright: float

https://basedpyright.com/?typeCheckingMode=all&code=GYJw9gtgBALgngBwJYDsDmUkQWEMoDKApgDbAA0UYAbkSCEgCZEBQbAxiQIYDOPUAMRJguMAGwAWABTBhogJQAuFlFVQAAjToNmKtc2BQA%2Bka6NGJqT1IUoAD0VRZImJQD08qAFoAfIRvKakFQIEQwAK4gKLCIRFY28lJ28nqqqRpa9EysQQbGRiBmFkbxZJQOTnKuUB7efsRkgcGqoRFRUNZkAHQmRZbJbCyhtFwkRvAIcQCMXQAMUADUglWSUgBMc-IpQA

and it's the same thing with the other binary arithmetic ops

@DetachHead
Copy link
Owner

i see. i think it's basically the same issue though but with different dunders. will leave this issue open since #1018 is for the in-place operators

@DetachHead DetachHead added rejected upstream also a bug in pyright/pylance or feature that isn't in pyright/pylance - they refused to address it and removed awaiting response waiting for more info from the author - will eventually close if they don't respond labels Feb 2, 2025
@jorenham
Copy link
Contributor Author

jorenham commented Feb 2, 2025

This one has nothing to do with variance or mutability though 🤔

@DetachHead
Copy link
Owner

i think the underlying issue in both of these cases is that when a class defines a dunder (eg. __add__ or __or__) but a subtype defines a different dunder (eg. __radd__ or __ior__), those ones get called instead, but pyright has no way of knowing whether a subtype does this. that essentially means these dunders need to be considered in the same way as overrides (that they must be a valid subtype), even though they're "overriding" a different method

@KotlinIsland
Copy link
Collaborator

again, this is also related to #1025, within the family of "the subtype defines something that the super type doesn't know about, but clobbers some functionality that exists on the super type"

@jorenham
Copy link
Contributor Author

jorenham commented Feb 2, 2025

Usually, a + b is evaluated roughly as follows:

  1. try a.__add__(b), but if this raises or returns NotImplemented, continue to 2.
  2. return b.__radd__(a).

However, if, and only if, B <: A, then this order of evaluation is swapped:

  1. try b.__radd__(a), but if this raises continue to 2.
  2. try a.__add__(b), and raise if it returns NotImplemented

This is described in https://docs.python.org/3/library/numbers.html#implementing-the-arithmetic-operations (specifically, step 5.), where they also explain why this is type-safe.


So it's not a type-safety thing, it's a python-data-model thing.

@jorenham
Copy link
Contributor Author

jorenham commented Feb 2, 2025

>>> class A:
...     def __add__(self, x, /):
...         print(f"A.__add__({x!r})")
...     def __radd__(self, x, /):
...         print(f"A.__radd__({x!r})")
...         
>>> class B(A):
...     def __radd__(self, x, /):
...         print(f"B.__radd__({x!r})")
...         
>>> A() + A()
A.__add__(<__main__.A object at 0x762e6ecb6fd0>)
>>> A() + B()
B.__radd__(<__main__.A object at 0x762e6ecb6c10>)

while usually

>>> class X:
...     def __add__(self, x, /):
...         print(f"X.__add__({x!r})")
...     def __radd__(self, x, /):
...         print(f"X.__radd__({x!r})")
...         
>>> A() + X()
A.__add__(<__main__.X object at 0x762e6eab52b0>)

@jorenham
Copy link
Contributor Author

jorenham commented Feb 2, 2025

And surprisingly, mypy already does this correctly.

@randolf-scholz
Copy link

randolf-scholz commented Feb 3, 2025

The ideal solution to this, imho:

  1. If you see a+b, read it as operator.add(a, b)
  2. Properly overload operator.add in typeshed in a manner that is consistent with runtime behavior.

However, this requires HKTs + intersection types + proper function types:

@overload  # RHS is a subtype of LHS and defines __radd__ that can consume LHS.
def add[L, R: L & SupportsRAdd[L]](left: L, right: R) -> ReturnType[R.__radd__, L]: ...
@overload # LHS supports __add__.
def add[R, L: SupportsAdd[R]](left: L, right: R) -> ReturnType[L.__add__, R]: ...

@jorenham
Copy link
Contributor Author

jorenham commented Feb 3, 2025

The ideal solution to this, imho:

  1. If you see a+b, read it as operator.add(a, b)
  2. Properly overload operator.add in typeshed in a manner that is consistent with runtime behavior.

However, this requires HKTs + intersection types + proper function types:

@overload # RHS is a subtype of LHS and defines radd that can consume LHS.
def add[L, R: L & SupportsRAdd[L]](left: L, right: R) -> ReturnType[R.radd, L]: ...
@overload # LHS supports add.
def add[R, L: SupportsAdd[R]](left: L, right: R) -> ReturnType[L.add, R]: ...

Yes! That's how should be! I actually wrote optype to make it easier to write annotations like these.

But it's too bad that this isn't going to work in practice, because as it turns out, Protocol breaks down once there are overloads involved...

So if in this case, __add__ were to be overloaded, then it either wouldn't be accepted, or L and R would only bind to the respective types in the first overload, ignoring all others.

This is why divmod simply doesn't work in pyright when the type has an overloaded __divmod__. I'm not sure why it works in mypy, but maybe divmod is special-cased or something. In other cases mypy also behaves as incorrectly as pyright does.

I pointed this out in microsoft/pyright#9663, but it was rejected because it would be "too complex" to fix... So that effectively means that either structural typing shouldn't be used, or that you should never overload your methods. Because if you don't, there will always be a scenario that will be inferred incorrectly.

Relavant bpr issue: #989

@randolf-scholz
Copy link

@jorenham I took a look at the comments in microsoft/pyright#9663, I think this problem would also resolve itself if the features I noted above were available, because then instead of

def call_f[X, R](obj: CanF[X, R], arg: X) -> R: ...

you could do

def call_f[X, C: CanF[X, Any]](obj: C, arg: X) -> ReturnType[C.f, X]: ...

So then call_f(a, 0) should bind X=L[0] and C=A, and correctly infer return type as ReturnType[C.f, X] = ReturnType[A.f, X] = str.

@jorenham
Copy link
Contributor Author

jorenham commented Feb 3, 2025

Yea, it would indeed be a great alternative, @randolf-scholz. The declarative style is really nice, and doesn't require defining a huge amount of Protocols.

@KotlinIsland
Copy link
Collaborator

KotlinIsland commented Feb 18, 2025

i haven't seen any discussion here regarding the consequences of NotImplemented

from __future__ import annotations

from typing import reveal_type


class A:
    def __add__(self, other: A) -> A:
        return A()
class B(A):
    def __radd__(self, other: A) -> B:
        return NotImplemented


reveal_type(A() + B())  # runtime: A, mypy: B, pyright: A

maybe if the signature was -> B | NotImplemented then it would be correct

@jorenham
Copy link
Contributor Author

jorenham commented Feb 19, 2025

maybe if the signature was -> B | NotImplemented then it would be correct

oof, don't get me started on NotImplemented...

@KotlinIsland
Copy link
Collaborator

KotlinIsland commented Feb 19, 2025

__radd__ priority fail:

from __future__ import annotations

from typing import reveal_type


class A:
    def __add__(self, other: object) -> A:
        return A()
class B(A):
    def __radd__(self, other: object) -> B:
        return B()

class C(A):
    pass

def f(a: A, b: B):
    reveal_type(a + b)  # runtime: A, mypy: B, pyright: A

f(C(), B())

basically, we can't say that __radd__ of the RHS will always take priority, because it's not guaranteed that RHS is a subtype of LHS

@KotlinIsland
Copy link
Collaborator

maybe if the signature was -> B | NotImplemented then it would be correct

oof, don't get me started on NotImplemented...

@jorenham

This comment has been minimized.

@KotlinIsland
Copy link
Collaborator

KotlinIsland commented Feb 19, 2025

could be anything else

not true, if B.__radd__ must be a subtype of A.__radd__ (falling back to A.__add__), then can be be sure that it won't be 'anything else'

  • C <: A and C.__radd__(?) -> RC
    • C() + B() gives RC at runtime

this isn't correct, it will be RA, updated the egg to remove the red herring

@jorenham
Copy link
Contributor Author

could be anything else

not true, if B.__radd__ must be a subtype of A.__radd__ (falling back to A.__add__), then can be be sure that it won't be 'anything else'

>>> from __future__ import annotations
>>> from typing import Literal
>>> class A:
...     def __add__(self, other: object) -> A:
...         return A()
... 
... class B(A):
...     def __radd__(self, other: object) -> Literal["anything else"]:
...         return "anything else"
... 
... class C(A):
...     def __radd__(self, other: object) -> A:
...         return A()
...         
>>> A() + B()
'anything else'
>>> C() + B()
<__main__.A object at 0x7bf636ef0050>

@KotlinIsland
Copy link
Collaborator

def __radd__(self, other: object) -> Literal["anything else"]:

this is invalid, it's not a subtype of A.__radd__ falling back to A.__add__

what i mean is, if A has __radd__ it must subtype that, otherwise it must subtype A.__add__

@jorenham
Copy link
Contributor Author

this is invalid, it's not a subtype of A.__radd__ falling back to A.__add__

My point is that this actually is valid, at least if we assume conventional LSP rules. So currently, pyright and mypy don't flag this as invalid.

@KotlinIsland
Copy link
Collaborator

mypy does flag it as invalid

and if we consider + to be a compound operation, that can call either or both of __add__ and __radd__. then we have to consider both methods as a single unit in terms of LSP compatability

@jorenham
Copy link
Contributor Author

what i mean is, if A has __radd__ it must subtype that, otherwise it must subtype A.__add__

I'm not sure I follow. Are you suggesting that the __radd__ of a subtype must follow the same rules as if it were overriding a __add__?

@jorenham
Copy link
Contributor Author

then we have to consider both methods as a single unit in terms of LSP compatability

Yea, I think that makes sense.

@KotlinIsland
Copy link
Collaborator

KotlinIsland commented Feb 19, 2025

https://mypy-play.net/?mypy=latest&python=3.12&gist=8a93cbf631e9506c446617d459ce3159

although i don't really understand why this doesn't error:
https://mypy-play.net/?mypy=latest&python=3.12&gist=4d9ba8a294222d05078337e459263b02

@KotlinIsland KotlinIsland changed the title Check that B.__radd__ is a subtype of A.__add__ for B <: A, and support __radd__ priority. Check that B.__radd__ is a subtype of A.__radd__/A.__add__ for B <: A Feb 19, 2025
@KotlinIsland KotlinIsland changed the title Check that B.__radd__ is a subtype of A.__radd__/A.__add__ for B <: A Check that B.__radd__ is a subtype of A.__add__ for B <: A Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected upstream also a bug in pyright/pylance or feature that isn't in pyright/pylance - they refused to address it type checking / linting issues relating to existing diagnostic rules or proposals for new diagnostic rules
Projects
None yet
Development

No branches or pull requests

4 participants