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

Possible Regression in v4.9: Conditionals using Unlisted Property Narrowing with the in Operator #52812

Closed
mattkrick opened this issue Feb 16, 2023 · 3 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@mattkrick
Copy link

Bug Report

πŸ”Ž Search Terms

v4.9 conditionals unlisted property narrowing in operator #50666

πŸ•— Version & Regression Information

v4.9+, including nightly
working in v4.8.4

  • This changed between versions 4.8.4 and 4.9

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type HandledResult = {data: any, errors?: any}
type UnhandledResult = {message: string} 
const result = null as any as HandledResult | UnhandledResult

const noWork = (): HandledResult => {    
    if ('data' in result || 'errors' in result) return result // when checking an optional property second, I get an error 
    return {data: null, errors: [result.message]}
}

const work = (): HandledResult => {
    if ('errors' in result || 'data' in result ) return result // reversing the order fixes the problem
    return {data: null, errors: [result.message]}
}

πŸ™ Actual behavior

In the noWork function, typescript could not narrow down the type to being HandledResult. When the or clause is switched, it works. Since Boolean(A || B) === Boolean(B || A) this seems like an error.

πŸ™‚ Expected behavior

The ordering of the conditionals should not impact the narrowing of the type.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Feb 17, 2023

This code is confusing to reason about because it is written in a way that presupposes an invariant violation occurring somewhere along the way.

Our logic effectively thinks of it this way:

In "noWork", it goes like this:

  • If "data" in result, then it's clearly a HandledResult -> OK
  • If not "data" in result, then it's clearly not a HandledResult, since that's a mandatory property, so it must be an UnhandledResult
  • If errors is present, then it's must be a UnhandledResult & { errors: unknown }; that's the only thing it can be. That's not a valid return type, so the program is correctly rejected

In "work", it goes like this:

  • If "errors" in result then it's clearly a HandledResult, since that's one of its listed optional properties
  • If "data" in result then it's clearly a HandledResult, since that's one of its listed required properties
  • Either way this code is fine

The functions don't make any sense if you assume the types are closed, which is the only operative condition in which using in makes any sense in the first place. I don't really see a way to change this without breaking more reasonable code that would expect not "data" in result to produce an UnhandledResult

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Feb 17, 2023
@fatcerberus
Copy link

fatcerberus commented Feb 18, 2023

In "work", it goes like this: ...

It's interesting to me that in the "working" case, TS never considers the possibility that "data" in result might be false, like it does in the "non-working" case. It seems like it should, since || is commutative (short-circuiting of side effects notwithstanding).

@mattkrick
Copy link
Author

thank you so much for your thoughtful explanation. πŸ™
it took a few re-reads, but it eventually makes sense why it works the way it does! I was treating them like open types where {data?: any, errors: any} could possibly exist & it prevented that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

3 participants