Skip to content
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

Use reserved width to include line suffix measurement #6901

Merged
merged 11 commits into from
Aug 30, 2023

Conversation

cnpryer
Copy link
Contributor

@cnpryer cnpryer commented Aug 26, 2023

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
3 tasks
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 26, 2023

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

crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
crates/ruff_python_formatter/src/comments/format.rs Outdated Show resolved Hide resolved
@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 marked this pull request as ready for review August 27, 2023 20:00
@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 force-pushed the line-suffix-client branch 2 times, most recently from 15ebfaf to 3c210c4 Compare August 29, 2023 02:22
@cnpryer cnpryer changed the title Use reserved width to measure line suffixes Use reserved width to include line suffix measurement Aug 29, 2023
@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
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
16 checks passed
@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
Labels
formatter Related to the formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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