Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify arity errors, rewording spread errors #43855

Merged
merged 9 commits into from
Apr 29, 2021
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Apr 28, 2021

Arity errors can produce some really strange messages. When I went back and re-learned the arity error rules, I decided the biggest problem is that spread errors are actually a special case, different from other arity errors: spreads of non-tuples are only allowed when the target parameter is a rest:

declare function fn(x, ...args)
fn(...[1,2,3]) // error
fn(1, ...[2,3]) // ok

This is true regardless of type (module having a tuple type, eg [1,2,3] as const), which is why I didn't annotate any in the example above.

Previously, the error message for a mismatched spread was similar to the normal error: "Expected 2 arguments, but got at least n." This didn't indicate the true problem at all. Add to this that n was wrong for calls with multiple spreads, and that the error span was wrong too. It used the span for "too many arguments", which is nonsensical.

This PR:

  • Issues a new error message for disallowed spreads.
  • Doesn't mention any parameter counts, since they're not relevant.
  • Uses the spread as the error span.
  • Does not attempt to add new call resolution rules for non-tuple spreads. (I have a couple of ideas here, but I'm not sure how widely applicable or feasible they are.)

It also restructures almost all the code in getArgumentArityError and renames most variables. The function had become baroque even for its previous requirements, but simplifying the spread error message allows it to read in a much straighter line than before. I'd recommend viewing this in side-by-side diff view on a wide-ish monitor.

Fixes #42891
Fixes #28010
Addresses #42508 but does not fix it. I added its test case for this PR.

I'm going to experiment with adding non-tuple spread call resolution rules next.

The second test actually requires node types
The two simple fixes, in arity error reporting, are in, and the
simplification of arity error reporting is half-done. I haven't started
on any improvements to call assignability.
And reword error a little.
@sandersn
Copy link
Member Author

@amcasey @andrewbranch I added you to the review because you usually care about error messages, and because you were involved in the precipitating bugs. You don't necessarily need to review the code, but of course you are welcome.

@DanielRosenwasser
Copy link
Member

A spread must either have a tuple type or be passed to a rest parameter.

So this is true today - however, I can totally imagine a future where the following could be allowed:

function foo<T extends [string, number, boolean]>(stuff: T) {
    bar(...stuff)
}

function bar(x: string, y: number, z: boolean) {
}

and so I guess the core of this change depends on us always rejecting this code.

@sandersn
Copy link
Member Author

The core of this change is to stop reporting confusing param/arg indices where we should be reporting an arbitrary limitation on spreads. I'm not sure I've explained that limitation well in the error. And the explanation may need to change if the limitation loosens in the future.

@sandersn
Copy link
Member Author

@typescript-bot run dt

(Unrelated to this PR, I want to see if my DT on-demand fix worked)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 28, 2021

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 8a265fb. You can monitor the build here.

@andrewbranch
Copy link
Member

Addresses #42508 but does not fix it. I added its test case for this PR.

I'm going to experiment with adding non-tuple spread call resolution rules next.

To clarify, are you saying that #42508 should be a wontfix / design limitation, or are you saying you still hope to fix it soon, but in a separate PR?

@sandersn
Copy link
Member Author

sandersn commented Apr 28, 2021

I hope to fix #42508 soon. I'm going to try treating [number, number] | [number, number, number] like [number, number, number?] but it may not work out. I don't expect any fix I make there to generalise to all non-tuple spreads.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this closes #28010 since the decision was just to clean up the nonsense error, which this does.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Apr 28, 2021
Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the explanation may need to change if the limitation loosens in the future.

It would be easy to forget to update this resource string, so it would be nice if we could come up with text that would continue to work if Daniel's plausible change is made.

Otherwise, seems sensible. I only skimmed the product changes.

src/compiler/checker.ts Show resolved Hide resolved
tests/baselines/reference/callWithSpread3.errors.txt Outdated Show resolved Hide resolved
@sandersn
Copy link
Member Author

I'll line up a time to talk with @DanielRosenwasser and see if we can come up with a message that is forward-compatible, although I think the current state is already close to fit for that purpose.

@sandersn
Copy link
Member Author

A draft of a couple more expansions for spread arguments is up at #43882. However, after implementing the prototype, I'm even less confident that they're correct.

@sandersn sandersn merged commit 004b3ae into master Apr 29, 2021
@sandersn sandersn deleted the reform-arity-errors branch April 29, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error messages: expected N arguments Expected at least x arguments, but got x or more
5 participants