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.

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

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

This should be easy now ;)

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

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.

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.

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

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.

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:

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?

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)

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.

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

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

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.

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

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.

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.

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.

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] ..//[email protected]/flowlib/core.js:1865: interface $Iterable<+Yield,+Return,-Next>{                                                                      ^ [2]
    

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)

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.

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)

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.

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

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

I tried writing a test here and realized that the problem with this flag is that it was initialized to true and only updated with |=, meaning it could never become false. Follow-up PR in #16387