Don't special case nullable types in isTypeEqualityComparableTo
#59559
Conversation
This file contains 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
typescript-bot
added
the
For Uncommitted Bug
|
The TypeScript team hasn't accepted the linked issue #11920. If you can get it accepted, this PR will have a better chance of being reviewed. |
|
Why not @typescript-bot test it |
|
Really excited about this one. This has been costing us real bugs uncaught by TS. When we checked undefined but didn't check for null and undefined was literally not even a potential value. |
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: authorizenet Package: google-protobuf Package: snapchat-lens-studio Package: dockerode-compose Package: google-ads-scripts Package: mithril Package: meteor Package: w3c-web-usb Package: angular-es Package: fscreen Package: node/v16 Package: node-mysql-wrapper Package: watch Package: yuka Package: node/v18 Package: create-subscription Package: screeps Package: underscore Package: jest Package: node/v20 Package: node Package: deoxxa-content-type Package: insight Package: react-native-modals Package: smooch Package: pleasanter-web-script Package: protractor-beautiful-reporter Package: d3-selection/v2 Package: xrm/v8 Package: arconnect Package: chrome-apps Package: knockout Package: node-localstorage Package: webappsec-credential-management Package: d3-selection Package: nodemailer Package: webmidi Package: bootstrap-fileinput Package: rdf-ext Package: vinyl Package: react-bootstrap-typeahead Package: 11ty__eleventy-img Package: express-fileupload Package: bluebird Package: chai Package: d3-selection/v1 |
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@jakebailey Here are some more interesting changes from running the user tests suite Details
|
|
@jakebailey Here are some more interesting changes from running the user tests suite Details
|
|
@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 Something interesting changed - please have a look. Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
@jakebailey Here are some more interesting changes from running the top 400 repos suite Details
|
|
Asking the bot for the list of breaking changes this incurs... |
|
heh, at least it has been proven to be quite breaking 😅 so I guess now the question is if you'd be willing to pick this up as a new opt-in flag outside of the regular |
|
I briefly looked over the issues the bot has found and it seems to me that it found loads of code that shouldn't be there? |
|
The first link I opened led to Effect-TS/effect#3435, so that's a good sign, but wow that's a lotta damage |
|
For javascript-obfuscator, since it was interesting:
|
"just" doesn't give this credit 😅 most people don't use this (including the compiler's code ;p). This kind of break will be the most painful but it can usually be easily refactored to |
|
Yeah "just" was doing a lot of work in that sentence 😅 |
This was referenced Aug 10, 2024
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #11920
Potential downsides of this PR:
if (window.matchMedia) {}is already an error)noUncheckedIndexedAccess. This can easily be mitigated by including| undefinedin those index signatures - if the user wants to check against missing values then it makes sense to include those nullable types in the index signatures themselves. However, perhaps the biggest issue here is related to how people might want to cross-reference different-sized arraysNote that the above downsides are not arguments in favor of special casing
null. However, a more general "defensive programming" argument could still be used in its favorAdvantages of this PR:
===that is easier to understand for people. The current logic is a special case and many people don't know that it is one@RyanCavanaugh mentions:
I'm not sold, personally, on introducing a new flag to enable this behavior. Would it even be a
strictflag? or one of the other opt-ins likenoUncheckedIndexedAccess?@DanielRosenwasser mentions:
I'm also not sure if this is needed. Note that both comments are pretty old at this point and the TS landscape and user expectations changed a lot since then.
I don't mind implementing any of the above. This PR is meant as a conversation starter - I'd expect the final decision about this will require a design meeting either way and I can only address the async feedback received based on that.
I also expect user tests to reveal this to be quite breaking and at the same time, it might be hard to judge if those are good breaks or not. I could categorize them and count them by those categories to help aid the final decision though. Regardless, it will be interesting to see how breaking it turns out to be in its current form.
Open questions:
{} == null(and similar) be disallowed too? I think it's OK to allow this even if the left/right types wouldn't pass comparability + coercion check