Parenthesize unsplittable nodes if it makes them fit #6734

MichaReiser · 2023-08-21T15:22:48Z

Summary

This PR implements black's logic where it parenthesizes unsplittable nodes like constants or identifiers
if it makes them fit the line

closes #6271

Test Plan

I added new test plans, diffed the output for django to ensure this PR introduces no regression, and verified that the similarity indices improve

main

project	similarity index
cpython	0.75473
django	0.99805
transformers	0.99620
twine	0.99876
typeshed	0.99953
warehouse	0.99601
zulip	0.99727

This PR

project	similarity index
cpython	0.75475
django	0.99880
transformers	0.99629
twine	0.99929
typeshed	0.99953
warehouse	0.99613
zulip	0.99852

This reverts commit 9a8ba58.

MichaReiser · 2023-08-21T15:22:59Z

Current dependencies on/for this PR:

main
- PR Parenthesize unsplittable nodes if it makes them fit #6734 👈

This comment was auto-generated by Graphite.

MichaReiser · 2023-08-21T15:26:20Z

crates/ruff_python_formatter/src/expression/mod.rs

@@ -223,6 +223,35 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
                    }
                }
            },
+            OptionalParentheses::IfFits => match parenthesize {


I'm not very happy with this name. The branches are the same as for Never except for IfBreaks.

BetterFitOnly?

MichaReiser · 2023-08-21T15:27:37Z

crates/ruff_formatter/src/builders.rs

The formatter changes bring back the BestFitting::print_mode

MichaReiser · 2023-08-21T15:28:53Z

crates/ruff_formatter/src/printer/mod.rs

@@ -311,6 +314,14 @@ impl<'a> Printer<'a> {
                    _ => {
                        self.state.measured_group_fits = true;

+                        // Set the print mode for this group in case this group was measured as part of a
+                        // previous `fits_group` call.
+                        if let Some(id) = id {


This was a bug in the Printer where group modes set during a previous fits test stuck around.

github-actions · 2023-08-21T16:35:37Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.3±0.01ms    12.3 MB/sec    1.01      3.3±0.01ms    12.2 MB/sec
formatter/numpy/ctypeslib.py               1.00    672.9±9.44µs    24.7 MB/sec    1.01   682.9±10.25µs    24.4 MB/sec
formatter/numpy/globals.py                 1.00     70.9±0.64µs    41.6 MB/sec    1.01     71.4±0.46µs    41.3 MB/sec
formatter/pydantic/types.py                1.00  1339.1±13.73µs    19.0 MB/sec    1.03  1385.0±18.36µs    18.4 MB/sec
linter/all-rules/large/dataset.py          1.00     10.4±0.03ms     3.9 MB/sec    1.00     10.4±0.03ms     3.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      2.9±0.00ms     5.8 MB/sec    1.00      2.8±0.01ms     5.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    319.8±1.09µs     9.2 MB/sec    1.02    325.5±1.31µs     9.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.4±0.01ms     4.7 MB/sec    1.00      5.4±0.02ms     4.7 MB/sec
linter/default-rules/large/dataset.py      1.00      5.6±0.01ms     7.3 MB/sec    1.00      5.6±0.01ms     7.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01   1188.7±2.86µs    14.0 MB/sec    1.00   1179.5±3.44µs    14.1 MB/sec
linter/default-rules/numpy/globals.py      1.00    123.3±0.48µs    23.9 MB/sec    1.01    125.2±1.79µs    23.6 MB/sec
linter/default-rules/pydantic/types.py     1.01      2.5±0.03ms    10.1 MB/sec    1.00      2.5±0.01ms    10.1 MB/sec

Windows

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.00      4.2±0.27ms     9.7 MB/sec     1.14      4.7±0.30ms     8.6 MB/sec
formatter/numpy/ctypeslib.py               1.00   859.5±44.50µs    19.4 MB/sec     1.14   980.8±74.39µs    17.0 MB/sec
formatter/numpy/globals.py                 1.00     91.6±9.00µs    32.2 MB/sec     1.04     95.2±6.78µs    31.0 MB/sec
formatter/pydantic/types.py                1.00  1810.5±124.71µs    14.1 MB/sec    1.08  1954.3±152.00µs    13.0 MB/sec
linter/all-rules/large/dataset.py          1.03     17.3±0.96ms     2.4 MB/sec     1.00     16.8±0.57ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.5±0.22ms     3.7 MB/sec     1.00      4.5±0.22ms     3.7 MB/sec
linter/all-rules/numpy/globals.py          1.04   606.3±44.76µs     4.9 MB/sec     1.00   584.6±72.40µs     5.0 MB/sec
linter/all-rules/pydantic/types.py         1.01      8.7±0.44ms     2.9 MB/sec     1.00      8.6±0.31ms     3.0 MB/sec
linter/default-rules/large/dataset.py      1.00      9.0±0.41ms     4.5 MB/sec     1.09      9.8±0.74ms     4.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1884.6±109.84µs     8.8 MB/sec    1.07      2.0±0.11ms     8.2 MB/sec
linter/default-rules/numpy/globals.py      1.00   224.4±14.22µs    13.1 MB/sec     1.10   246.2±12.56µs    12.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.9±0.15ms     6.5 MB/sec     1.06      4.1±0.19ms     6.2 MB/sec

MichaReiser · 2023-08-21T17:52:16Z

Ouch, this is, not too surprisingly, not good for performance.

charliermarsh · 2023-08-21T18:01:29Z

Is that mostly due to the best_fitting! or is there something else going on?

MichaReiser · 2023-08-21T18:27:43Z

It's a combination of memoized and best fitting, both of them allocate, and best fitting requires a fits pass for every variant except the last.

The solution is to limit the condition of when to use this layout. It should only be necessary if the content length is close to the max line length (I have to do some math tomorrow)

MichaReiser · 2023-08-23T12:10:57Z

Splitting the PR must have confused graphite. See #6816

MichaReiser added 2 commits August 21, 2023 10:46

Parenthesize NamedExpr if target breaks

867b5eb

Revert "Remove mode from BestFitting"

7b29029

This reverts commit 9a8ba58.

MichaReiser changed the title ~~Revert "Remove mode from BestFitting"~~ Parenthesize unsplittable nodes if it makes them fit Aug 21, 2023

MichaReiser added the formatter Related to the formatter label Aug 21, 2023

MichaReiser commented Aug 21, 2023

View reviewed changes

crates/ruff_formatter/src/builders.rs Outdated

Copy link

Member Author

MichaReiser Aug 21, 2023

Choose a reason for hiding this comment

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

The formatter changes bring back the BestFitting::print_mode

MichaReiser commented Aug 21, 2023

View reviewed changes

MichaReiser force-pushed the parenthesize-unsplittable branch from 6524c22 to 93f9521 Compare August 21, 2023 15:49

MichaReiser force-pushed the parenthesize-unsplittable branch 2 times, most recently from 3668767 to 33c1161 Compare August 22, 2023 07:02

Add OptionalParentheses::MultilineIfFits

431973a

MichaReiser force-pushed the parenthesize-unsplittable branch from 33c1161 to 431973a Compare August 22, 2023 07:09

MichaReiser mentioned this pull request Aug 22, 2023

Preserve yield parentheses #6766

Merged

MichaReiser closed this Aug 23, 2023

MichaReiser deleted the parenthesize-unsplittable branch January 16, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parenthesize unsplittable nodes if it makes them fit #6734

Parenthesize unsplittable nodes if it makes them fit #6734

MichaReiser commented Aug 21, 2023 •

edited

Loading

MichaReiser commented Aug 21, 2023

MichaReiser Aug 21, 2023

konstin Aug 21, 2023

MichaReiser Aug 21, 2023

MichaReiser Aug 21, 2023

github-actions bot commented Aug 21, 2023 •

edited

Loading

MichaReiser commented Aug 21, 2023

charliermarsh commented Aug 21, 2023

MichaReiser commented Aug 21, 2023 •

edited

Loading

MichaReiser commented Aug 23, 2023

Parenthesize unsplittable nodes if it makes them fit #6734

Parenthesize unsplittable nodes if it makes them fit #6734

Conversation

MichaReiser commented Aug 21, 2023 • edited Loading

Summary

Test Plan

MichaReiser commented Aug 21, 2023

MichaReiser Aug 21, 2023

Choose a reason for hiding this comment

konstin Aug 21, 2023

Choose a reason for hiding this comment

MichaReiser Aug 21, 2023

Choose a reason for hiding this comment

MichaReiser Aug 21, 2023

Choose a reason for hiding this comment

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

PR Check Results

Benchmark

Linux

Windows

MichaReiser commented Aug 21, 2023

charliermarsh commented Aug 21, 2023

MichaReiser commented Aug 21, 2023 • edited Loading

MichaReiser commented Aug 23, 2023

MichaReiser commented Aug 21, 2023 •

edited

Loading

github-actions bot commented Aug 21, 2023 •

edited

Loading

MichaReiser commented Aug 21, 2023 •

edited

Loading