Fixed issue with spreading a generic call expression into generic JSX and gather intra expression inference sites from spread expressions #53444
Conversation
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
typescript-bot
added
the
For Milestone Bug
Andarist
commented
Mar 22, 2023
| @@ -30130,7 +30130,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| hasComputedNumberProperty = false; | |||
| hasComputedSymbolProperty = false; | |||
| } | |||
| const type = getReducedType(checkExpression(memberDecl.expression)); | |||
| const type = getReducedType(checkExpression(memberDecl.expression, checkMode & CheckMode.Inferential)); | |||
There was a problem hiding this comment.
This forwards CheckMode.Inferential as that's a requirement for addIntraExpressionInferenceSite to be called
There was a problem hiding this comment.
Dub question, but, why not just pass check mode alone? (I wish we made the parameter required everywhere...)
There was a problem hiding this comment.
That’s a question i can’t answer. I just figured out that the original issue was caused by the fact that it was passed down and that it was inconsistent with regular calls. So I unified both paths to work the same.
that other issue with spreads and intra expression inferences was accidentally discovered - it wasn’t exactly what was reported as broken for lack of this feature in JSX. It just was an example of a workaround and when I was fixing that issue I just copy pasted all of the examples from the issue. If not that, it’s likely that we would never know about this being broken - although in context of JSX there are some people who prefer to spread object literals (which is not common with regular calls, at least not in such a simple form). @weswigham mentioned in the comment that this case looked alright to him and that we don't want to break it… so here we are :)
weswigham
approved these changes
Mar 23, 2023
There was a problem hiding this comment.
Filtering the checkMode we pass forward is a bit odd, but it wouldn't be the first place we, at least implicitly, do so (usually we implicitly replace the active checkMode with Normal). This is probably going to cause some edge cases to change in object literal contextually typed function member inference (since the Contextual checkmode isn't being forwarded anymore), but maybe not. Let's just run the tests, since I can't come up with an example that'd break offhand.
|
@typescript-bot test this |
|
Heya @weswigham, I'm starting to run the parallelized Definitely Typed test suite on this PR at 0c72dc2. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
|
Heya @weswigham, I'm starting to run the diff-based top-repos suite on this PR at 0c72dc2. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
|
Heya @weswigham, I'm starting to run the extended test suite on this PR at 0c72dc2. Hold tight - I'll update this comment with the log link once the build has been queued. |
|
Heya @weswigham, I'm starting to run the perf test suite on this PR at 0c72dc2. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here they are:
CompilerComparison Report - main..53444
System
Hosts
Scenarios
TSServerComparison Report - main..53444
System
Hosts
Scenarios
StartupComparison Report - main..53444
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham is this good for 5.1? |
|
This PR mainly unifies behavior of JSX and regular calls so I think that it should go in sooner than later. |
jakebailey
approved these changes
May 8, 2023
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.
This PR:
cc @jakebailey @weswigham