Pass parent to `NeedsParentheses` #5708

MichaReiser · 2023-07-12T13:08:39Z

Summary

This PR refactors NeedsParentheses so that implementations get access to the parent node. Having access to the parent node is e.g. required for omitting parentheses from the walrus operator when not strictly necessary.

The main challenge that I ran into is that simply storing the parent node in Parenthesize::Optional isn't possible because Parenthesize then requires lifetimes, and AsFormat (with FormatRuleWithOptions) does not support lifetimes... ugh

This forced me to rethink the problem and I'm not entirely unhappy with the outcome (There's room for improvement on the naming).

FormatExpr: Accepts a Parentheses option that can either be Always, Never, Preserve, where Preserve is the default. This covers the base case of expression formatting
maybe_parenthesize_expression is a new builder that may parenthesize an expression depending on Parenthesized and the expressions NeedsParentheses implementation.

I went along and removed Parentheses::Custom as part of this PR. This caused me some headaches with string formatting until I realized that we currently make an exception for string formatting which we do not for other nodes. The old implementation forced strings to be flat when used in an Expression statement to match Black, but we don't enforce the same for binary expressions and other expressions nodes. My take is that we should solve this holistically rather than for some nodes only.

Test Plan

Most of this is pure refactor. I reviewed the snapshot tests and they seem reasonable.

MichaReiser · 2023-07-12T13:08:54Z

Current dependencies on/for this PR:

main
- PR Handle right parens in join comma builder #5711
  - PR Pass parent to NeedsParentheses #5708 👈

This comment was auto-generated by Graphite.

MichaReiser · 2023-07-12T13:14:37Z

...es/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap

