Skip to content
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.

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

Fix Printer group modes #6815

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Fix Printer group modes #6815

merged 1 commit into from
Aug 24, 2023

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 23, 2023

Summary

This fixes a bug in the Printer where the mode for a group was stale when testing if some content fits.

The input must have been of the form

group(1, 
	...
)
... (as long as no line break)
group(2, 
	if_group_breaks(2, text("()),
	...
)

What happened is:

  • The printer measured if group 1 fits
    • The printer sees no line break up to group 2 and, therefore, starts measuring group 2
    • But the printer first writes that the group 2 is printed in Expanded mode to the print_modes state that is shared between printing and measuring
    • The printer return Fits::No, because the line ultimately doesn't fit, but it doesn't undo the written group_modes[2] = Expand
  • The printer now tests if at least the group 2 fits
    • The if_group_breaks looks up the value of group 2 in group_modes which still contains the stale Expand entry (it breaks)
    • The printer, incorrectly assumes that it will have to print the (. It won't print the (, but it assumes the layout as if group 2 breaks when measuring if it fits
    • The printer decides that the group fits (when it shouldn't), and renders it in flat mode

This PR implements a fix by always writing group_modes[group_id] = Flat of the current group that should be measured. We already do this after measuring and inside of fits_groups. This only extends it to override any stale entry for the outermost group too.

This seems to come at a performance cost because there's now one additional write for every group (and there are many!). What would be nice is if there's a cheap way to restore group_modes after measuring fits but that would require that the group ids in the document are monotonic. While it's true that group ids are always increasing, it isn't guaranteed that their groups are written in the same order as the ids have been created.

I used the chance to remove the (last?) unwrap in the Printer by returning gan Err if a document uses a group_id that hasn't been seen to this point.

@MichaReiser
Copy link
Member Author

MichaReiser commented Aug 23, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2023

PR Check Results

Benchmark

Linux

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.00      4.2±0.13ms     9.7 MB/sec     1.00      4.2±0.21ms     9.7 MB/sec
formatter/numpy/ctypeslib.py               1.00   859.6±28.32µs    19.4 MB/sec     1.02   875.4±55.38µs    19.0 MB/sec
formatter/numpy/globals.py                 1.04     88.5±5.11µs    33.3 MB/sec     1.00     85.2±4.07µs    34.7 MB/sec
formatter/pydantic/types.py                1.00  1717.0±35.70µs    14.9 MB/sec     1.00  1710.8±69.81µs    14.9 MB/sec
linter/all-rules/large/dataset.py          1.02     13.0±0.41ms     3.1 MB/sec     1.00     12.7±0.56ms     3.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.09ms     5.1 MB/sec     1.04      3.4±0.11ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.00   484.6±13.25µs     6.1 MB/sec     1.01   487.5±21.99µs     6.1 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.6±0.22ms     3.8 MB/sec     1.00      6.6±0.25ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.00      6.8±0.24ms     6.0 MB/sec     1.05      7.1±0.33ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1464.0±130.36µs    11.4 MB/sec    1.01  1478.6±49.64µs    11.3 MB/sec
linter/default-rules/numpy/globals.py      1.01    182.9±8.64µs    16.1 MB/sec     1.00    181.2±8.70µs    16.3 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.06ms     8.5 MB/sec     1.07      3.2±0.11ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.8±0.21ms     8.5 MB/sec    1.01      4.8±0.23ms     8.5 MB/sec
formatter/numpy/ctypeslib.py               1.04   979.6±70.68µs    17.0 MB/sec    1.00   946.0±53.84µs    17.6 MB/sec
formatter/numpy/globals.py                 1.00     92.6±5.63µs    31.9 MB/sec    1.04    96.2±10.59µs    30.7 MB/sec
formatter/pydantic/types.py                1.00  1936.2±84.49µs    13.2 MB/sec    1.00  1934.1±100.69µs    13.2 MB/sec
linter/all-rules/large/dataset.py          1.00     15.9±0.61ms     2.6 MB/sec    1.00     15.9±0.54ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.4±0.18ms     3.8 MB/sec    1.00      4.4±0.16ms     3.8 MB/sec
linter/all-rules/numpy/globals.py          1.02   551.3±32.45µs     5.4 MB/sec    1.00   541.6±30.36µs     5.4 MB/sec
linter/all-rules/pydantic/types.py         1.00      8.2±0.29ms     3.1 MB/sec    1.01      8.3±0.38ms     3.1 MB/sec
linter/default-rules/large/dataset.py      1.04      9.2±0.66ms     4.4 MB/sec    1.00      8.8±0.39ms     4.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1883.2±74.42µs     8.8 MB/sec    1.01  1910.0±90.43µs     8.7 MB/sec
linter/default-rules/numpy/globals.py      1.00   232.6±36.87µs    12.7 MB/sec    1.01   235.3±13.48µs    12.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.9±0.14ms     6.6 MB/sec    1.05      4.1±0.23ms     6.3 MB/sec

@MichaReiser MichaReiser force-pushed the re-introduce-mode-for-best-fitting branch from c8543f8 to 04d372f Compare August 23, 2023 13:33
@MichaReiser MichaReiser marked this pull request as ready for review August 23, 2023 13:53
Base automatically changed from re-introduce-mode-for-best-fitting to main August 23, 2023 15:23
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 23, 2023
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

I'm out of my depth here as I've never grappled with the printer -- but I did spend 10 minutes or so reading the PR summary and the code.

StartTagMissing { kind: TagKind },
StartTagMissing {
kind: TagKind,
},
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@MichaReiser MichaReiser merged commit e4c1384 into main Aug 24, 2023
17 checks passed
@MichaReiser MichaReiser deleted the fix-printer-group-modes branch August 24, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants