Discriminated unions behave inconsistently since v4.6

[azhiv]

Bug Report

🔎 Search Terms

Discriminated union

🕗 Version & Regression Information

• This changed between versions 4.5.5 and 4.6.4

⏯ Playground Link

Playground link with relevant code

💻 Code

class Model {
  constructor(public flag: boolean) {}
}

type DiscriminatedUnion = { flag: true } | { flag: false };
class A<T extends DiscriminatedUnion> {
  constructor(public model: T) { }
}

// Uncomment the two lines below to get rid of the compiler error:
// function foo(_model: DiscriminatedUnion) { }
// foo({} as Model);

class B extends A<Model> { }    // Type 'Model' is not assignable to type '{ flag: false; }'.

🙁 Actual behavior

Class A doesn't accept Model as generic parameter with the following error:

Type 'Model' is not assignable to type '{ flag: false; }'.
  Types of property 'flag' are incompatible.
    Type 'boolean' is not assignable to type 'false'.

The basic example above hadn't emitted any errors prior to v4.6. Furthermore, if you uncomment the code that declares and executes function foo, the compiler error vanishes.

🙂 Expected behavior

The code is compilable in 4.6+ without any workarounds.

[fatcerberus]

Isn’t this a correct error? I thought that in general { foo: T | U } is not compatible with { foo: T } | { foo: U } (mainly because the relation is potentially combinatorially explosive)

[RyanCavanaugh]

I didn't bother penciling it out, because something is totally screwed up if uncommenting the two commented-out lines changes the errorness of the program. That's the key bug here.

[jakebailey]

What's interesting here is that while we decide that Model is not related to either { flag: true } or { flag: false }, we do determine that they are related via typeRelatedToDiscriminatedType. This is true whether or not that extra code is commented out or not. But, somehow before, we didn't report this as an error, and now we do (unless that other code is present).

I bisected this to #47907 (the 4.6 cherry pick of #47738), which doesn't seem to have been intended to add errors, only remove unrelated elaborations.

To me it seems like these two types are supposed to be related as they used to be in 4.5 but we are now reporting an error on this class case. If we check related twice, then ISTM what is happening is that we get the cached "yes, it's related" result and then don't error, but something about the class case is different. I'm still debugging.

[jakebailey]

It was as I thought above. The linked refactor must have changed things enough such that the error didn't get cleared out when typeRelatedToDiscriminatedType; it didn't have to do with union discrimination at all.

@azhiv after the #49718, this error should go away again.

@fatcerberus while that's true, there's an exception via typeRelatedToDiscriminatedType; in this case we seem to determine that it is related because DiscriminatedUnion is a discriminated union on flags, and all of the other properties are related (rather, there are no more properties, so therefore it's related).

[fatcerberus]

because DiscriminatedUnion is a discriminated union on flags, and all of the other properties are related (rather, there are no more properties, so therefore it's related).

vacuous truth is always a fun bit of logical trickery isn’t it 😄

[azhiv]

@jakebailey I'm utterly glad this error will be gone and we won't have to rewrite those pieces of our codebase. Thank you very much for the thorough investigation you have conducted!

[andreyfel]

@jakebailey Will the fix be backported to 4.6.x and/or 4.7.x?

[DanielRosenwasser]

I don't think this will be back-ported, but you can use a nightly in the meantime until 4.8 is out.

npm install -D typescript@next