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

Constraints should be inference sites for other type parameters #7234

Closed
vsiao opened this issue Feb 25, 2016 · 28 comments
Closed

Constraints should be inference sites for other type parameters #7234

vsiao opened this issue Feb 25, 2016 · 28 comments
Assignees
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@vsiao
Copy link

vsiao commented Feb 25, 2016

TypeScript Version:

1.8.0

Code

interface Class<T> {
    new(): T;
}
declare function create1<T>(ctor: Class<T>): T;
declare function create2<T, C extends Class<T>>(ctor: C): T;

class A {}
let a1 = create1(A); // a: A --> OK
let a2 = create2(A); // a: {} --> Should be A

Context

The example above is simplified to illustrate the difference between create1 and create2. I need both type parameters for the use case I have in mind (React) because it returns a type which is parameterized by both T and C:

declare function createElement<T, C extends Class<T>>(type: C): Element<T, C>;
var e = createElement(A); // e: Element<{}, typeof A> --> Should be Element<A, typeof A>

declare function render<T>(e: Element<T, any>): T;
var a = render(e); // a: {} --> Should be A

Again, this is simplified, but the motivation is to improve the return type inference of ReactDOM.render.

@vsiao
Copy link
Author

vsiao commented Feb 25, 2016

#6783 may be related, but I'm not sure.

@vsiao
Copy link
Author

vsiao commented Feb 26, 2016

@RyanCavanaugh any thoughts? Do you think this is a bug?

@vsiao
Copy link
Author

vsiao commented Feb 26, 2016

I should note that the very same issue is preventing me from removing React.Props<T> entirely. Currently, React developers explicitly declare the instance type T, and react.d.ts infers the props type P. It doesn't seem like I can infer both P and T simultaneously:

declare function createElement<P, T extends Component<P, {}>>(
    type: ComponentClass<P>,
    props?: P & { key?: Key, ref?: Ref<T> },
    ...children: ReactNode[]): ReactElement<P>;

interface FooProps {
    foo: string;
}
class Foo extends Component<FooProps, {}> {
    bar() { }
}

// <P, T> inferred as <{}, Foo> --> Should be <FooProps, Foo>
createElement(Foo, {
    ref: foo => foo.bar(), // OK
}); // OK -> Should be ERROR (missing property foo)

cc @jbrantly

@jbrantly
Copy link

Not sure how much I can add atm except to say this is related.

@RyanCavanaugh
Copy link
Member

I need someone more informed on generic type inference to weigh in here.

My speculation is that the root cause is because constraints aren't considered inference sites, but they'd be straightforward to add.

@vsiao vsiao changed the title Type parameter inferred incorrectly when used to constrain other parameters Type parameter constraint should be used as inference site for other type parameters Mar 1, 2016
@vsiao vsiao changed the title Type parameter constraint should be used as inference site for other type parameters Constraints should be inference sites for other type parameters Mar 1, 2016
@vsiao
Copy link
Author

vsiao commented Mar 1, 2016

Thanks, that makes sense. I changed the name of this issue to better reflect the problem.

@JHawkley
Copy link

I was able to fool the type-system to behave properly, at least when working with constructor functions, by using a union type. Modifying @vsiao's example:

interface Class<T> { new(): T; }
// Note the new type for 'ctor': C & (new(...args) => T)
declare function create2<T, C extends Class<T>>(ctor: C & (new() => T)): T;
class A {}
let a2 = create2(A); // a2: A, which is as it should be.

This work-around has a bad smell to it, but its the only thing I've found to work with the 1.8 compiler.

If you end up using it with constructors that require parameters, make sure you use a type that takes them into account, like new(...args) => T, instead.

@vsiao
Copy link
Author

vsiao commented Mar 13, 2016

Wow, great find @JHawkley. That works for my use case.

@vsiao
Copy link
Author

vsiao commented Mar 13, 2016

I guess it's not a complete workaround, though, since no inference is made for C as a constituent of an intersection type.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Mar 15, 2016
@RyanCavanaugh RyanCavanaugh modified the milestones: Community, TypeScript 2.0 Mar 15, 2016
@RyanCavanaugh
Copy link
Member

Accepting PRs on this, or we can take care of it a bit later.

@DanielRosenwasser DanielRosenwasser added the Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". label Mar 17, 2016
@myitcv
Copy link

myitcv commented Apr 5, 2016

I stumbled across this issue with a use case that I think is related... if someone can tell me this is a different case I'll happily raise another issue. Compiling the following with 1.9.0-dev.20160323:

interface HasKeyClass<T extends HasKey<K>, K> {
    new(): T;
}

interface HasKey<T> {
    readonly Key: T;
}

interface F1<H> {
    (v: H): H;
}

function F1<H>(v: H): H {
    return v;
}

function F3<T extends HasKey<K>, K, R>(v: HasKeyClass<T, K>, f: (k: K) => R): R {
    return undefined;
}

class M1 {
    get Key(): string { return "test"; }
}

let x = F3(M1, F1);                       // ERROR: x has type {}; expected string
let y = F3<M1, string, string>(M1, F1);   // OK:    y has type string;
let z = F3<M1, string, number>(M1, F1);   // ERROR: z has type number; expected string

Edit: fixed bug in code, see later comment

@myitcv
Copy link

myitcv commented Apr 6, 2016

I'll reference here that @vsiao's original example (and mine for that matter) are also related to #7848.

Indeed do we need this issue to be fixed before we can attempt #7848?

@myitcv
Copy link

myitcv commented Apr 9, 2016

Thanks to @RyanCavanaugh and @JsonFreeman, I've dug into my example above and a) fixed a bug but also b) separated my issue into two parts.

For the variable x, I believe this to be another instance of the problem originally described in this issue, #7234:

let x = F3(M1, F1);                       // ERROR: x has type {}; expected string

However, when it comes to the variable z, this is an instance of the #138:

let z = F3<M1, string, number>(M1, F1);   // ERROR: z has type number; expected string

Just adding these notes here for the record, in case anyone else encounters this when passing generic functions as arguments (or more generally with assignment of generic functions).

@JsonFreeman
Copy link
Contributor

@myitcv I believe that fixing #138 would make the z line an error. Is that what you mean?

There is one place where we do contextual signature instantiation today, and it is in the context of inference. I believe that should make x a string in your example. My guess is that there is a bug related to F-bounded polymorphism.

Actually, I think you're right that the x line has to do with the present bug.

The original example by @vsiao is interesting. In the call to create2, how is it possible to know (from the invocation) that T should be A? If T is {}, that leads to a consistent answer because K (in this case Class<A>) does indeed extend Class<{}>, so it satisfies the constraint. In other words, K has no inference candidates with respect to values passed in. This is also the problem for x in your more recent example.

@shlomiassaf
Copy link

@JsonFreeman If you debug (TS repo) @vsiao 's example as a test case you'll see that TS knows about A, it also knows that C (Class) has a typeArgument T and that C is a new()=>T.
From all that we can infer T if its not preset, and the other way around infer C if we do:

declare function create2<T, C extends Class<T>>(ctor: new() => T): T;

The missing part here is connecting the 2 types together.

@myitcv
Copy link

myitcv commented Apr 9, 2016

@JsonFreeman

@myitcv I believe that fixing #138 would make the z line an error. Is that what you mean?

Exactly, at least based on my understanding from our other thread.

This is also the problem for x in your more recent example.

Are you referring to this example?

I'll take the issue we think is attributable to #138 out of the equation and rewrite the example:

interface HasKeyClass<T extends HasKey<K>, K> {
    new(): T;
}

interface HasKey<T> {
    readonly Key: T;
}

function F4<T extends HasKey<K>, K>(v: HasKeyClass<T, K>): K {
    return undefined;
}

class M1 {
    get Key(): string { return "test"; }
}

let x1 = F4(M1);  // ERROR: x1 has type {}; should be string

Is it your understanding that x1 should have type {} or string?

Incidentally, I'm not a fan of generic inference ever returning the type {} for a type parameter - I believe, as has been mooted in other threads, if {} is ever inferred (and not explicitly supplied) then the compiler should error. We have just implemented a type checking rule in tsvet that does exactly this (error).

@JsonFreeman
Copy link
Contributor

Yes, that was the example I meant. Like you and many other users, I agree that if it is not possible to do inference correctly, an error is more desirable than inferring {}. I think @DanielRosenwasser tried to introduce errors when there were no inference candidates, and ran into some issues with optional parameters.

I am a little bit confused about your example though. In particular, what is the purpose of T? I could imagine it being written in a different way, with only one type parameter:

interface HasKeyClass<K> {
    new(): HasKey<K>;
}

interface HasKey<T> {
    readonly Key: T;
}

function F4<K>(v: HasKeyClass<K>): K {
    return undefined;
}

class M1 {
    get Key(): string { return "test"; }
}

let x1 = F4(M1);

@myitcv
Copy link

myitcv commented Apr 10, 2016

I think @DanielRosenwasser tried to introduce errors when there were no inference candidates, and ran into some issues with optional parameters.

Can you link me to that issue/PR? I will add to the thread there rather than go off on a tangent on this thread.

I am a little bit confused about your example though. In particular, what is the purpose of T? I could imagine it being written in a different way, with only one type parameter:

You're right, my attempt to reduce this to a minimal example has in fact obfuscated the original use case, which I now reproduce below (to distinguish from previous posts in this thread, I've called it example 3):

// *********
// example 3

interface Class<T> {
    new (...args: Array<any>): T;
}

interface HasKey<T> {
    Key: T;
}

interface Unit<T0> {
    __Type: "Unit";
    V0: T0;
}

interface Pair<T0, T1> {
    __Type: "Pair";
    V0: T0;
    V1: T1;
}

interface Triplet<T0, T1, T2> {
    V0: T0;
    V1: T1;
    V2: T2;
}

interface PairRowClass<T0 extends HasKey<K0>, K0, T1 extends HasKey<K1>, K1, KR> {
    New(v0: T0, v1: T1): PairRow<T0, K0, T1, K1, KR>;
}

interface PairRow<T0 extends HasKey<K0>, K0, T1 extends HasKey<K1>, K1, KR> {
    readonly Key: KR;
    readonly VO: T0;
    readonly V1: T1;
}

function PairRow<T0 extends HasKey<K0>, K0, T1 extends HasKey<K1>, K1, KR>(t0: Class<T0>, t1: Class<T1>, kb: (k0: K0, k1: K1) => KR): PairRowClass<T0, K0, T1, K1, KR> {
    let res: PairRowClass<T0, K0, T1, K1, KR>;
    // impl
    return res;
}

function joinUnitUnit<K0T0, K1T0>(k0: Unit<K0T0>, k1: Unit<K1T0>): Pair<K0T0, K1T0> {
    let res: Pair<K0T0, K1T0>;
    // impl
    return res;
}

function joinPairUnit<K0T0, K0T1, K1T0>(k0: Pair<K0T0, K0T1>, k1: Unit<K1T0>): Triplet<K0T0, K0T1, K1T0> {
    let res: Triplet<K0T0, K0T1, K1T0>;
    // impl
    return res;
}

class M1 {
    get Name(): string { return "Paul"; }
    get Valid(): boolean { return true; }
    get Key(): Pair<string, number> {
        let res: Pair<string, number>;
        return res;
    }
}

class M2 {
    get Key(): Unit<string> {
        let res: Unit<string>;
        return res;
    }
}


// ERROR: x1 has type PairRowClass<M1, {}, M2, {}, Triplet<{}, {}, {}>>
// should be: PairRowClass<M1, Pair<string, number>, M2, Unit<string>, Triplet<string, number, string>>
// believe this is because of #7234
let x1 = PairRow(M1, M2, joinPairUnit);
let eg1 = x1.New(new M1(), new M2());
eg1.Key;  // ERROR: type is Triplet<{}, {}, {}>, should be Triplet<string, number, string>
eg1.VO;   // OK: type is M1
eg1.V1;   // OK: type is M2

// OK: x2 has type PairRowClass<M1, Pair<string, number>, M2, Unit<string>, Triplet<string, number, string>>
let x2 = PairRow<M1, Pair<string, number>, M2, Unit<string>, Triplet<string, number, string>>(M1, M2, joinPairUnit);
let eg2 = x2.New(new M1(), new M2());
eg2.Key;  // OK: type is Triplet<string, number, string>
eg2.VO;   // OK: type is M1
eg2.V1;   // OK: type is M2

let prop1 = eg2.VO.Name; // OK type is string
let prop2 = eg2.VO.Valid; // OK type is string

// ERROR: x3 has type PairRowClass<M1, Pair<string, number>, M2, Unit<string>, Triplet<boolean, boolean, boolean>>
// this should be impossible because of the definition for joinPairUnit; we think this is "allowed" because of #138
let x3 = PairRow<M1, Pair<string, number>, M2, Unit<string>, Triplet<boolean, boolean, boolean>>(M1, M2, joinPairUnit);

// ERROR: x4 has type PairRowClass<M1, {}, M2, {}, Pair<{}, {}>>
// this should be impossible because the resulting Key cannot be of type Pair<K0, K1> given the definitions of M1 and M2
// this is a compound result of #7234 and #138
let x4 = PairRow(M1, M2, joinUnitUnit);

I'll make the following observations about example 3:

Hopefully the requirement for the T* parameters for interfaces PairRow etc is made clear by the RHS for variables prop{1,2}? In these situations it's important the type of the getter on PairRow, e.g. readonly V0 is the concrete type T0 and not the interface HasKey<K0>.

@shlomiassaf
Copy link

@myitcv search is:pr is:closed author:DanielRosenwasser candidates :)

@JsonFreeman
Copy link
Contributor

@shlomiassaf Are you referring to #220? I don't think that is the right issue. I tried searching around but could not find the issue I was thinking of.

@JsonFreeman
Copy link
Contributor

@myitcv Looking at your example, there's some unreferenced type parameters. What if you instead wrote it like this:

interface PairRow<K0, K1, KR> {
    readonly Key: KR;
    readonly VO: HasKey<K0>;
    readonly V1: HasKey<K1>;
}
interface PairRowClass<K0, K1, KR> {
    New(v0: HasKey<K0>, v1: HasKey<K1>): PairRow<K0, K1, KR>;
}
function PairRow<K0, K1, KR>(t0: Class<HasKey<K0>>, t1: Class<HasKey<K1>>, kb: (k0: K0, k1: K1) => KR): PairRowClass<K0, K1, KR> { }

Writing it this way would make it so that in the PairRow function, all the type parameters are somehow observed in the arguments list.

@myitcv
Copy link

myitcv commented Apr 10, 2016

@JsonFreeman

Looking at your example, there's some unreferenced type parameters

Really? I thought I'd checked a few times.. can you point out where?

Or are you talking K0 and K1 in interface PairRow? I didn't think these were unreferenced... because there are effectively referenced in the constraint of T0 and T1.

Writing it in this way...

interface PairRow<K0, K1, KR> {
    readonly Key: KR;
    readonly VO: HasKey<K0>;
    readonly V1: HasKey<K1>;
}

I don't think this works, because we then know nothing about the type of V0:

// 
let eg2: PairRow<Pair<string, number>, Unit<string>, Triplet<string, number, string>>;
eg2.VO.Name; // ERROR

Unless I'm missing something in your response?

@JsonFreeman
Copy link
Contributor

Yes I was referring to K0 and K1 in PairRow. But I see the problem now. You want to capture T0 itself, but you also want access to a particular property in T0, namely K0.

@JsonFreeman
Copy link
Contributor

Just curious, what happens if you use an intersection type instead of a constraint, like this:

function PairRow<T0, K0, T1, K1, KR>(t0: Class<T0 & HasKey<K0>>, t1: Class<T1 & HasKey<K1>>, kb: (k0: K0, k1: K1) => KR): PairRowClass<T0, K0, T1, K1, KR> { }

@myitcv
Copy link

myitcv commented Apr 10, 2016

@JsonFreeman - that fixes K0 and K1 but now T0 and T1 are inferred only as HasKey<K0> and HasKey<K1> respectively:

// example 4: example 3 rewritten to change definition of function PairRow
// all other definitions remain the same
function PairRow<T0 extends HasKey<K0>, K0, T1 extends HasKey<K1>, K1, KR>(t0: Class<T0 & HasKey<K0>>, t1: Class<T1 & HasKey<K1>>, kb: (k0: K0, k1: K1) => KR): PairRowClass<T0, K0, T1, K1, KR> {
    let res: PairRowClass<T0, K0, T1, K1, KR>;
    // impl
    return res;
}

// x1 has type PairRowClass<HasKey<Pair<string, number>>, Pair<string, number>, HasKey<Unit<string>>, Unit<string>, Triplet<string, number, string>>
let x1 = PairRow(M1, M2, joinPairUnit);

// eg1 has type PairRow<HasKey<Pair<string, number>>, Pair<string, number>, HasKey<Unit<string>>, Unit<string>, Triplet<string, number, string>>
let eg1 = x1.New(new M1(), new M2());

// hence the following fails to compile
eg1.VO.Name;  // ERROR: 'Name' does not exist on type 'HasKey<Pair<string, number>>'
eg1.VO.Valid; // ERROR: 'Valid' does not exist on type 'HasKey<Pair<string, number>>'

@JsonFreeman
Copy link
Contributor

Yeah in general, intersection does not work well with bare type parameters. I think something about inference would need to be tweaked for your scenario to work. I actually do prefer the intersection idiom because the type parameters are inferred directly from arguments that are passed in. It is more consistent with the TS philosophy on type argument inference.

@ahejlsberg
Copy link
Member

Note that with the changes in #8821 the pattern of using intersection types to infer multiple type parameters from a single argument now works really well. For example:

interface Class<T> {
    new(): T;
}

interface Item<T, C> { t: T, c: C }

declare function create<T, C>(ctor: C & Class<T>): Item<T, C>;

class A {}

let item = create(A);  // Item<A, typeof A>
item.t;  // A
item.c;  // typeof A

And another example:

interface KeyAndValue<K, V> { key: K, value: V }

interface ObjectWithKey<K> {
    key: K
}

function getKeyAndValue<T, K>(obj: T & ObjectWithKey<K>): KeyAndValue<K, T> {
    return { key: obj.key, value: obj };
}

class Foo {
    key: string;
    data1: number;
    data2: number;
}

class Bar {
    key: number;
    description: string;
}

const kv1 = getKeyAndValue(new Foo());  // KeyAndValue<string, Foo>
const kv2 = getKeyAndValue(new Bar());  // KeyAndValue<number, Bar>

@sandersn
Copy link
Member

After discussing with Anders, I think we'll go with the intersection-type based workaround. I'll add an example to Advanced Types in the handbook so that others can find the workaround.

@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
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests