-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add optimized best_fit_parenthesize
IR
#7475
Conversation
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
4997395
to
4941b6e
Compare
CodSpeed Performance ReportMerging #7475 will improve performances by 9.15%Comparing Summary
Benchmarks breakdown
|
87951d6
to
118ebee
Compare
best_fit_parenthesize
IR
-annotated_variable: Literal[ | ||
- "fakse_literal" | ||
-] = "This is a large string that has a type annotation attached to it. A type annotation should NOT stop a long string from being wrapped." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These didn't use the best_fitting
layout before because they were excluded by the use_best_fit
. This is more consistent
} else { | ||
OptionalParentheses::Never | ||
OptionalParentheses::BestFit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now cheap enough to always use BestFit
. We only need to avoid it in cases where we never want parentheses
comments.has_leading(*expression) || comments.has_trailing_own_line(*expression); | ||
let node_comments = comments.leading_dangling_trailing(*expression); | ||
|
||
let has_comments = node_comments.has_leading() || node_comments.has_trailing_own_line(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charliermarsh for adding these methods. It makes it straightforward to replace one API with the other :)
118ebee
to
bfefd85
Compare
927f1f6
to
7419718
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, and the speedups are impressive, though I'll admit I'm not very familiar with the printer internals.
7419718
to
fd3d7a5
Compare
fd3d7a5
to
81333ac
Compare
Summary
This PR adds a custom IR for content that should only be parenthesized if it makes it fit. The custom IR removes the need to use the costly
BestFitting
IR element, allowing us to remove the poor heuristic around when to use theBestFit
layout (because the IR is now cheap).The main downside of the custom IR is that it is less tested and further increases the complexity of the Printer (It is re-implementation of
BestFitting
with the three variants)Closes #7463
Closes #7067
Closes #7462
Test Plan
cargo test
Base
This PR
New transformers deviations: There are a few new transformers deviations of the form:
This is because the
BestFit
layout is now also applied for constants.The formatting is now consistent with a 6 character identifier: