Do not incorrectly add line separators for non-synthetic nodes when emitting node list #44070
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
|
|
devversion
force-pushed
the
fix/printer-node-list-emit-multiline
branch
3 times, most recently
from
44944a2 to
3c884e4
Compare
devversion
commented
May 13, 2021
| @@ -35,7 +35,8 @@ var y = { | |||
| "typeof": | |||
| }; | |||
| var x = (_a = { | |||
| a: a, : .b, | |||
| a: a, | |||
| : .b, | |||
There was a problem hiding this comment.
Note for reviewer: It looks like these baseline changes are due to us no longer checking if previous and next nodes (when emitting the node list) are on the same line if either one does not have a parent set. We only check if nodes are on the same line if they have the same parent container.
It looks like the logic for checking if there is a parent set has been introduced as part of the following refactoring (not sure if this was intentional; or if it's reasonable to continue doing that). 68b0323.
devversion
force-pushed
the
fix/printer-node-list-emit-multiline
branch
from
3c884e4 to
8e55aa0
Compare
typescript-bot
added
For Milestone Bug
andrewbranch
requested changes
May 13, 2021
src/compiler/emitter.ts
Outdated
Comment on lines
4567
to
4568
| else if (siblingNodePositionsAreComparable(previousNode, nextNode)) { | ||
| if (preserveSourceNewlines) { |
There was a problem hiding this comment.
To avoid incurring a performance penalty during normal program emit (i.e., via tsc), siblingNodePositionsAreComparable must not be called unless preserveSourceNewlines is true (which only happens in the language service for determining changes for codefixes and refactors).
|
@typescript-bot perf test this — I know I made this code path cheaper than it used to be, but I think we still want to avoid it during regular emit. |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 8e55aa0. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here they are:Comparison Report - master..44070
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hm, that actually seems ok. |
devversion
force-pushed
the
fix/printer-node-list-emit-multiline
branch
2 times, most recently
from
f0097f8 to
cb7bb0f
Compare
|
@andrewbranch Thanks for reviewing. I've updated the logic to never call Also I've explicitly added the fallback case for |
…mitting node list As of 3c32f6e, a line separator is added between nodes if the nodes are not synthetic and on separate lines. This it push s wrong and previously only happened if the non-synthetic nodes were on different lines but had the same parent. Fixes microsoft#44068.
devversion
force-pushed
the
fix/printer-node-list-emit-multiline
branch
from
cb7bb0f to
d192080
Compare
andrewbranch
approved these changes
May 14, 2021
devversion
added a commit
to devversion/angular
that referenced
this pull request
May 17, 2021
…t regression Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available.
devversion
added a commit
to devversion/angular
that referenced
this pull request
May 17, 2021
…t regression Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available.
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d192080. You can monitor the build here. |
|
@rbuckton can you take a quick look? @typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d192080. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here they are:Comparison Report - master..44070
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Here's what I'm thinking after chatting a bit with @andrewbranch: emit changes are hard a little bit risky. I think it's probably okay to bring this into our |
|
@DanielRosenwasser Makes sense to me. |
|
@typescript-bot cherry-pick this to release-4.3 |
|
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
|
Hey @DanielRosenwasser, I've opened #44187 for you. |
typescript-bot
pushed a commit
to typescript-bot/TypeScript
that referenced
this pull request
May 20, 2021
Component commits: d192080 Do not incorrectly add line separators for non-synthetic nodes when emitting node list As of 3c32f6e, a line separator is added between nodes if the nodes are not synthetic and on separate lines. This it push s wrong and previously only happened if the non-synthetic nodes were on different lines but had the same parent. Fixes microsoft#44068.
rbuckton
approved these changes
May 20, 2021
|
@DanielRosenwasser this is ready to go, right? From reading your comment, it sounds like we want to get this into the nightlies as soon as possible in order to decide whether to publish a 4.3 patch. |
devversion
added a commit
to devversion/angular
that referenced
this pull request
May 25, 2021
…t regression Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available.
devversion
added a commit
to devversion/angular
that referenced
this pull request
May 26, 2021
…t regression Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available.
|
@DanielRosenwasser @sandersn Is there any update on this change? It would be great to see this change in the nightly builds, given it being a change that could be considered a regression (at least in perspective of the printer as it preserves non-existent lines between list items as of TS 4.3). |
|
Let’s do it 👍 |
jessicajaniuk
pushed a commit
to angular/angular
that referenced
this pull request
Jun 4, 2021
…t regression (#42022) Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available. PR Close #42022
DanielRosenwasser
pushed a commit
that referenced
this pull request
Jun 16, 2021
Component commits: d192080 Do not incorrectly add line separators for non-synthetic nodes when emitting node list As of 3c32f6e, a line separator is added between nodes if the nodes are not synthetic and on separate lines. This it push s wrong and previously only happened if the non-synthetic nodes were on different lines but had the same parent. Fixes #44068. Co-authored-by: Paul Gschwendtner <[email protected]>
devversion
added a commit
to devversion/angular
that referenced
this pull request
Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer regression that caused unexpected JavaScript output with our compiler transforms. See: microsoft/TypeScript#44070.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer regression that caused unexpected JavaScript output with our compiler transforms. See: microsoft/TypeScript#44070.
devversion
added a commit
to devversion/angular
that referenced
this pull request
Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer regression that caused unexpected JavaScript output with our compiler transforms. See: microsoft/TypeScript#44070. Updates to TypeScript 4.3.4 which contains a fix for a printer
dylhunn
pushed a commit
to angular/angular
that referenced
this pull request
Jun 21, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer regression that caused unexpected JavaScript output with our compiler transforms. See: microsoft/TypeScript#44070. Updates to TypeScript 4.3.4 which contains a fix for a printer PR Close #42600
TeriGlover
pushed a commit
to TeriGlover/angular
that referenced
this pull request
Jul 21, 2021
…t regression (angular#42022) Updates the compiler-cli compliance goldens. The golden updates are required due to a regression in TypeScript 4.3 that causes the emitter to not incorrectly preserve lines when emitting a node list. This can be reverted once microsoft/TypeScript#44070 is available. PR Close angular#42022
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.
As of 3c32f6e, a line separator is added between nodes if the nodes are
not synthetic and on separate lines. This is wrong and previously only
happened if the non-synthetic nodes were on different lines but had the same parent.
Fixes #44068.