microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
99.25k stars 12.31k forks source link

[P in keyof T]: T[P] not accepting inferred base type via extends #37670

Open rodrigok opened 4 years ago

rodrigok commented 4 years ago

TypeScript Version: 3.8.3

Search Terms:

Code

interface IModel {
    id: string;
}

type Query<T> = {
    [P in keyof T]?: T[P]
}

export class Base<T extends IModel> {
    public find(query: Query<T>): void {
        // Forward the query to database
        console.log(query);
    }

    public findOneById(id: string): void {
        this.find({ id });
    }
}

Expected behavior:

It's expected to allow to execute the find with the properties defined on IModel as base of any other model;

Actual behavior:

It shows an error:

Argument of type '{ id: string; }' is not assignable to parameter of type 'Query<T>'.ts(2345)

Playground Link:

https://typescript-play.js.org/#code/JYOwLgpgTgZghgYwgAgJIFkD2ATCAbZAbwCgBIYbALmQGcwpQBzAbmIF9jiwBPABxQCKAV2jcAPABUAfMgC8RMgG0ACslDIA1hG6YYyCQF0A-NQkqD7ThAAevTFDDIEeODRrIAQq4iTkNyCDY7hg4+DIkpLxCAEZ4wAjIMKDYABQAjiJQ3NTCopJSAJTUAG6YFAqkpAD0VcgAYvYA7nBQ2MhgABYoGaLtmMjYcGBw0d5kpAiYIDSYeBAAdHiYjOmZ3AWspBxkUbHxickA8iAQHtyoqRTUdAwgjEXIpeURpJ3ANPNJgSmEam1sGzIHA4QA

Related Issues:

Didn't find any related issue

rodrigok commented 4 years ago

Just for info, my type Query example is a simplified version of what I'm trying to use from here

andriibodryi commented 4 years ago

same issue

Zzzen commented 4 years ago

I guess this is related to #35858

interface IModel {
  id: string;
}

type Query<T> = {
  [P in keyof T]?: T[P]
}

export class Base<T extends IModel> {
  public findOneById(id: string): void {
    this.findT({ id });
//              ^^^^^
// Argument of type '{ id: string; }' is not assignable to parameter of type 'T'.
//  '{ id: string; }' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'IModel'.
  }

  public findT(query: T): void {
    console.log(query);
  }
}
Zzzen commented 4 years ago

This is unlikely to be fixed because TS cannot expand Query in compile time. If Query is replaced with other types, you will find it reasonable. Playground

interface IModel {
  id: string;
}

type Query<T> = {
  [P in keyof T]: string
}

export class Base<T extends IModel> {
  public findOneById(id: string): void {
    this.findT({ id });
  }

  public findOneById2(id: string): void {
    this.findT({ id } as T);
  }

  public findT(query: T): void {
    console.log(query);
  }
}

const b = new Base<{id: string, name: string}>();
b.findT({id: '1', name: '1'})
rodrigok commented 4 years ago

@Zzzen shouldn't TS evaluate T as IModel since every type passed needs to be extended/compatible with IModel?

Zzzen commented 4 years ago

Maybe this simplified example can help you? playground @rodrigok

interface IModel {
  id: string;
}

class Store<T extends IModel> {
  constructor(public data: T) {

  }

  setData(data: T) {
    this.data = data;
  }

  reset() {
    // Although `{ id: '1' }` is an element of `IModel`, `T` could be instantiated with `{ id: string, name: string }`, which `{ id: '1' }` is not compatible with

    //   Argument of type '{ id: string; }' is not assignable to parameter of type 'T'.
    // '{ id: string; }' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'IModel'.
    this.setData({ id: '1' })
  }
}

const storeWithName = new Store<{ id: string, name: string }>({ id: '1', name: 'foo' });
storeWithName.reset();
// Error
// Argument of type '{ id: string; }' is not assignable to parameter of type '{ id: string; name: string; }'.
//   Property 'name' is missing in type '{ id: string; }' but required in type '{ id: string; name: string; }'.
storeWithName.setData({ id: '1' })
storeWithName.setData({ id: '1', name: 'bar' })
rodrigok commented 4 years ago

@Zzzen thanks but it's not quite right since on query typing used by the find/findOne it users the partial of the type, so only id should be valid still.

Playground

interface IModel {
  id: string;
}

export class Base<T extends IModel> {
  public findOneById(id: string): void {
    this.findT({ id });
  }

  public findT(query: Partial<T>): void {
    console.log(query);
  }
}

// Should work
const b = new Base<{id: string, name: string}>();
b.findT({id: '1', name: '1'})
b.findT({id: '1'})
b.findOneById('1');

// Should not work and it doesn't
const c = new Base<{id: number, name: string}>();
c.findT({id: 1, name: '1'})
c.findT({id: 1})
c.findOneById(1);
Zzzen commented 4 years ago

@Zzzen thanks but it's not quite right since on query typing used by the find/findOne it users the partial of the type, so only id should be valid still.

you are right, but that's far beyond the ability of TypeScript.

rodrigok commented 4 years ago

I understand that this is tricky, but I think it's expected from the developer perspective since the extends creates a base definition for the types you can pass it would be expected to be able to use those fields, even if it's needed to pass any other modifier to allow it.

Nathan-Fenner commented 4 years ago

One problem with the original typing is that it doesn't account for subtypes with refined types for id. For example,

const obj = new Base<{ id: "foo" | "bar" }>();
obj.find("abc");

Note that { id: "foo" | "bar" } is a valid subtype of { id: string } according to TypeScript's general rules, and hence if findByOneId type-checked, you'd be passing an arbitrary string as though it were a "foo" | "bar".

In this particular case it's very innocuous, since find doesn't do anything with the id besides compare it to existing ones (so it would simply miss) - but the general typing machinery can't afford to look into the implementation to know whether such a thing is safe. Hence, it would open up even more unsoundness to allow this.

In short, {id: string} does not have to be a subtype of Query<T> or of T just because T is a subtype of {id: string}.

On the other hand, TypeScript rejects the following bit of code, which corrects for this issue. Hence I still think the situation can be improved, just not to support the original type annotations:

export class Base<T extends IModel> {
    public find(query: Query<T>): void {
        // Forward the query to database
        console.log(query);
    }

    public findOneById(id: T["id"]): void {
        this.find({ id });
                 //       ^^^^^^
                 // Argument of type '{ id: T["id"]; }'
                 //is not assignable to parameter of type 'Query<T>'.
    }
}

This does seem to be erroneous, since it's guaranteed that T has a field called "id" since T extends { id: _ }, and therefore { id } should be assignable to a Query<T>, which cannot have any required fields; and the only field we are passing is a T["id"] which surely is exactly the type that Query<T> has at key "id".

benkroeger commented 4 years ago

running into similar issues (also related to mongodb queries). It seems this is only happening on generics.

see Playground Link

might be related to #13995

grumd commented 3 years ago

I'm not sure if my example is an instance of this same issue, but here's a very simple issue I encountered:


type Foo = {
    foo: string;
};

function stuff1<T extends Foo>(arg: string): Partial<T> {
    return {
        foo: arg,
    };
    // Type '{ foo: string; }' is not assignable to type 'Partial<T>'.
}

function stuff2(arg: string): Partial<Foo> {
    return {
        foo: arg,
    };
    // Works fine
}

https://www.typescriptlang.org/play?#code/FAFwngDgpgBA8gIwFYwLwwN7BjmAzAewIC4YBnEAJwEsA7AcwG5gBfZ4PAV1oGMRqCtGABMCAHgAqMKAA8QUWsLLxkAPgAUAQ0r1SFGgwCUpAArb+mgDaTVmbLkpQQnSkKy4P+IqW30ANPY4bKzAQA

Nathan-Fenner commented 3 years ago

@grumd your code has the same bug that I mentioned: T could be {foo: "abc" | "def"} and yet you could pass "xyz" as arg. The resulting value would not be a Partial<T> since "xyz" is not an "abc" | "def".

In your case, there's not much reason for you to not just return a {foo: string}; the caller can decide to use it as a Partial<Whatever> if they feel inclined.

grumd commented 3 years ago

@Nathan-Fenner Oh, I guess I should have used arg: T['foo'] instead of string.

anlauren commented 3 years ago

I was running in the same problem before i understood the concept of more restrictive subtypes. So, i've tried to use the type from the generic itslef, T["foo"] in the above example but still no luck there. Should it be reported as a bug or am I getting something wrong:

type BaseType = {id: string};
function fun<M extends BaseType>(s: M["id"]): Partial<M> {
  return { id: s }; // Type '{ id: M["id"]; }' is not assignable to type 'Partial<M>'.
}
jonhue commented 3 years ago

I submitted a fix with #42382, would really appreciate some feedback! πŸ˜„