Split implicit concatenated strings before binary expressions #7145

MichaReiser · 2023-09-05T09:53:36Z

Summary

This PR improves Black compatibility by breaking implicit concatenated string before any binary expression.

(
    b + c + d + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
    "bbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
    "cccccccccccccccccccccccccc" % aaaaaaaaaaaa + x
)

You can see how Black inserts line breaks between the implicit concatenated string parts but it keeps the binary expression operators and operands on the same line.

The challenge with implementing this formatting is that it is necessary that the outermost group is the group around the implicit concatenated strings but this doesn't match our AST structure: The strings are leaf nodes in the AST. See the tree for a very basic example a + 'b' 'c'

graph
	+ --> a
	+ --> A
    A["'b' 'c'"]

The existing implementation used a custom layout for when the implicit concatenated string is on the left side but it isn't able to handle implicit concatenated strings on the right or in nested binary expressions.

This PR solves this by flattening the binary expression. From the code documentation

/// Binary chain represented as a flat vector where operands are stored at even indices and operators
/// add odd indices.
///
/// ```python
/// a + 5 * 3 + 2
/// ```
///
/// Gets parsed as:
///
/// ```text
/// graph
/// +
/// ├──a
/// ├──*
/// │   ├──b
/// │   └──c
/// └──d
/// ```
///
/// The slice representation of the above is closer to what you have in source. It's a simple sequence of operands and operators,
/// entirely ignoring operator precedence (doesn't flatten parenthesized expressions):
///
/// ```text
/// -----------------------------
/// | a | + | 5 | * | 3 | + | 2 |
/// -----------------------------
/// ```
///
/// The advantage of a flat structure are:
/// * It becomes possible to adjust the operator / operand precedence. E.g splitting implicit concatenated strings before `+` operations.
/// * It allows arbitrary slicing of binary expressions for as long as a slice always starts and ends with an operand.
///
/// A slice is guaranteed to always start and end with an operand. The smallest valid slice is a slice containing a single operand.
/// Operands in multi-operand slices are separated by operators.

The flat representation allows to first split the binary expression by implicit concatenated strings and only then, operator by operator based on the operator precedence.

Test Plan

I added new tests, reviewed that the changes now match Black's formatting.

The PR improves the black compatibility

PR

project	similarity index	total files	changed files
cpython	0.76083	1789	1633
django	0.99966	2760	58
transformers	0.99928	2587	454
twine	0.99982	33	1
typeshed	0.99978	3496	2173
warehouse	0.99823	648	23
zulip	0.99948	1437	28

Base

project	similarity index	total files	changed files
cpython	0.76083	1789	1632
django	0.99957	2760	67
transformers	0.99928	2587	456
twine	0.99982	33	1
typeshed	0.99978	3496	2173
warehouse	0.99818	648	24
zulip	0.99942	1437	32

MichaReiser · 2023-09-05T09:53:47Z

Current dependencies on/for this PR:

main
- PR Split implicit concatenated strings before binary expressions #7145 👈
  - PR Formatter: Implicit concatenation in compare expressions #7193
    - PR Add FString support to binary like formatting #7215
      - PR Add PreviewMode option to formatter #7217
        
        PR Extract string part and normalized string #7219
        
        PR Introduce StringContinuation data structure #7242
        
        PR POC of Black's string preview style formatting #7567

This comment was auto-generated by Graphite.

github-actions · 2023-09-06T12:36:31Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

MichaReiser · 2023-09-06T12:22:45Z

crates/ruff_python_formatter/src/comments/format.rs

 }

-pub(crate) enum FormatTrailingComments<'a> {
-    Node(AnyNodeRef<'a>),


I removed the Node variance because this PR removes the last trailing_node_comments reference

MichaReiser · 2023-09-06T12:23:29Z

crates/ruff_python_formatter/src/expression/operator.rs

+use ruff_python_ast::Operator;
+
+#[derive(Copy, Clone)]
+pub struct FormatOperator;


I moved this out of the bin_expr formatting because it is also used in augmented assign and it removes the size of the bin_expr file a bit. The code itself is unchanged.

MichaReiser · 2023-09-06T12:24:33Z

crates/ruff_python_formatter/src/lib.rs

@@ -168,10 +168,9 @@ pub fn format_node<'a>(
 }

 /// Public function for generating a printable string of the debug comments.
-pub fn pretty_comments(formatted: &Formatted<PyFormatContext>, source: &str) -> String {
-    let comments = formatted.context().comments();
+pub fn pretty_comments(root: &Mod, comment_ranges: &CommentRanges, source: &str) -> String {


This is not strictly related to the PR but I ran into situations where I missed to format some comments but was unable to see the comments in the playground because it extracted them from the formatted output...

This change now allows to see the comments even if the formatting itself fails.

MichaReiser · 2023-09-06T12:25:17Z

crates/ruff_python_formatter/tests/snapshots/format@expression__binary_implicit_string.py.snap

    "bbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
-    "cccccccccccccccccccccccccc" % aaaaaaaaaaaa
-    + x
+    "cccccccccccccccccccccccccc" % aaaaaaaaaaaa + x
 )

 (


I'll add support for compare expressions in a separate PR

MichaReiser · 2023-09-06T12:26:12Z

crates/ruff_python_formatter/src/expression/expr_bin_op.rs

+        let chain = BinaryChain::from_binary_expression(item, &comments, f.context().source());
+
+        let source = f.context().source();
+        let mut string_operands = chain


I'll add support for f-strings in a separate PR

MichaReiser · 2023-09-06T12:27:38Z

crates/ruff_python_formatter/src/expression/expr_bin_op.rs

@@ -27,120 +29,188 @@ pub struct FormatExprBinOp;
 impl FormatNodeRule<ExprBinOp> for FormatExprBinOp {
    fn fmt_fields(&self, item: &ExprBinOp, f: &mut PyFormatter) -> FormatResult<()> {
        let comments = f.context().comments().clone();
-
-        match Self::layout(item, f.context()) {
-            BinOpLayout::LeftString(expression) => {


The LeftString was a "cheap" implementation of the new formatting logic that only supported implicit concatenated strings at the left of a binary expression. The new formatting supports implicit concatenated strings in arbitrary positions.

MichaReiser · 2023-09-06T12:28:53Z

crates/ruff_python_formatter/src/expression/expr_bin_op.rs

+        #[allow(unsafe_code)]
+        unsafe {
+            // SAFETY: `BinaryChainSlice` has the same layout as a slice because it uses `repr(transparent)`
+            &*(slice as *const [OperandOrOperator<'a>] as *const BinaryChainSlice<'a>)
+        }


Using the same layout is necessary to implement Deref on BinaryChain

crates/ruff_python_formatter/src/expression/expr_bin_op.rs

MichaReiser · 2023-09-06T14:08:48Z

I moved the formatting to its own file as preparation for re-using it for ExprCompare. It makes rebasing and reviewing the followup PR easier

codspeed-hq · 2023-09-06T14:20:19Z

CodSpeed Performance Report

Merging #7145 will degrade performances by 2.44%

_{Comparing flatten-binary-expression (668d1c4) with main (fa0b6f4)}

Summary

❌ 1 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`flatten-binary-expression`	Change
❌	`linter/all-rules[numpy/ctypeslib.py]`	32.9 ms	33.8 ms	-2.44%

MichaReiser · 2023-09-06T14:34:43Z

CodSpeed Performance Report

Merging #7145 will degrade performances by 2.44%

Comparing flatten-binary-expression (668d1c4) with main (fa0b6f4)

Summary

❌ 1 regressions ✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main flatten-binary-expression Change
❌ linter/all-rules[numpy/ctypeslib.py] 32.9 ms 33.8 ms -2.44%

I doubt that ;)

## Summary This PR implements the logic for breaking implicit concatenated strings before compare expressions by building on top of #7145 The main change is a new `BinaryLike` enum that has the `BinaryExpression` and `CompareExpression` variants. Supporting both variants requires some downstream changes but doesn't introduce any new concepts. ## Test Plan I added a few more tests. The compatibility improvements are minor but we now perfectly match black on twine 🥳 **PR** | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1632 | | django | 0.99966 | 2760 | 58 | | transformers | 0.99928 | 2587 | 454 | | **twine** | 1.00000 | 33 | 0 | <-- improved | typeshed | 0.99978 | 3496 | 2173 | | **warehouse** | 0.99824 | 648 | 22 | <-- improved | zulip | 0.99948 | 1437 | 28 | **Base** | project | similarity index | total files | changed files | |--------------|------------------:|------------------:|------------------:| | cpython | 0.76083 | 1789 | 1633 | | django | 0.99966 | 2760 | 58 | | transformers | 0.99928 | 2587 | 454 | | twine | 0.99982 | 33 | 1 | | typeshed | 0.99978 | 3496 | 2173 | | warehouse | 0.99823 | 648 | 23 | | zulip | 0.99948 | 1437 | 28 |

MichaReiser force-pushed the flatten-binary-expression branch from 7f88fc6 to 8251e70 Compare September 5, 2023 10:02

MichaReiser added the formatter Related to the formatter label Sep 5, 2023

MichaReiser force-pushed the flatten-binary-expression branch from 8251e70 to 863bbba Compare September 6, 2023 12:21

MichaReiser added this to the Formatter: Beta milestone Sep 6, 2023

MichaReiser marked this pull request as ready for review September 6, 2023 12:42

This comment was marked as outdated.

Sign in to view

MichaReiser commented Sep 6, 2023

View reviewed changes

MichaReiser requested review from charliermarsh and konstin September 6, 2023 12:44

MichaReiser force-pushed the flatten-binary-expression branch from 863bbba to a5c514d Compare September 6, 2023 12:46

charliermarsh approved these changes Sep 6, 2023

View reviewed changes

MichaReiser force-pushed the flatten-binary-expression branch from a5c514d to dc12ba4 Compare September 6, 2023 14:03

MichaReiser mentioned this pull request Sep 6, 2023

Formatter: Implicit concatenation in compare expressions #7193

Merged

MichaReiser force-pushed the flatten-binary-expression branch from 668d1c4 to 8f4ae4c Compare September 7, 2023 09:45

This was referenced Sep 7, 2023

Add FString support to binary like formatting #7215

Merged

Add PreviewMode option to formatter #7217

Merged

Extract string part and normalized string #7219

Merged

MichaReiser added 3 commits September 8, 2023 08:36

Flatten binary expressions

fc8a473

Try string layout

1fb47b4

Move binary like formatting to its own file

c2d42e9

MichaReiser force-pushed the flatten-binary-expression branch from 8f4ae4c to c2d42e9 Compare September 8, 2023 06:38

MichaReiser enabled auto-merge (squash) September 8, 2023 06:39

MichaReiser merged commit e376c3f into main Sep 8, 2023
15 checks passed

MichaReiser deleted the flatten-binary-expression branch September 8, 2023 06:51

MichaReiser mentioned this pull request Sep 8, 2023

Introduce StringContinuation data structure #7242

Closed

This was referenced Sep 17, 2023

Formatter undocumented deviation: implicit string concatenation #7449

Closed

POC of Black's string preview style formatting #7567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split implicit concatenated strings before binary expressions #7145

Split implicit concatenated strings before binary expressions #7145

MichaReiser commented Sep 5, 2023 •

edited

Loading

MichaReiser commented Sep 5, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

This comment was marked as outdated.

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser commented Sep 6, 2023

codspeed-hq bot commented Sep 6, 2023

MichaReiser commented Sep 6, 2023

CodSpeed Performance Report

Merging #7145 will degrade performances by 2.44%

Summary

Benchmarks breakdown

Split implicit concatenated strings before binary expressions #7145

Split implicit concatenated strings before binary expressions #7145

Conversation

MichaReiser commented Sep 5, 2023 • edited Loading

Summary

Test Plan

MichaReiser commented Sep 5, 2023 • edited Loading

github-actions bot commented Sep 6, 2023 • edited Loading

PR Check Results

Ecosystem

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Sep 6, 2023

codspeed-hq bot commented Sep 6, 2023

Merging #7145 will degrade performances by 2.44%

Summary

Benchmarks breakdown

MichaReiser commented Sep 6, 2023

Merging #7145 will degrade performances by 2.44%

Summary

Benchmarks breakdown

MichaReiser commented Sep 5, 2023 •

edited

Loading

MichaReiser commented Sep 5, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading