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

[5.3] Type narrowing regression with property comparison to boolean false #56482

Closed
meyfa opened this issue Nov 21, 2023 · 4 comments Β· Fixed by #56504
Closed

[5.3] Type narrowing regression with property comparison to boolean false #56482

meyfa opened this issue Nov 21, 2023 · 4 comments Β· Fixed by #56504
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue Recent Regression This is a new regression just found in the last major/minor version of TypeScript.

Comments

@meyfa
Copy link

meyfa commented Nov 21, 2023

πŸ”Ž Search Terms

narrowing, strict equals, property access, boolean false, undefined

πŸ•— Version & Regression Information

  • This changed between versions 5.2 and 5.3
  • This also happens in the nightly

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.3.1-rc#code/JYOwLgpgTgZghgYwgAgGIHt3IN4ChnIBGcUAXEZgDYRwi4C+uuC6IAzmMgOYRgZYBeZAAoAlOX7IAPsgCuIACYQYoCAuQCAfDnzIovWVBDIAsnDAALAHRRaC9AFsxybQAYrAVmQB+HERLk8JRsKPTI5PJKKiBqDEws7JwwmBrcvPxiuMAwIsno3lbEUBoCQkEhojoECWzo1FaU6FzCeaJxuAD0HcgAAmBsALQQAB4ADhAIYENQUOjFA8h5yMBsyKPobGzAhJQAnnKKyqoKzKy19Y3NeYUkbUA

πŸ’» Code

interface Foo {
  bar: boolean
}

const getFoo = (): Foo | undefined => {
  return Math.random() > 0.5 ? { bar: false } : undefined
}

const foo = getFoo()
if (foo?.bar === false) {
  console.log(foo)
}

// @ts-expect-error - foo is possibly undefined
console.log(foo.bar)

πŸ™ Actual behavior

In line 13 and following, foo is incorrectly narrowed to just Foo (whereas it was previously Foo | undefined), although we learned nothing about it from the preceding if as that did not influence control flow.

Unused '@ts-expect-error' directive.(2578)

πŸ™‚ Expected behavior

TS should report an error when accessing foo.bar as foo can potentially be undefined.

Additional information about the issue

I originally discovered this issue in JSX with a snippet similar to:

function Component () {
  const foo = getFoo()
  return (
    <div>
      {foo?.bar === false && 'foo'}
      {foo.bar ? 'true' : 'false'}
    </div>
  )
}

There as well, the access foo.bar is allowed due to the preceding check, although they are completely separate.

Note that changing it to === true or removing the checks entirely fixes the problem.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript Recent Regression This is a new regression just found in the last major/minor version of TypeScript. labels Nov 21, 2023
@Andarist
Copy link
Contributor

Andarist commented Nov 21, 2023

It looks very much related to my change: #53714 . I'll investigate this soon. Thanks for the issue report!

@akwodkiewicz
Copy link

akwodkiewicz commented Nov 22, 2023

Here's a different reproduction resembling the one we have in our project, maybe you find it helpful:

export interface Options {
    a?: boolean;
    b?: boolean;
}

function foobar(options?: Options) {
    // @ts-expect-error 'options' is possibly 'undefined'
    if (options?.a === false || options.b) {
    }
}

https://www.typescriptlang.org/play?ts=5.3.2#code/KYDwDg9gTgLgBASwHY2FAZgQwMbDgeTBgQiQGc4BvAKDjrkwH4AuOAIwggBthMkBuWvTYt2nHn0EBfatXQBXJNmKk46Tm0xQAFBCIlyowivIBKKkLoB6K3AACMMgFpQYYMpdQo0OAHI9JmS+iBSQZGQIbFwAnn6KACbA6MjA8b6WiOhwuvqkZIwAdJhwALxlaphcZHgAPjVwAQZkBWzmNPRwMlJAA

EDIT: I tested it with the nightly version of TS from your PR and yes, it's fixed there.

@jakebailey
Copy link
Member

This issue has a lot of thumbs up; out of curiosity, are people finding this naturally? Or is there some Twitter thread that's directing people here?

The original PR didn't fail any of the extended test suites, so it's somewhat surprising to me to have a regression like this.

@Andarist
Copy link
Contributor

My guess is that it was posted on a company’s Slack channel or smth and a couple of devs from that channel +1ed this πŸ˜‰

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 Recent Regression This is a new regression just found in the last major/minor version of TypeScript.
Projects
None yet
6 participants