Refactor CallOutcome to Result

[MichaReiser]

Summary

This PR refactors Type::call to better support the three main use cases:

• To check an explicit call, e.g. a(1, 2): We want to warn about incorrect arguments.
• To check an implicit call, e.g. a += 1, with a:, if a (__bool__), len(a): We want to test if the operation is supported or use a specific error message if the operation is not supported because the type doesn’t implement the expected Protocol or convention (“protocol”) but we don’t want to emit errors about missing or incorrect arguments
• To query the “best known” return type. This is, indirectly, used when evaluating visibility constraints (__bool__) but may also be used to test if a type implements a specific “protocol” or Protocol. We don’t want to emit any errors.

The existing implementation already supported checking explicit calls well. However, it did emit diagnostics for implicit calls when the arguments didn't match the expected signature.

class Manager:
    def __enter__() -> str:
        return "foo"

    def __exit__(self, exc_type, exc_value, traceback): ...

context_expr = Manager()

with context_expr as f:
    reveal_type(f) 

Red Knot emitted a rather confusing error about incorrect arguments before:

crates/red_knot_python_semantic/resources/mdtest/with/sync.md:55 unexpected error: [too-many-positional-arguments] "Too many positional arguments to function `__enter__`: expected 0, got 1"

My main finding is that Type::call should consider the call as failed if there are any errors, including binding errors. This is important for implicit call checking as demonstrated by the error above or when probing if a Class implements a specific dunder method.

Type::call should return an Err if the call fails for any reason and leave the proper handling to the caller. However, it should retain enough information for the call site to create a useful diagnostic. There's a tension here because including very accurate information is expensive to collect and the work is wasted if the caller doesn't care about it (because it only wants to know if the call succeeded). I also think that collecting the appropriate information probably requires knowing the context in which the method was called. I don't have a concrete example but we may want to implement custom diagnostics for when an operator failed and I suspect that collecting the necessary information will be its own side-adventure (@BurntSushi told me that this is at least the case for Rustc).

The main work of this PR is to change Type::call, Type::dunder_call and Type::bound_call to now return a Result and dealing with the fallout from downstream usages. I also had to implement a few workarounds where the new behavior is too accurate. E.g. I had to explicitly ignore binding errors in a few cases where we also did so before. We should tackle those TODOs in separate PRs (my goal was to preserve existing behavior in most places).

Returning a Result is now often less ergonomic on the call site than the old outcome.return_type. This is intentional because using return_type is often wrong. I want to make the decision about what should happen if the call fails explicit: Do we have to emit a diagnostic? Can we ignore certain errors? What's the best recovery logic for this specific inference? That's why it's now necessary to match the variants and call Ok(outcome) => outcome.return_type(db), Err(err) => err.unwrap_return_type(db), or Err(err) => err.return_type(db) to get the return type best suited for this call site.

What's next

We should refactor our other Outcome types to use Result. We should explore whether we can move some methods that take a context and node into infer.rs. We should also consider whether bool, len, and member (and even to_instance) should return a Result,.

[MichaReiser]

I moved this to infer_call_expression because those checks are only relevant when we perform an actual call (It would be surprising if an implicit call, e.g. from a += emits a revealed diagnostic.

[MichaReiser]

I don't feel too confident about the unrolling of the inner call result here. I'd appreciate a careful review (it now moved into CallOutcome::try_call so that I can reuse it between call and call_bound

[MichaReiser]

This is a small change but a not callable type error now takes precedence over a possibly unbound error.

[MichaReiser]

This should be easy now ;)

[AlexWaygood]

A review of the tests. Haven't looked at the code yet ;)

[MichaReiser]

@carljm you might be disappointed by this but I don't think the diagnostics should be part of CallOutcome because they are only relevant if we perform an actual function call. Implicit calls want to use their own custom diagnostics, so it doesn't make sense to expose this call expression specific behavior on the generic CallOutcome.

[carljm]

they are only relevant if we perform an actual function call. Implicit calls want to use their own custom diagnostics

I'm just not convinced that this is universally true. I still think we will probably need some form of nested or chained diagnostics to provide adequate detailed debugging information to the user in a case where something like (for example) an "object is not callable" error is raised as part of an implicit call, e.g. because some non-callable object has been used as the value of some dunder. So I do expect that we will need the equivalent of some of the below diagnostics to be rendered as part of some errors on implicit calls. But it's possible that this could be done just by extracting the necessary diagnostics into standalone functions in diagnostics.rs and calling them from multiple callsites.

[MichaReiser]

That's possible. Although it comes with its own challenges because we don't have a call node. So what do you pass as the node that the highlighting works as expected? Either way. My follow up PRs introduce a pattern where the Err type has methods to emit the corresponding diagnostics and get the return type.

[MichaReiser]

One case that I'm not entirely sure yet how to handle is how to propagate the error for an incorrect bool(argument) call. It's the only case where we use bool today where we need to propagate an error. I expect len to be similar

https://github.com/astral-sh/ruff/blob/6e4cd33f529999d736874cd296e123c83ace3051/crates/red_knot_python_semantic/src/types.rs#L2148-L2154

I see two options:

• This is sort of a binding check. We could check the argument inside of Type::call and push a binding error if bool(argument) isn't a valid operation. The nice thing about that is that it integrates nicely into the rest of call checking.
• We move the checking out from Type::call (we only keep the narrowing of the return type) into infer_expression_call where we duplicate some of the logic.

Edit: This is actually unrelated to bool. It seems we don't type check those constructors at all. So maybe this will automatically fall out from binding the calls?

[MichaReiser]

I'm pretty happy with the test improvements :)

[carljm]

I've only reviewed the tests so far, not the implementation. Submitting this just in case I don't make it through the implementation before I'm done for the day. It's great to see all those operator TODOs removed!

[carljm]

Similar to another case below, this diagnostic message is not right.

"Not callable" means something very specific in Python: the object does not implement the tp_call slot, or the call protocol, or however you want to name it. Both f1 and f2, and the union f1 | f2, are definitely callable, and we should not say otherwise in our diagnostic or error code (so we should not use the call-non-callable rule code).

The problem here is that we failed to bind the call arguments to one of the union elements, which is very different from one of them not being callable. The diagnostic here should ideally tell us what the argument binding error was, and for which union element (i.e. that Literal[3] is not assignable to str). (This is one of the cases where I think two separate "chained" diagnostics might make sense instead of trying to squeeze it all into one message.)

If we want to delay providing this helpful diagnostic as a later improvement, then I think the message we use for now still should not use the words "not callable", it should say something more vague like "unable to call with the given arguments." (I think this vague message would still deserve a TODO comment to make sure we come back and make it useful, but at least it wouldn't be wrong.)

[MichaReiser]

I'd prefer improving this for now because I get the impression that we may also want to rename the lint rule itself to something more generic? But I could see how this itself requires some discussion.

[AlexWaygood]

This looks excellent overall. There's definitely still a few outstanding issues but I'm overall leaning towards adding TODOs for the issues and landing this, then iterating. It's a big PR that is liable to accumulate merge conflicts, and it definitely seems like a big improvement over what we have now

[AlexWaygood]

Is this doc-comment accurate? Both variants of the CallOutcome enum appear to wrap one or more CallBinding instances, and it looks like CallBinding has an errors field:

ruff/crates/red_knot_python_semantic/src/types/call/bind.rs

Lines 149 to 150 in b5cd4f2

	/// Call binding errors, if any.
	errors: Vec<CallBindingError<'db>>,

So it looks like a CallOutcome instance doesn't necessarily represent a call where all arguments are valid? I think it represents a call of an object that is definitely callable, but the arguments to the call were not necessarily of the correct type?

[MichaReiser]

I decided not to split CallOutcome into two versions where one is statically known to not have any errors and one that has. Instead, guaranteeing that the Ok variant never contains errors is left to Type::call. I haven't explored how complicated it would be to split CallBinding but it is something we could look into as a follow up (e.g. bind_call could return a Result too? Although we still want to get a binding even if there are errors)

[AlexWaygood]

I'm not objecting to the code organisation. It seems like a reasonable split to me. I just think the docs need to be updated, because right now the doc comment seems to imply that the code is doing something slightly different to what it's actually doing.

[MichaReiser]

I'm not sure what you mean. We only return CallOutcome if there are no binding errors (all arguments are valid).

[MichaReiser]

As discussed async. @AlexWaygood and I will follow up on this after landing this PR (if it still is an issue)

[carljm]

This is great, I definitely think it's a major improvement over the status quo, thank you for digging into this area!

I agree with Alex that I don't see substantive issues here that can't be TODOed and iterated on later. The main thing I would like to address before landing (mostly because I don't think it's hard to address) is some naming stuff, particularly being consistent about what "not callable" means, both in diagnostics and in code.

[carljm]

they are only relevant if we perform an actual function call. Implicit calls want to use their own custom diagnostics

I'm just not convinced that this is universally true. I still think we will probably need some form of nested or chained diagnostics to provide adequate detailed debugging information to the user in a case where something like (for example) an "object is not callable" error is raised as part of an implicit call, e.g. because some non-callable object has been used as the value of some dunder. So I do expect that we will need the equivalent of some of the below diagnostics to be rendered as part of some errors on implicit calls. But it's possible that this could be done just by extracting the necessary diagnostics into standalone functions in diagnostics.rs and calling them from multiple callsites.

[MichaReiser]

Ha, less code, more tests and higher accuracy!

[MichaReiser]

This patching of the not_callable_ty here seems hacky but we should address this separately

[MichaReiser]

Thank you both for the excellent review.

I'm sorry for deferring so many diagnostic improvements but I see my priority as solving an architectural problem and I should then get back to working on the CLI. I added the requested todos and pushed some improvements to the NotCallable that hopefully addresses all terminology concerns.

There's one behavior change that I want to call out. We now only report the first error if a union variant isn't callable or has binding errors, similar to what TypeScript does (It's no longer "X isn't callable (because of element reason)" but is now "element isn't callable"). This improved some errors but I think we can do better. I added a todo in code.

[carljm]

This seems to me like clearly a regression, not an improvement; it's not clear to me why this change was made. Are there other error messages where it resulted in an improvement?

In any case, we can certainly follow up on this later; I can create an issue for it.

[carljm]

Ah, ok, I suspect it was made to improve cases where we need more detailed errors from "inside" the union (like argument-specific call binding errors). That makes sense, but I continue to feel like in the end we won't be able to get away with choosing just one or the other here (either the union-level info or the per-union-element details), we will ultimately need to be able to chain details.

[MichaReiser]

I think it is more useful in an editor context where you can just hover the type. But I agree it's not as useful in a CLI context. However, handling union errors is complicated because they're kind of recursive and you probably also don't want to list all errors (because that's overwhelming). This needs some design work.

[MichaReiser]

It's probably worth tagging @BurntSushi because this might be a use case where we want sub-diagnostics (although we still need a way to truncate to avoid going too deep).

@BurntSushi the diagnostic use case we have here is that we want to warn if a user tries to call a type that can't be called with the given arguments. The part where our current system (and what I did in this PR) falls short is if the called type is a union where only some of the variants can't be called (because it isn't a function or the arguments don't match its signature).

I did look at how different type checker handle this case when I started working on this refactor:

Pyright

from ast import Call
from typing import reveal_type


class NotCallable:
    pass

class Callable:
    def __call__(self):
        pass


def coinflip() -> NotCallable | Callable:
    return random.choice([NotCallable(), Callable()])

a = coinflip()

reveal_type(a)

a()

It only reports an error for the not callable variant (similar to what I did in this PR). It does have some form of sub diagnostic.

Object of type "NotCallable" is not callable
  Attribute "__call__" is unknownPylancereportCallIssue

Typescript

class NonCallable {

}

class AlsoNotCallable {

}


let x: NonCallable | AlsoNotCallable | (<T>(a: T) => T) | undefined = a => a;

x("test")

    This expression is not callable.
      Not all constituents of type 'NonCallable | AlsoNotCallable | (<T>(a: T) => T)' are callable.
        Type 'NonCallable' has no call signatures.

Similar to Pyright where it uses sub-diagnostics.

But the quality of errors depends on the use case. E.g. there's no special union handling for iterable:

class MyIterable implements Iterable<number>  {
    [Symbol.iterator](): Iterator<number> {
        return [1, 2, 3, 4]
    }
}



class NotIterable {

}

let a:MyIterable | NotIterable= new MyIterable();

for (let i of a) {
    console.log(i);
}

Type 'MyIterable | NotIterable' must have a '[Symbol.iterator]()' method that returns an iterator.ts(2488)

It doesn't tell you that NotIterable is the problematic element.

Flow

Same examples as for TypeScript

    12: x("test")
        ^ Cannot call `x` because undefined [1] is not a function. [not-a-function]
        References:
        10: let x: NonCallable| (<T>(a: T) => T) | undefined = undefined;
                                                   ^ [1]
    12: x("test")
        ^ Cannot call `x` because a call signature declaring the expected parameter / return type is missing in `NonCallable` [1]. [prop-missing]
        References:
        10: let x: NonCallable| (<T>(a: T) => T) | undefined = undefined;
                   ^ [1]

15: for (let i of a) {
                  ^ property `@@iterator` is missing in `NotIterable` [1] but exists in `$Iterable` [2]. [prop-missing]

    References:

    13: let a:MyIterable | NotIterable= new MyIterable();
                           ^ [1]

    [LIB] ..//try-flow-website-js@0.260.0/flowlib/core.js:1865: interface $Iterable<+Yield,+Return,-Next>{                                                                      ^ [2]
    

[carljm]

I filed #16241 just so we don't lose track of this discussion (and in particular this excellent comment which I linked to from the issue)

[carljm]

The updates look great, thank you! Reads much clearer to me now. And really appreciate all the TODOs for things we want to try to further improve.

[carljm]

Unless we would have an empty union (which is not possible), I don't see how we could ever have empty bindings and not_callable not also be true? In other words, why do we need the not_callable boolean? If I remove it (and leave this as just else if bindings.is_empty()), all tests pass.

[MichaReiser]

Line 47 is important.

You could have a case where one variant is NotCallable and the other variant has a BindingError. We don't want to return NotCallable in this case because we would loose information (and it's not true that the type isn't callable, it's not always callable)

[carljm]

But I can remove the not_callable boolean, and every reference to it (including line 47), and all tests still pass. So if it's important, either we aren't currently handling it correctly or we are missing some needed test.

In the case you describe, bindings will not be empty, so we won't enter this clause to return NotCallable anyway; we'll short-circuit this test and we won't even check the value of the not_callable boolean.

[MichaReiser]

I don't think bindings will be empty because we only push to bindings if the call succeeded (without any binding errors). That we miss some tests is not unlikely

[carljm]

Oh, I see! I missed that we don't push bindings with errors to bindings. Makes sense, I agree then it just looks like we don't have a test for the case where one union variant has a binding error and the other one is not callable (or we do have a test for that case but its assertions fail to distinguish?)