microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.94k stars 12.48k forks source link

Type narrowing of generic types in control flow analysis #50652

Open adrienharnay opened 2 years ago

adrienharnay commented 2 years ago

Bug Report

Hello,

🔎 Search Terms

type narrowing, generic types, control flow analysis

🕗 Version & Regression Information

(see Playground) When trying to narrow generic types (unions), the control flow analysis is not aware of the narrowing. I also see that this PR: https://github.com/microsoft/TypeScript/pull/43183 was supposed to address this.

Please keep and fill in the line that best applies:

⏯ Playground Link

Playground link with relevant code

💻 Code

const INITIAL_STATE = {
  1: 'test1',
  2: 'test2',
};
type State = typeof INITIAL_STATE;

const stateReducer = <Type extends keyof State>(
  state: State,
  action: { type: Type; value: string } | { type: 'clear'; value: number },
) => {
  if (action.type === 'clear') {
    // action.value is "string | number", but should be "number"
    return action.value;
  }

  return {
    ...state,
    [action.type]: action.value,
  };
};

stateReducer(INITIAL_STATE, { type: 'clear', value: 0 });

🙁 Actual behavior

action.value is "string | number"

🙂 Expected behavior

but should be "number"

Thanks in advance!

whzx5byb commented 2 years ago

{ type: Type; value: string } | { type: 'clear'; value: number } is not a discriminated union so it may not be narrowed as the way you expected. To make it work, you have to distribute over the generic type Type.

const stateReducer = <Type extends keyof State>(
  state: State,
-  action: { type: Type; value: string } | { type: 'clear'; value: number },
+  action: Type extends unknown ? { type: Type; value: string } | { type: 'clear'; value: number } : never,
) => {
andrewbranch commented 2 years ago

It’s a fair question why type isn’t a discriminant property, though. It’s not that every constituent in the union needs a unit type, because this is discriminable:

-  action: { type: Type; value: string } | { type: 'clear'; value: number },
+  action: { type: keyof State; value: string } | { type: 'clear'; value: number },

The fact that Type is a type parameter, though constrained to a union of literal types, prevents us from recognizing the property as a discriminant. The conditional type distributes, yes, but it also kind of un-genericizes by filling in the constraint, which lets us recognize the property as a discriminant. I don’t immediately see a great reason why we couldn’t say that a type parameter whose constraint is a literal type is a valid discriminant... but these things have a way of having unexpected rippling effects.

fatcerberus commented 2 years ago

It’s not that every constituent in the union needs a unit type

It's funny you say this because this was also my understanding for a long time. It's only due to past comments by @RyanCavanaugh that I found out the rule is the discriminant can either be a unit type or a union of unit types (thus why keyof State works as a discriminant). So I certainly wouldn't blame anyone who didn't realize that.

It's interesting that that conditional type trick works. I would expect it to just defer since it's distributive over a type parameter.

wbt commented 1 year ago

Here is another toy example demonstrating what I think is the same issue:

interface UpdateReport<B extends boolean = true> {
    newValue: string;
    oldValue: B extends true ? string : undefined;
}
export const processReport = function<B extends boolean>(
    report: UpdateReport<B>,
    hasOldValue: readonly [B],
) {
    if(hasOldValue[0]) {
        //Error TS(2345): Argument of type 'UpdateReport<B>'
        //is not assignable to parameter of type 'UpdateReport<true>'.
        //Type 'B' is not assignable to type 'true'.
        //Type 'boolean' is not assignable to type 'true'.
        //However, within this conditional check / type guard,
        //TS should be able to figure out that B is 'true'.
        let oldValue = getOldValueFromReport(report); //...
    }
}
const getOldValueFromReport = function(
    report: UpdateReport<true>
) {
    return report.oldValue;
};

Search terms: Type guard narrowing assignable to parameter of type 2345

thetutlage commented 1 year ago

Another one when using unions with generics.

type BcryptConfig = {
  rounds: number
  type: 'bcrypt'
}

type ArgonConfig = {
  variant: number
  type: 'argon'
}

function defineConfig<T extends { [key: string]: ArgonConfig | BcryptConfig }>(config: T): T {
  return config
}

defineConfig({
  passwords: {
    type: 'argon',
    variant: 1,
    rounds: 1,
  }
})

I expect the rounds: 1 parameter to be disallowed, since the property does not exists on the ArgonConfig.

rayzr522 commented 1 year ago

any update on this? I find myself FREQUENTLY running into this issue, especially when dealing with things like events where there's multiple different types with different payloads. here's an example:

(ts playground)

type EventType = 'NEW_MESSAGE' | 'NEW_USER'

type AppEvent =
  | { type: 'NEW_MESSAGE'; message: string }
  | { type: 'NEW_USER'; user: string }

function fetchEvents<Type extends EventType>(
  eventType: Type
): Extract<AppEvent, { type: Type }> {
  if (eventType === 'NEW_MESSAGE') {
    // this errors, because typescript can't figure out that Type must be 'NEW_MESSAGE'
    return {
      type: 'NEW_MESSAGE',
      message: 'hello, world',
    }
  } else if (eventType === 'NEW_USER') {
    // this errors, because typescript can't figure out that Type must be 'NEW_USER'
    return {
      type: 'NEW_USER',
      user: 'rayzr',
    }
  } else {
    throw new Error(`Unknown event type: ${eventType}`)
  }
}

I do this kinda thing all the time when making abstractions to handle multiple types of input/output, and it's a pain to have to typecast the return types

jcalz commented 1 year ago

This looks like the same thing except the generic is constrained to something other than a union... but it should still probably be discriminable:

type Foo<T extends {}> =
    { a: undefined, b: string } | { a: T, b: number }

function foo<T extends {}>(f: Foo<T>) {
    if (f.a == null) {
        f.b // should be string, is actually string | number
    }
}

Playground link

reiss-d commented 1 year ago

I think this is a slightly different example of the control flow not narrowing the generic, only it's not a property access:

declare function takesString(value: string): void

const data = { a: 'foo', b: 1 }
type Data = typeof data

function getValue_generic<K extends keyof Data>(key: K) {
   if (key === 'a') {
      key // K extends "a" | "b"
      const value = data[key] // { a: string; b: number }[K]
      takesString(value) // Argument of type 'string | number' is not assignable to parameter of type 'string'.
   }
}

function getValue_non_generic(key: keyof Data) {
   if (key === 'a') {
      key // "a"
      const value = data[key] // string
      takesString(value) // ok
   }
}

Playground

Lordfirespeed commented 1 month ago

And another toy example (I believe):

const foo = {
  bar: ['hello', (value: 'hello' | 'bye') => {}],
  baz: [3, (value: 3 | 5) => {}],
} as const;

function doWithFoo<T extends keyof typeof foo>(key: T) {
  if (key === 'bar') {
    const [value, setValue] = foo[key];
    setValue('bye'); // error here
  }
}

https://stackblitz.com/edit/typescript-repro-eqknuz?file=src%2Fmain.ts