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

if ('A' in el) {} causes code below to error #53425

Closed
ben-clayton opened this issue Mar 21, 2023 · 9 comments · Fixed by #53435
Closed

if ('A' in el) {} causes code below to error #53425

ben-clayton opened this issue Mar 21, 2023 · 9 comments · Fixed by #53435
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@ben-clayton
Copy link

Bug Report

🔎 Search Terms

narrowing in if
Argument of type 'unknown'

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about type narrowing

⏯ Playground Link

Playground link with relevant code

💻 Code

export interface IA {
  arr: { A: number; }[];
}

export interface IAB {
  arr: { A: number; B: number; }[];
}

export function F(x: IA | IAB) {
  const useB = (t: number) => {};
  for (const el of x.arr) {
    if ('A' in el) {} // comment out this line to make 'useB(el.B)' compile without error
    if ('B' in el) {
      // When 'if ('A' in el) {}' above is not commentted out, the line below will error with:
      // error : Argument of type 'unknown' is not assignable to parameter of type 'number'.(2345)
      useB(el.B);     
    }
  }
}

🙁 Actual behavior

useB(el.B);
     ^^^^
error : Argument of type 'unknown' is not assignable to parameter of type 'number'.(2345)

The above error is raised if the line if ('A' in el) {} is not commented out.
Commenting out if ('A' in el) {} compiles without error.

🙂 Expected behavior

That the line if ('A' in el) {} does not narrow the type of el in a way that prevents code outside of the if from compiling.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 21, 2023
@ben-clayton
Copy link
Author

A slightly further reduced case:

export interface I {
  arr: 
     | { A: number; }[] 
     | { A: number; B: number; }[];
}

export function F(i: I) {
  const useB = (n: number) => {};
  for (const el of i.arr) {
    if ('A' in el) {} // comment out this line to make 'useB(el.B)' compile without error
    if ('B' in el) {
      // When 'if ('A' in el) {}' above is not commentted out, the line below will error with:
      // error : Argument of type 'unknown' is not assignable to parameter of type 'number'.(2345)
      useB(el.B);     
    }
  }
}

@fatcerberus
Copy link

This looks like a symptom of subtype reduction. At some point (probably after CFA recombines the types at the edge of the first if block) IA | IAB gets subtype-reduced to just IA, leaving B to be re-manufactured from whole cloth… as unknown

In the past subtype reduction issues like this have been considered unfortunate, but ultimately intended behavior.

@ahejlsberg
Copy link
Member

This is indeed an effect of subtype reduction. However, subtype reduction is supposed to occur only in cases where "foreign" types (meaning types that aren't part of the declared type) are injected by narrowing, and there are no such foreign types in the example. Yet there is a never type occurring in the empty false branch of the first if statement, and that never is mistakenly considered a foreign type, thus triggering subtype reduction. That is something we should fix. It might even help performance as it is fairly common to have branches in which something narrows to never.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Mar 22, 2023
@ahejlsberg ahejlsberg added this to the TypeScript 5.1.0 milestone Mar 22, 2023
@fatcerberus
Copy link

@ahejlsberg Huh, this is the first time I’m hearing about a hard rule for when subtype reduction is allowed to happen. Ryan usually just handwaves it as, subtype reduction can happen any time, so don’t depend on it not happening.

@ahejlsberg
Copy link
Member

@fatcerberus As a rule, subtype reduction can happen any time, but since it is costly (worst case quadratic) we labor hard to avoid it when possible. The "foreign" type detection in CFA is an example of that.

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 22, 2023

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.)

@fatcerberus
Copy link

… difference between IA and IA | IAB. Ideally you wouldn't be able to observe a difference between the two …

I agree in principle, though my suspicion is that so long as 1. users can write the latter without it being immediately reduced, and 2. unions can be narrowed, the difference will always be observable—and thus users will keep writing the latter because being able to narrow to a subtype using idiomatic (if admittedly unsound) syntax is indeed useful.

@ben-clayton
Copy link
Author

Thank you ever so much for the impressively fast fix for this!

@fatcerberus
Copy link

fatcerberus commented Mar 23, 2023

@ben-clayton Just be aware that the original error is technically correct according to the rules of the type system (which is what Anders is getting at above when he says that IA and IA | IAB are equivalent): { A: number, B: string } is assignable to IA. Depending on who’s calling F, that might be a footgun you need to watch out for.1

Footnotes

  1. In general, this pitfall is unavoidable without Exact Types #12936.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants