Skip to content

Incorrect narrowing of Union type after discrimination when one type is assignable to the other (even when explicit type annotation is present) #56106

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

Open
niieani opened this issue Oct 13, 2023 · 17 comments
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@niieani
Copy link

niieani commented Oct 13, 2023

🔎 Search Terms

"discriminating assignable union", "narrowing unions", "unions simplifying with type guards", "type narrowing with union types", "type discrimination with assignable types", "union type annotation issue", "type guard narrowing problem"

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about Type Guards and Structural Typing

Playground Link

💻 Code

type ObjWithoutFlags = {withoutFlags: WithoutFlags};
type ObjWithFlags = {withFlags: WithFlags}

export interface WithoutFlags {
  id: string
  // if you add any additional field here, it suddenly works correctly:
  // extraField: any
}

export interface WithFlags {
  id: string
  flags: Array<string>
}

declare const props: ObjWithoutFlags | ObjWithFlags

// broken case:
// regardless of whether I explicitly define that type of `obj1` is a union of `WithFlags | WithoutFlags`,
// TS assumes that `obj1` is always `WithoutFlags` when used in conjunction with type discrimination like so:
const obj1: WithFlags | WithoutFlags = 'withoutFlags' in props ? props.withoutFlags : props.withFlags;

// obj1 is narrowed to `WithoutFlags`, despite having an explicit type annotation

if ('flags' in obj1) {
  // obj is incorrectly narrowed to `WithoutFlags & Record<"flags", unknown>`
  // error, because event.flags is 'unknown', even though it should be an `Array<string>`
  obj1.flags[0]
}

// working case, almost identical:
// there's no discrimination at play, and it works as intended:
declare const obj2: WithFlags | WithoutFlags;

if ('flags' in obj2) {
  // obj is narrowed to `ObjWithFlags`
  // no error, because event.flags is correctly `Array<string>`
  obj2.flags[0]
}

🙁 Actual behavior

When accessing properties of a discriminated union type, where one resulting type is assignable to the other, TypeScript incorrectly narrows down the resulting type to the intersection of its types, rather than their union. Simply adding an additional property to the type that's assignable to the other makes TypeScript switch behavior, and use a union type instead.

This occurs even when an explicit type annotation is present on the variable. This behavior is observed not just with conditional ternary expressions but with any type guards.
This leads to an erroneous assumption, for instance, that the object type only contains the fields of the simpler type - in the example case, it is always of type WithoutFlags, even if the explicit annotation indicates a union of WithFlags | WithoutFlags.

const obj1: {flags: string[], id: string} | {id: string} = 'withoutFlags' in props ? props.withoutFlags : props.withFlags;

if ('flags' in obj1) {
  // Error: obj1.flags is considered 'unknown' instead of `Array<string>`
  obj1.flags[0]
}

🙂 Expected behavior

  1. TypeScript should always create unions of possible types, unless both types are mutually assignable. Currently it's enough if one of the types is assignable to the other.

  2. When a variable is annotated with an explicit type, TypeScript should disregard any magic inference behavior that happens in its initializer

Additional information about the issue

We've stumbled upon this issue accidentally in the real world, because code that used to work, suddenly was erroring. A refactor of GraphQL Fragments that unified two distinct property types into one, suddenly made TypeScript behave as if only one of those distinct types was correct, and all the excess properties were missing, even after applying a type guard to test for which one of the types is being used.

@IllusionMH
Copy link
Contributor

IllusionMH commented Oct 14, 2023

This is expected behavior as you union (even explicit) will be widened to just ObjWithoutFlags as it contains all shared properties between two types.
See https://www.typescriptlang.org/docs/handbook/type-inference.html#best-common-type
Most likely in first case it's widened in ternary expression and then constant picks same wide type from assignment.

You would need something like #12936 to prevent this behavior

@niieani
Copy link
Author

niieani commented Oct 14, 2023

Hmm...

Here's another example that might better illustrate the problem:

this works:

const fn1 = (obj: WithFlags | WithoutFlags) => {
  if ('flags' in obj) {
    // no error, because 'obj' is narrowed to WithFlags
    obj.flags[0]
  }
}

this doesn't:

const fn2 = (wrapperObj: ObjWithFlags | ObjWithoutFlags) => {
  const obj = 'withFlags' in wrapperObj ? wrapperObj.withFlags : wrapperObj.withoutFlags
  //    ^^ WithoutFlags                              ^^ WithFlags           ^^ WithoutFlags
  //    ^^ should be: WithFlags | WithoutFlags

  if ('flags' in obj) {
    // error:
    obj.flags[0]
  }
}

The only difference is that in the 2nd case the type is wrapped.
The issue has to do with the type guard on the wrapper object.

See Playground.

If this were somehow expected behavior then it's extremely unintuitive and I'd argue it should be changed.

Removing a property from one of the types, so that it accidentally becomes assignable to the other shouldn't change how the rest of the surrounding code works. That's some wild spooky behavior at a distance. 😄

Esp. in our case this is a type we cannot change, because it's generated directly from GraphQL types - I can't easily add a fake property to it.

@Andarist
Copy link
Contributor

This is unfortunate but all of this builds on top of things that are pretty unchangeable in TypeScript today. The declared type on a variable isn't always used as the final initial~ type of that variable, see Ryan's comment here.

The other bit is just a subtype reduction at play and it's something that you can't easily fight here.

To work around this you can introduce a function to "hide" your conditional expression it it and annotate the return type of that function. Or you can... use the satisfies+as combo:

const fn2 = (wrapperObj: ObjWithFlags | ObjWithoutFlags) => {
  const obj = ("withFlags" in wrapperObj
    ? wrapperObj.withFlags
    : wrapperObj.withoutFlags) satisfies WithFlags | WithoutFlags as
    | WithFlags
    | WithoutFlags;
  if ("flags" in obj) {
    // works!
    obj.flags[0];
  }
};

@MartinJohns
Copy link
Contributor

Esp. in our case this is a type we cannot change, because it's generated directly from GraphQL types - I can't easily add a fake property to it.

If they're interfaces you should be able to use declaration merging.

@niieani
Copy link
Author

niieani commented Oct 16, 2023

things that are pretty unchangeable in TypeScript today.
The declared type on a variable isn't always used as the final initial.

But in that case, why does TypeScript behave correctly when the same union type is used in the annotation for an argument of a function, but incorrectly when used as a type annotation for a variable? Why would these two be different?

This seems internally inconsistent.

If they're interfaces you should be able to use declaration merging.

Unfortunately no, they're defined as types. Even if they were interfaces, they could be nested object unions, which means declaration merging wouldn't help. I don't think we should assume the user has full control over the types in any situation.

@Andarist
Copy link
Contributor

But in that case, why does TypeScript behave correctly when the same union type is used in the annotation for an argument of a function

That annotation is the only information about the parameter's type. The parameter isn't subject to the control flow analysis.

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript Help Wanted You can do this labels Oct 16, 2023
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Oct 16, 2023
@fatcerberus
Copy link

fatcerberus commented Oct 18, 2023

@RyanCavanaugh Out of curiosity, why is this considered a bug? AFAICT this comes down to just another inconvenient subtype reduction which historically has not been considered to be a defect.

@niieani
Copy link
Author

niieani commented Oct 18, 2023

Automatic narrowing happens when defining an array with types of various interfaces (but not classes):

// automatic narrowing to WithoutFlags:
const array1 = [withFlags, withoutFlags]
//      ^? WithoutFlags[]

// but I can provide the type manually:
const array2: (WithFlags | WithoutFlags)[] = [withFlags, withoutFlags]
//      ^? (WithFlags | WithoutFlags)[]

playground

and @IllusionMH mentions the handbook, but the example it gives behaves exactly as I would expect. By default it keeps the union intact, but you can provide an annotation to widen the type to use the common parent class (Animal).

It seems the reason it works that was is that the example in the handbook uses class instances.
TS seems to behave differently based on whether the type is a class or an interface. When the same thing is redefined as an interface, it suddenly behaves differently (playground):

class Animal {}
class Rhino extends Animal {}
class Elephant extends Animal {trunk = true}

let zoo = [new Rhino(), new Elephant()];
//   ^? (Rhino | Elephant)[]

interface IAnimal {}
interface IRhino extends IAnimal {}
interface IElephant extends IAnimal {trunk: true}

declare const rhino: IRhino
declare const elephant: IElephant

let izoo = [rhino, elephant]
//   ^? IRhino[]

I understand that with structural typing IRhino and IAnimal are equivalent / mutually assignable, so it doesn't matter what the resulting name is, but with the case of IRhino and IElephant, they are not. IElephant has additional properties, that IRhino does not. What's the point of implicitly lossy reduction of the type to the widest possible one here? Why is that something anyone would want from a type system?

And given that classes are also plain objects that are compared structurally, why would a class instance be treated differently from an object interface in this case?

My main question is: Why in such a case is assignability checked only one way, but not the other? (i.e. is IElephant assignable to IRhino, but not IRhino to IElephant)

This is what causes the spooky behavior at a distance, in that adding a property to the other interface, suddenly causes the type to flip to a union.

Note that with these examples you at the very least have a workaround whereby you can specify the explicit type manually.
In the case of the issue in the first comment, there is no way to do this, even with an explicit type annotation.

@fatcerberus
Copy link

fatcerberus commented Oct 18, 2023

What's the point of implicitly lossy reduction of the type to the widest possible one here?

Subtype reduction is indeed lossy, but the information you're losing is only that which leads to unsoundness anyway:

#53425 (comment)

I should add that the core issue here is the unfortunate difference between IA and IA | IAB. Ideally you wouldn't be able to observe a difference between the two, but we'll likely never get to that point. (For example, in a world with no difference, el.B would have type unknown in both cases because B doesn't exist in IA.)

To clarify that remark, in the context of the original example:

export interface WithWeirdFlags {
  id: string
  flags: bigint
}

let weird: WithWeirdFlags = { id: "fooey", flags: 777n };
let noFlags: WithoutFlags = weird;  // valid subtype, so no error
let wut = noFlags as WithFlags | WithoutFlags;  // cast to avoid CF narrowing
// later...
if ('flags' in wut) {
    wut.flags  // string[], oh no!
}

In other words, because object types are not sealed (see #12936), the reduced type is arguably more correct and definitely more typesafe.

@niieani
Copy link
Author

niieani commented Oct 19, 2023

@fatcerberus I don't buy it. You introduce unsoundness on this line:

let wut = noFlags as WithFlags | WithoutFlags;  // cast to avoid CF narrowing

By casting the type, you manually tell the compiler that it could be either this or that, even though the union you cast it to is incorrect, because it omits the WithWeirdFlags. You'd have to cast it as WithFlags | WithWeirdFlags | WithoutFlags, in which case the inference for wut.flags would be a union too.

But this is a step you're taking manually. Here, we're talking about what the decision the compiler makes, and also the fact that you cannot even override these decisions by explicitly stating the type annotation.

@fatcerberus
Copy link

fatcerberus commented Oct 19, 2023

@niieani The cast isn't unsound. weird is assignable to WithFlags | WithoutFlags even without it, I just didn't write it as a type annotation because then wut would be narrowed to WithoutFlags (i.e. the very behavior you're arguing against). Here's a version without any casts:

export interface WithWeirdFlags {
  id: string
  flags: bigint
}

let weird: WithWeirdFlags = { id: "fooey", flags: 777n };
wut(weird);  // valid subtype of WithoutFlags, so no error

function wut(value: WithFlags | WithoutFlags) {
    if ('flags' in value) {
        value.flags  // string[], oh no!
    }
}

The point is: WithWeirdFlags is a legal inhabitant of WithoutFlags, as is WithFlags, so adding WithFlags to the union doesn't give you any additional information about the type's inhabitants unless you're willing to risk unsoundness.

@fatcerberus
Copy link

To put it more simply: If you have an Animal | Dog and want to check whether it's a dog by checking whether it can bark, the compiler will cooperate, but it might turn out at runtime that you actually have a Seal. To minimize the potential damage, in cases where a ternary expression can produce either an Animal (presumably of unknown species) or a Dog, the compiler simply infers Animal because that's safer.

@niieani
Copy link
Author

niieani commented Oct 19, 2023

I see. The example with the function makes more sense, thank you.
The problem I'm having is with TS allowing wut(weird) to be called with an argument that's actually of the WithWeirdFlags type.

Even though the argument is expected to be a union of WithFlags | WithoutFlags, the compiler should notice that some members of that union have conflicting properties with the input type (i.e. WithFlags has the same member: flags as WithWeirdFlags, defined as a different type), which can lead to this unsoundness that you cited (value.flags being narrowed as string[]).

My proposed improvement to the compiler would be to make that call illegal, unless cast to the subtype first.
wut(weird) should error at the call site, unless you explicitly cast it to WithoutFlags: wut(weird as WithoutFlags), by which you would be explicitly telling the compiler to loose some critical type information.

@fatcerberus
Copy link

The problem I'm having is with TS allowing wut(weird) to be called with an argument that's actually of the WithWeirdFlags type.

You're ultimately looking for #12936 then. It's by design that objects can have extra properties not mentioned in their type. There is an excess property check for directly-provided object literals, by that's mainly just a lint check to detect typos and is not a real type rule.

@niieani
Copy link
Author

niieani commented Oct 19, 2023

Not exactly. Exact types would help to solve this problem, yes, but I think a general improvement to current behavior as described above would help everyone, even those not using exact types today. My proposal is to:

  • find which member of the union the type is assignable to (this is what happens today, and this is where compiler stops)
  • extract the excess properties that don't exist in the assignable type
  • check assignability of only those excess properties to other union members
  • is any of the union members are not assignable, issue an error

Exact types have their use cases, but I think overall usability of TS could improve significantly if an algorithm like the above would be implemented to non-exact types. Of course, there's a matter of how much of a breaking change would this be. Would be interesting to test that for sure.

@wraiford
Copy link

wraiford commented Jun 1, 2024

This may be related to a bug I seem to have found related to using a discriminated type and guards.

interface Base {
    id: string;
    name: 'A' | 'B';
}

interface TypeA extends Base {
    description?: string;
}

interface TypeB extends Base {
    description?: string;
}

type Concrete = TypeA | TypeB;

function isTypeA(obj: Concrete): obj is TypeA {
    return obj.name === 'A';
}
function isTypeB(obj: Concrete): obj is TypeB {
    return obj.name === 'B';
}

function foo(obj: Concrete): void {
    if (isTypeA(obj)) {
        console.dir(obj); // obj is inferred as `Concrete`
    } else if (isTypeB(obj)) {
        console.dir(obj); // obj is inferred as `never`
    }
}

@miguel-leon
Copy link

This problem also appears while using nullish coalescing.
The following example is something pretty vanilla, but is hindered by the narrowing because B extends A.

interface A {
    a: string;
}
interface B extends A {
    b: number;
}

declare function f1(): A | undefined;
declare function f2(): B;

const x = f1() ?? f2();
   // ^? A

if ('b' in x) {
    x.b;
   // ^? unknown
}

It would be A | B if B didn't extend A. It's inconsistent.

In any case, why would I want to lose the type information from B just by being in an union with A?
The common properties are still accessible. Just don't do the narrowing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

8 participants