Uh oh!
There was an error while loading. Please reload this page.
Propagate the error any type in union and intersection construction - #58610
Merged
Wesley Wigham (weswigham) merged 3 commits intoMay 23, 2024
Merged
Conversation
Jake Bailey (jakebailey)
commented
May 21, 2024
Member
Is this at all related to #56429? |
Wesley Wigham (weswigham)
commented
May 21, 2024
MemberAuthor
It's related insofar as better |
Wesley Wigham (weswigham)
marked this pull request as ready for review
May 21, 2024 22:25
Wesley Wigham (weswigham)
requested review from
Jake Bailey (jakebailey) and Nathan Shively-Sanders (sandersn)May 21, 2024 22:25
Nathan Shively-Sanders (sandersn)
approved these changes
May 22, 2024
Nathan Shively-Sanders (sandersn)
left a comment
Member
There was a problem hiding this comment.
Looks good, some quibbles with formatting. And we should see how DT/top400 tests look.
| includes & TypeFlags.IncludesWildcard ? wildcardType : anyType : | ||
| includes & TypeFlags.IncludesWildcard ? wildcardType : | ||
| includes & TypeFlags.IncludesError ? errorType : anyType : | ||
| unknownType; |
There was a problem hiding this comment.
These ternaries are officially off the chain now.
MemberAuthor
There was a problem hiding this comment.
🤷♀️ I just write the ternaries, I let the formatter figure out how it wants to indent them.
| } | ||
| if (includes & TypeFlags.Any) { | ||
| return includes & TypeFlags.IncludesWildcard ? wildcardType : anyType; | ||
| return includes & TypeFlags.IncludesWildcard ? wildcardType : includes & TypeFlags.IncludesError ? errorType : anyType; |
There was a problem hiding this comment.
it would be nice to have multiple lines here
| /** @internal */ | ||
| Reserved1 = 1 << 29, // Used by union/intersection type construction | ||
| /** @internal */ | ||
| Reserved2 = 1 << 30, // Used by union/intersection type construction |
There was a problem hiding this comment.
remind me, do we have 31 bits available?
There was a problem hiding this comment.
In modern Node, yep, we do.
Jake Bailey (jakebailey)
commented
May 22, 2024
Member
TypeScript Bot (@typescript-bot) test it |
Contributor
TypeScript Bot (typescript-bot)
commented
May 22, 2024
Contributor
Jake Bailey (@jakebailey) Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
May 22, 2024
Contributor
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. Everything looks the same! |
TypeScript Bot (typescript-bot)
commented
May 22, 2024
Contributor
Jake Bailey (@jakebailey) Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Wesley Wigham (weswigham)
commented
May 22, 2024
MemberAuthor
TypeScript Bot (@typescript-bot) pack this |
Contributor
Contributor
Hey Wesley Wigham (@weswigham), 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 |
TypeScript Bot (typescript-bot)
commented
May 22, 2024
Contributor
Jake Bailey (@jakebailey) Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
MemberAuthor
TypeScript Bot (@typescript-bot) user test this |
Contributor
TypeScript Bot (typescript-bot)
commented
May 22, 2024
Contributor
Wesley Wigham (@weswigham) Here are the results of running the user tests comparing Something interesting changed - please have a look. Details
|
MemberAuthor
Something is funky with Jake Bailey (@jakebailey) is the runner silently failing to install/link dependencies correctly for that user test or something? That's the only thing I could think of, which is why I reran the test. And it would make sense - these kinda of errors are what I'd expect to see if some intersections in class base types contained unresolved types that now get propagated out (instead of quietly |
Jake Bailey (jakebailey)
commented
May 22, 2024
Member
The test thing infers packages and tsconfigs and attempts to install them and run on each of them, so it's possible that it's just doing things "wrong" for that repo. We can convert it to a script test or something where it more clearly tries to run things, but we'll lose the inference of configs and such. I'm not sure I have the time to try and fix the root problem, though. |
MemberAuthor
The project does have one diff in the real output - a new error - but it's a follow-on caused by what I said above - a class has an error type intersected into its' base types, so all its' base types get erased now, rather than Big downside of using |
Wesley Wigham (weswigham)
commented
May 22, 2024
MemberAuthor
Useful investigation regardless, even if there wasn't really much related to this issue, disabling |
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
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
You might not think this is the obvious fix for the issue, but it is! Type parameter constraints, when they see a non-error
any, remap it tounknown(there are historical reasons for this). So by swallowing the error-any(and replacing it with a normalany), union and intersection construction resulted in the constraint calculation logic not witnessing the error type, and thus remapping the constraint (and, thus, making the constraint from the type node not match the cached constraint on the type parameter itself!)This should also just behave a bit better with respect to follow on errors in some other cases, too.
Fixes#58598