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

sharkdp · 2025-02-25T11:54:19Z

Summary

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

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.

sharkdp · 2025-02-25T12:34:35Z

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

@@ -259,11 +259,7 @@ class A:
 class B:
    __add__ = A()

-# TODO: this could be `int` if we declare `B.__add__` using a `Callable` type


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).

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.

MichaReiser

Nice how this ended up simplifying the implementation. I, obviously, didn't review the semantic changes. I'll leave that to someone else.

AlexWaygood

excellent!

AlexWaygood · 2025-02-25T14:25:34Z

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`"


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!)

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.

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.

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.

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.

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

carljm

Excellent!

carljm · 2025-02-25T16:29:56Z

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

@@ -259,11 +259,7 @@ class A:
 class B:
    __add__ = A()

-# TODO: this could be `int` if we declare `B.__add__` using a `Callable` type


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.

carljm · 2025-02-25T16:32:20Z

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`"


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.

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

sharkdp · 2025-02-25T19:14:20Z

...ources/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


@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?

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 :-)

Alright, thanks!

AlexWaygood · 2025-02-25T19:17:26Z

...tic/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): ...`)


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

* main: [red-knot] Rename constraint to predicate (#16382) [red-knot] Correct modeling of dunder calls (#16368) [red-knot] Handle possibly-unbound instance members (#16363)

sharkdp added the red-knot Multi-file analysis & type inference label Feb 25, 2025

sharkdp requested review from carljm, MichaReiser and AlexWaygood as code owners February 25, 2025 11:54

sharkdp commented Feb 25, 2025

View reviewed changes

MichaReiser approved these changes Feb 25, 2025

View reviewed changes

sharkdp mentioned this pull request Feb 25, 2025

[red-knot] Use try_call_dunder infrastructure consistently #16371

Open

AlexWaygood approved these changes Feb 25, 2025

View reviewed changes

carljm approved these changes Feb 25, 2025

View reviewed changes

Base automatically changed from david/possibly-unbound-instance-members to main February 25, 2025 19:00

sharkdp added 5 commits February 25, 2025 20:04

[red-knot] Correct modeling of dunder calls

6e5af88

Clarifying comment (and additional test) as to why we union with Unknown

1a7e9df

Add reference to descriptor guide find_name_in_mro

726b448

Class is instance of its metaclass

1e926ea

Adapt snapshot tests

001033c

sharkdp force-pushed the david/dunder-calls branch from b2a24ef to 001033c Compare February 25, 2025 19:10

sharkdp commented Feb 25, 2025

View reviewed changes

AlexWaygood reviewed Feb 25, 2025

View reviewed changes

Call __getitem__ on instance for demonstration purposes

e907248

sharkdp merged commit 86b01d2 into main Feb 25, 2025
21 checks passed

sharkdp deleted the david/dunder-calls branch February 25, 2025 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sharkdp commented Feb 25, 2025 •

edited

Loading

sharkdp Feb 25, 2025 •

edited

Loading

carljm Feb 25, 2025

MichaReiser left a comment

AlexWaygood left a comment

AlexWaygood Feb 25, 2025

sharkdp Feb 25, 2025

carljm Feb 25, 2025

sharkdp Feb 25, 2025 •

edited

Loading

carljm Feb 25, 2025

carljm left a comment

carljm Feb 25, 2025

carljm Feb 25, 2025

sharkdp Feb 25, 2025

AlexWaygood Feb 25, 2025 •

edited

Loading

sharkdp Feb 25, 2025

AlexWaygood Feb 25, 2025

	# 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`"

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

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

Conversation

sharkdp commented Feb 25, 2025 • edited Loading

Summary

Test Plan

sharkdp Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

AlexWaygood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Feb 25, 2025 •

edited

Loading

sharkdp Feb 25, 2025 •

edited

Loading

sharkdp Feb 25, 2025 •

edited

Loading

AlexWaygood Feb 25, 2025 •

edited

Loading