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

undefined extends all types when accessing a generic in a mapped typed #27470

Closed
squirly opened this issue Oct 1, 2018 · 6 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@squirly
Copy link

squirly commented Oct 1, 2018

TypeScript Version: 3.2.0-dev.20180929 (First noticed in 3.1.1)

strictNullChecks must be on otherwise this is expected behaviour.

It seems that an issue has been introduced when accessing attributes of a mapped type in a conditional type.

Search Terms:
conditional type undefined extends
mapped type undefined extends
conditional type in mapped type access

Code

type UndefinedKeys<T extends Record<string, any>> = {
  [K in keyof T]: undefined extends T[K] ? K : never
};

type MyType = {a: string, b: string | undefined}

type Result1 = UndefinedKeys<MyType>;

const a1: Result1['a'] = 'a'; // $ExpectError
      ~~  There should be an error here
const b1: Result1['b'] = 'b';

// manually mapping the type, error is show as expected
type Result2 = {
  a: undefined extends MyType['a'] ? 'a' : never;
  b: undefined extends MyType['b'] ? 'b' : never;
};

const a2: Result2['a'] = 'a'; // $ExpectError
const b2: Result2['b'] = 'b';

To simplify this even more line 5 can be changed to

type MyType = { a: string, b: undefined };

Expected behavior:

  1. The type Result1 should be {a: never; b: 'b'}.
  2. Line 9 should have an error: Type '"a"' is not assignable to type 'never'.

Actual behavior:

  1. The type Result1 is {a: 'a'; b: 'b'}.
  2. Line 9 has no error.

Playground Link:

TypeScript Version Playground Link
2.8.1 Playground
3.0.1 Playground
3.1.6 Playground
3.2.1 Playground
3.2.1-insiders.20181128 (Official Playground) Playground <---- ENABLE strictNullChecks IN OPTIONS
3.3.0-dev.20190104 Fixed?

Related Issues:
Potentially: #26942

Initially thought it was an issue with Distributive conditional types but the Generic type is in the assignable to position.

@jack-williams
Copy link
Collaborator

jack-williams commented Oct 1, 2018

The issue seems to be the constraint on T. The following has the intended behavior:

type UndefinedKeys<T> = {
  [K in keyof T]: undefined extends T[K] ? K : never
};

The index access expressions T[K] seems to be pulling in the constraint where all property types extend any, and therefore undefined is related.

I think this is a minimal repro (using arrays).

type X<T extends any> = undefined extends T ? true : false;
type Y<T extends any[]> = undefined extends T[number] ? true : false;

type A = X<number>; // ignores the `any` constraint and returns false
type B = Y<number[]>; // returns true;

The fundamental issue (I think) is that arrays and objects are covariant, which is unsound for update operations.

EDIT: Alternate implementation with constraint:

type UndefinedKeys<T extends Record<string, any>> = {
    [K in keyof T]: undefined extends Extract<T[K], undefined> ? K : never;
};

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Oct 1, 2018
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Oct 1, 2018
@ahejlsberg
Copy link
Member

ahejlsberg commented Oct 1, 2018

This one is rather involved. We currently have the following rule:

  • A type S is related to a type T[K] if S is related to C, where C is the base constraint of T[K].

This rule is unsound because T[K] may be a subtype of C upon instantiation, but we nonetheless need it for several of reasons. One key reason is the fact that we don't use higher order types for property accesses through type variables. For example, for a property access expression this.xxx in a method of a class, we resolve to the declared type of xxx, not type this["xxx"], even though the latter is more precise. Effectively, we resolve to the constraint of this["xxx"]. We do this for reasons of backwards compatibility and also to avoid drowning in generic types. But it would be odd indeed if the expression this.xxx was not assignable to the type this["xxx"]--hence the rule that allows the assignment if it matches the constraint.

Now, in 3.1 we fixed several issues relating to us not examining all constraints of a type. Because of those fixes we now drill all the way down and find the any constraint for T[K] in your example above. And since anything is assignable to any we effectively turn off type checking. To wit, none of the following assignments generate errors:

function test<T extends Record<string, any>, K extends keyof T>(t: T, k: K) {
    t[k] = 42;
    t[k] = "hello";
    t[k] = undefined;
}

That's not good. But the question is how to fix this without breaking the this.xxx scenario I mentioned above (and other similar ones).

So far I think the best contender is to modify the rule above as follows:

  • A type S is related to a type T[K], where T and K aren't both type variables, if S is related to C, where C is the base constraint of T[K].

In other words, we should limit the unsoundness to situations where one or the other part of the indexed access is an actual zero-order type, but forbid it when both are higher order (since at that point it becomes too unsound).

I ran the experiment on our test suites and there just a few changes, all of which are reasonable. Even better, the RWC suites show no baseline changes at all. I will put this up in a PR.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Oct 1, 2018
@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Oct 1, 2018
@jack-williams
Copy link
Collaborator

jack-williams commented Oct 2, 2018

And since anything is assignable to any we effectively turn off type checking. To wit, none of the following assignments generate errors

FWIW I don't think it is because type-checking is turned off, but because any is a top type (I don't know if you consider this a distinction). You get the same behavior with unknown.

type UndefinedKeys<T extends Record<string, unknown>> = {
  [K in keyof T]: undefined extends T[K] ? K : never
};
type MyType = {a: string, b: string | undefined}
type Result1 = UndefinedKeys<MyType>;
const a1: Result1['a'] = 'a'; // no error

The conditional type is essentially a type-level version of this:

const x: {a: string} = { a: "a" };
const baseConstraint: Record<string, unknown> = x;
baseConstraint.a = undefined; // unsound

A type S is related to a type T[K], where T and K aren't both type variables, if S is related to C, where C is the base constraint of T[K].

Am I right in say that this would not address:

type Y<T extends any[]> = undefined extends T[number] ? true : false;?

(This isn't a complaint, just wanting to know if I read the rule ok).

That's not good. But the question is how to fix this without breaking the this.xxx scenario I mentioned above (and other similar ones).

From an uninformed/outside perspective it seems like the problem trying to be solved in the initial post is to constrain T to some shape (a record, but it could be an array). The programmer only really cares about the outermost data constructor and the any is just there because it causes little friction. Though when you pull down the constraints the any starts causing problems and you can't control the variance.

I wonder whether it would be possible to have a special way of marking shapes for types where you really don't care about, or particularly, want their constituent constraints. Something like:

type UndefinedKeys<T extends Record<string, *>> = {
  [K in keyof T]: undefined extends T[K] ? K : never
};

T extends Record<string, *> can be read as: T should be some record but other than that we don't care about the type of the value, please don't pull the constraint down because I'm going to use T[K] in a contravariant way! * is basically an alias for any that doesn't get pulled out from constraints, or an inference where we discard the result.

@ahejlsberg
Copy link
Member

FWIW I don't think it is because type-checking is turned off, but because any is a top type (I don't know if you consider this a distinction). You get the same behavior with unknown.

That is indeed what I meant.

I wonder whether it would be possible to have a special way of marking shapes for types where you really don't care about, or particularly, want their constituent constraints.

I would be more inclined to introduce a new type relationship (e.g. "soundly assignable") that excludes the unsound rule and then use that relationship when checking the extends clause of a conditional type. But we really try hard not to have too many type relationships because we end up consuming more time and memory resources.

@aleclarson
Copy link

Is this related? playground

Using "brackets on a mapped type" inside an "extends" clause lets false extends true evaluate to true.

@jack-williams
Copy link
Collaborator

@aleclarson Yes this seems to be related. The fix made only works for index accesses of the form T[K], where both T and K are parameters; your code uses T['a'].

Using "brackets on a mapped type" inside an "extends" clause lets false extends true evaluate to true.

This isn't 100% true in the sense that what is going on is that the condition false extends T['a'] is being eagerly reduced using the constraint Foo. So the condition is being evaluated as false extends boolean, which correctly selects the true branch. The conditional type is being resolved before the function ever gets called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

5 participants