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

Unexpected behavior of generic constraints #3410

Closed
kseo opened this issue Jun 8, 2015 · 15 comments
Closed

Unexpected behavior of generic constraints #3410

kseo opened this issue Jun 8, 2015 · 15 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Fixed A PR has been merged for this issue

Comments

@kseo
Copy link

kseo commented Jun 8, 2015

The following code snippet compiles without type errors though the type variable T is constrained to string type.

function foo1(f: (s: string) => string)
{
    return f("hello world");
}

function foo2(f: (s: number) => number)
{
    return f(123);
}

function genericBar<T extends string>(arg: T): T
{
    return arg;
}

var x1 = foo1(genericBar);
var x2 = foo2(genericBar);

Non generic version nonGenericBar works as expected.

function nonGenericBar(arg: string)
{
    return arg;
}

var y1 = foo1(nonGenericBar);
var y2 = foo2(nonGenericBar);  // Type error

Of course, genericBar function is useless because constraining a type variable to a primitive type can be replaced by a non generic function like nonGenericBar. However, the behaviour is somewhat unexpected and inconsistent.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 13, 2015

Pinging @JsonFreeman

@JsonFreeman
Copy link
Contributor

Our assignability relation ignores generics and constraints in signatures. It just replaces all type parameters with any.

@JsonFreeman JsonFreeman added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Jun 13, 2015
@JsonFreeman
Copy link
Contributor

I will add that we ignore generics because we believe taking them into account will be slow. And in general it leads to never-ending recursion if the signature was in a generic type. Because of this it seems not worth it.

@fernandezpablo85
Copy link

I understand the reasons to not enforce this, but why allow it in the first place?

@RyanCavanaugh
Copy link
Member

why allow it in the first place?

Can you clarify? We "allow" it because we don't do the computation that would tell us whether or not the code might be an error. Given a lack of answer, we can't simply disallow all uses of generics.

@fernandezpablo85
Copy link

Sure. If I understood correctly, generic constraints are replaced with any (as per @JsonFreeman comment above). So given these three cases:

//  fails
function foo<T extends { someNumber: number }>(thingWithNumber: T): number {
    return thingWithNumber.someNumber;
}
[1, 2].map(foo);

// also fails.
interface WithNumber {
    someNumber: number;
}
function bar <T extends WithNumber> (thingWithNumber: T): number {
    return thingWithNumber.someNumber;
}
[1, 2].map(bar);

// works.
function baz(thingWithNumber: WithNumber): number {
    return thingWithNumber.someNumber;
}
[1, 2].map(baz); // compile error

Only the latter works. Seems to me that the <T extends SomeType> should be disallowed, since it does not enforce the type constraint and gives you a false sensation of safety.

@RyanCavanaugh
Copy link
Member

Constraints still provide many errors:

interface Foo {
    x: number;
}

interface Bar {
    y: string;
}

function myFunc<T extends Foo>(a: T): T {
    console.log(a.x); // OK
    console.log(a.y); // Error
    return a;   
}

var f: Foo, b: Bar;

myFunc(f); // OK
myFunc(b); // Error

@fernandezpablo85
Copy link

Oh, okay, so what are the cases where those constraints are not enforced? I could do this in your example:

[b].map(myFunc)

is it only when passing myFunc around?

@JsonFreeman
Copy link
Contributor

When you call a function with a constrained type parameter, that function's type parameter constraint is checked. When you pass a function with a type parameter constraint, the constraint is not checked.

@gcnew
Copy link
Contributor

gcnew commented Mar 21, 2017

May we revisit the current behaviour? I believe another incarnation of this issue is: #12970.

@JsonFreeman
Copy link
Contributor

Yes, both issues share the theme of type parameters being erased for signature comparison.

@masaeedu
Copy link
Contributor

masaeedu commented Apr 16, 2017

@JsonFreeman I've gone into the type checker and simply disabled the condition that asserts for non-genericity of call signatures before allowing their use in contextual typing (can't link to it because checker.ts is too large for Github now, but it's on line 12784 in master @ commit af64ef8). You can see the branch here: https://github.com/Microsoft/TypeScript/compare/master...masaeedu:contextuallytypegenerics?expand=1

This only affects inferred types within the function expression you're assigning/passing, and nowhere else, so I think it is unlikely to have far-reaching effects. Aside from 7 testcases that are deliberately asserting looser inference than can be gleaned, all tests pass, and the use-case I've described in #15016 starts working.

Can you think of cases where this would break, or where performance would be significantly degraded? I'm not sure if there's a performance test suite, but anecdotally, working on the compiler codebase itself doesn't seem any slower.

@JsonFreeman
Copy link
Contributor

@masaeedu As I recall, there are two separate, but related issues here. One is the refusal to use a generic signature to contextually type a function expression. That is what you've changed. The second issue is that when two signatures are compared for assignability, their type parameters are erased to any, which happens after contextual typing. It is this second behavior that's motivated by fear of slowness, or infinite recursion. So changing the contextual typing rules does not seem likely to impact these concerns.

In terms of semantic consequences, two things come to mind. One is that in general, the effects of contextual typing might not be as local as you might think. If you are passing a function expression as an argument to an overloaded function, the way that argument is contextually typed could affect which overload is selected, if it changes the argument's compatibility with particular overloads. This could be something to investigate.

The second question has to do with the function instantiateTypeWithSingleGenericCallSignature in checker.ts. The intent is to flow types by instantiating generic functions in certain situations. Here's an example:

declare function foo<T>(x: T): T;
declare function applyFn<T, U>(arg: T, fn: (x: T) => U): U;
applyFn(0, foo); // Returns number

The contextual signature supplied by fn is not generic in this example. With your change, I'd expect the types to flow even if fn is generic, if the example were something like:

declare function applyFn<T, U>(arg: T, fn: <V extends T>(x: V) => U): U;

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2017

related to #5616

@gcnew
Copy link
Contributor

gcnew commented May 26, 2017

I've put up a PR (#16104) fixing this issue. The observation is that after the inference pass, the inferred signature is type checked separately. In that second, type checking phase, all generic arguments are being erased. However, the type parameters of the signature have already been fixed and we can do better by inferring the arguments against them. The proposed logic will still be correct if proper polymorphic unifications gets implemented, because even if there is an infinite substitution, it will be caught while inferring the signature, i.e. at the type checking step the inference will always be between a generic and a non-generic type.

@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Jun 8, 2017
@mhegazy mhegazy added this to the TypeScript 2.4.1 milestone Jun 12, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
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

No branches or pull requests

8 participants