Skip to content

For T extends M where M is a mapped type, T[keyof T] doesn't consider M #30217

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

Closed
rkirov opened this issue Mar 5, 2019 · 15 comments
Closed

For T extends M where M is a mapped type, T[keyof T] doesn't consider M #30217

rkirov opened this issue Mar 5, 2019 · 15 comments
Labels
Question An issue which isn't directly actionable in code

Comments

@rkirov
Copy link
Contributor

rkirov commented Mar 5, 2019

TypeScript Version: 3.4.0-dev.201xxxxx and 3.3.3333 (used to work in TS 3.2)

Search Terms: Mapped type, T[keyof T]

Code

// Takes type -> {a: string}
// returns type -> {a: string | null} 
type Nullable<T extends object> = {
  [P in keyof T]: T[P] | null;
};

// This function should only accept objects, such that all props can be assigned null.
function f<T extends Nullable<T>>(t: T, key: keyof T) {
  let x = t[key];
  x = null;
}

Expected behavior:
x = null to be accepted as T's lower type bound is Nullable<_> thus all properties have | null.

Actual behavior:
This was accepted up to TS 3.2, but started erring with TS 3.3 with the error:

ERROR(11,3): : Type 'null' is not assignable to type 'T[keyof T]'.

** Some notes **
There is nothing special about Nullable, same error happens with other mapped types.

This is not my code and there is a valid question why even bother writing f as written, since TS accepts:

let x: {a: string} = {a: ''}; 
f(x, 'a');

I.e. the Nullable lower bound doesn't seem to be checked at the call-site.

@weswigham
Copy link
Member

You have your bounds confused - extends Nullable<O> doesn't force the properties of T to include null, it merely states that they can - this is a valid instantiation of f:

f<{ x: string }, { x: string }>({x: ""}, "x");

the argument for T does not allow x to be null - allowing the assignment within the body would be erroneous.

@jack-williams
Copy link
Collaborator

jack-williams commented Mar 5, 2019

This checks the call site, but does not work correctly in the body.

function f<T extends [Record<keyof T, null>] extends [T] ? unknown : never>(t: T, key: keyof T) {
  let x = t[key];
  x = null;
}

I wonder if it would be possible to look for constraints of the form U extends T ? unknown : never for type variables T, and interpret them as lower bounds.

@rkirov
Copy link
Contributor Author

rkirov commented Mar 5, 2019

@weswigham My bad, I had I typo, it should read extends Nullable<T>. Fixed the description, still succeeds in TS 3.2 and fails in TS 3.3

@rkirov
Copy link
Contributor Author

rkirov commented Mar 5, 2019

You have your bounds confused - extends Nullable doesn't force the properties of T to include null, it merely states that they can - this is a valid instantiation of f:

I think I see your reasoning here. Let me try to paraphrase it (please correct me if you meant something else):

For a concrete type, e.g. type A = {a: string}, Nullable<A> = {a: string | null}, so Nullable<A> is the supertype and A is the subtype, thus A extends Nullable<A>. In fact that is trivially true for every type A, the bound doesn't put any constraint on A. Given that A is any type, it is reasonable to reject setting a prop to null.

But, I can't seem to square off that reasoning with the following simpler behavior in TS (which succeeds without an error):

function f<T extends {a:string|null}>(x: T) {
  x.a = null;
}

f<{a: string}>({a: ''});

@rkirov
Copy link
Contributor Author

rkirov commented Mar 5, 2019

Also, maybe important for the discussion - I am not emotionally invested in this pattern (or #30218). These are just the only two errors we saw when we upgraded google's internal codebase to TS 3.3.3333 from TS3.2.

@RyanCavanaugh RyanCavanaugh added the Question An issue which isn't directly actionable in code label Mar 5, 2019
@rkirov
Copy link
Contributor Author

rkirov commented Mar 7, 2019

I found another instance, of what I think is the same issue in our codebase:

export function setValue<V, T extends {[KeyT in keyof T]: V}>(
    obj: T, key: keyof T, value: V) {
  obj[key] = value;
}

I tried to narrow it down so it is more readable at a glance, but let me know if you prefer the "real-world" code to understand why the devs are even writing these type of types.

@jack-williams
Copy link
Collaborator

I think the root of the issue might be this fix PR #27490?

@weswigham
Copy link
Member

Probably, yeah. @Rikov isn't this the much simpler way to write that?:

export function setValue<T, K extends keyof T>(
    obj: T, key: K, value: T[K]) {
  obj[key] = value;
}

as written, that setValue can be invoked with type params which would make that assignment unsound, eg:

export function setValue<V, T extends {[KeyT in keyof T]: V}>(
    obj: T, key: keyof T, value: V) {
  obj[key] = value;
}

setValue<number, { "ok": 6 }>({ ok: 6 }, "ok", 0); // allows setting `0` because `V` can be less specialized than T[keyof T]

@weswigham
Copy link
Member

By the way, as for a way to write your original example in a typesafe way:

function f<K extends string | number | symbol>(
    t: Record<K, null>,
    key: K
) {
  let x = t[key];
  x = null;
}

should work?

@agopshi
Copy link

agopshi commented Mar 7, 2019

@weswigham

Probably, yeah. @Rikov isn't this the much simpler way to write that?:

export function setValue<T, K extends keyof T>(
    obj: T, key: K, value: T[K]) {
  obj[key] = value;
}

The real world example is this utility function:

export function appendToOptionalArray<
    K extends string, V, U extends V, T extends {[KeyT in K]?: V[]}>(
    object: T, key: K, value: U) {
  const array = object[key];
  if (array) {
    array.push(value);
  } else {
    object[key] = [value];
  }
}

// e.g.
const foo: {x?: number[]; y?: string[]; } = {};
appendToOptionalArray(foo, 'x', 123);
appendToOptionalArray(foo, 'y', 'bar');

What's the correct way to rewrite this? We also had this simpler version in the past, but something else was wrong with it, so we changed it to the above:

export function
appendToOptionalArray<K extends string, V, T extends Partial<Record<K, V[]>>>(
    object: T, key: K, value: ElementType<T[K]>) {
  const array = object[key];
  if (array) {
    array.push(value);
  } else {
    object[key] = [value];
  }
}

Separately, I'm also curious to understand why @rkirov's example (without the mapped type) works:

function f<T extends {a:string|null}>(x: T) {
  x.a = null;
}

f<{a: string}>({a: ''});

@rkirov
Copy link
Contributor Author

rkirov commented Mar 7, 2019

Thanks, #27490 provides enough context for me to understand what is changing and why. The change in TS3.3.3333 is intentional and there is a soundness justification for it. It is surprising that it only affects T[keyof T] types.

In terms of impact this affected 3 code locations in our giant repo, so as far as I am concerned this is a very minor change.

@agopshi is one of the original authors of one of the affected code snippetes, so you can continue the discussion on a proper way to express the authors intent in types.

@weswigham
Copy link
Member

@agopshi I'd have written it like so:

// Using a homomorphic mapped type over `T`
// Produces a lower-priority inference for `T` than other
// positions, allowing one to override the priority the argument
// order would usually imply
type Lower<T> = { [K in keyof T]: T[K] };

export function appendToOptionalArray<
  K extends string | number | symbol,
  T
>(
  object: { [x in K]?: Lower<T>[] },
  key: K,
  value: T
) {
  const array = object[key];
  if (array) {
    array.push(value);
  } else {
    object[key] = [value];
  }
}

// e.g.
const foo: {x?: number[]; y?: string[]; } = {};
appendToOptionalArray(foo, 'x', 123);   // ok
appendToOptionalArray(foo, 'y', 'bar'); // ok
appendToOptionalArray(foo, 'y', 12);    // should fail
appendToOptionalArray(foo, 'x', "no");  // should fail

(and I will fully admit that knowing how to create a lower priority inference target is arcane typesystem knowledge) - except doing so has exposed a bug in our inference algorithm (where successive inferences higher-priority to the same type parameter fail to reset the flag used to control inference result literal-ness), which I've opened #30265 to fix.

Anyways, the core of it is that you can't let the type of object itself be generic, because then the members would be allowed to vary to subtypes of array (which is why attempting to assign an array should fail) - so you need to describe the shape of the relevant part of object instead, without letting object be a type parameter itself.

@agopshi
Copy link

agopshi commented Mar 8, 2019

@weswigham Hm... so, if I understand correctly, your example doesn't currently work either, right?

Is there a way to write this - right now - that works, and is more correct than [value] as unknown as T[]?

Lastly, is there documentation on lower priority inference targets? A quick search reveals a StackOverflow answer that quotes #14829 in a not-so-reassuring way: "I would move this from the "definitely don't depend on this" column to the "it's probably going to work for the foreseeable future" column."

@weswigham
Copy link
Member

The code I wrote above should work just dandy in the next nightly and beyond. 💓

@weswigham
Copy link
Member

Lastly, is there documentation on lower priority inference targets?

We've never formalized it, since we've oft debated making an explicit noinfer position which would be preferable in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question An issue which isn't directly actionable in code
Projects
None yet
Development

No branches or pull requests

6 participants