Report multiple overload errors #32092

sandersn · 2019-06-25T21:43:56Z

Typescript currently reports assignability errors on the last overload for functions with multiple overloads, and it doesn't tell you that the function has more than one overload. Together, these two problems make fixing type errors on functions with overloads extremely frustrating.

This PR fixes the second problem for all overloaded functions, and addresses the first problem for overloaded functions with 3 or fewer overloads, which is 99.32% of them [1].

The resulting error output looks like this (from emacs; the formatting is similar in VS Code and the terminal [3]):

Code: 2755 Category: error

No suitable overload for this call.

Related Information: 

app.tsx:17 Overload 1 of 2, '(props: Readonly<{ x?: any; }>): BigGreeter', gave the following error.
  Argument of type '{ ref: (input: BigGreeter) => void; }' is not assignable to parameter of type 'Readonly<{ x?: any; }>'.
    Property 'ref' does not exist on type 'Readonly<{ x?: any; }>'. [2757]

app.tsx:17 Overload 2 of 2, '(props: { x?: any; }, context?: any): BigGreeter', gave the following error.
  Argument of type '{ ref: (input: BigGreeter) => void; }' is not assignable to parameter of type '{ x?: any; }'.
    Property 'ref' does not exist on type '{ x?: any; }'. [2757]

If there are more than 3 overloads, there are no related spans. Instead you see a single elaboration, which states that it applies to the last overload:

Code: 2755 Category: error

No suitable overload for this call.
  The last overload gave the following error.
    Type 'string' is not assignable to type 'void'.
      Type 'string' is not assignable to type 'void'.

Limitations and quirks:

We report multiple errors, even when some couldn't reasonably apply. It's also possible to guess a single, best overload to report errors on, and this will be obvious to humans in many cases. It's also complex to get right, and annoying when it's wrong.
We only report errors on overloads whose arity could match. This happens as a side effect of the way call resolution exits early for arity mismatch, but is usually the right thing.
We report errors as related spans, which don't show up in VS (and require an extra command to expand in emacs).
We report related spans of errors as related spans of related spans, which don't show up in any editor (or the terminal). [2]
We report multiple errors per overloads when object/array literal elaboration reports multiple errors. In other words words, this program reports four errors, two for each element in the array literal:

declare function f(ns: number[]): void
declare function f(bs: boolean[]): void
f(['hi', 'there'])

I don't think any of these limitations are worth fixing in this PR since it hits a good balance of effort compared to value. Since we do some filtering already, as in (2), it is a good idea to play with a bit more of it at some point. For example, I believe the surprising number of 100+ signature overload sets arise from the on(kind: "string") pattern, which has to be repeated in subclasses. This is one case where filtering by type, probably literal type only, would help a lot. But it's only a tiny percentage of functions.

Fixes #27249

[1]

[2]

[3] Indentation of elaborations of related spans is incorrect on the terminal: #32091

There are still separate errors instead of one + related spans for each sub-error.

In the worst possible way. Now it's sort of ready for creating a DiagnosticMessageTree.

Formatting is wrong, and I might want to format it as non-related spans, but the structure is exactly a tree.

JSX is still broken

The JSX code path stands on its own

sandersn

Some comments to explain where to start reading.

sandersn · 2019-06-25T21:47:00Z

src/compiler/checker.ts

@@ -11688,45 +11697,64 @@ namespace ts {
            return !!(type.flags & TypeFlags.Conditional || (type.flags & TypeFlags.Intersection && some((type as IntersectionType).types, isOrHasGenericConditional)));
        }

-        function elaborateError(node: Expression | undefined, source: Type, target: Type, relation: Map<RelationComparisonResult>, headMessage: DiagnosticMessage | undefined): boolean {
+        function elaborateError(


the bulk of the lines in this PR go to making elaborateError and children handle containingMessageChain and errorOutputContainer the way that checkTypeRelatedTo already does. These two parameters are the mechanism for returning errors rather than immediately logging them, which allows me to build a tree of errors. That actually happens near the bottom of this change in checker.

sandersn · 2019-06-25T21:52:37Z

src/compiler/checker.ts

                    if (target.symbol && length(target.symbol.declarations)) {
-                        addRelatedInfo(resultObj.error, createDiagnosticForNode(
+                        addRelatedInfo(resultObj.errors[resultObj.errors.length - 1], createDiagnosticForNode(


I add related info to the most recently added error. This doesn't display currently.

sandersn · 2019-06-25T21:53:35Z

src/compiler/checker.ts

        }

-        function checkApplicableSignature(
+        function getSignatureApplicabilityError(


the return type of this function flips, from true=applicable, false=not to Diagnostic[]=error(s), undefined=applicable.

sandersn · 2019-06-25T21:54:35Z

src/compiler/checker.ts

@@ -12355,6 +12427,7 @@ namespace ts {
         * @param errorNode The suggested node upon which all errors will be reported, if defined. This may or may not be the actual node used.
         * @param headMessage If the error chain should be prepended by a head message, then headMessage will be used.
         * @param containingMessageChain A chain of errors to prepend any new errors found.
+         * @param errorOutputContainer Return the diagnostic. Do not log if 'skipLogging' is truthy.


skipLogging is new, and prevents immediate logging of the error in checkTypeRelatedTo.

DanielRosenwasser · 2019-06-25T22:17:07Z

Initial message

No suitable overload for this call.

Why are we dropping words? At the very least, I think this would be better.

- No suitable overload for this call.
+ There is no suitable overload for this call.

or maybe even

No overload matched with this call.

If you have an identifier or property access, you can write

No overload of '{0}' was suitable for this call.

Count of candidates

Overload 1 of 2

Are these always guaranteed to have an order? Do they have some relevant ordering within source code? Maybe it makes more sense to elaborate on the original error messages with something like.

There is no suitable overload for this call.
  We found '{0}' likely overloads which you may be trying to use.

Falling back to the last overload

The last overload gave the following error.

Which overload is that? Give me a related span to point me in the direction of the overload if it wasn't applicable, or tell me the signature of the overload.

Arity filtering

We only report errors on overloads whose arity could match. This happens as a side effect of the way call resolution exits early for arity mismatch, but is usually the right thing.

We need to be wary of how this affects editor scenarios. In editor scenarios, we used to check whether a call was still in the process of being typed to make decisions on filtering out higher-arity candidates. That may still be the case.

Display as bare messages

We report errors as related spans, which don't show up in VS (and require an extra command to expand in emacs).
We report errors as related spans, which don't show up in any editor (or the terminal). [2]

I think this might actually be a blocker and we need to find a way to surface this for VS users.

Duplicates on arrays/object literals

We report multiple errors per overloads when object/array literal elaboration reports multiple errors. In other words words, this program reports four errors, two for each element in the array literal:

I don't think that's an acceptable experience and I would really like us not to look stupid here. In these cases I think I as a user would prefer that when no overloads apply, TypeScript should pick an overload, link me to it in a related span, and report as we used to.

DanielRosenwasser · 2019-06-25T22:23:58Z

I should also mention, JSX cases seem to need a bit of improvement, but we chatted about that offline.

Apart from that, I'm really excited about this work!

sandersn · 2019-06-25T23:12:49Z

cc @amcasey

sandersn · 2019-06-25T23:21:49Z

After some in-person discussion:

Message: I like your wording much better.
Count: There is definitely an order, and it's usually easy to understand (except for interface or intersection merging). There might be a better way to word the message.
Fallback to the last overload: 👍 on improving the message here.
Arity filtering: I will leave as-is for now, but I need to make a reliable way to detect being called from quickinfo and remove the arity filtering in that case. Edit: I will check to see how many new signatures show up without the filtering. If it's not many, then it might be fine to remove arity filtering.
Bare messages: I miswrote the second point, but will talk to @amcasey about the first.
Duplicates: We decided to check whether any overload returns multiple errors, and in that case show only one overload -- the one with the fewest errors (or last one if there are multiple with the same number).

sandersn · 2019-06-25T23:29:56Z

After talking to @amcasey in person, it sounds like pre-flattening the error to text will work for VS (and any other editor that doesn't want to support related spans).

I'm going to create a prototype and make sure the experience is OK.
Then I'll figure out the protocol changes needed and ship that as a separate PR in 3.6.

sandersn · 2019-06-26T16:26:45Z

Fun fact: since this PR changes error reporting for all overloads, I can see that we only have 44 / 13,000 tests with errors on overloaded functions.

sandersn · 2019-06-26T18:00:03Z

After talking to @amcasey: the most likely solution is to change the diagnostic protocol to start flattening related spans text into text: string, and add a new property headText: string that is what text is today -- the error without any related spans. That way old editors will see related spans by default in their message text, although editors with related-span support will have to update to use headText instead of text.

If there are multiple diagnostics per signature, choose the signature with the fewer diagnostics to report. If there are more than one with the minimum, choose the latest in the overload set.

weswigham · 2019-06-26T23:20:30Z

I think this might actually be a blocker and we need to find a way to surface this for VS users.

Could always flatten the related spans at the protocol layer. So instead of

Overload 1
  - span 1
  - span 2
Overload 2
  - span 1
  - span 2

you get

Overload 1
span 1
span 2
Overload 2
span 1
span 2

they'll display in-order in the editor, after all.

sandersn · 2019-06-27T16:12:56Z

@weswigham my plan is to do exactly that, but start putting the flattened version in text so that old editors will start seeing related spans as flattened text always. New editors will have to switch to headText to get the text that excludes related spans.

weswigham · 2019-06-27T17:05:45Z

Isn't that going to regress our UI in every non-VS editor (until they all update in a way that to them is just pointless churn)? Seems like we should be able to deal with it differently, like by adding a VS-specific command or, alternatively, by flattening the text before display, in VS...

amcasey · 2019-06-27T17:24:34Z

@weswigham It's not just VS that's affected - it's any version of any editor that consumes the text property. VS is a particularly bad case since people keep using the same version for many years, but any user not on the latest version of their editor (assuming their editor has picked up the changes) will also be affected.

sandersn · 2019-06-27T17:25:04Z

Adding things to VS doesn't fix 3.6 for users of old VS. I believe most of our other users update their editors (or at least their typescript plugins) regularly and that this is not true of VS. I'm going to discuss our options with the VS and VS Code people at our next meeting, so I'll have a better idea after that.

For editors that don't yet support related spans, this will also be an improvement. That includes vim and sublime, as far as I can tell from their respective plugins' source.

Edit: atom and eclipse don't support related spans either. So it's just VS Code and emacs. Oh, maybe webstorm too? I don't have webstorm installed, and I think it's closed source, so I don't know whether it handles related spans.

weswigham · 2019-06-27T17:38:03Z

For editors that don't yet support related spans, this will also be an improvement.

I doubt that can be made as a blanket statement - most of the information in related spans is a related spans precisely because it's too much information for the original error (and is of limited use without a linkified span - file.ts:20:21 And here isn't particularly useful as text)

I'm going to confidently state that we would never consider such a backwards incompatible reversal of protocol behavior (since that would cause everyone currently displaying related spans to display spans once badly in the message with no linkage and a second time correctly in their own UI) for anything other than the pressure applied by VS... And even with that pressure, this is a huge UI visible break compared to most changes we make, and should not be made lightly. Inlining everything into text will break the UI of every current and updated language server client.

amcasey · 2019-06-27T17:46:32Z

My bar is that the experience has to remain reasonable for any editor currently consuming either just text or text and relatedSpans. From a client perspective, the ideal way to do that would be to retain the current output of those properties and put anything differently formatted in a new property. In practice, that might result in duplicate work and ugly server code, so I think @sandersn is looking for a compromise. Without having reviewed the behavior proposed by this PR, I agree that some massaging will probably be required.

weswigham · 2019-06-27T17:50:12Z

Also: why are these overload elaborations related spans anyway? They're not associated with a new location or anything, they're just a lot of text - either they should be displayed, and therefore be inlined in the error (which simply necessitates a kind of flattenDiagnosticInformation that allows detenting), or not... If we think the error is unsolvable without the information, then it should be in the error message proper.

sandersn · 2019-06-27T18:05:32Z

I doubt that can be made as a blanket statement - most of the information in related spans is a related spans precisely because it's too much information for the original error.

I disagree. For the UIs we care most about -- VS Code and the terminal -- related spans display every time the error does. Probably the other editors should behave the same.

Also: why are these overload elaborations related spans anyway?

I think the error should be a tree structure internally, and related spans are the existing method tree structure, so that's what I used. We could create two kinds of tree structure, one for errors and one for related spans. But the value of that is low if we always display related spans.

They're not associated with a new location or anything

The declaration of the overload should be the location. Looks like that's wrong right now, though.

sandersn · 2019-06-28T23:24:52Z

OK, I switched from related spans to a tree of elaborations. @weswigham mind taking a look?

weswigham · 2019-07-01T18:29:12Z

src/compiler/checker.ts

+                        const related = map(
+                            max > 1 ? allDiagnostics[minIndex] : flatten(allDiagnostics),
+                            d => typeof d.messageText === "string" ? (d as DiagnosticMessageChain) : d.messageText);
+                        diagnostics.add(createDiagnosticForNodeFromMessageChain(node, chainDiagnosticMessages(related, Diagnostics.No_overload_matches_this_call)));


Suggested change

diagnostics.add(createDiagnosticForNodeFromMessageChain(node, chainDiagnosticMessages(related, Diagnostics.No_overload_matches_this_call)));

const diags = max > 1 ? allDiagnostics[minIndex] : flatten(allDiagnostics);

const related = map(

diags,

d => typeof d.messageText === "string" ? (d as DiagnosticMessageChain) : d.messageText);

diagnostics.add(createDiagnosticForNodeFromMessageChain(node, chainDiagnosticMessages(related, Diagnostics.No_overload_matches_this_call), reduceLeft(diags, (acc, d) => acc.concat(d.relatedInformation || []), [] as DiagnosticRelatedInformation[])));

?

DiagnosticRelatedInformation doesn't have a property named relatedInformation, just Diagnostic. I'll see whether a cast is safe here.

The cast in that suggestion is on an empty array, just so it's not inferred as a never[] - or were you referencing something else?

In acc.concat(d.relatedInformation || []) : d: DiagnosticRelatedInformation has no property relatedInformation. Only its subtype Diagnostic does.

src/compiler/utilities.ts

fatcerberus · 2019-07-03T16:44:03Z

It's also possible to guess a single, best overload to report errors on, and this will be obvious to humans in many cases. It's also complex to get right

It's so amazing how things that are incredibly obvious to humans can be difficult (or even undecidable!) for computers.

sandersn added 14 commits June 18, 2019 08:30

Initial attempt. Totally doesn't work.

c65d9f2

Working, just not the way I would like

beddf9c

There are still separate errors instead of one + related spans for each sub-error.

Notes on how to return errors from checkTypeRelatedTo

caff266

Merge branch 'master' into report-multiple-overload-errors

e5fd876

Re-add multiple errors

1f6ef05

In the worst possible way. Now it's sort of ready for creating a DiagnosticMessageTree.

Use related spans to form a tree of errors.

afecb87

Formatting is wrong, and I might want to format it as non-related spans, but the structure is exactly a tree.

Share code a bit better

ef0a875

Retain related info for 1-overloads

b85796c

Improve error message and update baselines

720ad5b

Use existing reporting mechanism

ecfa902

Update baselines, except JSX

f20f700

JSX is still broken

Don't double-check JSX calls

66244f7

The JSX code path stands on its own

Merge branch 'master' into report-multiple-overload-errors

67b8ca7

Fix lint and remove done TODOs

b58932e

sandersn commented Jun 25, 2019

View reviewed changes

sandersn added 2 commits June 26, 2019 10:05

Improve error messages and related spans

68968fd

Merge branch 'master' into report-multiple-overload-errors

c0ff286

Do not report multiple diagnostics per signature.

0436cfc

If there are multiple diagnostics per signature, choose the signature with the fewer diagnostics to report. If there are more than one with the minimum, choose the latest in the overload set.

Fix lint, remove overloads

35dda10

Switch DiagnosticMessageChain to be a tree

ba9d8e2

dragomirtitian mentioned this pull request Jun 28, 2019

Improved error message for calling/constructing types #32034

Merged

sandersn added 4 commits June 28, 2019 15:42

Convert related spans to message chains

47bbc95

Merge branch 'master' into report-multiple-overload-errors

c48018f

Fix lint

9778474

Merge branch 'master' into report-multiple-overload-errors

71276a2

sandersn added 3 commits June 28, 2019 16:26

Update baselines with new error numbers

34c4047

Merge branch 'master' into report-multiple-overload-errors

cce2b93

Update baselines

1906142

weswigham requested changes Jul 1, 2019

View reviewed changes

sandersn added 3 commits July 1, 2019 15:06

Address PR comments

f139455

Update baselines

de52797

Merge branch 'master' into report-multiple-overload-errors

e62d84c

sandersn added 5 commits July 3, 2019 09:49

Update baselines

d92e8ea

Remove TODO

722917f

Merge branch 'master' into report-multiple-overload-errors

19b593e

Merge branch 'master' into report-multiple-overload-errors

834a476

Update baselines

2233eba

weswigham approved these changes Jul 8, 2019

View reviewed changes

Update more baselines (?)

05a4e8f

sandersn merged commit fb50920 into master Jul 8, 2019

sandersn deleted the report-multiple-overload-errors branch July 8, 2019 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report multiple overload errors #32092

Report multiple overload errors #32092

sandersn commented Jun 25, 2019 •

edited

Loading

sandersn left a comment

sandersn Jun 25, 2019

sandersn Jun 25, 2019

sandersn Jun 25, 2019

sandersn Jun 25, 2019

DanielRosenwasser commented Jun 25, 2019 •

edited

Loading

DanielRosenwasser commented Jun 25, 2019

sandersn commented Jun 25, 2019

sandersn commented Jun 25, 2019 •

edited

Loading

sandersn commented Jun 25, 2019

sandersn commented Jun 26, 2019

sandersn commented Jun 26, 2019

weswigham commented Jun 26, 2019

sandersn commented Jun 27, 2019

weswigham commented Jun 27, 2019

amcasey commented Jun 27, 2019

sandersn commented Jun 27, 2019 •

edited

Loading

weswigham commented Jun 27, 2019 •

edited

Loading

amcasey commented Jun 27, 2019

weswigham commented Jun 27, 2019

sandersn commented Jun 27, 2019

sandersn commented Jun 28, 2019

weswigham Jul 1, 2019

sandersn Jul 1, 2019

weswigham Jul 1, 2019

sandersn Jul 2, 2019 •

edited

Loading

fatcerberus commented Jul 3, 2019

Report multiple overload errors #32092

Report multiple overload errors #32092

Conversation

sandersn commented Jun 25, 2019 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jun 25, 2019

Choose a reason for hiding this comment

sandersn Jun 25, 2019

Choose a reason for hiding this comment

sandersn Jun 25, 2019

Choose a reason for hiding this comment

sandersn Jun 25, 2019

Choose a reason for hiding this comment

DanielRosenwasser commented Jun 25, 2019 • edited Loading

Initial message

Count of candidates

Falling back to the last overload

Arity filtering

Display as bare messages

Duplicates on arrays/object literals

DanielRosenwasser commented Jun 25, 2019

sandersn commented Jun 25, 2019

sandersn commented Jun 25, 2019 • edited Loading

sandersn commented Jun 25, 2019

sandersn commented Jun 26, 2019

sandersn commented Jun 26, 2019

weswigham commented Jun 26, 2019

sandersn commented Jun 27, 2019

weswigham commented Jun 27, 2019

amcasey commented Jun 27, 2019

sandersn commented Jun 27, 2019 • edited Loading

weswigham commented Jun 27, 2019 • edited Loading

amcasey commented Jun 27, 2019

weswigham commented Jun 27, 2019

sandersn commented Jun 27, 2019

sandersn commented Jun 28, 2019

weswigham Jul 1, 2019

Choose a reason for hiding this comment

sandersn Jul 1, 2019

Choose a reason for hiding this comment

weswigham Jul 1, 2019

Choose a reason for hiding this comment

sandersn Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

fatcerberus commented Jul 3, 2019

sandersn commented Jun 25, 2019 •

edited

Loading

DanielRosenwasser commented Jun 25, 2019 •

edited

Loading

sandersn commented Jun 25, 2019 •

edited

Loading

sandersn commented Jun 27, 2019 •

edited

Loading

weswigham commented Jun 27, 2019 •

edited

Loading

sandersn Jul 2, 2019 •

edited

Loading