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 bug #3038

Closed
zpdDG4gta8XKpMCd opened this issue May 5, 2015 · 14 comments · Fixed by #30215
Closed

Inference bug #3038

zpdDG4gta8XKpMCd opened this issue May 5, 2015 · 14 comments · Fixed by #30215
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Fixed A PR has been merged for this issue

Comments

@zpdDG4gta8XKpMCd
Copy link

export function keyOf<a>(value: { key: a; }): a {
    return value.key;
}
export interface Data {
    key: number;
    value: Date;
}

var data: Data[] = [];

export function toKeys<a>(values: a[], toKey: (value: a) => string): string[] {
    return undefined;
}

toKeys(data, keyOf); // <-- actual ok, expected error, since `keyOf` should be inferred as `number` rather than `string`

image

Reproduced at playground: http://www.typescriptlang.org/Playground#src=export%20function%20keyOf%3Ca%3E(value%3A%20%7B%20key%3A%20a%3B%20%7D)%3A%20a%20%7B%0A%09return%20value.key%3B%0A%7D%0Aexport%20interface%20Data%20%7B%0A%09key%3A%20number%3B%0A%09value%3A%20Date%3B%0A%7D%0A%0Avar%20data%3A%20Data%5B%5D%20%3D%20%5B%5D%3B%0A%0Aexport%20function%20toKeys%3Ca%3E(values%3A%20a%5B%5D%2C%20toKey%3A%20(value%3A%20a)%20%3D%3E%20string)%3A%20string%5B%5D%20%7B%0A%09return%20undefined%3B%0A%7D%0A%0AtoKeys(data%2C%20keyOf)%3B%20%2F%2F%20%3C--%20expected%20compile%20error%2C%20since%20keyOf%20should%20have%20inferred%20a%20as%20%60number%60%20rather%20than%20%60string%60%0A

@RyanCavanaugh
Copy link
Member

Simplifying this somewhat:

function keyOf<T>(value: { key: T; }): T {
    return undefined;
}

interface Data {
    key: number;
}

var data: Data[] = [];

function toKeysNonGeneric(values: Data[], toKey: (value: Data) => string): Data {
    return undefined;
}

toKeysNonGeneric(data, keyOf);

Paging @JsonFreeman

@ahejlsberg
Copy link
Member

This actually behaves according to spec. We instantiate the generic keyOf function during type inference (to facilitate making further type inferences from it's return type), but for purposes of assignment compatibility we substitute type any for all type parameters. This behavior has been in place since 1.0. We went that way because of infinite recursion issues relating to contextual signature instantiation with Promises for which we never found meaningful solutions.

@danquirk danquirk added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label May 6, 2015
@danquirk danquirk closed this as completed May 6, 2015
@zpdDG4gta8XKpMCd
Copy link
Author

I understand that inference can be really hard. What I don't understand is why instead of displaying an error message saying it that inference cannot be done, you decided to go a simpler way that somehow compiles making code unsound and leading to nasty bugs.

Are there any plans to fix it?

@JsonFreeman
Copy link
Contributor

I think we are conflating some concepts here. The inference itself succeeds, as it should. In your example, number is the result of the inference. However, the assignability check succeeds unsoundly. As @ahejlsberg explained, the original reason for this unsoundness is indeed that historically, checking this in a sound way led to an infinite recursion. But we have a new way of dealing with infinite recursion that would be resilient to this sound way of checking signatures. So in theory, we could check this in a way that would fail, using contextual signature instantiation.

@zpdDG4gta8XKpMCd
Copy link
Author

@JsonFreeman, so now that you say you have that new way round that infinite recursion problem, do you think we should reopen this issue or the other related one that got closed too #3055?

not sure what it means when you guys close valid issue reports

@JsonFreeman
Copy link
Contributor

In the past, when we had this behavior, we found that many people were confused by it, and it disallowed lots of code that seemed reasonable even though the code was technically not sound. Also, generic signatures that are part of generic types do lead to this deep recursion, and even though we have a way of limiting the depth of the recursion, it would still deepen the assignability checks drastically and make assignability checks take much longer. So while there are no longer technical limitations in the way, fixing this would seem to hamper most users' experiences more than it would benefit them.

@zpdDG4gta8XKpMCd
Copy link
Author

Is there a reason (besides budget) for not having an option to instruct the compiler to do thorough checks?

@JsonFreeman
Copy link
Contributor

I'd say this falls into the category of checks explored by #274. @RyanCavanaugh, is that issue a good meter for the popularity of a "stricter" mode?

@danquirk
Copy link
Member

danquirk commented May 8, 2015

@Aleksey-Bykov Feel free to add a comment in that issue with a small description or link to this issue as far as an additional check.

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Oct 11, 2016

gentlemen, how come the following work?

export function keyOf<a>(value: { key: a; }): a {
    return value.key;
}
export interface Data {
    key: number;
    value: Date;
}

var data: Data[] = [];

export function toKeys<a>(values: a[], toKey: (value: a) => string): string[] {
    return undefined;
}

// BEFORE: toKeys(data, keyOf);
// AFTER:

 toKeys(data, x => keyOf(x)); // <-- fails as expected

@zpdDG4gta8XKpMCd
Copy link
Author

zpdDG4gta8XKpMCd commented Oct 11, 2016

will there be an infinite recursion still if TS replaces point free notation , toKeyOf with an explicit lambda , x => keyOf(x) behind the scene during compiling?

@JsonFreeman
Copy link
Contributor

This happens for the same reason discussed above. The a in keyOf is replaced by any during assignability checks. The reason it works in the explicit lambda case is because the lambda is not generic. So a generic function keyOf is replaced with a non-generic function that makes the inference explicit.

I agree this is not intuitive. Again, I think this qualifies as a good feature for stricter type checking, though arguably it should be part of the basic type check mode as well.

@sergey-shandar
Copy link
Contributor

sergey-shandar commented Oct 11, 2016

@JsonFreeman I vote with two hands for the stricter type checking mode. As far as I know, non strict type checking in TypeScript was one of the reason why Facebook/Flow was created.

@ahejlsberg
Copy link
Member

Fixed in #30215.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants