[red-knot] Correct modeling of dunder calls (#16368)

## Summary

Model dunder-calls correctly (and in one single place), by implementing
this behavior (using `__getitem__` as an example).

```py
def getitem_desugared(obj: object, key: object) -> object:
    getitem_callable = find_in_mro(type(obj), "__getitem__")
    if hasattr(getitem_callable, "__get__"):
        getitem_callable = getitem_callable.__get__(obj, type(obj))

    return getitem_callable(key)
```

See the new `calls/dunder.md` test suite for more information. The new
behavior also needs much fewer lines of code (the diff is positive due
to new tests).

## Test Plan

New tests; fix TODOs in existing tests.

crates/red_knot_python_semantic/resources/mdtest/binary/instances.md

@@ -259,11 +259,17 @@ class A:
 class B:
     __add__ = A()
 
-# TODO: this could be `int` if we declare `B.__add__` using a `Callable` type
-# TODO: Should not be an error: `A` instance is not a method descriptor, don't prepend `self` arg.
-#   Revealed type should be `Unknown | int`.
-# error: [unsupported-operator] "Operator `+` is unsupported between objects of type `B` and `B`"
-reveal_type(B() + B())  # revealed: Unknown
+reveal_type(B() + B())  # revealed: Unknown | int
+```
+
+Note that we union with `Unknown` here because `__add__` is not declared. We do infer just `int` if
+the callable is declared:
+
+```py
+class B2:
+    __add__: A = A()
+
+reveal_type(B2() + B2())  # revealed: int
 ```
 
 ## Integration test: numbers from typeshed

crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md

@@ -82,7 +82,7 @@ class C:
 
 c = C()
 
-# error: 15 [invalid-argument-type] "Object of type `Literal["foo"]` cannot be assigned to parameter 2 (`x`) of function `__call__`; expected type `int`"
+# error: 15 [invalid-argument-type] "Object of type `Literal["foo"]` cannot be assigned to parameter 2 (`x`) of bound method `__call__`; expected type `int`"
 reveal_type(c("foo"))  # revealed: int
 ```
 
@@ -96,7 +96,7 @@ class C:
 
 c = C()
 
-# error: 13 [invalid-argument-type] "Object of type `C` cannot be assigned to parameter 1 (`self`) of function `__call__`; expected type `int`"
+# error: 13 [invalid-argument-type] "Object of type `C` cannot be assigned to parameter 1 (`self`) of bound method `__call__`; expected type `int`"
 reveal_type(c())  # revealed: int
 ```
 

crates/red_knot_python_semantic/resources/mdtest/call/dunder.md

@@ -0,0 +1,128 @@
+# Dunder calls
+
+## Introduction
+
+This test suite explains and documents how dunder methods are looked up and called. Throughout the
+document, we use `__getitem__` as an example, but the same principles apply to other dunder methods.
+
+Dunder methods are implicitly called when using certain syntax. For example, the index operator
+`obj[key]` calls the `__getitem__` method under the hood. Exactly *how* a dunder method is looked up
+and called works slightly different from regular methods. Dunder methods are not looked up on `obj`
+directly, but rather on `type(obj)`. But in many ways, they still *act* as if they were called on
+`obj` directly. If the `__getitem__` member of `type(obj)` is a descriptor, it is called with `obj`
+as the `instance` argument to `__get__`. A desugared version of `obj[key]` is roughly equivalent to
+`getitem_desugared(obj, key)` as defined below:
+
+```py
+from typing import Any
+
+def find_name_in_mro(typ: type, name: str) -> Any:
+    # See implementation in https://docs.python.org/3/howto/descriptor.html#invocation-from-an-instance
+    pass
+
+def getitem_desugared(obj: object, key: object) -> object:
+    getitem_callable = find_name_in_mro(type(obj), "__getitem__")
+    if hasattr(getitem_callable, "__get__"):
+        getitem_callable = getitem_callable.__get__(obj, type(obj))
+
+    return getitem_callable(key)
+```
+
+In the following tests, we demonstrate that we implement this behavior correctly.
+
+## Operating on class objects
+
+If we invoke a dunder method on a class, it is looked up on the *meta* class, since any class is an
+instance of its metaclass:
+
+```py
+class Meta(type):
+    def __getitem__(cls, key: int) -> str:
+        return str(key)
+
+class DunderOnMetaClass(metaclass=Meta):
+    pass
+
+reveal_type(DunderOnMetaClass[0])  # revealed: str
+```
+
+## Operating on instances
+
+When invoking a dunder method on an instance of a class, it is looked up on the class:
+
+```py
+class ClassWithNormalDunder:
+    def __getitem__(self, key: int) -> str:
+        return str(key)
+
+class_with_normal_dunder = ClassWithNormalDunder()
+
+reveal_type(class_with_normal_dunder[0])  # revealed: str
+```
+
+Which can be demonstrated by trying to attach a dunder method to an instance, which will not work:
+
+```py
+def external_getitem(instance, key: int) -> str:
+    return str(key)
+
+class ThisFails:
+    def __init__(self):
+        self.__getitem__ = external_getitem
+
+this_fails = ThisFails()
+
+# error: [non-subscriptable] "Cannot subscript object of type `ThisFails` with no `__getitem__` method"
+reveal_type(this_fails[0])  # revealed: Unknown
+```
+
+However, the attached dunder method *can* be called if accessed directly:
+
+```py
+# TODO: `this_fails.__getitem__` is incorrectly treated as a bound method. This
+# should be fixed with https://github.com/astral-sh/ruff/issues/16367
+# error: [too-many-positional-arguments]
+# error: [invalid-argument-type]
+reveal_type(this_fails.__getitem__(this_fails, 0))  # revealed: Unknown | str
+```
+
+## When the dunder is not a method
+
+A dunder can also be a non-method callable:
+
+```py
+class SomeCallable:
+    def __call__(self, key: int) -> str:
+        return str(key)
+
+class ClassWithNonMethodDunder:
+    __getitem__: SomeCallable = SomeCallable()
+
+class_with_callable_dunder = ClassWithNonMethodDunder()
+
+reveal_type(class_with_callable_dunder[0])  # revealed: str
+```
+
+## Dunders are looked up using the descriptor protocol
+
+Here, we demonstrate that the descriptor protocol is invoked when looking up a dunder method. Note
+that the `instance` argument is on object of type `ClassWithDescriptorDunder`:
+
+```py
+from __future__ import annotations
+
+class SomeCallable:
+    def __call__(self, key: int) -> str:
+        return str(key)
+
+class Descriptor:
+    def __get__(self, instance: ClassWithDescriptorDunder, owner: type[ClassWithDescriptorDunder]) -> SomeCallable:
+        return SomeCallable()
+
+class ClassWithDescriptorDunder:
+    __getitem__: Descriptor = Descriptor()
+
+class_with_descriptor_dunder = ClassWithDescriptorDunder()
+
+reveal_type(class_with_descriptor_dunder[0])  # revealed: str
+```

crates/red_knot_python_semantic/resources/mdtest/comparison/instances/rich_comparison.md

@@ -371,3 +371,21 @@ class Comparable:
 
 Comparable() < Comparable()  # fine
 ```
+
+## Callables as comparison dunders
+
+```py
+from typing import Literal
+
+class AlwaysTrue:
+    def __call__(self, other: object) -> Literal[True]:
+        return True
+
+class A:
+    __eq__: AlwaysTrue = AlwaysTrue()
+    __lt__: AlwaysTrue = AlwaysTrue()
+
+reveal_type(A() == A())  # revealed: Literal[True]
+reveal_type(A() < A())  # revealed: Literal[True]
+reveal_type(A() > A())  # revealed: Literal[True]
+```

crates/red_knot_python_semantic/resources/mdtest/loops/for.md

@@ -321,7 +321,7 @@ def _(flag: bool):
         # TODO... `int` might be ideal here?
         reveal_type(x)  # revealed: int | Unknown
 
-    # error: [not-iterable] "Object of type `Iterable2` may not be iterable because its `__iter__` attribute (with type `Literal[__iter__] | None`) may not be callable"
+    # error: [not-iterable] "Object of type `Iterable2` may not be iterable because its `__iter__` attribute (with type `<bound method `__iter__` of `Iterable2`> | None`) may not be callable"
     for y in Iterable2():
         # TODO... `int` might be ideal here?
         reveal_type(y)  # revealed: int | Unknown

crates/red_knot_python_semantic/resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly-not-callable_`__getitem__`_method.snap

@@ -78,7 +78,7 @@ error: lint:not-iterable
    |
 26 |     # error: [not-iterable]
 27 |     for y in Iterable2():
-   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it has no `__iter__` method and its `__getitem__` attribute (with type `Literal[__getitem__] | None`) may not be callable
+   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it has no `__iter__` method and its `__getitem__` attribute (with type `<bound method `__getitem__` of `Iterable2`> | None`) may not be callable
 28 |         # TODO... `int` might be ideal here?
 29 |         reveal_type(y)  # revealed: int | Unknown
    |

crates/red_knot_python_semantic/resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_invalid_`__getitem__`_methods.snap

@@ -48,7 +48,7 @@ error: lint:not-iterable
    |
 19 |     # error: [not-iterable]
 20 |     for x in Iterable1():
-   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because it has no `__iter__` method and its `__getitem__` attribute (with type `Literal[__getitem__] | None`) may not be callable
+   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because it has no `__iter__` method and its `__getitem__` attribute (with type `<bound method `__getitem__` of `Iterable1`> | None`) may not be callable
 21 |         # TODO: `str` might be better
 22 |         reveal_type(x)  # revealed: str | Unknown
    |
@@ -75,7 +75,7 @@ error: lint:not-iterable
    |
 24 |     # error: [not-iterable]
 25 |     for y in Iterable2():
-   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it has no `__iter__` method and its `__getitem__` method (with type `Literal[__getitem__, __getitem__]`) may have an incorrect signature for the old-style iteration protocol (expected a signature at least as permissive as `def __getitem__(self, key: int): ...`)
+   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it has no `__iter__` method and its `__getitem__` method (with type `<bound method `__getitem__` of `Iterable2`> | <bound method `__getitem__` of `Iterable2`>`) may have an incorrect signature for the old-style iteration protocol (expected a signature at least as permissive as `def __getitem__(self, key: int): ...`)
 26 |         reveal_type(y)  # revealed: str | int
    |
 

crates/red_knot_python_semantic/resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_invalid_`__iter__`_methods.snap

@@ -52,7 +52,7 @@ error: lint:not-iterable
    |
 16 |     # error: [not-iterable]
 17 |     for x in Iterable1():
-   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because its `__iter__` method (with type `Literal[__iter__, __iter__]`) may have an invalid signature (expected `def __iter__(self): ...`)
+   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because its `__iter__` method (with type `<bound method `__iter__` of `Iterable1`> | <bound method `__iter__` of `Iterable1`>`) may have an invalid signature (expected `def __iter__(self): ...`)
 18 |         reveal_type(x)  # revealed: int
    |
 
@@ -78,7 +78,7 @@ error: lint:not-iterable
    |
 27 |     # error: [not-iterable]
 28 |     for x in Iterable2():
-   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because its `__iter__` attribute (with type `Literal[__iter__] | None`) may not be callable
+   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because its `__iter__` attribute (with type `<bound method `__iter__` of `Iterable2`> | None`) may not be callable
 29 |         # TODO: `int` would probably be better here:
 30 |         reveal_type(x)  # revealed: int | Unknown
    |

crates/red_knot_python_semantic/resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_unbound_`__iter__`_and_possibly_invalid_`__getitem__`.snap

@@ -59,7 +59,7 @@ error: lint:not-iterable
    |
 30 |     # error: [not-iterable]
 31 |     for x in Iterable1():
-   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because it may not have an `__iter__` method and its `__getitem__` attribute (with type `Literal[__getitem__] | None`) may not be callable
+   |              ^^^^^^^^^^^ Object of type `Iterable1` may not be iterable because it may not have an `__iter__` method and its `__getitem__` attribute (with type `<bound method `__getitem__` of `Iterable1`> | None`) may not be callable
 32 |         # TODO: `bytes | str` might be better
 33 |         reveal_type(x)  # revealed: bytes | str | Unknown
    |
@@ -86,7 +86,7 @@ error: lint:not-iterable
    |
 35 |     # error: [not-iterable]
 36 |     for y in Iterable2():
-   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it may not have an `__iter__` method and its `__getitem__` method (with type `Literal[__getitem__, __getitem__]`)
+   |              ^^^^^^^^^^^ Object of type `Iterable2` may not be iterable because it may not have an `__iter__` method and its `__getitem__` method (with type `<bound method `__getitem__` of `Iterable2`> | <bound method `__getitem__` of `Iterable2`>`)
                             may have an incorrect signature for the old-style iteration protocol (expected a signature at least as permissive as `def __getitem__(self, key: int): ...`)
 37 |         reveal_type(y)  # revealed: bytes | str | int
    |

crates/red_knot_python_semantic/resources/mdtest/snapshots/invalid_argument_type.md_-_Invalid_argument_type_diagnostics_-_Tests_for_a_variety_of_argument_types_-_Synthetic_arguments.snap

@@ -28,7 +28,7 @@ error: lint:invalid-argument-type
   |
 5 | c = C()
 6 | c("wrong")  # error: [invalid-argument-type]
-  |   ^^^^^^^ Object of type `Literal["wrong"]` cannot be assigned to parameter 2 (`x`) of function `__call__`; expected type `int`
+  |   ^^^^^^^ Object of type `Literal["wrong"]` cannot be assigned to parameter 2 (`x`) of bound method `__call__`; expected type `int`
   |
  ::: /src/mdtest_snippet.py:2:24
   |

crates/red_knot_python_semantic/src/types.rs

@@ -2284,44 +2284,6 @@ impl<'db> Type<'db> {
         }
     }
 
-    /// Return the outcome of calling an class/instance attribute of this type
-    /// using descriptor protocol.
-    ///
-    /// `receiver_ty` must be `Type::Instance(_)` or `Type::ClassLiteral`.
-    ///
-    /// TODO: handle `super()` objects properly
-    fn try_call_bound(
-        self,
-        db: &'db dyn Db,
-        receiver_ty: &Type<'db>,
-        arguments: &CallArguments<'_, 'db>,
-    ) -> Result<CallOutcome<'db>, CallError<'db>> {
-        match self {
-            Type::FunctionLiteral(..) => {
-                // Functions are always descriptors, so this would effectively call
-                // the function with the instance as the first argument
-                self.try_call(db, &arguments.with_self(*receiver_ty))
-            }
-
-            Type::Instance(_) | Type::ClassLiteral(_) => self.try_call(db, arguments),
-
-            Type::Union(union) => CallOutcome::try_call_union(db, union, |element| {
-                element.try_call_bound(db, receiver_ty, arguments)
-            }),
-
-            Type::Intersection(_) => Ok(CallOutcome::Single(CallBinding::from_return_type(
-                todo_type!("Type::Intersection.call_bound()"),
-            ))),
-
-            // Cases that duplicate, and thus must be kept in sync with, `Type::call()`
-            Type::Dynamic(_) => Ok(CallOutcome::Single(CallBinding::from_return_type(self))),
-
-            _ => Err(CallError::NotCallable {
-                not_callable_type: self,
-            }),
-        }
-    }
-
     /// Look up a dunder method on the meta type of `self` and call it.
     ///
     /// Returns an `Err` if the dunder method can't be called,
@@ -2332,13 +2294,24 @@ impl<'db> Type<'db> {
         name: &str,
         arguments: &CallArguments<'_, 'db>,
     ) -> Result<CallOutcome<'db>, CallDunderError<'db>> {
-        match self.to_meta_type(db).member(db, name) {
-            Symbol::Type(callable_ty, Boundness::Bound) => {
-                Ok(callable_ty.try_call_bound(db, &self, arguments)?)
+        let meta_type = self.to_meta_type(db);
+
+        match meta_type.static_member(db, name) {
+            Symbol::Type(callable_ty, boundness) => {
+                // Dunder methods are looked up on the meta type, but they invoke the descriptor
+                // protocol *as if they had been called on the instance itself*. This is why we
+                // pass `Some(self)` for the `instance` argument here.
+                let callable_ty = callable_ty
+                    .try_call_dunder_get(db, Some(self), meta_type)
+                    .unwrap_or(callable_ty);
+
+                let result = callable_ty.try_call(db, arguments)?;
+
+                if boundness == Boundness::Bound {
+                    Ok(result)
+                } else {
+                    Err(CallDunderError::PossiblyUnbound(result))
                 }
-            Symbol::Type(callable_ty, Boundness::PossiblyUnbound) => {
-                let call = callable_ty.try_call_bound(db, &self, arguments)?;
-                Err(CallDunderError::PossiblyUnbound(call))
             }
             Symbol::Unbound => Err(CallDunderError::MethodNotAvailable),
         }

crates/red_knot_python_semantic/src/types/infer.rs

@@ -4177,36 +4177,27 @@ impl<'db> TypeInferenceBuilder<'db> {
                     }
                 }
 
-                // TODO: Use `call_dunder`?
-                let call_on_left_instance = if let Symbol::Type(class_member, _) =
-                    left_class.member(self.db(), op.dunder())
-                {
-                    class_member
-                        .try_call(self.db(), &CallArguments::positional([left_ty, right_ty]))
+                let call_on_left_instance = left_ty
+                    .try_call_dunder(
+                        self.db(),
+                        op.dunder(),
+                        &CallArguments::positional([right_ty]),
+                    )
                     .map(|outcome| outcome.return_type(self.db()))
-                        .ok()
-                } else {
-                    None
-                };
+                    .ok();
 
                 call_on_left_instance.or_else(|| {
                     if left_ty == right_ty {
                         None
                     } else {
-                        if let Symbol::Type(class_member, _) =
-                            right_class.member(self.db(), op.reflected_dunder())
-                        {
-                            // TODO: Use `call_dunder`
-                            class_member
-                                .try_call(
+                        right_ty
+                            .try_call_dunder(
                                 self.db(),
-                                    &CallArguments::positional([right_ty, left_ty]),
+                                op.reflected_dunder(),
+                                &CallArguments::positional([left_ty]),
                             )
                             .map(|outcome| outcome.return_type(self.db()))
                             .ok()
-                        } else {
-                            None
-                        }
                     }
                 })
             }
@@ -4848,19 +4839,16 @@ impl<'db> TypeInferenceBuilder<'db> {
         let db = self.db();
         // The following resource has details about the rich comparison algorithm:
         // https://snarky.ca/unravelling-rich-comparison-operators/
-        let call_dunder = |op: RichCompareOperator,
-                           left: InstanceType<'db>,
-                           right: InstanceType<'db>| {
-            match left.class().class_member(db, op.dunder()) {
-                Symbol::Type(class_member_dunder, Boundness::Bound) => class_member_dunder
-                    .try_call(
+        let call_dunder =
+            |op: RichCompareOperator, left: InstanceType<'db>, right: InstanceType<'db>| {
+                Type::Instance(left)
+                    .try_call_dunder(
                         db,
-                        &CallArguments::positional([Type::Instance(left), Type::Instance(right)]),
+                        op.dunder(),
+                        &CallArguments::positional([Type::Instance(right)]),
                     )
                     .map(|outcome| outcome.return_type(db))
-                    .ok(),
-                _ => None,
-            }
+                    .ok()
             };
 
         // The reflected dunder has priority if the right-hand side is a strict subclass of the left-hand side.