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

Typescript incorrectly resolves discriminated unions of indexed types #37052

Closed
bibachhetri opened this issue Feb 26, 2020 · 9 comments
Closed
Assignees
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@bibachhetri
Copy link

TypeScript Version: 3.8-Beta

Search Terms: discriminated unions indexed types

Expected behavior:
b1 should be assignable to a1

Actual behavior:
The following error Type '"B"' is not assignable to type '"A"'

Code

type TinyModel = {
    "A": "A" | "B";
    "B": "B";
}

type TinyId<T extends keyof TinyModel> = {
    type: TinyModel[T];
}

let a1: TinyId<"A">;
let b1: TinyId<"B">;

// Should be semantically equivalent to a1 & b1
let a2: { type: "A" | "B" };
let b2: { type: "B" };

function fn1() { a1 = b1; } // this should work, but doesn't
function fn3() { a2 = b2; }
Output
"use strict";
let a1;
let b1;
// Should be semantically equivalent to a1 & b1
let a2;
let b2;
function fn1() { a1 = b1; } // this should work, but doesn't
function fn3() { a2 = b2; }
Compiler Options
{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "declaration": true,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext"
  }
}

Playground Link: Provided

@jcalz
Copy link
Contributor

jcalz commented Feb 26, 2020

Haven't checked if this is a duplicate yet but whatever the issue is doesn't seem to have to do with discriminated unions (or even necessarily unions) or indexed access either per se:

type Foo = {
    "a": unknown
    "b": string;
}

type Bar<T extends keyof Foo> = {
    type: Foo[T];
}

const err: Bar<"a"> = null! as Bar<"b">; // error
// Bar<"b"> is not assignable to Bar<"a">

const okay: { type: Foo["a"] } = null! as { type: Foo["b"] }; 

Playground

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Feb 26, 2020
@RyanCavanaugh
Copy link
Member

Not sure why we're not falling back to structural comparison here.

Workaround

type TinyId<T extends keyof TinyModel> = T extends keyof TinyModel ? { type: TinyModel[T] } : never;

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.0 milestone Feb 26, 2020
@ahejlsberg
Copy link
Member

I believe the issue here is that we measure indexed access types such as TinyModel[T] as invariant, and since TinyId<T> contains a property of such a type, we consider it to be invariant in T. Effectively our variance measurement is more conservative than a structural check in this case. We do have provisions for tagging measurements as unreliable (which causes a follow-up structural check in the failure case), but we only do that for certain mapped types and rest parameters. We could try to do it for indexed access types as well, but it may have undesirable performance impact.

@ahejlsberg
Copy link
Member

@weswigham Don't recall if you've experimented with this before.

@weswigham
Copy link
Member

TinyModel[T] is only covariant in T - but "a" and "b" are obviously not covariantly related; the issue is that variance reasoning about how T is possibly related between instances does not include any information from the constraint of TinyModel[T] as a whole (which requires information about TinyModel to be known). I don't think I've directly experimented with it, but adding Unreliable markers to access positions when the object type is non-generic should allow the structural fallback, yeah.

@mmorearty
Copy link
Contributor

It's also worth mentioning that the compiler behavior changed at some point. The code in the playground example compiles in TS 3.3.3, but not in TS 3.5.1. (Also, thanks for the workaround, @RyanCavanaugh!)

@ahejlsberg
Copy link
Member

@weswigham I does seem that when a type variable is used as the index type in an indexed access type, its variance effectively becomes unmeasurable (because a relation between property names implies nothing about a relation between the types of those properties).

@jchurchill
Copy link

I've found a simpler workaround that doesn't involve conditional types: the type indexing must occur external to the definition of the object's shape. As an example, here's a slight modification to fix @jcalz 's example:

type Foo = {
    "a": unknown
    "b": string;
}

type BarInner<T> = { type: T };
type Bar<T extends keyof Foo> = BarInner<Foo[T]>;

const err: Bar<"a"> = null! as Bar<"b">; // no longer an error!

@ahejlsberg
Copy link
Member

I'm closing this as a design limitation. The core issue is that we conservatively measure most indexed access types as invariant, where structural comparisons might be less restrictive. For example, see my comment here.

@ahejlsberg ahejlsberg added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone labels Oct 26, 2020
@ahejlsberg ahejlsberg removed this from the TypeScript 4.1.0 milestone Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

7 participants