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

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

Open
tusharf5 opened this issue Jan 30, 2024 · 6 comments Β· May be fixed by #57236
Open

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

tusharf5 opened this issue Jan 30, 2024 · 6 comments Β· May be fixed by #57236
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@tusharf5
Copy link

tusharf5 commented Jan 30, 2024

πŸ”Ž 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.

@Andarist
Copy link
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
Copy link

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
Copy link
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.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Jan 30, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jan 30, 2024
@ahejlsberg
Copy link
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
Copy link

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
Copy link
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
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants