Handle pattern parentheses in `FormatPattern` #6800

charliermarsh · 2023-08-23T01:58:17Z

Summary

This PR fixes the duplicate-parenthesis problem that's visible in the tests from #6799. The issue is that we might have parentheses around the entire match-case pattern, like in (1) here:

match foo:
    case (1):
        y = 0

In this case, the inner expression (1) will think it's parenthesized, but we'll also detect the parentheses at the case level -- so they get rendered by the case, then again by the expression. Instead, if we detect parentheses at the case level, we can force-off the parentheses for the pattern using a design similar to the way we handle parentheses on expressions.

Closes #6753.

Test Plan

cargo test

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

github-actions · 2023-08-23T02:34:58Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.7±0.05ms     8.7 MB/sec    1.00      4.7±0.04ms     8.6 MB/sec
formatter/numpy/ctypeslib.py               1.02    999.6±5.17µs    16.7 MB/sec    1.00   983.7±10.99µs    16.9 MB/sec
formatter/numpy/globals.py                 1.01     95.4±1.12µs    30.9 MB/sec    1.00     94.8±1.33µs    31.1 MB/sec
formatter/pydantic/types.py                1.01  1943.6±18.59µs    13.1 MB/sec    1.00  1915.0±32.64µs    13.3 MB/sec
linter/all-rules/large/dataset.py          1.00     11.9±0.17ms     3.4 MB/sec    1.01     12.0±0.14ms     3.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      3.2±0.02ms     5.2 MB/sec    1.00      3.2±0.04ms     5.2 MB/sec
linter/all-rules/numpy/globals.py          1.00    456.0±6.73µs     6.5 MB/sec    1.00    456.6±4.04µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.02      6.3±0.06ms     4.0 MB/sec    1.00      6.2±0.08ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      6.4±0.06ms     6.4 MB/sec    1.00      6.4±0.07ms     6.4 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1411.5±13.50µs    11.8 MB/sec    1.04  1464.2±83.98µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    165.5±2.07µs    17.8 MB/sec    1.00    165.6±2.57µs    17.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.9±0.02ms     8.8 MB/sec    1.00      2.9±0.03ms     8.8 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.03      6.4±0.29ms     6.4 MB/sec    1.00      6.2±0.21ms     6.6 MB/sec
formatter/numpy/ctypeslib.py               1.02  1283.4±57.25µs    13.0 MB/sec    1.00  1257.8±48.67µs    13.2 MB/sec
formatter/numpy/globals.py                 1.00    115.2±5.20µs    25.6 MB/sec    1.01    116.5±8.42µs    25.3 MB/sec
formatter/pydantic/types.py                1.01      2.5±0.11ms    10.1 MB/sec    1.00      2.5±0.09ms    10.1 MB/sec
linter/all-rules/large/dataset.py          1.00     17.9±0.76ms     2.3 MB/sec    1.03     18.5±0.61ms     2.2 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.8±0.16ms     3.4 MB/sec    1.02      4.9±0.18ms     3.4 MB/sec
linter/all-rules/numpy/globals.py          1.03   615.4±44.17µs     4.8 MB/sec    1.00   599.2±22.48µs     4.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      9.2±0.38ms     2.8 MB/sec    1.01      9.2±0.36ms     2.8 MB/sec
linter/default-rules/large/dataset.py      1.00      9.9±0.27ms     4.1 MB/sec    1.00      9.9±0.31ms     4.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01      2.1±0.10ms     8.0 MB/sec    1.00      2.1±0.07ms     8.1 MB/sec
linter/default-rules/numpy/globals.py      1.03   266.6±22.51µs    11.1 MB/sec    1.00   257.9±10.99µs    11.4 MB/sec
linter/default-rules/pydantic/types.py     1.01      4.5±0.15ms     5.7 MB/sec    1.00      4.4±0.10ms     5.8 MB/sec

dhruvmanila · 2023-08-23T04:48:01Z

...python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_simple.py.snap

-+    case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf | (y)]:
+    case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf | (y))]:


Regarding the addition of parentheses around unions, I believe that will be handled in MatchOr?

dhruvmanila · 2023-08-23T04:49:30Z

...python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap

-    case (
-        4 as d,
-        5 as e,
-        NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g,
-        *NOT_YET_IMPLEMENTED_PatternMatchStar,
-    ):
+    case 4 as d, (5 as e), (
+        NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g
+    ), *NOT_YET_IMPLEMENTED_PatternMatchStar:


Unrelated but shouldn't we consider the magic trailing comma and keep each pattern on a separate line?

We probably should

dhruvmanila · 2023-08-23T04:53:17Z

crates/ruff_python_formatter/src/other/match_case.rs

+                            parenthesized(
+                                "(",
+                                &pattern.format().with_options(Parentheses::Never),
+                                ")",
+                            )


Do we actually need to preserve the parentheses? Black does but in other nodes we don't:

while (True): pass

Gets formatted to:

while True: pass

Our rule for expressions are:

Top level expression (inside a statement): Remove unnecessary parentheses, see your while (True) example

nested expressions: Preserve the parentheses (except for await where Black removes parentheses).

IMO the behavior should be the same for Patterns. Remove unnecessary parentheses around the outermost pattern but preserve them for nested patterns. See #6753

Meaning, we should use parenthesize_if_expands here instead of preserving the parentheses.

I'll try out parenthesize_if_expands.

I think this also requires knowing whether the pattern has its own parentheses (e.g., for sequence patterns).

MichaReiser

Does this address #6753 ? If not, what's missing? Could we implement the required changes to match the expected output

MichaReiser · 2023-08-23T06:38:39Z

crates/ruff_python_formatter/src/other/match_case.rs

+                            parenthesized(
+                                "(",
+                                &pattern.format().with_options(Parentheses::Never),
+                                ")",
+                            )


Our rule for expressions are:

Top level expression (inside a statement): Remove unnecessary parentheses, see your while (True) example

nested expressions: Preserve the parentheses (except for await where Black removes parentheses).

IMO the behavior should be the same for Patterns. Remove unnecessary parentheses around the outermost pattern but preserve them for nested patterns. See #6753

Meaning, we should use parenthesize_if_expands here instead of preserving the parentheses.

crates/ruff_python_formatter/src/pattern/mod.rs

MichaReiser · 2023-08-23T06:42:36Z

...python_formatter/tests/snapshots/black_compatibility@py_310__pattern_matching_extras.py.snap

-    case (
-        4 as d,
-        5 as e,
-        NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g,
-        *NOT_YET_IMPLEMENTED_PatternMatchStar,
-    ):
+    case 4 as d, (5 as e), (
+        NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g
+    ), *NOT_YET_IMPLEMENTED_PatternMatchStar:


We probably should

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

MichaReiser · 2023-08-23T06:45:36Z

crates/ruff_python_formatter/src/other/match_case.rs

