-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Don't compare "missing" to undefined
in compareProperties
under exactOptionalPropertyTypes
#61683
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
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot test it |
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 Everything looks good! |
undefined
in compareProperties
under exactOptionalPropertyTypes
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@jakebailey given the likelihood of this being an oversight when 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 |
@typescript-bot pack this |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
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 |
The example seemingly implies that under |
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. |
@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
I'm still not seeing these 2 failures you're mentioning... opening your playground link gives me what I would expect:
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.) |
@jakebailey correct, because Let me know if you need anything else from me! 😃 |
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).