[red-knot] Correct modeling of dunder calls by sharkdp · Pull Request #16368 · astral-sh/ruff

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

 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

 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

 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

 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

 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

 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 ty Multi-file analysis & type inference label Feb 25, 2025

sharkdp requested review from AlexWaygood, MichaReiser and carljm 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 May 7, 2025

Use try_call_dunder infrastructure consistently astral-sh/ty#190

Closed

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

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

Comments

Conversation

sharkdp commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

sharkdp Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharkdp Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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