Format `PatternMatchStar` #6653

harupy · 2023-08-17T15:30:14Z

Summary

Test Plan

harupy · 2023-08-17T15:34:57Z

As a test, I formatted this code:

x = [1, 2, 3, 4]

match x:
    case [
        1,
        *
        rest,
    ]:
        print(rest)
    case [
        1,
        2,
        *
        # hello
        rest,
    ]:
        print(rest)
    case [*_]:
        print("no match")

and got:

x = [1, 2, 3, 4]

match x:
    case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
        print(rest)
    case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
        print(rest)
    case [NOT_YET_IMPLEMENTED_PatternMatchSequence, 2]:
        print("no match")

How can I test this?

github-actions · 2023-08-17T16:03:10Z

PR Check Results

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      3.6±0.04ms    11.3 MB/sec    1.01      3.7±0.03ms    11.1 MB/sec
formatter/numpy/ctypeslib.py               1.00    732.8±2.47µs    22.7 MB/sec    1.03    755.2±2.78µs    22.0 MB/sec
formatter/numpy/globals.py                 1.00     74.6±0.49µs    39.6 MB/sec    1.08     80.2±0.34µs    36.8 MB/sec
formatter/pydantic/types.py                1.00   1452.6±6.26µs    17.6 MB/sec    1.03  1501.2±11.63µs    17.0 MB/sec
linter/all-rules/large/dataset.py          1.00     10.2±0.05ms     4.0 MB/sec    1.00     10.2±0.04ms     4.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.7±0.00ms     6.1 MB/sec    1.00      2.7±0.01ms     6.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    386.5±0.71µs     7.6 MB/sec    1.00    386.0±0.54µs     7.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.3±0.03ms     4.8 MB/sec    1.00      5.3±0.02ms     4.8 MB/sec
linter/default-rules/large/dataset.py      1.00      5.4±0.02ms     7.5 MB/sec    1.00      5.4±0.02ms     7.5 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1202.9±18.95µs    13.8 MB/sec    1.00   1207.8±1.23µs    13.8 MB/sec
linter/default-rules/numpy/globals.py      1.00    140.8±1.14µs    21.0 MB/sec    1.00    140.2±1.03µs    21.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.4 MB/sec    1.01      2.5±0.01ms    10.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      5.0±0.13ms     8.1 MB/sec    1.03      5.1±0.26ms     7.9 MB/sec
formatter/numpy/ctypeslib.py               1.00  1011.0±60.02µs    16.5 MB/sec    1.00  1011.4±46.37µs    16.5 MB/sec
formatter/numpy/globals.py                 1.00    102.3±7.89µs    28.9 MB/sec    1.03    105.3±6.89µs    28.0 MB/sec
formatter/pydantic/types.py                1.00      2.0±0.14ms    12.5 MB/sec    1.01      2.1±0.08ms    12.4 MB/sec
linter/all-rules/large/dataset.py          1.00     15.4±0.31ms     2.6 MB/sec    1.01     15.6±0.34ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.3±0.33ms     3.9 MB/sec    1.00      4.2±0.12ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.00   522.6±20.53µs     5.6 MB/sec    1.02   531.2±22.75µs     5.6 MB/sec
linter/all-rules/pydantic/types.py         1.00      8.0±0.20ms     3.2 MB/sec    1.03      8.3±0.27ms     3.1 MB/sec
linter/default-rules/large/dataset.py      1.00      8.6±0.21ms     4.7 MB/sec    1.02      8.8±0.22ms     4.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1828.4±50.56µs     9.1 MB/sec    1.02  1866.5±52.73µs     8.9 MB/sec
linter/default-rules/numpy/globals.py      1.00   213.1±12.28µs    13.8 MB/sec    1.04   221.7±18.09µs    13.3 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.9±0.13ms     6.6 MB/sec    1.03      4.0±0.31ms     6.4 MB/sec

harupy · 2023-08-17T16:23:33Z

Tweaked the PatternMatchSequence (should #6645 be resolved first?) implementation and got this:

x = [1, 2, 3, 4]

match x:
    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *rest,
    ]:
        print("a")
        print(rest)
    case [
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        "NOT_YET_IMPLEMENTED_PatternMatchValue",
        *rest# hello
        ,
    ]:
        print(rest)
    case [*_]:
        print("no match")

The comment location seems incorrect.

harupy · 2023-08-17T17:04:51Z

crates/ruff_python_formatter/src/pattern/pattern_match_star.rs

+        let comments = f.context().comments().clone();
+        let dangling = comments.dangling_comments(item);


Should comments be handled in PatternMatchSequence?

It depends on the comments. What's the code snipped that requires the dangling comment handling?

@MichaReiser

For example:

x = [1, 2, 3, 4] match x: case [ 1, * # comment here rest, ]: print(rest)

MichaReiser · 2023-08-18T07:45:45Z

Hmm yeah, it may be challenging to test this just now without match as, singleton formatting, or sequence formatting. There are PRs for as and singleton but it may be good to do sequence first.

harupy · 2023-08-18T11:41:54Z

Hmm yeah, it may be challenging to test this just now without match as, singleton formatting, or sequence formatting. There are PRs for as and singleton but it may be good to do sequence first.

Can I tackle #6645 first?

MichaReiser · 2023-08-18T11:59:48Z

Hmm yeah, it may be challenging to test this just now without match as, singleton formatting, or sequence formatting. There are PRs for as and singleton but it may be good to do sequence first.

Can I tackle #6645 first?

Sure. You'll have to comment on the issue so that I can assign it to you.

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py

harupy · 2023-08-23T10:56:40Z

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py

+    case [1, 2, * # comment
+        rest]:
+        pass
+    case [1, 2, * # comment
+        _]:
+        pass


This is formatted first to:

case [ "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", *rest # comment , ]: pass case [ "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", *_ # comment , ]: pass

then formatted to:

case [ "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", *rest, # comment ]: pass case [ "NOT_YET_IMPLEMENTED_PatternMatchValue", "NOT_YET_IMPLEMENTED_PatternMatchValue", *_, # comment ]: pass

If you look at

match foo: case [* # comment rest, 1]: pass case [*rest # comment , 1]: pass

you get

{ Node { kind: PatternMatchStar, range: 21..45, source: `* # comment⏎`, }: { "leading": [], "dangling": [ SourceComment { text: "# comment", position: EndOfLine, formatted: true, }, ], "trailing": [], }, Node { kind: PatternMatchStar, range: 74..79, source: `*rest`, }: { "leading": [], "dangling": [], "trailing": [ SourceComment { text: "# comment", position: EndOfLine, formatted: true, }, ], }, }

so i think the best solution is to manually reassign the comment from dangling to trailing.

@konstin Thanks for the comment. How can we access this leading-dangiling-trailing mapping?

@harupy - I typically run something like cargo run foo.py --emit=stdout --print-comments from ruff/crates/ruff_python_formatter.

Oh sorry. You're asking how to modify these assignments. Any such modifications need to be done in placement.rs -- those are essentially custom rules that let us change the association of comments. See, e.g., handle_trailing_expression_starred_star_end_of_line_comment which is somewhat similar.

You have to add a case in handle_enclosed_comment for when the enclosing node is PatternMatchStar. The general logic to fix comment placement is in placement.rs/place_comment.

konstin · 2023-08-23T15:44:40Z

crates/ruff_python_formatter/src/comments/placement.rs

+/// ```
+fn handle_pattern_match_star_comment(comment: DecoratedComment) -> CommentPlacement {
+    debug_assert!(comment.enclosing_node().is_pattern_match_star());
+    let AnyNodeRef::PatternMatchStar(range, ..) = comment.enclosing_node() else {


You can get the ast::PatternMatchStar from the AnyNodeRef::PatternMatchStar(_) in the match and then pass it to the function (like handle_leading_class_with_decorators_comment)

diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 01dc6b905..a367e6043 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -206,7 +206,7 @@ fn handle_enclosed_comment<'a>( } AnyNodeRef::WithItem(_) => handle_with_item_comment(comment, locator), AnyNodeRef::PatternMatchAs(_) => handle_pattern_match_as_comment(comment, locator), - AnyNodeRef::PatternMatchStar(_) => handle_pattern_match_star_comment(comment), + AnyNodeRef::PatternMatchStar(range, ..) => CommentPlacement::trailing(range, comment), AnyNodeRef::StmtFunctionDef(_) => handle_leading_function_with_decorators_comment(comment), AnyNodeRef::StmtClassDef(class_def) => { handle_leading_class_with_decorators_comment(comment, class_def)

Can we just do this?

Ah I see, this doesn't work for the following case:

case [ 1, * # comment rest, ]: pass

I think I'd suggest treating these as dangling, and then rendering them as leading comments within the node.

(Leading is more consistent with how we treat comments in starred expressions.)

(Tweaked this to merge.)

harupy · 2023-08-24T01:59:12Z

@charliermarsh Thanks for the update and merging!

charliermarsh · 2023-08-24T02:26:17Z

Thanks @harupy for another great contribution!

harupy commented Aug 17, 2023

View reviewed changes

konstin added the formatter Related to the formatter label Aug 21, 2023

Format PatternMatchStar

84ed8e8

harupy force-pushed the format-PatternMatchStar branch from c0e1161 to 84ed8e8 Compare August 23, 2023 09:55

harupy marked this pull request as ready for review August 23, 2023 09:56

Add test

4a54663

konstin approved these changes Aug 23, 2023

View reviewed changes

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py Show resolved Hide resolved

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py Outdated Show resolved Hide resolved

Address comments

38ab405

harupy commented Aug 23, 2023

View reviewed changes

harupy added 6 commits August 23, 2023 22:22

Handle dangling comments

4554581

More tests

4d20b60

Handle comments between star and id

1ec8d04

Fix clippy error

940609b

merge main

a554733

snapshots

2da6dea

konstin reviewed Aug 23, 2023

View reviewed changes

Use dangling comments

c035020

charliermarsh enabled auto-merge (squash) August 24, 2023 01:49

charliermarsh merged commit 205d234 into astral-sh:main Aug 24, 2023
16 checks passed

harupy deleted the format-PatternMatchStar branch August 24, 2023 01:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format `PatternMatchStar` #6653

Format `PatternMatchStar` #6653

harupy commented Aug 17, 2023

harupy commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

harupy commented Aug 17, 2023 •

edited

Loading

harupy Aug 17, 2023

MichaReiser Aug 18, 2023

harupy Aug 18, 2023

MichaReiser commented Aug 18, 2023

harupy commented Aug 18, 2023

MichaReiser commented Aug 18, 2023

harupy Aug 23, 2023 •

edited

Loading

konstin Aug 23, 2023

harupy Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

konstin Aug 23, 2023

konstin Aug 23, 2023

harupy Aug 23, 2023

harupy Aug 23, 2023 •

edited

Loading

charliermarsh Aug 23, 2023

charliermarsh Aug 23, 2023

charliermarsh Aug 24, 2023

harupy commented Aug 24, 2023

charliermarsh commented Aug 24, 2023

		let comments = f.context().comments().clone();
		let dangling = comments.dangling_comments(item);

Format PatternMatchStar #6653

Format PatternMatchStar #6653

Conversation

harupy commented Aug 17, 2023

Summary

Test Plan

harupy commented Aug 17, 2023 • edited Loading

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

PR Check Results

Benchmark

Linux

Windows

harupy commented Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Aug 18, 2023

harupy commented Aug 18, 2023

MichaReiser commented Aug 18, 2023

harupy Aug 23, 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

harupy Aug 23, 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

harupy commented Aug 24, 2023

charliermarsh commented Aug 24, 2023

Format `PatternMatchStar` #6653

Format `PatternMatchStar` #6653

harupy commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

harupy commented Aug 17, 2023 •

edited

Loading

harupy Aug 23, 2023 •

edited

Loading

harupy Aug 23, 2023 •

edited

Loading