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

Inference from multiple signatures produces unknown instead of any #31189

Closed
sandersn opened this issue May 1, 2019 · 13 comments · Fixed by #31515
Closed

Inference from multiple signatures produces unknown instead of any #31189

sandersn opened this issue May 1, 2019 · 13 comments · Fixed by #31515
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@sandersn
Copy link
Member

sandersn commented May 1, 2019

#30637 breaks webpack in 3 places in a method chain starting with anys.filter(Boolean).reduce(.... Discovered by the user tests.

Note that this stops happening when there is only one signature. As soon as there are multiple signatures of either kind, it repros.

Code
(Edit: switched to isolated repro)

interface Bullean { }
interface BulleanConstructor {
    new(value?: any): Bullean;
    <T>(value?: T): value is T;
}

interface Ari<T> {
    filter<S extends T>(callbackfn: (value: T) => value is S): Ari<S>;
    filter(callbackfn: (value: T) => unknown): Ari<T>;
}
declare var Bullean: BulleanConstructor;
declare let anys: Ari<any>;
var xs: Ari<any>;
var xs = anys.filter(Bullean); // error in 3.5: Ari<unknown>, from first overload of filter.

h/t @RyanCavanaugh for the type name Bullean

Expected behavior:
xs : any[]

Actual behavior:
xs : unknown[]

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label May 1, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.5.0 milestone May 1, 2019
@ExE-Boss
Copy link
Contributor

ExE-Boss commented May 6, 2019

Well, unknown is the better any type, and this would be the expected behaviour with #27265.

@sandersn
Copy link
Member Author

sandersn commented May 21, 2019

Update: here's a more isolated repro that removes the dependency on Array. You also need filter to be overloaded to get any before #30637. Pre-#30637, we don't take the first overload at all, and the second overload is what gives any[] for the return type of filter.

interface Bullean { }
interface BulleanConstructor {
    new(value?: any): Bullean;
    <T>(value?: T): value is T;
}

interface Ari<T> {
    filter<S extends T>(callbackfn: (value: T) => value is S): Ari<S>;
    filter(callbackfn: (value: T) => unknown): Ari<T>;
}
declare var Bullean: BulleanConstructor;
declare let anys: Ari<any>;
var xs: Ari<any>;
var xs = anys.filter(Bullean)

@weswigham Could this be because T ⇏ {} but T ⇒ unknown? I thought our rules for that assignability from type parameters were also incorrect until this change, but maybe not.

@DanielRosenwasser
Copy link
Member

This change can technically fix this specific occurrence of the problem

 interface Bullean { }
 interface BulleanConstructor {
     new(value?: any): Bullean;
-    <T>(value?: T): value is T;
+    <T extends any>(value?: T): value is T;
 }

My hunch is that there's a weird interaction between the subtyping pass for overload resolution, the changes of implicit bounds, and...something else?

@weswigham
Copy link
Member

If I had to guess, it's because everything is a proper subtype of unknown, but our empty object assignability thing for unconstrained type parameters was an assignment-only hack, so didn't affect subtype relation checks, so didn't affect the subtype pass of overload resolution. Now, with an unknown-d signature, there's a signature that can pass in the subtype pass, so we never get to the assignability pass (where we'd choose the any'd signature, since it's first).

@sandersn
Copy link
Member Author

None of those steps are incorrect then. Sigh. In fact they're better because there are fewer hacks.

OK, I'm going to:

  1. Confirm that this is actually what's happening.
  2. See whether this could affect any other parts of the DOM.
  3. Make the change to Boolean's type parameter constraint and see whether it hurts DT/RWC/user test results.

@weswigham
Copy link
Member

Make the change to Boolean's type parameter constraint and see whether it hurts DT/RWC/user test results.

Urrrr.... I'd not consider that a good fix, in the context of #29571 . Reordering the overload would be better, imo.

@sandersn
Copy link
Member Author

Reordering the overloads of Bullean doesn't help in the example. Did you mean the overloads of Ari? Or would the real Boolean and Array overloads be different than the miniature example?

@sandersn
Copy link
Member Author

So far, both master and release-3.4 find their respective signatures on the subtype pass.

@weswigham
Copy link
Member

Oh, right, reordering the overloads isn't going to help the subtype thing. Just change the constructor parameter type from any to unknown instead, maybe?

@sandersn
Copy link
Member Author

nop

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 21, 2019

If I had to guess, it's because everything is a proper subtype of unknown, but our empty object assignability thing for unconstrained type parameters was an assignment-only hack, so didn't affect subtype relation checks, so didn't affect the subtype pass of overload resolution

I'm not sure if the phrasing here is actually alluding to something else, but if there's a hacky assignability check we removed against unconstrained type parameters, why did we remove it?

@sandersn
Copy link
Member Author

sandersn commented May 21, 2019

  1. Yes, the problem is that anyunknown but any{}. Ugh I need a better operator for "is related to" that distinguishes subtype-related versus assignment-related. Here, let's use a caret:

any ⇒^ unknown but any ⇏^ {}

Edit: in response to @DanielRosenwasser, this isn't really a constraint of a type parameter, it's an instantiation of it (Boolean<T>(value: T): value is T gets instantiated with T=any based on inference from filter's callback: (value: T) => value is S). The special case in assignability never got hit anyway; we just chose the second overload of filter during the subtype pass.

  1. Haven't checked for other overload patterns yet.
  2. The additional overload to filter fixes this problem without breaking anything in DT/RWC/user tests.

@sandersn
Copy link
Member Author

Update.

  1. Only other overload pattern in es5.d.ts at least is ReadonlyArray.filter.
  2. I mistakenly changed ReadonlyArray.filter, so now I need to re-run with Array.filter changed and see what breaks. It's considerably more than 0 this time.

@sandersn sandersn added the Fixed A PR has been merged for this issue label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants