Skip to content

discriminated union type matching behaviour changed starting from 5.2.x #57231

Open
@tusharf5

Description

@tusharf5

πŸ”Ž Search Terms

"key order when matching object types" "discriminated unions key order"

πŸ•— Version & Regression Information

  • This changed between versions 5.1 and 5.2

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.2.2#code/FAFwngDgpgBAYgewQExgXhgcgIYQgGykxgB8sEAnbAOwHMiBuUSWANSnpGwCND0sAzhACW1bAGMAFsTKZx2ChQQhMTcNBgB5EJKgV+mALbD8AaxlYA7thB7VzDQGEArgJAJDB7MIoXMAhBN7B1gYABUFTnRgGFi4sgBvGLiU2OxnZGEoanEoAC4sKAUde1TUwyL8POSylLIXNw8a2tiyAAMAMwpnYRAAfQASBMQUAF825pb2gDcOKC5eKEGE9k4eQnHJ2vblXQpl7T3xphbRrcStlPTM7NyCowUBPwArZxFbXxOWmArsKpg3BRRLQvmUzsBgOIENQ3DBZmtFgVVvN1rAMHIFEoVExIdDYVwKJwChFCfN+DAklcMlkcvlCsVpAAaZq-f5teEoxbLDkLDYTUY4qEwkAwAmcABMxMiZIwFJZlQK7LmvKWQx5qPGzKpN1p9yKFBKwAFwCAA

πŸ’» Code

type Food = 'apple' | 'orange';
type Vegetable = 'spinach' | 'carrot';
type Other = 'milk' | 'water';
type Custom = 'air' | 'soil';

type  Target =
      | {
          audience: 'earth';
          meal:
            | Custom
            | `fruit_${Food}`
            | `vegetable_${Vegetable}`
            | `other_${Other}`;
        }
      | {
          audience: 'mars' | 'jupiter';
          meal: string;
        }


const vegetable: Vegetable = 'carrot';

// ok
const target: Target =  {
    audience: 'earth',
    meal: `vegetable_${vegetable}`
};

// TS Error
// Type '{ meal: string; audience: "earth"; }' is not assignable to type 'Target'.
//  Types of property 'audience' are incompatible.
//    Type '"earth"' is not assignable to type '"mars" | "jupiter"'
const target2: Target =  {
    meal: `vegetable_${vegetable}`,
    audience: 'earth'
};
Output
"use strict";
const vegetable = 'carrot';

const target = { // ok
    audience: 'earth',
    meal: `vegetable_${vegetable}`
};

const target2 = { // error
    meal: `vegetable_${vegetable}`,
    audience: 'earth'
};
Compiler Options
{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

Playground Link: Provided

πŸ™ Actual behavior

In the code both target and target2 have the correct structure based on the Target type.

But the order of keys is reversed in target2 which used to work but not anymore.

πŸ™‚ Expected behavior

Both target and target2 should be valid.

Additional information about the issue

Discriminated unions didn't rely on the key ordering AFAIK.

Activity

Andarist

Andarist commented on Jan 30, 2024

@Andarist
Contributor

I have bisected this to #53907 . It means that this code with this change should roughly be equivalent to the one with vegetable variable inlined:

type Food = 'apple' | 'orange';
type Vegetable = 'spinach' | 'carrot';
type Other = 'milk' | 'water';
type Custom = 'air' | 'soil';

type  Target =
      | {
          audience: 'earth';
          meal:
            | Custom
            | `fruit_${Food}`
            | `vegetable_${Vegetable}`
            | `other_${Other}`;
        }
      | {
          audience: 'mars' | 'jupiter';
          meal: string;
        }

const target: Target =  {
    audience: 'earth',
    meal: `vegetable_carrot`
};

const target2: Target =  {
    meal: `vegetable_carrot`,
    audience: 'earth'
};

This one doesn't error though. I'll investigate further what's the difference and what the behavior should be.

fatcerberus

fatcerberus commented on Jan 30, 2024

@fatcerberus

type Vegetable = 'spinach' | 'carrot';

What, no tomatoes?

...oh no, they didn't turn carnivorous and go on a rampage did they? because if so... run

Andarist

Andarist commented on Jan 30, 2024

@Andarist
Contributor

This is fun πŸ˜…

type Food = "apple" | "orange";
type Vegetable = "spinach" | "carrot";
type Other = "milk" | "water";
type Custom = "air" | "soil";

type Target =
  | {
      audience: "earth";
      meal:
        | Custom
        | `fruit_${Food}`
        | `vegetable_${Vegetable}`
        | `other_${Other}`;
    }
  | {
      audience: "mars" | "jupiter";
      meal: string;
    };

const vegetable1: Vegetable = "carrot";

// it errors but it shouldn't
const target1: Target = {
  meal: `vegetable_${vegetable1}`,
  audience: "earth",
};

const vegetable2: "carrot" = "carrot";

// it errors but it shouldn't
const target2: Target = {
  meal: `vegetable_${vegetable2}`,
  audience: "earth",
};

const vegetable3 = "carrot";

// ok
const target3: Target = {
  meal: `vegetable_${vegetable3}`,
  audience: "earth",
};

I'll push out a fix for this soon.

added
Possible ImprovementThe current behavior isn't wrong, but it's possible to see that it might be better in some cases
on Jan 30, 2024
added this to the Backlog milestone on Jan 30, 2024
ahejlsberg

ahejlsberg commented on Jan 30, 2024

@ahejlsberg
Member

A simpler repro that isn't fixed by #57236:

type Vegetable = 'spinach' | 'carrot';

type  Target =
    | { audience: 'earth', meal: `vegetable_${Vegetable}` }
    | { audience: 'mars', meal: string };

declare const vegetable: Vegetable;

// Ok
const target: Target =  {
    audience: 'earth',
    meal: `vegetable_${vegetable}`
};

// Errors
const target2: Target =  {
    meal: `vegetable_${vegetable}`,
    audience: 'earth'
};

The root problem is that contextual types are narrowed by discriminant properties in the order those discriminant properties are written (as opposed to some canonical narrowing that considers all of them at the same time). Above, the assignment to target succeeds because the contextual type Target is first narrowed by the audience: "earth" property. However, the assignment to target2 fails because no narrowing as (yet) taken place, so the contextual type for the template literal is string.

In reality, meal shouldn't really be considered a discriminant property since one of the property types is a supertype of every discriminant. We don't currently reason about it that way, but I'll look into a PR for that.

fatcerberus

fatcerberus commented on Jan 31, 2024

@fatcerberus

In reality, meal shouldn't really be considered a discriminant property since one of the property types is a supertype of every discriminant.

Given how often people want to be able to write "foo" | "bar" | string and have that mean something, I feel like there's almost certainly code in the wild that does stuff like

type DU =
    | { type: "foo", foo: string }
    | { type: "bar", bar: string }
    | { type: string, catchall?: string }

which would likely be broken by type no longer being considered a discriminant. And we don't have negated types yet, so...

ahejlsberg

ahejlsberg commented on Jan 31, 2024

@ahejlsberg
Member

I feel like there's almost certainly code in the wild that does stuff like...

There probably is, but it is hard to see what it accomplishes. The only property that can be made accessible through narrowing is catchall and only when narrowed to a type that isn't "foo" or "bar".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Help WantedYou can do thisPossible ImprovementThe current behavior isn't wrong, but it's possible to see that it might be better in some cases

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      Participants

      @fatcerberus@ahejlsberg@RyanCavanaugh@Andarist@tusharf5

      Issue actions

        discriminated union type matching behaviour changed starting from 5.2.x Β· Issue #57231 Β· microsoft/TypeScript