microsoft
Don't compare "missing" to `undefined` in `compareProperties` under `exactOptionalPropertyTypes`
Fixes #61547
I'm aware that the underlying issue is (currently) labeled "not a defect", however, the behavior is undeniably strange, most likely simply overlooked when exactOptionalPropertyTypes was implemented, in my view. Therefore I am opening this PR to demonstrate what an easy fix it is (and that no other tests are broken as a result).
Starting jobs; this comment will be updated as builds start and complete.
| Command | Status | Results |
|---|---|---|
test top400 |
✅ Started | ✅ Results |
user test this |
✅ Started | ✅ Results |
run dt |
✅ Started | ✅ Results |
perf test this faster |
✅ Started | 👀 Results |
Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/61683/merge:
Everything looks good!
@jakebailey The results of the perf run you requested are in!
Here they are:
tsc
Comparison Report - baseline..pr| Metric | baseline | pr | Delta | Best | Worst | p-value |
|---|---|---|---|---|---|---|
| Compiler-Unions - node (v18.15.0, x64) | ||||||
| Errors | 34 | 34 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 62,390 | 62,390 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 50,395 | 50,395 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 194,755k (± 1.00%) | 193,619k (± 0.74%) | ~ | 192,937k | 196,535k | p=0.230 n=6 |
| Parse Time | 1.31s (± 0.39%) | 1.30s (± 0.94%) | ~ | 1.29s | 1.32s | p=0.241 n=6 |
| Bind Time | 0.73s | 0.73s | ~ | ~ | ~ | p=1.000 n=6 |
| Check Time | 9.73s (± 0.39%) | 9.76s (± 0.54%) | ~ | 9.68s | 9.82s | p=0.221 n=6 |
| Emit Time | 2.73s (± 0.50%) | 2.73s (± 0.67%) | ~ | 2.71s | 2.76s | p=0.591 n=6 |
| Total Time | 14.51s (± 0.21%) | 14.53s (± 0.30%) | ~ | 14.48s | 14.59s | p=0.376 n=6 |
| angular-1 - node (v18.15.0, x64) | ||||||
| Errors | 56 | 56 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 949,240 | 949,240 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 411,065 | 411,065 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 1,225,103k (± 0.00%) | 1,225,092k (± 0.00%) | ~ | 1,225,021k | 1,225,126k | p=0.378 n=6 |
| Parse Time | 6.63s (± 0.95%) | 6.67s (± 0.84%) | ~ | 6.56s | 6.71s | p=0.289 n=6 |
| Bind Time | 1.88s (± 0.43%) | 1.88s (± 0.40%) | ~ | 1.87s | 1.89s | p=0.729 n=6 |
| Check Time | 31.92s (± 0.27%) | 31.93s (± 0.22%) | ~ | 31.83s | 32.00s | p=0.936 n=6 |
| Emit Time | 15.22s (± 0.25%) | 15.20s (± 0.74%) | ~ | 15.11s | 15.42s | p=0.127 n=6 |
| Total Time | 55.66s (± 0.22%) | 55.68s (± 0.19%) | ~ | 55.53s | 55.82s | p=0.872 n=6 |
| mui-docs - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 2,439,920 | 2,439,920 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 872,553 | 872,553 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,448,534k (± 0.00%) | 2,448,463k (± 0.00%) | ~ | 2,448,390k | 2,448,583k | p=0.066 n=6 |
| Parse Time | 10.54s (± 0.65%) | 10.58s (± 0.62%) | ~ | 10.47s | 10.66s | p=0.377 n=6 |
| Bind Time | 2.72s (± 0.45%) | 2.71s (± 1.01%) | ~ | 2.67s | 2.75s | p=0.462 n=6 |
| Check Time | 88.08s (± 1.75%) | 87.44s (± 2.62%) | ~ | 86.06s | 92.06s | p=0.298 n=6 |
| Emit Time | 0.37s (± 2.67%) | 0.37s (± 2.41%) | ~ | 0.36s | 0.38s | p=0.931 n=6 |
| Total Time | 101.70s (± 1.54%) | 101.10s (± 2.27%) | ~ | 99.84s | 105.74s | p=0.378 n=6 |
| self-build-src - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,228,241 | 1,228,241 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 267,270 | 267,270 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 3,094,833k (± 0.02%) | 3,095,338k (± 0.02%) | ~ | 3,094,660k | 3,096,308k | p=0.093 n=6 |
| Parse Time | 6.72s (± 0.34%) | 6.70s (± 0.32%) | ~ | 6.67s | 6.72s | p=0.128 n=6 |
| Bind Time | 2.13s (± 0.78%) | 2.16s (± 0.86%) | +0.03s (+ 1.56%) | 2.14s | 2.18s | p=0.030 n=6 |
| Check Time | 42.79s (± 0.49%) | 42.88s (± 0.22%) | ~ | 42.79s | 43.05s | p=0.575 n=6 |
| Emit Time | 3.50s (± 1.57%) | 3.42s (± 2.81%) | ~ | 3.33s | 3.57s | p=0.128 n=6 |
| Total Time | 55.13s (± 0.43%) | 55.16s (± 0.16%) | ~ | 55.01s | 55.24s | p=0.575 n=6 |
| self-build-src-public-api - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,228,241 | 1,228,241 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 267,270 | 267,270 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,674,444k (±14.04%) | 3,038,397k (± 9.78%) | ~ | 2,431,009k | 3,161,183k | p=0.173 n=6 |
| Parse Time | 6.87s (± 1.84%) | 6.95s (± 2.40%) | ~ | 6.65s | 7.16s | p=0.575 n=6 |
| Bind Time | 2.21s (± 1.55%) | 2.15s (± 2.59%) | ~ | 2.10s | 2.26s | p=0.077 n=6 |
| Check Time | 42.75s (± 0.50%) | 42.98s (± 0.34%) | ~ | 42.79s | 43.13s | p=0.066 n=6 |
| Emit Time | 3.56s (± 2.74%) | 3.48s (± 2.92%) | ~ | 3.37s | 3.63s | p=0.378 n=6 |
| Total Time | 55.39s (± 0.72%) | 55.54s (± 0.60%) | ~ | 55.13s | 56.02s | p=0.471 n=6 |
| self-compiler - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 263,442 | 263,442 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 107,097 | 107,097 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 441,827k (± 0.01%) | 441,786k (± 0.02%) | ~ | 441,682k | 441,930k | p=0.298 n=6 |
| Parse Time | 3.56s (± 1.22%) | 3.56s (± 0.73%) | ~ | 3.54s | 3.61s | p=0.871 n=6 |
| Bind Time | 1.33s (± 0.95%) | 1.33s (± 1.00%) | ~ | 1.31s | 1.35s | p=0.788 n=6 |
| Check Time | 19.04s (± 0.33%) | 19.06s (± 0.17%) | ~ | 19.02s | 19.12s | p=0.124 n=6 |
| Emit Time | 1.54s (± 0.79%) | 1.52s (± 0.59%) | -0.02s (- 1.08%) | 1.51s | 1.53s | p=0.039 n=6 |
| Total Time | 25.46s (± 0.30%) | 25.47s (± 0.14%) | ~ | 25.44s | 25.53s | p=0.936 n=6 |
| ts-pre-modules - node (v18.15.0, x64) | ||||||
| Errors | 71 | 71 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 225,981 | 225,981 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 94,356 | 94,356 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 371,270k (± 0.01%) | 371,373k (± 0.05%) | ~ | 371,210k | 371,633k | p=0.689 n=6 |
| Parse Time | 3.58s (± 0.86%) | 3.59s (± 0.95%) | ~ | 3.55s | 3.63s | p=0.629 n=6 |
| Bind Time | 1.96s (± 1.04%) | 1.97s (± 0.75%) | ~ | 1.95s | 1.99s | p=0.370 n=6 |
| Check Time | 20.50s (± 0.40%) | 20.46s (± 0.11%) | ~ | 20.43s | 20.49s | p=0.261 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 26.04s (± 0.31%) | 26.01s (± 0.23%) | ~ | 25.94s | 26.08s | p=0.629 n=6 |
| vscode - node (v18.15.0, x64) | ||||||
| Errors | 51 | 51 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 3,427,972 | 3,427,972 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 1,156,170 | 1,156,170 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 3,477,054k (± 0.01%) | 3,477,182k (± 0.00%) | ~ | 3,477,048k | 3,477,353k | p=0.230 n=6 |
| Parse Time | 18.03s (± 0.35%) | 18.02s (± 0.34%) | ~ | 17.92s | 18.09s | p=0.688 n=6 |
| Bind Time | 5.78s (± 0.37%) | 5.77s (± 0.40%) | ~ | 5.74s | 5.80s | p=0.413 n=6 |
| Check Time | 116.14s (± 2.64%) | 114.09s (± 2.69%) | ~ | 112.29s | 120.32s | p=0.066 n=6 |
| Emit Time | 37.01s (± 2.21%) | 36.28s (± 0.37%) | ~ | 36.07s | 36.46s | p=0.261 n=6 |
| Total Time | 176.97s (± 2.15%) | 174.17s (± 1.75%) | ~ | 172.47s | 180.36s | p=0.066 n=6 |
| webpack - node (v18.15.0, x64) | ||||||
| Errors | 2 | 2 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 317,848 | 317,848 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 140,485 | 140,485 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 473,085k (± 0.01%) | 473,071k (± 0.02%) | ~ | 472,881k | 473,168k | p=0.936 n=6 |
| Parse Time | 4.12s (± 1.05%) | 4.14s (± 0.91%) | ~ | 4.08s | 4.19s | p=0.336 n=6 |
| Bind Time | 1.82s (± 1.30%) | 1.83s (± 1.58%) | ~ | 1.79s | 1.86s | p=0.936 n=6 |
| Check Time | 21.00s (± 0.39%) | 20.89s (± 0.33%) | ~ | 20.81s | 20.98s | p=0.065 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 26.95s (± 0.38%) | 26.86s (± 0.48%) | ~ | 26.71s | 26.98s | p=0.199 n=6 |
| xstate-main - node (v18.15.0, x64) | ||||||
| Errors | 5 | 5 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 570,733 | 570,733 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 191,446 | 191,446 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 501,130k (± 0.04%) | 501,354k (± 0.02%) | ~ | 501,253k | 501,465k | p=0.066 n=6 |
| Parse Time | 4.33s (± 0.68%) | 4.32s (± 0.54%) | ~ | 4.28s | 4.35s | p=0.372 n=6 |
| Bind Time | 1.53s (± 1.91%) | 1.54s (± 1.06%) | ~ | 1.53s | 1.57s | p=0.805 n=6 |
| Check Time | 25.97s (± 3.61%) | 25.01s (± 0.47%) | ~ | 24.80s | 25.13s | p=0.230 n=6 |
| Emit Time | 0.00s | 0.00s (±244.70%) | ~ | 0.00s | 0.01s | p=0.405 n=6 |
| Total Time | 31.83s (± 2.82%) | 30.87s (± 0.36%) | ~ | 30.66s | 30.96s | p=0.229 n=6 |
- node (v18.15.0, x64)
- Compiler-Unions - node (v18.15.0, x64)
- angular-1 - node (v18.15.0, x64)
- mui-docs - node (v18.15.0, x64)
- self-build-src - node (v18.15.0, x64)
- self-build-src-public-api - node (v18.15.0, x64)
- self-compiler - node (v18.15.0, x64)
- ts-pre-modules - node (v18.15.0, x64)
- vscode - node (v18.15.0, x64)
- webpack - node (v18.15.0, x64)
- xstate-main - node (v18.15.0, x64)
| Benchmark | Name | Iterations |
|---|---|---|
| Current | pr | 6 |
| Baseline | baseline | 6 |
Developer Information:
@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/61683/merge:
Everything looks good!
@jakebailey given the likelihood of this being an oversight when exactOptionalPropertyTypes was implemented and the simplicity of the fix, do you see it likely we'd be able to remove the "not a defect" label from the original issue (and hopefully add it to the backlog so that this PR can be reviewed)?
As you saw, I was hoping to get confirmation from the original author @ahejlsberg that it was simply a case of "missed a spot" when changing getTypeOfSymbol to getNonMissingTypeOfSymbol, but I haven't heard anything back as of yet.
Starting jobs; this comment will be updated as builds start and complete.
| Command | Status | Results |
|---|---|---|
pack this |
✅ Started | ✅ Results |
Hey @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/165319/artifacts?artifactName=tgz&fileId=4F665885689AA916692B97D478FF2A8630D9C111C45A58BF75E4C117B832487302&fileName=/typescript-5.9.0-insiders.20250609.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/[email protected]".;
Testing #61547 with Playground Link, are these errors right? The first one is the reported one, sure, but it's seemingly missing two errors.
I think this PR should be encoding all of the test provided in the issue in any case. But, it's not clear if the new behavior is correct either.
@jakebailey hi! Can you explain what you mean by "missing two errors"? Not sure I follow. The issue is complaining that the Assert<Equal<T1, T2>> is not considered an error in v5.8.3, but it should be considered an error. (While all of the assertions that follow this are correct and their behavior should not be changed--these code paths are already heavily tested, hence why I did not include additional tests for them.)
The example seemingly implies that under exactOptionalPropertyTypes that all of those assertions should pass, except that the first one doesn't. But with this PR, the first is eliminated but the next two now fail.
Hm, I might have some wires crossed in my explanation given there are 2 playgrounds, 2 settings, and 4 assertions (sorry, I need to explain it better), but the gist is that it seems like this PR introduces errors into one of the variants that weren't there previously in 5.8.
The example seemingly implies that under
exactOptionalPropertyTypesthat all of those assertions should pass, except that the first one doesn't.
@jakebailey I can see how the issue could be misconstrued, however, simply changing the environment to 5.8.3 shows that the first assertion does pass currently.
The issue was raised to highlight that the first assertion should fail but currently passes. This is the reason for the "❌" in the comment above just that one assertion: identifying that this current behavior is incorrect.
The subsequent assertions are only provided to show that under exactOptionalPropertyTypes, T2 is not assignable to T1: evidence that T2 & T1 should not be considered identical types, and therefore that the first assertion is incorrectly passing.
with this PR, the first is eliminated but the next two now fail
I'm still not seeing these 2 failures you're mentioning... opening your playground link gives me what I would expect:
it seems like this PR introduces errors into one of the variants that weren't there previously in 5.8.
Yes, this is absolutely correct and very much intended! The whole point of the issue is to do exactly that, to introduce one error into the first assertion. (If there were any other assertions besides the first that became an error, that would definitely be a bug in my implementation, but I don't see how that could be possible because I haven't touched those code paths.)
Let me go through the matrix:
| Version | exactOptionalPropertyTypes=false |
exactOptionalPropertyTypes=true |
|---|---|---|
| 5.8.3 | ||
| This PR |
I think I was just mistaken. exactOptionalPropertyTypes=false is unaffected.
@jakebailey correct, because getNonMissingTypeOfSymbol is semantically identical to getTypeOfSymbol under exactOptionalPropertyTypes=false, so this PR is a no-op in that case.
Let me know if you need anything else from me! 😃
@jakebailey anything else you need from me on this and/or anything blocking this PR being accepted? Should I click "Update branch" or does that not matter?
@jakebailey follow-up: I see that typescript has been publishing "6.0.0-dev" versions on npm for a while now, does that imply that the tree has opened for 6.0 and the code freeze has ended or did you mean something else?
@jakebailey I noticed the branch is already out of date again: do you need me to keep merging from main every time I see it goes out of date to remain unblocked? Happy to do so if that's the case.
Nope, I asked for a merge because we added some new required CI checks that weren't present on the branch the last time CI ran.