-+    )(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(
-+        User.created_at.desc()
-+    ).with_for_update(key_share=True).all()
+        filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(


Requires call chain formatting

yep i had opened #5343 to track this

MichaReiser · 2023-07-12T13:15:04Z

...s/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__expression.py.snap

-for (
-    z
-) in (NOT_YET_IMPLEMENTED_generator_key for NOT_YET_IMPLEMENTED_generator_key in []):
+for z in (NOT_YET_IMPLEMENTED_generator_key for NOT_YET_IMPLEMENTED_generator_key in []):


Requires proper generator formatting

MichaReiser · 2023-07-12T13:16:13Z

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

@@ -203,7 +203,20 @@ String \"\"\"

 "Let's" "start" "with" "a" "simple" "example"

-"Let's" "start" "with" "a" "simple" "example" "now repeat after me:" "I am confident" "I am confident" "I am confident" "I am confident" "I am confident"
+(


This per se is a regression because black keeps the string flat inside of ExprStmt. However, this is the same as that black does not try to wrap binary expressions on statement level (except if they contain a list). I decided to remove the work around for strings so that we have the same behavior for all expressions and either solve it holistically or not at all.

implicit string concatenation (vs. triple quoted strings) at statement level seems like an odd choice, do you by chance have any usage examples?

I didn't find any in the whole django project... (as expected?)

MichaReiser · 2023-07-12T13:16:37Z

crates/ruff_python_formatter/src/expression/expr_name.rs

+        _parent: AnyNodeRef,
+        _context: &PyFormatContext,
+    ) -> OptionalParentheses {
+        OptionalParentheses::Never


Names never require optional parentheses... There's nothing to split by

MichaReiser · 2023-07-12T13:17:52Z

crates/ruff_python_formatter/src/expression/expr_call.rs

-            Parentheses::Optional => Parentheses::Never,
-            parentheses => parentheses,
+    ) -> OptionalParentheses {
+        self.func.needs_parentheses(parent, context)


Never is incorrect. It requires parentheses if the func node requires parentheses (see attribute chain)

yep i should have placed a todo comment about missing fluent style support there

MichaReiser · 2023-07-12T13:58:13Z

crates/ruff_python_formatter/src/expression/expr_constant.rs

+        if self.value.is_str() {
+            let contents = context.locator().slice(self.range());
+            // Don't wrap triple quoted strings
+            if is_multiline_string(self, context.source()) || !is_implicit_concatenation(contents) {


This is somewhat slow. We should consider extracting the ranges of all multiline strings (similar to Ruff)

github-actions · 2023-07-12T14:12:22Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      8.0±0.02ms     5.1 MB/sec    1.00      7.9±0.02ms     5.2 MB/sec
formatter/numpy/ctypeslib.py               1.02   1871.4±2.93µs     8.9 MB/sec    1.00   1832.7±2.17µs     9.1 MB/sec
formatter/numpy/globals.py                 1.00    204.5±0.38µs    14.4 MB/sec    1.00    204.8±0.52µs    14.4 MB/sec
formatter/pydantic/types.py                1.01      4.0±0.01ms     6.3 MB/sec    1.00      4.0±0.01ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.00     13.3±0.02ms     3.0 MB/sec    1.01     13.4±0.03ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.4±0.01ms     4.9 MB/sec    1.00      3.4±0.02ms     4.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    431.2±0.58µs     6.8 MB/sec    1.00    431.3±0.46µs     6.8 MB/sec
linter/all-rules/pydantic/types.py         1.05      6.3±0.08ms     4.1 MB/sec    1.00      6.0±0.02ms     4.3 MB/sec
linter/default-rules/large/dataset.py      1.00      6.7±0.01ms     6.1 MB/sec    1.01      6.8±0.02ms     6.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1487.4±1.40µs    11.2 MB/sec    1.00   1482.8±1.61µs    11.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    169.2±0.86µs    17.4 MB/sec    1.00    168.9±0.22µs    17.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.1±0.01ms     8.3 MB/sec    1.00      3.1±0.01ms     8.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01     12.4±0.59ms     3.3 MB/sec    1.00     12.2±0.49ms     3.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.7±0.13ms     6.1 MB/sec    1.01      2.8±0.15ms     6.0 MB/sec
formatter/numpy/globals.py                 1.00   305.5±18.11µs     9.7 MB/sec    1.04   317.8±22.72µs     9.3 MB/sec
formatter/pydantic/types.py                1.00      6.0±0.27ms     4.3 MB/sec    1.01      6.0±0.32ms     4.2 MB/sec
linter/all-rules/large/dataset.py          1.02     20.9±0.92ms  1993.5 KB/sec    1.00     20.6±0.70ms  2024.7 KB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      5.3±0.22ms     3.2 MB/sec    1.04      5.5±0.52ms     3.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   643.5±39.84µs     4.6 MB/sec    1.00   644.1±26.30µs     4.6 MB/sec
linter/all-rules/pydantic/types.py         1.01      9.2±0.33ms     2.8 MB/sec    1.00      9.0±0.39ms     2.8 MB/sec
linter/default-rules/large/dataset.py      1.03     10.7±0.42ms     3.8 MB/sec    1.00     10.3±0.54ms     3.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02      2.2±0.10ms     7.5 MB/sec    1.00      2.2±0.09ms     7.7 MB/sec
linter/default-rules/numpy/globals.py      1.00   268.5±16.21µs    11.0 MB/sec    1.01   272.3±13.31µs    10.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      4.7±0.15ms     5.5 MB/sec    1.01      4.7±0.24ms     5.5 MB/sec

konstin · 2023-07-12T14:29:11Z

crates/ruff_python_formatter/src/expression/expr_call.rs

-            Parentheses::Optional => Parentheses::Never,
-            parentheses => parentheses,
+    ) -> OptionalParentheses {
+        self.func.needs_parentheses(parent, context)


yep i should have placed a todo comment about missing fluent style support there

konstin · 2023-07-12T14:31:40Z

...es/ruff_python_formatter/tests/snapshots/black_compatibility@simple_cases__comments4.py.snap

-+    )(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(
-+        User.created_at.desc()
-+    ).with_for_update(key_share=True).all()
+        filter(db.not_(User.is_pending.astext.cast(db.Boolean).is_(True))).order_by(


yep i had opened #5343 to track this

konstin · 2023-07-12T14:33:07Z

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

@@ -203,7 +203,20 @@ String \"\"\"

 "Let's" "start" "with" "a" "simple" "example"

-"Let's" "start" "with" "a" "simple" "example" "now repeat after me:" "I am confident" "I am confident" "I am confident" "I am confident" "I am confident"
+(


implicit string concatenation (vs. triple quoted strings) at statement level seems like an odd choice, do you by chance have any usage examples?

konstin · 2023-07-12T14:35:44Z

crates/ruff_python_formatter/src/expression/expr_named_expr.rs

+        _context: &PyFormatContext,
+    ) -> OptionalParentheses {
+        // Unlike tuples, named expression parentheses are not part of the range even when
+        // mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.


it's actually a TODO on my end

Suggested change

// mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.

// mandatory. TODO(konstin): Implement [PEP 572](https://peps.python.org/pep-0572/) rules.

MichaReiser commented Jul 12, 2023

View reviewed changes

MichaReiser changed the base branch from main to right-parens-join-comma July 12, 2023 13:42

MichaReiser force-pushed the refactor-needs-parentheses branch from ceb3a94 to a3145c1 Compare July 12, 2023 13:42

MichaReiser mentioned this pull request Jul 12, 2023

Handle right parens in join comma builder #5711

Merged

MichaReiser force-pushed the refactor-needs-parentheses branch from a3145c1 to f1b9008 Compare July 12, 2023 13:43

MichaReiser requested a review from konstin July 12, 2023 13:51

MichaReiser marked this pull request as ready for review July 12, 2023 13:51

MichaReiser force-pushed the refactor-needs-parentheses branch from f1b9008 to 393b0b2 Compare July 12, 2023 13:52

This was referenced Jul 12, 2023

Fix format named expr comments #5705

Closed

Format assert statement #5168

Merged

MichaReiser commented Jul 12, 2023

View reviewed changes

MichaReiser added the formatter Related to the formatter label Jul 12, 2023

konstin approved these changes Jul 12, 2023

View reviewed changes

MichaReiser force-pushed the refactor-needs-parentheses branch from 393b0b2 to 02d81a9 Compare July 12, 2023 16:20

Base automatically changed from right-parens-join-comma to main July 12, 2023 16:21

Pass parent to NeedsParentheses

89885ca

MichaReiser force-pushed the refactor-needs-parentheses branch from 02d81a9 to 89885ca Compare July 12, 2023 16:22

MichaReiser merged commit 067b2a6 into main Jul 13, 2023

MichaReiser deleted the refactor-needs-parentheses branch July 13, 2023 06:57

konstin pushed a commit that referenced this pull request Jul 19, 2023

Pass parent to NeedsParentheses (#5708)

a3ff014

This was referenced Jul 19, 2023

Omit tuple parentheses inside comprehensions #5790

Merged

Update TupleParentheses usage #5810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass parent to `NeedsParentheses` #5708

Pass parent to `NeedsParentheses` #5708

MichaReiser commented Jul 12, 2023 •

edited

Loading

MichaReiser commented Jul 12, 2023 •

edited

Loading

MichaReiser Jul 12, 2023

konstin Jul 12, 2023

MichaReiser Jul 12, 2023

MichaReiser Jul 12, 2023

konstin Jul 12, 2023

MichaReiser Jul 13, 2023

MichaReiser Jul 12, 2023

MichaReiser Jul 12, 2023

konstin Jul 12, 2023

MichaReiser Jul 12, 2023

github-actions bot commented Jul 12, 2023 •

edited

Loading

konstin Jul 12, 2023

konstin Jul 12, 2023

konstin Jul 12, 2023

konstin Jul 12, 2023

	// mandatory. See [PEP 572](https://peps.python.org/pep-0572/) for details.
	// mandatory. TODO(konstin): Implement [PEP 572](https://peps.python.org/pep-0572/) rules.

Pass parent to NeedsParentheses #5708

Pass parent to NeedsParentheses #5708

Conversation

MichaReiser commented Jul 12, 2023 • edited Loading

Summary

Test Plan

MichaReiser commented Jul 12, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 12, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

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

Pass parent to `NeedsParentheses` #5708

Pass parent to `NeedsParentheses` #5708

MichaReiser commented Jul 12, 2023 •

edited

Loading

MichaReiser commented Jul 12, 2023 •

edited

Loading

github-actions bot commented Jul 12, 2023 •

edited

Loading