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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Feb 25, 2025
+210 −97
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e5af88
[red-knot] Correct modeling of dunder calls
sharkdp Feb 25, 2025
1a7e9df
Clarifying comment (and additional test) as to why we union with Unknown
sharkdp Feb 25, 2025
726b448
Add reference to descriptor guide find_name_in_mro
sharkdp Feb 25, 2025
1e926ea
Class is instance of its metaclass
sharkdp Feb 25, 2025
001033c
Adapt snapshot tests
sharkdp Feb 25, 2025
e907248
Call __getitem__ on instance for demonstration purposes
sharkdp Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions crates/red_knot_python_semantic/resources/mdtest/binary/instances.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,17 @@ class A:
class B:
__add__ = A()

# TODO: this could be `int` if we declare `B.__add__` using a `Callable` type
Copy link
Contributor Author

@sharkdp sharkdp Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We can also get rid of the Unknown by declaring it as A (__add__: A = A()), so I don't think it's worthy a TODO. The test is fine as is, I think. In fact, declaring it using a callable type would "hide" the actual type of this specific callable (A).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think this is less a TODO and more a commentary on why Unknown is in the revealed type, since it may not be obvious at first.

# 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
Expand Down
4 changes: 2 additions & 2 deletions crates/red_knot_python_semantic/resources/mdtest/call/callable_instance.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`"
Copy link
Member

Choose a reason for hiding this comment

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

better might be

Suggested change
# error: 15 [invalid-argument-type] "Object of type `Literal["foo"]` cannot be assigned to parameter 2 (`x`) of bound method `__call__`; expected type `int`"
# error: 15 [invalid-argument-type] "Object of type `Literal["foo"]` cannot be assigned to parameter 2 (`x`) of bound method `C.__call__`; expected type `int`"

(definitely not a blocking comment!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to do something like this at first, but then we probably need to construct a String for callable names instead of relying on str slices. Happy to make that change though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same thought on the previous diff that improved error messages, that ultimately probably we don't need to describe "function" vs "bound method" vs "wrapper-descriptor", we can probably replace all of that with fully-qualified (within the module) function names (e.g. C.f.__get__ already clarifies that this is is the __get__ method of the function f in the class C, and is more similar to how things look in runtime messages). I think this is worth creating an issue for but isn't necessarily urgent.

Copy link
Contributor Author

@sharkdp sharkdp Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

e.g. C.f.__get__ already clarifies that this is is the __get__ method of the function f in the class C

Consider the CPython error messages here (and imagine that everything "above the fold" is not visible in the current view):

class C:
    def f(self, x: int) -> str:
        return str(x)

c = C()

# ------------

# C.f() missing 2 required positional arguments: 'self' and 'x'
C.f()

# C.f() missing 1 required positional argument: 'x'
c.f()

The notation C.f() which CPython uses does not clarify if we are calling the function f or the bound method f. Sure, the self gives it away, but adding that additional piece of information (function vs bound method) seems like it could be potentially helpful.

and is more similar to how things look in runtime messages

It feels to me like innovation in this area is not necessarily a bad thing, as long as we don't digress too far from established norms. We already do provide much better error messages than CPython. But that does not mean that they can't be better still. That's a general comment and not necessarily related to my changes earlier today. I'm happy to revisit them. I can open a ticket to discuss this.

Copy link
Contributor

Choose a reason for hiding this comment

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

adding that additional piece of information (function vs bound method) seems like it could be potentially helpful.

Yes, I agree this is helpful.

innovation in this area is not necessarily a bad thing

And I agree with this too!

I still think qualified name display will help messages be less ambiguous overall, and may allow us to remove some of the distinctions between function kinds -- but I definitely don't think we should closely hew to whatever the runtime does, and I think you're right that specifically distinguishing bound methods may be useful.

reveal_type(c("foo")) # revealed: int
```

Expand All @@ -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
```

Expand Down
128 changes: 128 additions & 0 deletions crates/red_knot_python_semantic/resources/mdtest/call/dunder.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
```
18 changes: 18 additions & 0 deletions ...d_knot_python_semantic/resources/mdtest/comparison/instances/rich_comparison.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
```
2 changes: 1 addition & 1 deletion crates/red_knot_python_semantic/resources/mdtest/loops/for.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion ...ces/mdtest/snapshots/for.md_-_For_loops_-_Possibly-not-callable_`__getitem__`_method.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexWaygood Looks like the half-life of these snapshot tests was rather small 😄. Do these new messages look okay to you? I think they are more correct now, but not necessarily more helpful. If we want to keep the current display-output of bound-method callable types, maybe we should consider reducing the verbosity in a different way here? I can look at this in a follow up, if we think it's worth doing?

Copy link
Member

@AlexWaygood AlexWaygood Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh no, what are you doing to my beautiful not-iterable diagnostics 😆

but yes, no need for the PR to be blocked on this. Ideally I think we'd make several changes to how we talk about callable types in diagnostic messages:

  • I don't think the Literal[f] display is working for us really; I think it's going to be pretty confusing for users. I'd vote for changing the display to <function f>
  • I still think using the fully qualified name (with the module prepended) would be clearer in nearly all cases, and wouldn't make things much more verbose (e.g. <function foo.f>
  • We may want to add a method to FunctionType and the various other CallableTypes that pretty-prints the callable signature ((str, int) -> bytes, or def f(x: str, y: int) -> bytes, or similar), for uses in diagnostics like this. It's obviously no good referring to the function by (qualified) name if there are two functions in the same scope with the same name, but different signatures.

All told, the function-literal display issues were already the weakest part of the not-iterable diagnostics prior to this PR, and I think we need to fix it holistically since it affects our diagnostic rendering in general -- it was already on my mind :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, thanks!

28 | # TODO... `int` might be ideal here?
29 | reveal_type(y) # revealed: int | Unknown
|
Expand Down
4 changes: 2 additions & 2 deletions ...sources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_invalid_`__getitem__`_methods.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand All @@ -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
|

Expand Down
4 changes: 2 additions & 2 deletions .../resources/mdtest/snapshots/for.md_-_For_loops_-_Possibly_invalid_`__iter__`_methods.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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): ...`)
Copy link
Member

Choose a reason for hiding this comment

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

oh no, I didn't think it could get worse than Literal[__iter__, __iter__] 😆

18 | reveal_type(x) # revealed: int
|

Expand All @@ -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
|
Expand Down
4 changes: 2 additions & 2 deletions .../for.md_-_For_loops_-_Possibly_unbound_`__iter__`_and_possibly_invalid_`__getitem__`.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand All @@ -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
|
Expand Down
2 changes: 1 addition & 1 deletion ...ument_type_diagnostics_-_Tests_for_a_variety_of_argument_types_-_Synthetic_arguments.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down
63 changes: 18 additions & 45 deletions crates/red_knot_python_semantic/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)?)
}
Symbol::Type(callable_ty, Boundness::PossiblyUnbound) => {
let call = callable_ty.try_call_bound(db, &self, arguments)?;
Err(CallDunderError::PossiblyUnbound(call))
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::Unbound => Err(CallDunderError::MethodNotAvailable),
}
Expand Down
Loading
Loading