Skip to content

Control Flow Analysis for Destructured Discriminated Unions doesn't work for let variables #48071

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
kasperpeulen opened this issue Mar 1, 2022 · 4 comments
Labels
Needs Investigation This issue needs a team member to investigate its status.
Milestone

Comments

@kasperpeulen
Copy link

kasperpeulen commented Mar 1, 2022

Bug Report

πŸ”Ž Search Terms

Control flow analysis destructuring assignment

πŸ•— Version & Regression Information

This was already there, but I thought it might have been fixed with the introduction of this feature in 4.6.

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

export const reduce = <S, T extends S>(self: Iterable<T>, operation: (acc: S, t: T) => S): S => {
  const iter = self[Symbol.iterator]();
  let { value, done }: IteratorResult<T, unknown> = iter.next();
  if (done) throw new Error("Empty iterable can't be reduced.");
  let acc: S = value; // Type 'unknown' is not assignable to type 'S'.
  while (!done) {
    acc = operation(acc, value);
    ({ value, done } = iter.next());
  }
  return acc;
};

Note that if you don't upcast iter.next(), you won't get errors, but not for good reasons:

let {value, done} = iter.next(); // type is IteratorResult<T, any>
if (done) throw new Error("Empty iterable can't be reduced.");
// value is any and can be assigned to anything

A smaller repo would be:

export function foo(iter: Iterator<number, string>) {
  let { value, done } = iter.next();
  if (done) return;
  let acc: number = value; // Type 'string | number' is not assignable to type 'number'. Type 'string' is not assignable to type 'number'.
  return acc;
}

πŸ™ Actual behavior

value above in the last example is inferred as number | string, it should be number

πŸ™‚ Expected behavior

I expect the same inference as without destructuring:

export const reduce2 = <S, T extends S>(self: Iterable<T>, operation: (acc: S, t: T) => S): S => {
  const iter = self[Symbol.iterator]();
  let result: IteratorResult<T, unknown> = iter.next();
  if (result.done) throw new Error("Empty iterable can't be reduced.");
  let acc: S = result.value; // result.value is T (extends S)
  while (!result.done) {
    acc = operation(acc, result.value);
    result = iter.next();
  }
  return acc;
};
@MartinJohns
Copy link
Contributor

  1. You forgot to fill out the issue template for bug reports.
  2. CFA for destructured types requires the type to be inferred, but you're explicitly setting the type. Remove the : IteratorResult<T, unknown> part from your variable declaration and it works.

@kasperpeulen
Copy link
Author

@MartinJohns Okay, I'm sorry, I will fill it out.

But, the bug is not fixed by leaving out IteratorResult<T, unknown>, see my last comment, it just won't give an error, because it is inferred as any, instead of unknown (second generic parameter is any by default).

You can also try:

function foo(iter: Iterator<number, unknown>) {
  let { value, done } = iter.next();
  if (done) throw new Error("Empty iterable can't be reduced.");
  let acc = value; // unknown, not number
}

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 3, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Mar 3, 2022
@kasperpeulen kasperpeulen changed the title Control Flow Analysis for Destructured Discriminated Unions doesn't work for IteratorResult Control Flow Analysis for Destructured Discriminated Unions doesn't work for let variables Apr 22, 2022
@kasperpeulen
Copy link
Author

kasperpeulen commented Apr 22, 2022

I have another example:

export const last = <T>(self: Iterable<T>): T => {
  let { found, last }: { found: false; last?: undefined } | { found: true; last: T } = {
    found: false,
  };
  for (const it of self) {
    ({ found, last } = { found: false, last: it });
  }
  if (!found) throw new NoSuchElementException("Iterable contains no element matching the predicate.");
  // can not infer last to be T
  return last;
};

@RyanCavanaugh @MartinJohns

@whzx5byb
Copy link

whzx5byb commented Aug 6, 2022

A shorter const vs let. Example taken from #46266.

type Action =
    | { kind: 'A', payload: number }
    | { kind: 'B', payload: string };

function f11(action: Action) {
    const { kind, payload } = action; // works
    if (kind === 'A') {
        payload.toFixed();
    }
    if (kind === 'B') {
        payload.toUpperCase();
    }
}

function _f11(action: Action) {
    let { kind, payload } = action; // doesn't work
    if (kind === 'A') {
        payload.toFixed();
    }
    if (kind === 'B') {
        payload.toUpperCase();
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

4 participants