Maybe parenthesize long constants and names#6816
Conversation
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
MichaReiser
force-pushed
the
fix-printer-group-modes
branch
from
5051db4 to
c80afd8
Compare
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
aa7e59b to
9ab2cb5
Compare
| let mut formatted_variants = Vec::with_capacity(variants.len()); | ||
|
|
||
| for variant in variants { | ||
| let mut buffer = VecBuffer::with_capacity(8, f.state_mut()); |
There was a problem hiding this comment.
Let's always initialize the vector with a non-zero capacity because it always writes at least 3 entries, and lets double it to avoid resizing.
| let mut enclosing = Vec::with_capacity(if self.is_empty() { | ||
| 0 | ||
| } else { | ||
| self.len().ilog2() as usize |
There was a problem hiding this comment.
This helped to improve performance quiet a bit. I haven't done any science but our groups very likely form a tree, the depth of a balanced binary tree is probably not the worst approximation.
...uff_python_formatter/tests/snapshots/black_compatibility@simple_cases__remove_parens.py.snap
Show resolved
Hide resolved
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
9ab2cb5 to
1c1420c
Compare
| ) | ||
|
|
||
| @@ -221,8 +221,8 @@ | ||
| @@ -221,8 +217,8 @@ |
There was a problem hiding this comment.
The assert above will be tricky. Because both the left and the right shouldn't have parentheses by default if it doesn't make them fit. Which is given, because the string is simply way too long, making it never fit. But we should then still break the expression.. hum. A Problem for another day
MichaReiser
force-pushed
the
fix-printer-group-modes
branch
from
c80afd8 to
3fa14d5
Compare
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
1c1420c to
701c645
Compare
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
701c645 to
d50042b
Compare
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
3 times, most recently
from
3f95414 to
56f175a
Compare
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
56f175a to
697d820
Compare
| ) -> OptionalParentheses { | ||
| OptionalParentheses::Never | ||
| let text_len = context.source()[self.range].len(); | ||
| if text_len > 4 && text_len < context.options().line_width().value() as usize { |
There was a problem hiding this comment.
This is a rather arbitrary number and a heuristic to avoid the cost of using best fitting for most literals and common names like self. It can result in the formatted text exceeding the line width even if parenthesizing would make it fit but IMO, I probably rather have a 4-character name exceed the line width by 2 than add parentheses and expand it vertically.
How did I come up with the number. I exclude the constants True, False and None and... I assumed that they all have a length of 4. So you see, I'm not good at math nor counting 🤣
I'll factor this method out and add a comment of why 4... uhm 5.
|
|
||
| if memchr2(b'\n', b'\r', text.as_bytes()).is_none() | ||
| && text.len() > 4 | ||
| && text.len() < context.options().line_width().value() as usize |
There was a problem hiding this comment.
This is always going to indented if it's parenthesized, right? So could we subtract the indent size from context.options().line_width().value()?
There was a problem hiding this comment.
We could, but we don't have an indent size option yet (only available in the printer). Let me add it in a follow up PR.
| .memoized(); | ||
|
|
||
| // Don't use best fitting if it is known that the expression can never fit | ||
| if format_expression.inspect(f)?.will_break() { |
There was a problem hiding this comment.
Ah okay, so this looks for newlines in the IR, interesting. How does will_break deal with things that might break conditionally?
There was a problem hiding this comment.
It looks for things that make group expand for certain: Like hard line breaks or expand_parent. The logic ignores any conditions (it returns true if content in if_group_breaks or if_group_fits contains a hard line break). It showed that conditions can mostly be ignored, especially because it is uncommon or even unneeded to have hard line breaks or expand parent inside of conditional content.
MichaReiser
force-pushed
the
maybe-parenthesize-constants-and-names
branch
from
697d820 to
33a6c16
Compare
3 participants
Summary
This PR implements #6271 for string, fstring, and number literals as well as for names. The stacked PR implements the behavior for non-fluent call expressions (Doing the changes separately helps to understand the compatibility and performance implications).
This PR is about parenthesizing unsplittable expressions if it makes them fit on a line, but the formatter should omit the parentheses if they still don't.
Test Plan
I added a few new test cases.
The similarity index improves for all projects except typeshed.
Main
This PR
Performance
The performance implication of this should be limited because constants and single names are rarely used in clause headers (
if "string":). However, constant are used frequently in assignments where this assignment applies as well and usingBestFittinghas a considerable cost:BestFittingallocates aVecfor each variant + one vec for itselfmemoized, which allocates toopropagate_expandsrequires special handling for interned (used by memoized) to avoid visiting the document multiple times.