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

generic parameter with default inferred too strictly since 2.7 #21553

Closed
aomarks opened this issue Feb 1, 2018 · 5 comments
Closed

generic parameter with default inferred too strictly since 2.7 #21553

aomarks opened this issue Feb 1, 2018 · 5 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@aomarks
Copy link

aomarks commented Feb 1, 2018

TypeScript Version: 2.8.0-dev.20180201

Search Terms: generic parameter default infer keyof 2.7

Code

interface X {
  a: string;
  b: string;
}

function foo<T = X>(x: keyof T, y: keyof T) { }

foo<X>('a', 'b');  // compiles cleanly
foo('a', 'b');  // error TS2345: Argument of type '"b"' is not assignable to parameter of type '"a"'.

Expected behavior:
Both calls to foo() compile with no errors.

Actual behavior:
In the second call, when the generic parameter T is not set explicitly, an error occurs: error TS2345: Argument of type '"b"' is not assignable to parameter of type '"a"'.

It seems that since 2.7, the generic parameter is tightly inferred from the first parameter to be something like {a: string}, so the default (which would have been compatible with both parameters) is not used.

This error does not occur in version 2.6.2. It does occur in version 2.7.1 and 2.8.0-dev.20180201.

Playground Link: https://www.typescriptlang.org/play/#src=%0Ainterface%20X%20%7B%0A%20%20a%3A%20string%3B%0A%20%20b%3A%20string%3B%0A%7D%0A%0Afunction%20foo%3CT%20%3D%20X%3E(x%3A%20keyof%20T%2C%20y%3A%20keyof%20T)%20%7B%20%7D%0A%0Afoo%3CX%3E('a'%2C%20'b')%3B%0Afoo('a'%2C%20'b')%3B

Related Issues:

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

On a side note, having a generic parameter that can not be inferred is a bad sign.

@mhegazy mhegazy added API Relates to the public API for TypeScript Bug A bug in TypeScript and removed API Relates to the public API for TypeScript labels Feb 2, 2018
@mhegazy mhegazy added this to the TypeScript 2.8 milestone Feb 2, 2018
@weswigham
Copy link
Member

Yeah, this is totally because inferring from source "a" to target keyof A produces an inference of {a: {}} for A now (which is better than before), whereas before it produced nothing and fell back to the default (because it couldn't produce anything).

@tmkn
Copy link

tmkn commented Mar 1, 2018

I think this is related to the problem I'm experiencing since I upgraded to 2.7.2, this used to work with 2.6.2.:

//TreeNode that takes custom data
interface TreeNode<T> {
    data: T;
}

//TreeView, generic to specify default custom data for TreeNode
interface TreeView<T> {
    //lookup a TreeNode by a key provided in the custom data, default to the one provided for the TreeView
    //property parameter should be limited to actual keys provided by the generic
    getNodeByProperty<K = T>(property: keyof K, value: string): TreeNode<K>
}

//my custom data
interface CustomNodeData {
    id: string;
    name: string;
}

let tree: TreeView<CustomNodeData>;
let node = tree.getNodeByProperty("id", "123"); //node should be TreeNode<CustomNodeData>

with 2.7.2 node is inferred as

TreeNode<{id: any}>

while 2.6.2 inferred the generic default parameter

TreeNode<CustomNodeData>

Additionally in 2.7.2, keyof K to limit the possible values is irrelevant, in fact I can write any string now for property and it is ok, like keyof isn't even there:

let tree: TreeView<CustomNodeData>;
let node = tree.getNodeByProperty("abc", "123");

again, no error, node is then inferred as

TreeNode<{abc: any}>

o_O

if I remove the keyof and use string:

interface TreeView<T> {
    getNodeByProperty<K = T>(property: string, value: string): TreeNode<K>
}

let tree: TreeView<CustomNodeData>;
let node = tree.getNodeByProperty("abc", "123");

node is then inferred as:

TreeNode<CustomNodeData>

I have no idea how or why keyof should change what's inferred

I can get the behavior of 2.6.2 if I change getNodeByProperty to:

interface TreeView<T> {
    getNodeByProperty<K = T, Y extends keyof K = keyof K>(property: Y, value: string): TreeNode<K>
}

But this seems unnecessarily complicated?
Is this how it should work, no?

@weswigham
Copy link
Member

weswigham commented Mar 13, 2018

Additionally in 2.7.2, keyof K to limit the possible values is irrelevant, in fact I can write any string now for property and it is ok, like keyof isn't even there:

That would be because K is unconstrained. A default does not impose a constraint; it just specifies a fallback when there's no possible inferences. What we changed was that we now make inferences in T in keyof T, meaning it won't always fall back to the default. For example, keyof T with a target of "a" | "b" implies T is an object literal with keys "a" and "b" (of unknown type, hence any).

The issue in the OP is maybe still a bug because when there are multiple keyof T inference sites as in his example, it may be desirable to intersect those inference results, rather than just choosing the first, as we are doing now. previously it just always feel back to the default, because we made no inferences, which is why is worked at all before. 😉

@weswigham
Copy link
Member

The simplest form of what you want is probably just:

interface TreeView<T> {
    getNodeByProperty<K extends keyof T>(property: K, value: string): TreeNode<{[KI in K]?: T[KI]}>
}

which states "the first argument is some specific set of the keys of T", and "the return type is a treenode of an object that may have any of the keys specified". You could tighten that further and say "exactly one of the possible keys passed" with a slightly more complex return type, if you needed the extra safety.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Mar 28, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants