Skip to content

Comments

Use reserved width to include line suffix measurement#6901

Merged
MichaReiser merged 11 commits intoastral-sh:mainfrom
cnpryer:line-suffix-client
Aug 30, 2023
Merged

Use reserved width to include line suffix measurement#6901
MichaReiser merged 11 commits intoastral-sh:mainfrom
cnpryer:line-suffix-client

Conversation

@cnpryer
Copy link
Contributor

@cnpryer cnpryer commented Aug 26, 2023 •
edited
Loading

Closes #6771

Summary

This PR adds logic to measure line suffixes using the new reserved_width field added in:

In order to resolve the instability mentioned here, I've added logic to include parentheses if relevant expressions break (added in e2b6f04). See below for more details on our options.

I made the decision to use option number 2 listed further down only because it had less impact on the current fixtures, and the similarity reports were roughly the same.

Test Plan

Current fixtures and added the following to the tuple fixtures

i1 = ("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",)  # This should break

And fixtures for https://play.ruff.rs/39e4c196-e81e-4b27-848e-4a535a746def which tests #6901 (comment)

TODO:

Here are the stability check/similarity results for two potential paths we can take to address the conditional logic needed in order to resolve the ecosystem checks:

  1. Retain parentheses if they exist and a trailing comment is found.
2023-08-26T01:29:46.973674Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/typeshed: 0 stability errors, 3496 files, similarity index 0.99947), took 1.44s, 0 input files contained syntax errors 
2023-08-26T01:29:48.560901Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/zulip: 0 stability errors, 1437 files, similarity index 0.99938), took 1.59s, 0 input files contained syntax errors 
2023-08-26T01:29:53.443121Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/cpython: 0 stability errors, 1789 files, similarity index 0.76074), took 4.88s, 2 input files contained syntax errors 
2023-08-26T01:29:59.402318Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/transformers: 0 stability errors, 2587 files, similarity index 0.99913), took 5.96s, 13 input files contained syntax errors 
2023-08-26T01:30:00.243994Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/warehouse: 0 stability errors, 648 files, similarity index 0.99657), took 0.84s, 0 input files contained syntax errors 
2023-08-26T01:30:02.818237Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/django: 0 stability errors, 2760 files, similarity index 0.99926), took 2.57s, 1 input files contained syntax errors 
2023-08-26T01:30:02.862987Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/twine: 0 stability errors, 33 files, similarity index 0.99911), took 0.04s, 0 input files contained syntax errors 
2023-08-26T01:30:02.863148Z  INFO Finished: 0 stability errors, 12750 files, tool 17.326796s, 16 input files contained syntax errors 

real	0m34.871s
user	2m9.043s
sys	0m2.703s
+ cat /Users/chrispryer/github/ruff/target/progress_projects_stats.txt
| project      | similarity index |
|--------------|------------------|
| cpython      | 0.76074          |
| django       | 0.99926          |
| transformers | 0.99913          |
| twine        | 0.99911          |
| typeshed     | 0.99947          |
| warehouse    | 0.99657          |
| zulip        | 0.99938          |
Diff 
diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs
index 52255ec4f..89121e38d 100644
--- a/crates/ruff_python_formatter/src/expression/mod.rs
+++ b/crates/ruff_python_formatter/src/expression/mod.rs
@@ -183,11 +183,12 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
         } = self;
 
         let comments = f.context().comments();
-        let preserve_parentheses = parenthesize.is_optional()
-            && is_expression_parenthesized((*expression).into(), f.context().source());
+        let has_parentheses = is_expression_parenthesized((*expression).into(), f.context().source());
+        let preserve_parentheses = parenthesize.is_optional() && has_parentheses;
 
-        let has_comments =
-            comments.has_leading(*expression) || comments.has_trailing_own_line(*expression);
+        let has_comments = comments.has_leading(*expression)
+            || comments.has_trailing_own_line(*expression)
+            || (has_parentheses && comments.has_trailing(*expression));
  1. Only add parentheses if the expression will break 👈 (chosen)
2023-08-26T23:30:12.751770Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/typeshed: 0 stability errors, 3496 files, similarity index 0.99947), took 3.30s, 0 input files contained syntax errors 
2023-08-26T23:30:14.374876Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/zulip: 0 stability errors, 1437 files, similarity index 0.99931), took 1.62s, 0 input files contained syntax errors 
2023-08-26T23:30:19.611808Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/cpython: 0 stability errors, 1789 files, similarity index 0.76073), took 5.24s, 2 input files contained syntax errors 
2023-08-26T23:30:26.275173Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/transformers: 0 stability errors, 2587 files, similarity index 0.99913), took 6.66s, 13 input files contained syntax errors 
2023-08-26T23:30:27.026908Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/warehouse: 0 stability errors, 648 files, similarity index 0.99657), took 0.75s, 0 input files contained syntax errors 
2023-08-26T23:30:29.709267Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/django: 0 stability errors, 2760 files, similarity index 0.99925), took 2.68s, 1 input files contained syntax errors 
2023-08-26T23:30:29.755079Z  INFO Finished /Users/chrispryer/github/ruff/target/progress_projects/twine: 0 stability errors, 33 files, similarity index 0.99911), took 0.05s, 0 input files contained syntax errors 
2023-08-26T23:30:29.755205Z  INFO Finished: 0 stability errors, 12750 files, tool 20.303127s, 16 input files contained syntax errors 

real	0m52.809s
user	2m7.643s
sys	0m3.115s
+ cat /Users/chrispryer/github/ruff/target/progress_projects_stats.txt
| project      | similarity index |
|--------------|------------------|
| cpython      | 0.76073          |
| django       | 0.99925          |
| transformers | 0.99913          |
| twine        | 0.99911          |
| typeshed     | 0.99947          |
| warehouse    | 0.99657          |
| zulip        | 0.99931          |
Diff 
diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs
index 52255ec4f..363eff950 100644
--- a/crates/ruff_python_formatter/src/expression/mod.rs
+++ b/crates/ruff_python_formatter/src/expression/mod.rs
@@ -247,10 +247,13 @@ impl Format<PyFormatContext<'_>> for MaybeParenthesizeExpression<'_> {
                     if format_expression.inspect(f)?.will_break() {
                         // The group here is necessary because `format_expression` may contain IR elements
                         // that refer to the group id
-                        group(&format_expression)
-                            .with_group_id(Some(group_id))
-                            .should_expand(true)
-                            .fmt(f)
+                        group(&format_args![
+                            text("("),
+                            soft_block_indent(&format_expression),
+                            text(")")
+                        ])
+                        .with_group_id(Some(group_id))
+                        .fmt(f)
                     } else {
                         // Only add parentheses if it makes the expression fit on the line.
                         // Using the flat version as the most expanded version gives a left-to-right splitting behavior

@cnpryer cnpryer mentioned this pull request Aug 26, 2023
Merged
3 tasks
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 26, 2023 •
edited
Loading

CodSpeed Performance Report

Merging #6901 will not alter performance

Comparing cnpryer:line-suffix-client (64ee0d6) with main (f33277a)

Summary

✅ 16 untouched benchmarks

@cnpryer cnpryer marked this pull request as ready for review August 26, 2023 23:57
@cnpryer cnpryer force-pushed the line-suffix-client branch 2 times, most recently from 45d5cc2 to 4e3783f Compare August 27, 2023 00:08
@MichaReiser MichaReiser added the formatter Related to the formatter label Aug 27, 2023
MichaReiser
MichaReiser requested changes Aug 27, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Looks good overall but we need to ensure that the comment written by format_comment and the width measured by measure_text match.

@cnpryer cnpryer force-pushed the line-suffix-client branch from 4e3783f to 64ee0d6 Compare August 27, 2023 12:58
@cnpryer cnpryer force-pushed the line-suffix-client branch from 64ee0d6 to cfe9587 Compare August 27, 2023 15:06
@cnpryer cnpryer marked this pull request as draft August 27, 2023 18:03
@cnpryer cnpryer force-pushed the line-suffix-client branch 2 times, most recently from e500d39 to cd97445 Compare August 27, 2023 18:17
@cnpryer cnpryer force-pushed the line-suffix-client branch from cd97445 to a4d21fd Compare August 27, 2023 18:44
@cnpryer cnpryer force-pushed the line-suffix-client branch from bccc124 to 0c0a43c Compare August 27, 2023 18:46
cnpryer
cnpryer commented Aug 27, 2023
View reviewed changes
@cnpryer cnpryer marked this pull request as ready for review August 27, 2023 20:00
MichaReiser
MichaReiser requested changes Aug 28, 2023
View reviewed changes
@cnpryer cnpryer marked this pull request as draft August 28, 2023 16:46
@cnpryer cnpryer force-pushed the line-suffix-client branch 3 times, most recently from b83d72b to 15ebfaf Compare August 28, 2023 21:00
cnpryer
cnpryer commented Aug 28, 2023
View reviewed changes
cnpryer
cnpryer commented Aug 28, 2023
View reviewed changes
@cnpryer cnpryer force-pushed the line-suffix-client branch 2 times, most recently from 15ebfaf to 3c210c4 Compare August 29, 2023 02:22
cnpryer
cnpryer commented Aug 29, 2023
View reviewed changes
@cnpryer cnpryer force-pushed the line-suffix-client branch from cc74835 to 0aa3a86 Compare August 29, 2023 04:30
@cnpryer cnpryer changed the title Use reserved width to measure line suffixes Use reserved width to include line suffix measurement Aug 29, 2023
MichaReiser
MichaReiser reviewed Aug 29, 2023
View reviewed changes
MichaReiser
MichaReiser reviewed Aug 29, 2023
View reviewed changes
@cnpryer cnpryer mentioned this pull request Aug 29, 2023
Merged
@cnpryer cnpryer force-pushed the line-suffix-client branch 4 times, most recently from 304b606 to caf0aae Compare August 29, 2023 21:49
@cnpryer cnpryer marked this pull request as ready for review August 29, 2023 22:01
@cnpryer cnpryer force-pushed the line-suffix-client branch from caf0aae to ba678b1 Compare August 29, 2023 22:02
cnpryer
cnpryer commented Aug 29, 2023
View reviewed changes
cnpryer
cnpryer commented Aug 29, 2023
View reviewed changes
cnpryer
cnpryer commented Aug 29, 2023
View reviewed changes
@cnpryer cnpryer mentioned this pull request Aug 29, 2023
Closed
@cnpryer cnpryer force-pushed the line-suffix-client branch from b050c9d to 1745d3d Compare August 29, 2023 23:31
MichaReiser
MichaReiser approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice. This greatly improves our black compatibility!

This PR

project similarity index total files changed files
cpython 0.76079 1789 1635
django 0.99948 2760 85
transformers 0.99925 2587 495
twine 0.99965 33 3
typeshed 0.99947 3496 2173
warehouse 0.99659 648 41
zulip 0.99938 1437 47

Base

project similarity index total files changed files
cpython 0.76082 1789 1634
django 0.99922 2760 114
transformers 0.99855 2587 586
twine 0.99982 33 2
typeshed 0.99953 3496 2173
warehouse 0.99648 648 41
zulip 0.99928 1437 54

I expect that changing our pragma comment handling will improve the compatibility further.

@MichaReiser MichaReiser enabled auto-merge (squash) August 30, 2023 07:49
@MichaReiser MichaReiser merged commit a3f4d77 into astral-sh:main Aug 30, 2023
@cnpryer cnpryer deleted the line-suffix-client branch August 30, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaReiser MichaReiser MichaReiser 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.

Include trailing end-of-line comments in the measured line width

2 participants

@cnpryer @MichaReiser