More explicit error message for function signature length mismatches

[bakkot]

Fixes #51429.

Now code like

function takesCallback(fn: (a: number) => void) {
  // ...
}

takesCallback((a: number, b: number) => {});

will get an error like

tests/cases/compiler/signatureLengthMismatchCall.ts(5,15): error TS2345: Argument of type '(a: number, b: number) => void' is not assignable to parameter of type '(a: number) => void'.
  Target signature provides too few arguments. Expected 2 or more, but got 1

instead of just the "is not assignable" part.

outdated commentary

The first commit is straightforward.

The second is less straightforward. All it's doing differentiating between "call signature" and "construct signature", which I'm only doing because other diagnostics look like they make that distinction. Differentiating requires threading through knowledge of which kind of signature is being analyzed, which is why it's a larger change. The factoring is kind of weird, but if you don't care feel free to ignore the remainder of this PR message. Or if a bare "signature" is fine instead of differentiate "call signature" from "construct signature", I can back out the second commit and use that wording instead.

---

The signature kind is also used to derive the earlier incompatibleErrorReporter parameter of compareSignaturesRelated ¹. This factoring is thus a bit weird, because kind is used both to derive a parameter and as a parameter itself. It might make more sense to derive incompatibleReporter inside of signaturesRelatedTo, though that would require moving more code around, and keeps the same number of parameters since the reportIncompatibleConstructSignatureReturn functions close over a value (namely reportIncompatibleError) which would need to get passed in.

I'm happy with any of

• leave it as is in this PR; it's good enough
• reword the error messages so that call and construct signatures do not need to be differentiated
• refactor compareSignaturesRelated so that the logic deriving incompatibleReporter is inside of compareSignaturesRelated
• some other option

Just let me know which you'd prefer.

Footnotes

1. as in

   const incompatibleReporter = kind === SignatureKind.Construct
     ? reportIncompatibleConstructSignatureReturn
     : reportIncompatibleCallSignatureReturn;
   // ...
   compareSignaturesRelated(source, target, /*...*/, incompatibleReporter(source, target), kind)
   

   ↩

[RyanCavanaugh]

I don't like that both types appear twice in the output; when types are long this is really cumbersome. I agree that the call/construct distinction is irrelevant here.

As a starting point, we could try this (please feel free to bikeshed/iterate):

tests/cases/compiler/signatureLengthMismatchCall.ts(5,15): error TS2345: Argument of type '(a: number, b: number) => void' is not assignable to parameter of type '(a: number) => void'.
  The target signature requires more arguments (2) than is provided by the source (1)

Note that correctly computing the arities here is tricky since optional parameters are allowed to satisfy required ones. I recommend this testcase, which should print (2) (1), not (2) (0).

function callee(n: number | undefined, m: string) { }

function caller(arg: (n?: number) => void) { }

caller(callee);

[bakkot]

I don't like that both types appear twice in the output

Happy to make that change, though note that when the types are named it's a lot less obvious. For example, with

type T = (a: number) => void
function takesCallback(fn: T) {
  // ...
}

type T2 = (a: number, b: number) => void;
let fn: T2 = (a: number, b: number) => {};
takesCallback(fn);

adopting your suggestion will mean that the error message will be

Argument of type 'T2' is not assignable to parameter of type 'T'.
  The target signature requires more arguments (2) than is provided by the source (1)

whereas with the current PR it's

Argument of type 'T2' is not assignable to parameter of type 'T'.
  Call signature '(a: number, b: number): void' expects more arguments than call signature '(a: number): void'.

Still, probably fine. I'll push it up (with your test) later today.

[bakkot]

@RyanCavanaugh Done. Two notes:

First, your suggested message had "target" and "source" backwards from how I (and the code) think of them. That is, in x = y, x is the target and y is the source (this assignment fails if y requires more arguments than x). I switched it in my message.

Second, when compareSignaturesRelated is called with SignatureCheckMode.StrictArity, it can reach this path for types like target:(arg: number, arg2: number) => {} + source: (...arg2: number[]) => void. From looking at the code, I think error reporting is never on in those cases, and have added an assert to that effect but I have a check which disables the error report in that case anyway.

But if I'm wrong, or if you'd like me to be more defensive, I could figure out a different error message for that case. (The normal message doesn't really make sense there.) I wouldn't be able to write a test exercising the new message, though.

Or I could simply not generate a message at all in the StrictArity case; it at least would be guaranteed to be no worse than the current state, that way.

Edit: went ahead and switched the assert to a runtime test. It ought to be redundant, but it's basically free and it's on an error case anyway, so it's harmless.

[fatcerberus]

FWIW the source/target distinction is confusing here because of contravariance. Are we talking about the source and target of the assignment, or of the arguments (note the error as written specifically says "arguments", not "parameters")? Even knowing upfront that it's the former, it just feels weird to say that a source having more arguments than its target requires is an error condition. It'd be better if the error could somehow be written to remove this ambiguity.

[bakkot]

it just feels weird to say that a source having more arguments than its target requires is an error condition

The current message says that the source requires more arguments than the target provides, and it seems like that's a pretty natural error condition to me?

I'm definitely open to other wordings if you have a suggestion though. Keep in mind that this gets triggered not just in a = b but also stuff like f(c); where you have function f(x: someFunctionType), so you can't speak of LHS/RHS, which is what I'd otherwise have used.

[fatcerberus]

I would probably say that the source has more required parameters, since “arguments” are the values provided by the caller (hence the ambiguity—the source of those values is the target type spoken of here, and vice versa). This gets rid of the ambiguity, since it doesn’t make any conceptual sense to talk about the “source” of parameters—they are bindings, not values—except as part of a larger source/target type distinction.

[bakkot]

So "The source signature has more required parameters (2) than are provided by the target (1)." ? It seems a little odd to talk about parameters being "provided", though I guess I don't mind too much.

[fatcerberus]

How about just “…then the target [signature] (1)”. “Provided by” feels like unnecessary fluff at that point anyway.

[bakkot]

Hm. I feel like that makes it a harder to tell what the problem is, compared to the original error message. "this requires more things than would be provided" is clearly an error, but "this has more required things than that does" is not clearly an error, to my eyes.

Happy to go whichever way the TS team prefers, though. Or maybe there's another, clearer wording, though I can't think of one.

[bakkot]

Fixed the merge conflict. Friendly ping for reviewers.

[RyanCavanaugh]

The source/target distinction is so confusing here that it even threw me off (I wrote a whole wrong comment explaining why they should be switched).

Bikeshedded with the team a bit and we figured it'd be better to mimic the concrete call mismatch error. We came up with

"Target signature provides too few arguments. Expected 2 or more, but got 1"

Overall the code/baselines look good otherwise, I think we just want the message change. Thanks!

[bakkot]

@RyanCavanaugh Sounds good to me, done.

[bakkot]

@jakebailey Should I keep updating this PR to address new merge conflicts, or is that something the team will take care of when merging? I don't want to add new commits if it'll mess up the merge process.

(I've spent probably 5x more time rebasing this PR than I did writing it, at this point...)

[jakebailey]

It's my fault as I merged #53193 which makes our handling of diagnostic args consistent. If you don't want to solve the conflict, I'm happy to do it for you, but it should only be a single line, I think.

[jakebailey]

Given this changes baselines for a common-ish error, I think I can just fix it and merge it ASAP so that future PRs fail if they don't have this new ellaboration.

[bakkot]

If you're up for taking care of the conflict and merging, please do. I can take care of it within the next couple days otherwise - doesn't look hard, I'm just swamped. Mostly I was worried that pushing a merge commit would confused the PR Backlog automation.

[jakebailey]

@RyanCavanaugh look good?