@@ -39,9 +39,13 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
                        write!(f, [text("case"), space()])?;

                        if is_match_case_pattern_parenthesized(item, pattern, f.context())? {


Can you explain why calling pattern.format isn't sufficient? Why can the pattern and the MatchCase both have trailing opening parentheses comments.

Consider:

match thing: case ( # outer [ # inner 1 ] )

The # outer is attached to the MatchCase, but the # inner is part of the pattern itself. (Not all patterns have this behavior.)

Hmm, so it depends on whether the pattern has its own parentheses? How does this work if you have nested pattern, which doesn't go through the match case formatting?

match thing: case [ ( # outer [ # inner 1, ] ) ]: ...

Both of these cases work as expected as of the following branch (#6801):

match thing: case [ ( # outer [ # inner 1, ] ) ]: ... case [ # outer ( # inner outer [ # inner 1, ] ) ]: ...

# inner outer gets assigned as a leading end-of-line comment via our standard parenthesized comment handling, which then gets rendered as an open parenthesis comment.

Actually, let me confirm that that's why it works, hang on.

Err, I think it works because it's a leading end-of-line comment which we always treat as an open parenthesis comment. (And the comments on the inner square brackets work because they're correctly marked as dangling for the sequence pattern type.)

I guess we kind of get this wrong:

case [ # outer # own line ( # inner outer [ # inner 1, ] ) ]: pass

It renders as:

case [ # outer ( # own line # inner outer [ # inner 1, ] ) ]:

We might be able to remove this, I'm not sure yet, I'll explore it.

(I'd thought we wanted to preserve the parentheses here always which complicated this a bit.)

Ok, I was able to remove the dangling comment from match_case.

charliermarsh · 2023-08-23T15:43:55Z

Does this address #6753 ? If not, what's missing?

Yes I believe it should (sorry, I missed that issue, I stumbled upon this independently). Added a test + linked the issue.

charliermarsh · 2023-08-23T15:44:07Z

I still need to figure out the parenthesize_if_expands, working on that now.

charliermarsh · 2023-08-23T16:41:59Z

Okay @MichaReiser I think this is ready for review based on your feedback.

charliermarsh · 2023-08-23T16:42:47Z

crates/ruff_python_formatter/src/other/match_case.rs

+                                OptionalParentheses::Never => {
+                                    pattern.format().with_options(Parentheses::Never).fmt(f)?;
+                                }
+                            }


This is relatively similar to maybe_parenthesize_expression...

MichaReiser

Nice, I love it that we can reuse the NeedsParentheses concept

charliermarsh force-pushed the charlie/paren-value branch from 2c3727a to b5b42ad Compare August 23, 2023 01:58

charliermarsh force-pushed the charlie/value branch from 62857fc to c9e664d Compare August 23, 2023 01:59

charliermarsh force-pushed the charlie/paren-value branch from b5b42ad to f5fab0d Compare August 23, 2023 01:59

charliermarsh commented Aug 23, 2023

View reviewed changes

crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap Outdated Show resolved Hide resolved

charliermarsh added the formatter Related to the formatter label Aug 23, 2023

charliermarsh requested a review from MichaReiser August 23, 2023 02:02

charliermarsh force-pushed the charlie/paren-value branch from f5fab0d to f221e7a Compare August 23, 2023 02:04

charliermarsh commented Aug 23, 2023

View reviewed changes

crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap Show resolved Hide resolved

charliermarsh force-pushed the charlie/value branch from c9e664d to 382e30f Compare August 23, 2023 04:49

charliermarsh force-pushed the charlie/paren-value branch from f221e7a to f457285 Compare August 23, 2023 04:51

dhruvmanila reviewed Aug 23, 2023

View reviewed changes

MichaReiser reviewed Aug 23, 2023

View reviewed changes

charliermarsh force-pushed the charlie/value branch from 382e30f to fb314e6 Compare August 23, 2023 13:35

Base automatically changed from charlie/value to main August 23, 2023 14:01

charliermarsh force-pushed the charlie/paren-value branch 3 times, most recently from 5d9d616 to 161e3c5 Compare August 23, 2023 15:37

charliermarsh force-pushed the charlie/paren-value branch from 161e3c5 to e3ce944 Compare August 23, 2023 16:13

charliermarsh requested a review from MichaReiser August 23, 2023 16:40

charliermarsh commented Aug 23, 2023

View reviewed changes

MichaReiser approved these changes Aug 24, 2023

View reviewed changes

konstin approved these changes Aug 24, 2023

View reviewed changes

charliermarsh added 2 commits August 24, 2023 23:36

Handle pattern parentheses in FormatPattern

1fc6145

Break

728e378

charliermarsh force-pushed the charlie/paren-value branch from 635c173 to 728e378 Compare August 25, 2023 03:38

charliermarsh enabled auto-merge (squash) August 25, 2023 03:39

charliermarsh merged commit 6f23469 into main Aug 25, 2023
2 checks passed

charliermarsh deleted the charlie/paren-value branch August 25, 2023 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle pattern parentheses in `FormatPattern` #6800

Handle pattern parentheses in `FormatPattern` #6800

charliermarsh commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

dhruvmanila Aug 23, 2023

dhruvmanila Aug 23, 2023

MichaReiser Aug 23, 2023

dhruvmanila Aug 23, 2023

MichaReiser Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

MichaReiser left a comment

MichaReiser Aug 23, 2023

MichaReiser Aug 23, 2023

MichaReiser Aug 23, 2023

charliermarsh Aug 23, 2023

MichaReiser Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh commented Aug 23, 2023

charliermarsh commented Aug 23, 2023

charliermarsh commented Aug 23, 2023

charliermarsh Aug 23, 2023

MichaReiser left a comment

		+ case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf \| (y)]:
		+ case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf \| (y))]:

		@@ -39,9 +39,13 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
		write!(f, [text("case"), space()])?;

		if is_match_case_pattern_parenthesized(item, pattern, f.context())? {

Handle pattern parentheses in FormatPattern #6800

Handle pattern parentheses in FormatPattern #6800

Conversation

charliermarsh commented Aug 23, 2023 • edited Loading

Summary

Test Plan

github-actions bot commented Aug 23, 2023 • edited Loading

PR Check Results

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a 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

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

charliermarsh commented Aug 23, 2023

charliermarsh commented Aug 23, 2023

charliermarsh commented Aug 23, 2023

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

Handle pattern parentheses in `FormatPattern` #6800

Handle pattern parentheses in `FormatPattern` #6800

charliermarsh commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading