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

Allow calls to overloaded functions when all possible combinations of union type parameters resolve to valid overloads #17471

Open
Igorbek opened this issue Jul 28, 2017 · 18 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@Igorbek
Copy link
Contributor

Igorbek commented Jul 28, 2017

TypeScript Version: 2.4.1

Code

Case 1

// overloaded functions
declare function f(a: number): number;
declare function f(a: string): string;

function g<T extends number | string>(a: T) {
    return f(a); // either of arguments is acceptable
//           ~
// Argument of type 'T' is not assignable to parameter of type 'string'.
//  Type 'string | number' is not assignable to type 'string'.
//    Type 'number' is not assignable to type 'string'.
}

Case 2

declare function f2<T>(a: Promise<T>): number;
declare function f2<T>(a: T): string;

function g2<T>(a: T) {
    const result: string = f2(a); // first overload is ignored, but the argument can be a Promise
    return result;
}

Expected behavior:

Case 1: compiles with no errors
Case 2: error that type string | number is not assignable to type string

Actual behavior:

Case 1: error

Argument of type 'T' is not assignable to parameter of type 'string'.
  Type 'string | number' is not assignable to type 'string'.
    Type 'number' is not assignable to type 'string'

Case 2: compiles without errors

Note

I remember here were some discussions about that, so that verifying all possible paths may result in N*M complexity (N overloads, M constituent types in unions). I could not find it.

The second case seems unsafe at all, because skips a possibly valid overload which may effect on return type. I expect that f2(a) would be of type number | string because either of these two overloads can play. It actually has the same result with a: any.

@Igorbek Igorbek changed the title Unsafe function overload resolution applied with generic arguments Unsafe function overload resolution when applied with generic arguments Jul 28, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Jul 28, 2017

If you say a generic without any constraints, like <T> then anything can be assigned to it, therefore the first overload pattern is ignored and the second one is selected. In the first example, to compare apples to apples you would be better to write:

declare function f(a: Promise<any>): number;
declare function f(a: any): string;

@Igorbek
Copy link
Contributor Author

Igorbek commented Jul 28, 2017

Why is first overload ignored? If T can accept anything, including Promise<X> that means, in run time, the first overload would also possibly be selected.

declare function f<T>(a: Promise<T>): number;
declare function f<T>(a: T): string;

const g = <T>(a: T) => f(a); // g is a simple proxy to f

declare const p: Promise<number>;

f(p); // first overload, number in compile-time, number in run-time
g(p); // g uses second overload of f in compile-time, so the result is of type string in compile-time
      // in fact, it is number in run-time

@kitsonk
Copy link
Contributor

kitsonk commented Jul 28, 2017

What is f2 and g doesn't have any overloads. Your code isn't matching what you are trying to explain.

@Igorbek
Copy link
Contributor Author

Igorbek commented Jul 28, 2017

@kitsonk sorry, it was a typo. I've updated the sample code and added more explanation.

@DanielRosenwasser
Copy link
Member

I remember here were some discussions about that, so that verifying all possible paths may result in N*M complexity (N overloads, M constituent types in unions). I could not find it.

Here you go! #1805.

@DanielRosenwasser
Copy link
Member

I was talking to @ahejlsberg about something like this a few days ago. Maybe we can bring it up again.

@DanielRosenwasser DanielRosenwasser added the In Discussion Not yet reached consensus label Jul 28, 2017
@Igorbek
Copy link
Contributor Author

Igorbek commented Jul 28, 2017

@DanielRosenwasser that would be great. I've walked through referenced #1805, and it seemed to only cover mostly case 1, and completely missing case 2 where you cannot really control function application.
When case 1 is just unfortunate, case 2 is really a source of compile-time and run-time inconsistency.

@RyanCavanaugh RyanCavanaugh added the Suggestion An idea for TypeScript label Jul 28, 2017
@simonbuchan
Copy link

Current real world example: the current typings for react mean that React.createElement() cannot take a ReactType = keyof JSX.IntrinsicElements | ComponentType or ComponentType<P = any> = StatelessComponent<P> | ComponentClass<P>, even though it can take keyof JSX.IntrinsicElements or StatelessComponent<P> or ComponentClass<P>. This is pretty confusing!

@Igorbek
Copy link
Contributor Author

Igorbek commented Aug 22, 2017

@tycho01 pointed that the issue was identified before in #12424 (comment) by @jcalz when discussing #6606

@simonbuchan
Copy link

That is delaying overload resolution until generic parameters are bound, this is synthesizing a union of possible result types when overload resolution would otherwise fail, they don't seem that similar?

@simonbuchan
Copy link

Oh, reread OP, maybe it's worth opening what I thought it was as a new issue?

@simonbuchan
Copy link

Sorry for the spam: nope, it's just #1805

@Igorbek
Copy link
Contributor Author

Igorbek commented Aug 22, 2017

@simonbuchan my point for claiming they're related is that they have the same overlooked case where run-time behavior that expressed by overloads is not reflected in the type system respectfully. However I agree that it may be worth to distinguish due to different implementation paths.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 23, 2017

The generics at the repro in 12424 were unnecessary:

interface Match {
  (o: object): 0;
  (o: any): 1;
}
type Wrap = <T>(v: T) => Match(T);
type A = Wrap(RegExp);
// falls thru to 1, `object` checked not with generic val `RegExp` but with its constraint (generic `any`)

(I'm using #17961 though that wasn't needed)

If I comment A it no longer evaluates the overloads, so at least it doesn't do it that early.

It appears the Wrap evaluation gets called inside of the A evaluation's resolveCallExpression though, which does in fact have access to that RegExp. Now to figure out how to propagate that such that it won't just default to the implicit any constraint on T in the checker's checkApplicableSignature...

Edit: the type parameter's type seems to propagate through the SymbolTable. Wonder if that could help.

Edit:

So our call stack, from deep to shallow, goes like this:

checkApplicableSignature
chooseOverload
resolveCall
resolveCallExpression
resolveSignature
getResolvedSignature
checkCallExpression
checkExpressionWorker
checkExpression
getTypeFromTypeCallNode
getTypeFromTypeNode
getSignatureReturnTypeFromDeclaration
getSignatureFromDeclaration
getSignaturesOfSymbol
resolveAnonymousTypeMembers
resolveStructuredTypeMembers
getSignaturesOfStructuredType
getSignaturesOfType
resolveCallExpression

The point to note there is how it loops from the resolveCallExpression of the outer call (bottom of the stack, Wrap(RegExp)) to the same function for the inner call (near the top of the stack, Match(T)). So the checkApplicableSignature is where our signature gets misjudged, because it hasn't taken into account the argument types and type arguments of our outer call, which influence the outer bound (~> constraint) of the type parameters (here just <T>) for outer function Wrap.

So the question is how to pass this info in from the top resolveCallExpression where it's available to where it's needed, 18 stack layers away.

For type arguments passed in from the top (here none, as T is implicitly inferred), the best I can think of would be to like pass an altered function node in such that the type parameters (or rather, their constraints) would be pre-filled from the type args explicitly passed in from the top call. Not sure if it'd work.

The bigger question seems regular argument's types. I'd want to just similarly inject them, but it's no longer a 1:1 mapping to type parameters like it is for type arguments. Would anyone know how the type parameters are normally filled out in a call signature? In checkApplicableSignature for one I don't even see mention of them.

@gcnew
Copy link
Contributor

gcnew commented Aug 24, 2017

For me the problem is the way Wrap type-checks. There is an obvious information loss there. I see two solutions:

  • either allow the type parameter T to stay unconstrained as is now, but resolve the call to Match as a union of all possible returns
  • or, somehow propagate the overload to the caller. This means that Wrap itself should become an overloaded function.

The first option is not very useful for type level programming. The second option seems hard to implement and not in line with the current design.

Edit:
Looking at OPs case 2 it looks like both options should work in conjunction. Resolving the result of an overloaded function call as a union of all possible returns is crucial for maintaining safety, while propagating the overloaded signature will alleviate some of the false-negatives and will be more precise.

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 24, 2017

@gcnew:

This means that Wrap itself should become an overloaded function.

Hm. The way I looked at it Match(T) shouldn't actually be resolved until the T is known, just like we van do T[K] and have it resolve when it knows what's what. Or at least tricking it into evaluating a version with the info available.
I imagine overload propagation could grow fast.

I mean, resolveCallExpression knows its argument types; I think that should be sufficient to resolve the type parameters' types before having to calculate the return type.

I hope that means applying that type parameter info for the purpose of calculating the return type, by e.g. pre-filling them to use those rather than just the constraints, should do the trick.

I may not seeing the full picture though, so any insights from those who do should help a lot.

@gcnew
Copy link
Contributor

gcnew commented Aug 24, 2017

Hm. The way I looked at it Match(T) shouldn't actually be resolved until the T is known

I think delaying the resolution is basically the same as propagating the overload. In both cases you'd have to compute the constraints to guarantee safety. In the propagation case you'd have the "overload" type at the top level, while in the other, you'd have to follow several indirections. I'm fine with either, as long as "all possible outcomes" (i.e. constraints) are properly computed and checked.

Note: the lazy approach would need bounds on the type variable as is the case with T[K].

@KiaraGrouwstra
Copy link
Contributor

KiaraGrouwstra commented Aug 24, 2017

I think delaying the resolution is basically the same as propagating the overload.

Oh, my interpretation had been that propagating the overload would mean storing the different possible outcomes, which may sound manageable in this toy example, but perhaps less so if we'd be nesting calls with respectively n, i, j overloads, making for n * i * j potential overloads throughout the evaluation from the top level.

I'm thinking we might not necessarily need much more complexity.
Knowing:

  • we just passed RegExp to v
  • inferring from this that T would be RegExp as well

... then if rather than <T>(v: T) => Match(T) we'd be evaluating the return type of signature <T>(v: T extends RegExp) => Match(T) instead, I'm thinking that might already suffice to evaluate to Match(RegExp) -> 0.

@RyanCavanaugh RyanCavanaugh changed the title Unsafe function overload resolution when applied with generic arguments Allow calls to overloaded functions when all possible combinations of union type parameters resolve to valid overloads Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants