Skip to content

Fix preserveNewlines printer option when a list child has the same start or end as its parent #37846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 21, 2020
Merged

Fix preserveNewlines printer option when a list child has the same start or end as its parent #37846

merged 3 commits into from
Apr 21, 2020

Conversation

andrewbranch
Copy link
Member

Fixes #37813

andrewbranch
andrewbranch commented Apr 8, 2020
View reviewed changes
src/compiler/emitter.ts
@@ -4397,6 +4396,21 @@ namespace ts {
return lines;
}

function writeLineSeparatorsAndIndentBefore(node: Node, parent: Node): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions are extracted from emitParenthesizedExpression because fixing the actual bug revealed that a JSX example was only working by accident because of the bug, and needed to borrow this code from emitParenthesizedExpression.

@andrewbranch andrewbranch mentioned this pull request Apr 8, 2020
Closed
@andrewbranch andrewbranch mentioned this pull request Apr 21, 2020
Closed
DanielRosenwasser
DanielRosenwasser reviewed Apr 21, 2020
View reviewed changes
src/compiler/utilities.ts
const prevPos = getPreviousNonWhitespacePosition(startPos, sourceFile);
return getLinesBetweenPositions(sourceFile, prevPos || 0, startPos);
const prevPos = getPreviousNonWhitespacePosition(startPos, stopPos, sourceFile);
return getLinesBetweenPositions(sourceFile, prevPos ?? stopPos, startPos);
Copy link
Member

@DanielRosenwasser DanielRosenwasser Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, I could imagine getPreviousNonWhitespacePosition just returns the stopPos instead of undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but if someone wanted to use this elsewhere without looking at the implementation, I think returning a position that's not actually non-whitespace would be really surprising.

@DanielRosenwasser
Copy link
Member

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 21, 2020 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 6b90e6e. You can monitor the build here.

Update: The results are in!

@DanielRosenwasser
Copy link
Member

🔔 other reviewers, my familiarity with the emitter is rusty

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..37846

Metric master 37846 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 327,622k (± 0.04%) 327,081k (± 0.01%) -541k (- 0.17%) 327,000k 327,201k
Parse Time 1.63s (± 0.45%) 1.63s (± 0.52%) -0.00s (- 0.18%) 1.61s 1.64s
Bind Time 0.89s (± 0.82%) 0.89s (± 0.95%) -0.00s (- 0.34%) 0.87s 0.90s
Check Time 4.79s (± 0.61%) 4.78s (± 0.43%) -0.01s (- 0.13%) 4.74s 4.82s
Emit Time 5.38s (± 0.86%) 5.33s (± 0.68%) -0.05s (- 0.85%) 5.24s 5.44s
Total Time 12.69s (± 0.50%) 12.63s (± 0.30%) -0.06s (- 0.45%) 12.58s 12.76s
Monaco - node (v10.16.3, x64)
Memory used 327,068k (± 0.01%) 327,059k (± 0.01%) -9k (- 0.00%) 326,980k 327,151k
Parse Time 1.27s (± 0.47%) 1.27s (± 0.51%) +0.00s (+ 0.16%) 1.26s 1.29s
Bind Time 0.78s (± 0.44%) 0.77s (± 0.62%) -0.00s (- 0.51%) 0.76s 0.78s
Check Time 4.78s (± 0.52%) 4.76s (± 0.42%) -0.01s (- 0.21%) 4.73s 4.83s
Emit Time 2.94s (± 1.32%) 2.93s (± 0.75%) -0.01s (- 0.31%) 2.89s 2.98s
Total Time 9.76s (± 0.58%) 9.74s (± 0.28%) -0.02s (- 0.18%) 9.69s 9.81s
TFS - node (v10.16.3, x64)
Memory used 292,012k (± 0.01%) 292,033k (± 0.01%) +21k (+ 0.01%) 291,952k 292,167k
Parse Time 0.96s (± 0.31%) 0.96s (± 0.88%) +0.00s (+ 0.10%) 0.94s 0.98s
Bind Time 0.75s (± 0.80%) 0.74s (± 0.66%) -0.00s (- 0.27%) 0.73s 0.75s
Check Time 4.30s (± 0.33%) 4.31s (± 0.38%) +0.01s (+ 0.23%) 4.28s 4.36s
Emit Time 3.05s (± 0.95%) 3.08s (± 0.50%) +0.04s (+ 1.15%) 3.06s 3.12s
Total Time 9.06s (± 0.43%) 9.11s (± 0.30%) +0.05s (+ 0.52%) 9.03s 9.15s
material-ui - node (v10.16.3, x64)
Memory used 450,609k (± 0.01%) 450,307k (± 0.01%) -303k (- 0.07%) 450,228k 450,428k
Parse Time 1.78s (± 0.29%) 1.78s (± 0.50%) -0.00s (- 0.22%) 1.76s 1.80s
Bind Time 0.68s (± 0.54%) 0.68s (± 1.21%) 0.00s ( 0.00%) 0.67s 0.71s
Check Time 12.66s (± 0.45%) 12.61s (± 0.31%) -0.05s (- 0.41%) 12.55s 12.70s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.12s (± 0.38%) 15.07s (± 0.26%) -0.05s (- 0.35%) 15.01s 15.17s
Angular - node (v12.1.0, x64)
Memory used 303,122k (± 0.02%) 302,636k (± 0.02%) -486k (- 0.16%) 302,502k 302,777k
Parse Time 1.58s (± 0.43%) 1.57s (± 0.45%) -0.01s (- 0.44%) 1.56s 1.59s
Bind Time 0.87s (± 0.71%) 0.87s (± 0.77%) +0.00s (+ 0.46%) 0.86s 0.89s
Check Time 4.65s (± 0.36%) 4.66s (± 0.40%) +0.02s (+ 0.37%) 4.63s 4.71s
Emit Time 5.52s (± 0.35%) 5.53s (± 0.96%) +0.01s (+ 0.18%) 5.46s 5.65s
Total Time 12.61s (± 0.24%) 12.63s (± 0.47%) +0.02s (+ 0.18%) 12.54s 12.78s
Monaco - node (v12.1.0, x64)
Memory used 307,025k (± 0.02%) 307,012k (± 0.02%) -13k (- 0.00%) 306,911k 307,139k
Parse Time 1.22s (± 0.49%) 1.21s (± 0.62%) -0.00s (- 0.16%) 1.20s 1.23s
Bind Time 0.75s (± 0.91%) 0.75s (± 0.67%) -0.00s (- 0.13%) 0.74s 0.76s
Check Time 4.59s (± 0.62%) 4.59s (± 0.42%) -0.01s (- 0.13%) 4.55s 4.64s
Emit Time 2.97s (± 0.49%) 2.95s (± 0.76%) -0.02s (- 0.74%) 2.92s 3.03s
Total Time 9.53s (± 0.46%) 9.50s (± 0.33%) -0.03s (- 0.30%) 9.46s 9.61s
TFS - node (v12.1.0, x64)
Memory used 274,336k (± 0.02%) 274,364k (± 0.02%) +29k (+ 0.01%) 274,252k 274,466k
Parse Time 0.94s (± 0.83%) 0.94s (± 0.79%) +0.00s (+ 0.11%) 0.92s 0.96s
Bind Time 0.72s (± 1.26%) 0.71s (± 1.36%) -0.01s (- 1.39%) 0.69s 0.73s
Check Time 4.24s (± 0.59%) 4.24s (± 0.44%) -0.01s (- 0.21%) 4.19s 4.27s
Emit Time 3.09s (± 0.87%) 3.10s (± 0.60%) +0.01s (+ 0.26%) 3.06s 3.14s
Total Time 9.00s (± 0.59%) 8.99s (± 0.37%) -0.01s (- 0.16%) 8.91s 9.05s
material-ui - node (v12.1.0, x64)
Memory used 428,018k (± 0.01%) 427,741k (± 0.01%) -277k (- 0.06%) 427,597k 427,844k
Parse Time 1.75s (± 0.34%) 1.76s (± 0.38%) +0.00s (+ 0.11%) 1.74s 1.77s
Bind Time 0.63s (± 0.88%) 0.63s (± 0.78%) +0.01s (+ 0.96%) 0.62s 0.64s
Check Time 11.30s (± 0.55%) 11.29s (± 0.30%) -0.01s (- 0.06%) 11.23s 11.39s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 13.68s (± 0.45%) 13.68s (± 0.24%) -0.01s (- 0.04%) 13.61s 13.77s
Angular - node (v8.9.0, x64)
Memory used 322,538k (± 0.01%) 322,070k (± 0.01%) -468k (- 0.15%) 321,940k 322,149k
Parse Time 2.13s (± 0.55%) 2.11s (± 0.47%) -0.02s (- 0.80%) 2.09s 2.13s
Bind Time 0.92s (± 0.72%) 0.92s (± 0.63%) 0.00s ( 0.00%) 0.91s 0.93s
Check Time 5.31s (± 1.55%) 5.43s (± 1.44%) +0.12s (+ 2.26%) 5.19s 5.53s
Emit Time 6.44s (± 1.60%) 6.27s (± 1.27%) -0.17s (- 2.64%) 6.05s 6.46s
Total Time 14.79s (± 0.42%) 14.73s (± 0.73%) -0.07s (- 0.45%) 14.37s 14.96s
Monaco - node (v8.9.0, x64)
Memory used 325,516k (± 0.01%) 325,525k (± 0.01%) +9k (+ 0.00%) 325,423k 325,591k
Parse Time 1.55s (± 0.43%) 1.56s (± 0.44%) +0.00s (+ 0.13%) 1.54s 1.57s
Bind Time 0.90s (± 0.50%) 0.90s (± 0.55%) -0.00s (- 0.22%) 0.89s 0.91s
Check Time 5.38s (± 0.39%) 5.36s (± 0.49%) -0.02s (- 0.33%) 5.29s 5.43s
Emit Time 3.51s (± 0.71%) 3.50s (± 0.46%) -0.00s (- 0.09%) 3.46s 3.54s
Total Time 11.34s (± 0.27%) 11.31s (± 0.31%) -0.02s (- 0.21%) 11.21s 11.39s
TFS - node (v8.9.0, x64)
Memory used 291,554k (± 0.02%) 291,559k (± 0.02%) +5k (+ 0.00%) 291,413k 291,668k
Parse Time 1.26s (± 0.47%) 1.26s (± 0.47%) -0.01s (- 0.63%) 1.24s 1.27s
Bind Time 0.74s (± 0.46%) 0.75s (± 0.74%) +0.00s (+ 0.67%) 0.74s 0.76s
Check Time 4.85s (± 0.51%) 4.97s (± 1.77%) +0.11s (+ 2.35%) 4.80s 5.13s
Emit Time 3.33s (± 0.97%) 3.24s (± 2.74%) -0.10s (- 2.97%) 3.04s 3.37s
Total Time 10.19s (± 0.37%) 10.21s (± 0.50%) +0.02s (+ 0.15%) 10.10s 10.31s
material-ui - node (v8.9.0, x64)
Memory used 453,117k (± 0.01%) 452,861k (± 0.01%) -256k (- 0.06%) 452,752k 452,959k
Parse Time 2.12s (± 0.60%) 2.12s (± 0.50%) +0.00s (+ 0.24%) 2.10s 2.15s
Bind Time 0.81s (± 0.82%) 0.82s (± 0.81%) +0.01s (+ 1.23%) 0.81s 0.84s
Check Time 16.87s (± 0.70%) 16.64s (± 0.62%) -0.23s (- 1.35%) 16.49s 16.90s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 19.80s (± 0.61%) 19.59s (± 0.55%) -0.22s (- 1.09%) 19.43s 19.86s
Angular - node (v8.9.0, x86)
Memory used 185,693k (± 0.03%) 185,449k (± 0.02%) -244k (- 0.13%) 185,373k 185,550k
Parse Time 2.06s (± 0.69%) 2.06s (± 0.69%) -0.00s (- 0.10%) 2.03s 2.09s
Bind Time 1.07s (± 0.85%) 1.07s (± 0.54%) +0.00s (+ 0.19%) 1.06s 1.09s
Check Time 5.00s (± 0.43%) 5.01s (± 0.41%) +0.01s (+ 0.20%) 4.96s 5.06s
Emit Time 6.08s (± 0.61%) 6.02s (± 0.56%) -0.06s (- 0.90%) 5.92s 6.08s
Total Time 14.21s (± 0.22%) 14.16s (± 0.41%) -0.05s (- 0.36%) 13.98s 14.25s
Monaco - node (v8.9.0, x86)
Memory used 185,383k (± 0.02%) 185,369k (± 0.02%) -14k (- 0.01%) 185,264k 185,467k
Parse Time 1.60s (± 0.66%) 1.60s (± 0.73%) +0.00s (+ 0.13%) 1.58s 1.63s
Bind Time 0.76s (± 0.62%) 0.77s (± 0.94%) +0.01s (+ 1.18%) 0.76s 0.79s
Check Time 5.40s (± 0.72%) 5.42s (± 0.25%) +0.02s (+ 0.33%) 5.39s 5.46s
Emit Time 2.86s (± 0.35%) 2.87s (± 0.31%) +0.01s (+ 0.35%) 2.85s 2.88s
Total Time 10.62s (± 0.48%) 10.66s (± 0.18%) +0.04s (+ 0.38%) 10.60s 10.71s
TFS - node (v8.9.0, x86)
Memory used 166,971k (± 0.02%) 166,978k (± 0.03%) +7k (+ 0.00%) 166,916k 167,092k
Parse Time 1.30s (± 0.68%) 1.29s (± 0.60%) -0.01s (- 0.46%) 1.28s 1.31s
Bind Time 0.72s (± 1.01%) 0.71s (± 0.91%) -0.01s (- 0.84%) 0.70s 0.73s
Check Time 4.67s (± 0.39%) 4.64s (± 0.54%) -0.03s (- 0.71%) 4.59s 4.71s
Emit Time 3.05s (± 2.67%) 2.97s (± 0.99%) -0.08s (- 2.50%) 2.89s 3.03s
Total Time 9.73s (± 0.96%) 9.61s (± 0.39%) -0.12s (- 1.21%) 9.52s 9.71s
material-ui - node (v8.9.0, x86)
Memory used 256,533k (± 0.02%) 256,419k (± 0.02%) -115k (- 0.04%) 256,341k 256,547k
Parse Time 2.18s (± 0.38%) 2.18s (± 0.39%) -0.01s (- 0.23%) 2.16s 2.20s
Bind Time 0.68s (± 1.39%) 0.69s (± 1.16%) +0.00s (+ 0.44%) 0.67s 0.71s
Check Time 15.35s (± 0.67%) 15.36s (± 0.61%) +0.00s (+ 0.00%) 15.22s 15.63s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.23s (± 0.54%) 18.22s (± 0.47%) -0.00s (- 0.03%) 18.10s 18.47s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 37846 10
Baseline master 10

@andrewbranch andrewbranch merged commit 7d4fc73 into microsoft:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser approved these changes

@rbuckton rbuckton Awaiting requested review from rbuckton

@sandersn sandersn Awaiting requested review from sandersn

Assignees

@rbuckton rbuckton

Labels
Author: Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactored code that uses ASI is borked
4 participants
@andrewbranch @DanielRosenwasser @typescript-bot @rbuckton