In services, when overload resolution fails, create a union signature

[ghost]

Fixes #19474
Edit: Fixes #19854 too

(getUnionSignatures seems like it would be useful but it returns a Signature[] and I just want a Signature.)

[ghost]

@sandersn Could you review?
Note we should try to avoid making the problem in #19686 any worse than it already is.

[sandersn]

@weswigham has spent considerable time thinking about unions of signatures.

1. How close is this to what he's worked on?
2. How close is his work to being merged into the main path code, instead of the error path?

@weswigham, care to comment on the previous two questions?

[sandersn]

1. I think the pick-longest-signature code would benefit from being in a separate function as well.
2. I think this condition would read better inverted in that case.
3. If you don't do (1) and (2), I would recommend pushing the negation inside some and inverting it to candidate.every(c => !c.typeParameters). ("every not" is easier to read as "none" than "not some")

[sandersn]

The idea seems OK, but I'd like @weswigham to review as well, since he's thought about this a lot more.

[sandersn]

instead of creating a small-but-already-leaky abstraction, what about creating a single-use createCombinedSymbol that accounts for other common code, like the calls to [try]GetTypeOf[Parameter/Rest], mapDefined and getUnionType.

That function would not misrepresent itself in its name, and would make this function considerably more readable.

[sandersn]

linter should catch the double space? I think?

[ghost]

There's no such rule currently. Note that we do equals-sign alignment in a few places so we'd have to change our style to conform to such a rule -- it might be worth it though.

[weswigham]

@sandersn #17819 Works well (if I remember, the only change it had were visible in the .type baselines when we had them and they were always improvements); but that's just for contextual types, not actual overload resolution. I believe we've been hesitant to go so far as to change overload resolution, since in the past we've found people who depend on its current behavior. It's also worth noting that I think this, as written, doesn't just affect services; this could also affect inference and resolution in JS (without checkJS, which I believe also has diagnostic reporting off).

@andy-ms Would you be willing to check that the merged signature creation going on here functions the same as in #17819? It looks like you don't bother with setting the return type, and your rest parameter handling is a bit different, and you don't combine type parameters. If this is just a best-effort thing intended for the LS's quickinfo and such, why not set a flag on the signature we return today and generate the combined signature in the language service? createSignature is (internally) exposed, after all. I think the bar would be much lower for that.

[ghost]

@weswigham It looks like #17819 is doing a different thing than this -- that only modifies getContextualSignature. Meaning it will affect you if you write (x, y) => ... and need a type for x, but won't affect you if you call a function and need a contextual type for the argument.

Since this is for a different use case I'm not actually sure what the correct behavior for using type parameters would be -- type inference failed the first time or we wouldn't get here, so if we synthesized type parameters for the union signature we would have to do type inference again, but that might fail again... I think it's best to avoid that case and just leave it as an error. We can't expect to get a good contextual type from a call to a generic function that fails anyway.
As for the return type I think it would make sense to use an intersection type as in your PR, that way if you have:

declare function f(x: number): number;
declare function f(x: string): string;
f().toUpperCase();

You won't get an "extra" error at toUpperCase() since it's number & string. But you will get an error if you write toUperCase.

We can't accomplish this PR's goal by trying to create a new signature after the fact, because outside of the checker we don't know where a contextual type is coming from.

[typescript-bot]

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.