FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fixed an issue in boolean comparison narrowing when the reference is an optional chain by Andarist · Pull Request #56504 · microsoft/TypeScript · GitHub

/ TypeScript Public

Fixed an issue in boolean comparison narrowing when the reference is an optional chain - #56504

Merged
Jake Bailey (jakebailey) merged 3 commits into
microsoft:mainfrom
Andarist:fix/boolean-comparison-narrowing-vs-optional-chains
Nov 22, 2023
ConversationCommits (3)ChecksFiles changed
Merged

Fixed an issue in boolean comparison narrowing when the reference is an optional chain#56504
Jake Bailey (jakebailey) merged 3 commits into
microsoft:mainfrom
Andarist:fix/boolean-comparison-narrowing-vs-optional-chains

Conversation

Mateusz Burzyński (Andarist) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 22, 2023
Mateusz Burzyński (Andarist) commented Nov 22, 2023
View reviewed changes
Comment thread src/compiler/checker.ts
return narrowTypeByConstructor(type, operator, left, assumeTrue);
}
if (isBooleanLiteral(right)) {
if (isBooleanLiteral(right) && !isAccessExpression(left)) {

Mateusz Burzyński (Andarist) Nov 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It turns out that the original type can be reassigned based on optionalChainContainsReference. That messed up the else branch because narrowTypeByBooleanComparison was operating on the wrong type.

I've also tried keeping originalType around and pass that to narrowTypeByBooleanComparison. That fixed the reported issue~ related to the wrong type in the else branch but it broke the same type within the if block. This happened because originally the intention was for that reassigned type to be returned through the return at the bottom of this function but I changed it to return originalType through narrowTypeByBooleanComparison.

So all in all, this seems like the simplest fix and reflects the intention the best. The motivation behind the original change was to improve non-access expression narrowing. Access expressions are already handled by discrimination-based narrowing etc.

Even though it worked previously (as far as I know) for non-optional chain access expressions, this helps them avoid some redundant work too.

Jake Bailey (jakebailey) Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This seems fine as a quick fix for now, though I suspect that we could try and do better later.

Jake Bailey (jakebailey) Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Then again, not sure what "better" even means; an expression like (options?.a === false || options.b is not the same as (!options?.a || options.b) so realistically I don't think there's any extra info gained here.

Jake Bailey (jakebailey) commented Nov 22, 2023

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) test top200
TypeScript Bot (@typescript-bot) user test this
TypeScript Bot (@typescript-bot) run dt

TypeScript Bot (@typescript-bot) perf test this
TypeScript Bot (@typescript-bot) pack this

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at b549581. You can monitor the build here.

Update: The results are in!

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at b549581. You can monitor the build here.

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at b549581. You can monitor the build here.

Update: The results are in!

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at b549581. You can monitor the build here.

Update: The results are in!

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to run the regular perf test suite on this PR at b549581. You can monitor the build here.

Update: The results are in!

TypeScript Bot (typescript-bot) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/158776/artifacts?artifactName=tgz&fileId=0354B9F2A4021077377EA088E6459FA61983E3E2334251E88120F6D99779965F02&fileName=/typescript-5.4.0-insiders.20231122.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-56504-6".;

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing main and refs/pull/56504/merge:

There were infrastructure failures potentially unrelated to your change:

  • 2 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,190k (± 0.00%) 295,192k (± 0.01%) ~ 295,147k 295,236k p=0.810 n=6
Parse Time 2.64s (± 0.15%) 2.64s (± 0.24%) ~ 2.63s 2.65s p=0.673 n=6
Bind Time 0.83s (± 0.62%) 0.82s (± 0.50%) ~ 0.82s 0.83s p=0.112 n=6
Check Time 8.04s (± 0.33%) 8.04s (± 0.17%) ~ 8.02s 8.06s p=0.868 n=6
Emit Time 7.10s (± 0.37%) 7.08s (± 0.34%) ~ 7.04s 7.11s p=0.506 n=6
Total Time 18.60s (± 0.10%) 18.59s (± 0.21%) ~ 18.52s 18.63s p=0.686 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,612k (± 1.55%) 192,091k (± 1.25%) ~ 190,683k 196,534k p=0.936 n=6
Parse Time 1.35s (± 1.37%) 1.35s (± 1.01%) ~ 1.34s 1.38s p=0.932 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.18s (± 0.38%) 9.18s (± 0.19%) ~ 9.16s 9.20s p=0.807 n=6
Emit Time 2.61s (± 0.62%) 2.62s (± 0.78%) ~ 2.59s 2.64s p=0.685 n=6
Total Time 13.86s (± 0.30%) 13.88s (± 0.23%) ~ 13.82s 13.91s p=0.746 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,365k (± 0.01%) 347,358k (± 0.00%) ~ 347,344k 347,385k p=0.575 n=6
Parse Time 2.46s (± 0.54%) 2.47s (± 0.43%) ~ 2.45s 2.48s p=0.737 n=6
Bind Time 0.92s (± 0.59%) 0.93s (± 0.44%) ~ 0.92s 0.93s p=0.282 n=6
Check Time 6.90s (± 0.57%) 6.89s (± 0.38%) ~ 6.87s 6.94s p=1.000 n=6
Emit Time 4.05s (± 0.41%) 4.05s (± 0.34%) ~ 4.03s 4.07s p=0.868 n=6
Total Time 14.34s (± 0.33%) 14.33s (± 0.19%) ~ 14.29s 14.37s p=0.807 n=6
TFS - node (v18.15.0, x64)
Memory used 302,623k (± 0.01%) 302,615k (± 0.01%) ~ 302,584k 302,638k p=0.748 n=6
Parse Time 1.99s (± 0.61%) 2.00s (± 0.61%) ~ 1.98s 2.01s p=0.219 n=6
Bind Time 1.00s (± 1.17%) 1.01s (± 0.97%) ~ 1.00s 1.02s p=0.314 n=6
Check Time 6.25s (± 0.16%) 6.24s (± 0.31%) ~ 6.21s 6.26s p=0.192 n=6
Emit Time 3.58s (± 0.38%) 3.58s (± 0.65%) ~ 3.55s 3.61s p=1.000 n=6
Total Time 12.83s (± 0.21%) 12.82s (± 0.15%) ~ 12.80s 12.85s p=0.870 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,611k (± 0.02%) 470,577k (± 0.02%) ~ 470,521k 470,711k p=0.471 n=6
Parse Time 2.56s (± 0.64%) 2.57s (± 0.45%) ~ 2.56s 2.59s p=0.510 n=6
Bind Time 1.00s (± 1.65%) 0.98s (± 1.23%) ~ 0.97s 1.00s p=0.285 n=6
Check Time 16.75s (± 0.64%) 16.78s (± 0.33%) ~ 16.69s 16.84s p=0.419 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.31s (± 0.60%) 20.33s (± 0.26%) ~ 20.26s 20.40s p=0.423 n=6
xstate - node (v18.15.0, x64)
Memory used 512,004k (± 0.02%) 512,003k (± 0.01%) ~ 511,941k 512,098k p=0.873 n=6
Parse Time 3.27s (± 0.25%) 3.28s (± 0.27%) ~ 3.27s 3.29s p=0.270 n=6
Bind Time 1.54s (± 0.27%) 1.54s (± 0.35%) ~ 1.54s 1.55s p=0.054 n=6
Check Time 2.81s (± 0.59%) 2.80s (± 0.94%) ~ 2.78s 2.85s p=0.418 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 4.99%) ~ 0.08s 0.09s p=0.405 n=6
Total Time 7.70s (± 0.18%) 7.70s (± 0.37%) ~ 7.67s 7.75s p=0.628 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,374ms (± 0.61%) 2,365ms (± 0.68%) ~ 2,342ms 2,383ms p=0.575 n=6
Req 2 - geterr 5,350ms (± 1.27%) 5,367ms (± 1.63%) ~ 5,303ms 5,481ms p=0.936 n=6
Req 3 - references 327ms (± 1.36%) 327ms (± 1.09%) ~ 323ms 333ms p=1.000 n=6
Req 4 - navto 279ms (± 1.01%) 278ms (± 1.14%) ~ 273ms 280ms p=1.000 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 81ms (± 3.58%) 84ms (± 5.48%) ~ 80ms 90ms p=0.286 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,471ms (± 1.01%) 2,493ms (± 0.83%) ~ 2,462ms 2,514ms p=0.128 n=6
Req 2 - geterr 4,132ms (± 0.97%) 4,090ms (± 1.88%) ~ 4,014ms 4,168ms p=0.873 n=6
Req 3 - references 336ms (± 1.31%) 339ms (± 1.57%) ~ 334ms 345ms p=0.565 n=6
Req 4 - navto 282ms (± 0.29%) 283ms (± 0.47%) ~ 281ms 284ms p=0.584 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 80ms (± 6.12%) 85ms (± 7.58%) ~ 78ms 91ms p=0.058 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,594ms (± 0.50%) 2,599ms (± 0.49%) ~ 2,579ms 2,616ms p=0.470 n=6
Req 2 - geterr 1,659ms (± 1.66%) 1,661ms (± 1.67%) ~ 1,625ms 1,695ms p=0.810 n=6
Req 3 - references 113ms (± 9.54%) 113ms (±10.38%) ~ 101ms 124ms p=0.467 n=6
Req 4 - navto 365ms (± 0.57%) 365ms (± 0.62%) ~ 363ms 369ms p=0.807 n=6
Req 5 - completionInfo count 2,073 (± 0.00%) 2,073 (± 0.00%) ~ 2,073 2,073 p=1.000 n=6
Req 5 - completionInfo 311ms (± 1.96%) 310ms (± 1.76%) ~ 302ms 316ms p=0.747 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstateTSServer - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v18.15.0, x64)
Execution time 152.47ms (± 0.21%) 152.27ms (± 0.20%) -0.21ms (- 0.13%) 151.22ms 155.84ms p=0.000 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time 227.91ms (± 0.16%) 227.64ms (± 0.15%) -0.26ms (- 0.11%) 226.15ms 232.48ms p=0.000 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time 229.30ms (± 0.21%) 229.35ms (± 0.17%) +0.05ms (+ 0.02%) 227.80ms 232.08ms p=0.021 n=600
typescript-startup - node (v18.15.0, x64)
Execution time 229.07ms (± 0.18%) 229.25ms (± 0.19%) +0.17ms (+ 0.08%) 227.79ms 235.58ms p=0.000 n=600
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • tsc-startup - node (v18.15.0, x64)
  • tsserver-startup - node (v18.15.0, x64)
  • tsserverlibrary-startup - node (v18.15.0, x64)
  • typescript-startup - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing main and refs/pull/56504/merge:

Everything looks good!

Jake Bailey (jakebailey) approved these changes Nov 22, 2023
View reviewed changes
Comment thread src/compiler/checker.ts
return narrowTypeByConstructor(type, operator, left, assumeTrue);
}
if (isBooleanLiteral(right)) {
if (isBooleanLiteral(right) && !isAccessExpression(left)) {

Jake Bailey (jakebailey) Nov 22, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This seems fine as a quick fix for now, though I suspect that we could try and do better later.

Jake Bailey (jakebailey) commented Nov 22, 2023

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.3

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to cherry-pick this into release-5.3 for you. Here's the link to my best guess at the log.

TypeScript Bot (typescript-bot) added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 22, 2023

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I was unable to cherry-pick this PR.

Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/6961303591

Jake Bailey (jakebailey) commented Nov 22, 2023

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.3

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to cherry-pick this into release-5.3 for you. Here's the link to my best guess at the log.

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I was unable to cherry-pick this PR.

Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/6961580628

Jake Bailey (jakebailey) commented Nov 22, 2023

Copy link
Copy Markdown
Member

TypeScript Bot (@typescript-bot) cherry-pick this to release-5.3

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Heya Jake Bailey (@jakebailey), I've started to cherry-pick this into release-5.3 for you. Here's the link to my best guess at the log.

TypeScript Bot (typescript-bot) commented Nov 22, 2023

Copy link
Copy Markdown
Contributor

Hey Jake Bailey (@jakebailey), I've created #56512 for you.

Merged
Jake Bailey (jakebailey) merged commit d4fbc9b into microsoft:main Nov 22, 2023

Daniel Rosenwasser (DanielRosenwasser) commented Nov 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Is using isAccessExpression just intended to not check more deeply on the expression?

Daniel Rosenwasser (DanielRosenwasser) commented Nov 22, 2023

Copy link
Copy Markdown
Member

Never mind, I think I understand now.

Jake Bailey (jakebailey) commented Nov 22, 2023

Copy link
Copy Markdown
Member

Is using isAccessExpression just intended to not check more deeply on the expression?

If you write an expression like options?.a === false || options.b or options?.a === undefined || options.b, it's not the same as like !options?.a || options.b; specifically, it doesn't provide any information about whether the equality was done on options or options.a, so we can't know if options was undefined or not for the following expression. Kinda makes sense; we're allowing a narrowing based on a specific falsy value.

It's possible that this could work with just ==, though, but I think the big hammer is okay.

David Sherret (dsherret) pushed a commit to denoland/TypeScript that referenced this pull request Dec 6, 2023
🤖 Pick PR microsoft#56504 (Fixed an issue in boolean compariso...) in…
f3808c4 
…to release-5.3 (microsoft#56512)

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Jake Bailey (jakebailey) jakebailey approved these changes

Assignees

No one assigned

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

TypeScript 5.3.3

Development

Successfully merging this pull request may close these issues.

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

4 participants

Back | FazBrowse Home | New Git URL