Skip to content

Comments

Fix Printer group modes#6815

Merged
MichaReiser merged 1 commit intomainfrom
fix-printer-group-modes
Aug 24, 2023
Merged

Fix Printer group modes#6815
MichaReiser merged 1 commit intomainfrom
fix-printer-group-modes

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Aug 23, 2023 •
edited
Loading

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 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

This was referenced Aug 23, 2023
Merged
Merged
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Aug 23, 2023 •
edited
Loading

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 force-pushed the fix-printer-group-modes branch from 5051db4 to c80afd8 Compare August 23, 2023 13:33
@MichaReiser MichaReiser mentioned this pull request Aug 23, 2023
Merged
@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 force-pushed the fix-printer-group-modes branch from c80afd8 to 3fa14d5 Compare August 23, 2023 16:06
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 23, 2023
charliermarsh
charliermarsh approved these changes Aug 23, 2023
View reviewed changes
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.

crates/ruff_formatter/src/diagnostics.rs
StartTagMissing { kind: TagKind },
StartTagMissing {
kind: TagKind,
},
Copy link
Member

@charliermarsh charliermarsh Aug 23, 2023

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
@MichaReiser MichaReiser deleted the fix-printer-group-modes branch August 24, 2023 06:43
@konstin konstin mentioned this pull request Aug 24, 2023
Closed
@MichaReiser MichaReiser mentioned this pull request Aug 24, 2023
Merged
@faultyserver faultyserver mentioned this pull request Nov 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

No one assigned

Labels

formatter Related to the formatter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MichaReiser @charliermarsh