Bind RHS of comma expressions too #47049
Merged
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
Author: Team
For Milestone Bug
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the parallelized community code test suite on this PR at 5be2594. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 5be2594. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@jakebailey Here they are:Comparison Report - main..47049
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
reviewed
Dec 7, 2021
src/compiler/binder.ts
Outdated
| @@ -1574,6 +1574,11 @@ namespace ts { | |||
| function onExit(node: BinaryExpression, state: WorkArea) { | |||
| if (!state.skip) { | |||
| const operator = node.operatorToken.kind; | |||
|
|
|||
| if (operator === SyntaxKind.CommaToken) { | |||
There was a problem hiding this comment.
Why would this be in onExit and not onRight?
There was a problem hiding this comment.
If you look up a bit, the node.left is processed in onOperator, presumably because there's some work that is completed after onLeft is completed. That's how it was added in #44487.
I tried putting it into onRight first and hit infinite recursion, then realized that the other call wasn't in onLeft and moved it down.
There was a problem hiding this comment.
Perhaps I can get away with moving these around, though, and try running them after maybeBind in onLeft and onRight instead. I'll try it. It'd sure look less weird.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I've moved things around to be a little less surprising.
weswigham
approved these changes
Jan 18, 2022
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.
Fixes #44487
This is an extension of #41312, applying
maybeBindExpressionFlowIfCallthe RHS too. The argument there was:I'd argue that this means that the entire chain of items in a comma expression (of any size) should have this applied, since they are being thought of here as a list of statements with potential side effects, and not just the leftmost expression.