Format `StmtTry` by davidszotten · Pull Request #5222 · astral-sh/ruff

davidszotten · 2023-06-20T18:43:48Z

Format StmtTry

konstin · 2023-06-21T15:17:08Z

Personally, i find it easier to add fixtures first so i can directly see what my code currently does, with fixtures in the PR it is also easier to give guidance since i can directly see how it currently behaves (I usually run cargo run --bin ruff_python_formatter -- --emit stdout --print-ir --print-comments scratch.py for a specific problem, then copy it to the fixture, run INSTA_UPDATE=always cargo test --package ruff_python_formatter and look at git diff, others like cargo insta review, etc.).

github-actions · 2023-06-21T16:06:22Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.8±0.18ms     5.2 MB/sec    1.00      7.8±0.13ms     5.2 MB/sec
formatter/numpy/ctypeslib.py               1.00  1831.3±26.59µs     9.1 MB/sec    1.01  1847.4±28.20µs     9.0 MB/sec
formatter/numpy/globals.py                 1.03    218.3±5.26µs    13.5 MB/sec    1.00    211.2±5.69µs    14.0 MB/sec
formatter/pydantic/types.py                1.04      4.1±0.05ms     6.2 MB/sec    1.00      4.0±0.07ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.04     16.2±0.32ms     2.5 MB/sec    1.00     15.6±0.28ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      4.0±0.05ms     4.1 MB/sec    1.00      3.9±0.07ms     4.3 MB/sec
linter/all-rules/numpy/globals.py          1.02    513.5±4.11µs     5.7 MB/sec    1.00    504.7±4.40µs     5.8 MB/sec
linter/all-rules/pydantic/types.py         1.05      7.2±0.06ms     3.5 MB/sec    1.00      6.9±0.11ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.10      8.5±0.09ms     4.8 MB/sec    1.00      7.7±0.17ms     5.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.08  1825.3±22.12µs     9.1 MB/sec    1.00  1690.6±33.14µs     9.8 MB/sec
linter/default-rules/numpy/globals.py      1.08    203.6±1.46µs    14.5 MB/sec    1.00    189.4±3.12µs    15.6 MB/sec
linter/default-rules/pydantic/types.py     1.09      3.8±0.06ms     6.7 MB/sec    1.00      3.5±0.04ms     7.3 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.1±0.10ms     5.0 MB/sec    1.19      9.6±0.11ms     4.2 MB/sec
formatter/numpy/ctypeslib.py               1.00  1817.4±33.40µs     9.2 MB/sec    1.14      2.1±0.02ms     8.0 MB/sec
formatter/numpy/globals.py                 1.00    211.1±4.34µs    14.0 MB/sec    1.09    230.9±7.00µs    12.8 MB/sec
formatter/pydantic/types.py                1.00      4.1±0.08ms     6.2 MB/sec    1.13      4.7±0.07ms     5.5 MB/sec
linter/all-rules/large/dataset.py          1.00     15.7±0.20ms     2.6 MB/sec    1.00     15.8±0.36ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.1±0.07ms     4.0 MB/sec    1.00      4.1±0.05ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    498.8±7.67µs     5.9 MB/sec    1.00    499.2±7.48µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.01      7.0±0.12ms     3.6 MB/sec    1.00      7.0±0.10ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.1±0.09ms     5.0 MB/sec    1.00      8.1±0.11ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1742.5±29.80µs     9.6 MB/sec    1.00  1746.1±21.72µs     9.5 MB/sec
linter/default-rules/numpy/globals.py      1.00    200.0±3.85µs    14.8 MB/sec    1.00    199.8±3.39µs    14.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.7±0.07ms     6.9 MB/sec    1.02      3.7±0.05ms     6.8 MB/sec

konstin · 2023-06-21T16:53:17Z

...thon_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__try_py.snap

+    ...
+# before except 2
+except (
+    KeyError  # except line 2


as far as i can see this is currently the only misplaced comment, right?

black also seems to remove the parentheses here even though it otherwise doesn't

comment placement looks ok to me here (except line 2)

but i don't understand what causes us to break the (KeyError) over multiple lines

note that this is just a call to .format(), on a tuple of types, nothing manual here
https://github.com/astral-sh/ruff/pull/5222/files/ccd96e6ae3d7e38b833f530d99f5d0b9d46b35ef#diff-30bdf42955f6a58b1f41492d192056475d73d2d27906cb200ba4beba5d010d75R39

black format

try: ... except (KeyError) as key: # except line 2 ...

as

try: ... except KeyError as key: # except line 2 ...

which looks better to me. This is another one of the rare cases where black removes parentheses

i agree that it's better to remove them. but i'm not sure what is preserving them (i was wrong above, here it isn't a tuple but just an ExprName)

You might need to change the attachment of # except line 2 in placement.rs, either attach it trailing to a node that doesn't break the line if it doesn't need to or mark it as dangling and manually format after the colon. we're doing the latter (formatting after : manually) in some other nodes

Yeah... The fact that there's no explicit node for bodies really makes things annoying. The issue here is that the except line 2 comment falls into the middle of the ExceptHandlerExceptHandler node (it is inside of that node's range). The last preceding node is the KeyError because key (Identifier) is not an AST node (it probably should, but it is not 😢).

We probably need something similar to (or extend)

ruff/crates/ruff_python_formatter/src/comments/placement.rs

Lines 551 to 554 in 8a73a7b

fn handle_trailing_end_of_line_condition_comment<'a>(

comment: DecoratedComment<'a>,

locator: &Locator,

) -> CommentPlacement<'a> {

davidszotten · 2023-06-21T18:16:57Z

add fixtures first

just to clarify: did you look at the pr before i committed the fixtures (a bit late in the process) or are you suggesting committing them first (even before any code changes?) in a separate commit to then see changes as the impl is added?

...thon_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__try_py.snap

crates/ruff_python_formatter/src/statement/stmt_try.rs

davidszotten · 2023-06-21T18:31:07Z

til about --print-ir and --print-comments

very helpful, have been doing lots of this manually with dbg statements, d'oh

konstin · 2023-06-21T18:42:02Z

just to clarify: did you look at the pr before i committed the fixtures (a bit late in the process) or are you suggesting committing them first (even before any code changes?) in a separate commit to then see changes as the impl is added?

No my idea was just to put code you tested in the fixtures so other people can also see them when looking at your PR. We squash merge PRs, so commit order or structure is not important for ruff PRs. Personally, i try to skim external contributor draft PRs once they are created and comment if i see something, and add a thorough review once the PR is marked as ready. (You can of course always ping me or others on draft PRs if you have specific questions)

davidszotten · 2023-06-21T18:58:01Z

just to clarify: did you look at the pr before i committed the fixtures (a bit late in the process) or are you suggesting committing them first (even before any code changes?) in a separate commit to then see changes as the impl is added?

No my idea was just to put code you tested in the fixtures so other people can also see them when looking at your PR. We squash merge PRs, so commit order or structure is not important for ruff PRs. Personally, i try to skim external contributor draft PRs once they are created and comment if i see something, and add a thorough review once the PR is marked as ready. (You can of course always ping me or others on draft PRs if you have specific questions)

are you still missing something i've referenced but not included here (in try.py)?

if this is about timing, makes sense, i'll try to commit my scratch into a fixture sooner

konstin · 2023-06-21T19:08:29Z

The try.py looks even better than what i had in mind, thanks for adding this! My second comment was meant to be about general ruff PR procedures, not about this PR specifically

crates/ruff_python_formatter/src/statement/stmt_try.rs

konstin · 2023-06-21T21:12:35Z

...thon_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__try_py.snap

+    ...
+# before except 2
+except (
+    KeyError  # except line 2


You might need to change the attachment of # except line 2 in placement.rs, either attach it trailing to a node that doesn't break the line if it doesn't need to or mark it as dangling and manually format after the colon. we're doing the latter (formatting after : manually) in some other nodes

crates/ruff_python_formatter/src/statement/stmt_try.rs

konstin · 2023-06-21T21:14:01Z

crates/ruff_python_formatter/src/statement/stmt_try.rs

+            }
+        }
+
+        leading_node_comments(except_handler_except_handler).fmt(f)?;


nit: can you merge those into the write call below?

crates/ruff_python_formatter/src/statement/stmt_try.rs

MichaReiser

This looks really good. I putting this back in your queue, mainly to move the ExceptHandler formatting to FormatExceptHandlerExceptHandler.

Feel free to ignore the incorrectly placed comment for now and instead add a TODO. We can tackle this as a separate pR

crates/ruff_python_formatter/src/statement/stmt_try.rs

MichaReiser · 2023-06-22T06:33:20Z

...thon_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__try_py.snap

+    ...
+# before except 2
+except (
+    KeyError  # except line 2


Yeah... The fact that there's no explicit node for bodies really makes things annoying. The issue here is that the except line 2 comment falls into the middle of the ExceptHandlerExceptHandler node (it is inside of that node's range). The last preceding node is the KeyError because key (Identifier) is not an AST node (it probably should, but it is not 😢).

We probably need something similar to (or extend)

ruff/crates/ruff_python_formatter/src/comments/placement.rs

Lines 551 to 554 in 8a73a7b

fn handle_trailing_end_of_line_condition_comment<'a>(

comment: DecoratedComment<'a>,

locator: &Locator,

) -> CommentPlacement<'a> {

davidszotten · 2023-06-22T07:32:36Z

thanks for the comments, will see how i get on. while waiting for them, i also tried to see if i could re-use this code for TryStar but had one annoyance and one blocker

annoyance: since their contents is the same i tried destructing the item and then passing the components to a helper function to do the work, but when borrowing them across the function barrier i had e.g. body: &[Stmt<TextRange>] instead of body: Vec<_> and i wasn't able to call body.format(). i tried a few of the compiler suggestions but this was already at the limit of my rust knowledge. i worked around this by making an enum and passing the entire item

blocker: the except statement is emitted by the handler fmt, how do i pass context in there (or where can it look) so it knows if it's inside a try or a trystar?

MichaReiser · 2023-06-22T08:25:48Z

thanks for the comments, will see how i get on. while waiting for them, i also tried to see if i could re-use this code for TryStar but had one annoyance and one blocker

annoyance: since their contents is the same i tried destructing the item and then passing the components to a helper function to do the work, but when borrowing them across the function barrier i had e.g. body: &[Stmt<TextRange>] instead of body: Vec<_> and i wasn't able to call body.format(). i tried a few of the compiler suggestions but this was already at the limit of my rust knowledge. i worked around this by making an enum and passing the entire item

One pattern that worked well in the past is to either:

Implement an enum that is a union over both types. See FormatAnyFunctionDef
Define a trait and implement it by your formatters. See FormatBinaryLike

I don't have a strong preference for either.

blocker: the except statement is emitted by the handler fmt, how do i pass context in there (or where can it look) so it knows if it's inside a try or a trystar?

You can implement FormatRuleWithOptions and then call your formatting like this

except_handler.format().with_options(Context::TryStar).fmt(f)

See FormatExpr as an example.

crates/ruff_python_formatter/src/statement/stmt_try.rs

crates/ruff_python_formatter/src/comments/placement.rs

crates/ruff_python_formatter/src/other/except_handler_except_handler.rs

crates/ruff_python_formatter/src/statement/stmt_try.rs

MichaReiser · 2023-06-23T10:03:53Z

I'm so impressed by how you tackle this. You're probably working on the most complicated statement!

davidszotten · 2023-06-23T11:30:10Z

I'm so impressed by how you tackle this. You're probably working on the most complicated statement!

Thanks, and again, thanks for your patience and help!

davidszotten · 2023-06-23T13:09:03Z

crates/ruff_python_formatter/src/comments/placement.rs

+
+    return CommentPlacement::dangling(comment.enclosing_node(), comment);
+}
+


would probably be a good idea to have some tests for these functions. not sure what we have available to make tests ergonomic. will have a look around, but you might have good ideas already

We have some snapshot tests in the comments/mod.rs file, that test if the comments get associated with the right node. But they are quiet similar to the formatting snapshot tests.

ruff/crates/ruff_python_formatter/src/comments/mod.rs

Lines 587 to 608 in d3d69a0

#[test]

fn if_elif_else_comments() {

let source = r#"

if x == y:

pass # trailing `pass` comment

# Root `if` trailing comment

# Leading elif comment

elif x < y:

pass

# `elif` trailing comment

# Leading else comment

else:

pass

# `else` trailing comment

"#;

let test_case = CommentsTestCase::from_code(source);

let comments = test_case.to_comments();

assert_debug_snapshot!(comments.debug(test_case.source_code));

}

I plan to review your PR on Monday.

yes i did find those and tried to (ab?)use them to generate input i could pass to the functions here like handle_trailing_end_of_line_except_comment but i failed.

Yeah. It is testing the whole place_comment and not only handle_treiling_end_of_line_except_comment. This is intentional because the construction of the DecoratedComment is very specific and easy to get wrong when constructing by hand (and we want to make sure that changes the DecoratedComment construction resulting in a failure of the except comment placement are catched too). But you can tailor your source code so that it calls your function with the arguments that you want to test.

MichaReiser

This looks good to me except that I think that some comment placement is incorrect. I'll leave it up to you if you want to:

Split the comment placement fixes into its own PR
Follow up as part of this PR

crates/ruff_python_formatter/src/comments/placement.rs

crates/ruff_python_formatter/src/statement/stmt_try.rs

MichaReiser · 2023-06-26T15:53:20Z

@davidszotten is this PR ready for merging from your side(besides the merge conflicts?). I lost the overview.

still working on comments

i think there are still issues with blank lines between the sections

seems to fix some other cases but not `except (KeyError): # comment`

…h_comments TODO: now misses a blank newline before a leading comment to the first except block

refactor `handle_in_between_except_handlers_or_except_handler_and_else_or_finally_comment` with early returns to avoid very nested indentation and add a check for subsequent except handlers to attach comments to (as leading) instead of leaving as dangling on the try for else/finally

davidszotten · 2023-06-26T18:22:50Z

@davidszotten is this PR ready for merging from your side(besides the merge conflicts?). I lost the overview.

#5222 (comment) and #5222 (comment) are waiting for your replies i think

…placement handler

davidszotten · 2023-06-28T10:04:47Z

🎉 thanks

MichaReiser · 2023-06-28T10:08:16Z

Thank you!

MichaReiser linked an issue Jun 21, 2023 that may be closed by this pull request

Formatter: Try / TryStar (includes ExceptHandler) #5245

Closed

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

davidszotten force-pushed the format-stmt-try branch 2 times, most recently from 153c3a8 to 106c0e3 Compare June 21, 2023 13:56

konstin reviewed Jun 21, 2023

View reviewed changes

davidszotten commented Jun 21, 2023

View reviewed changes

...thon_formatter/src/snapshots/ruff_python_formatter__tests__ruff_test__statement__try_py.snap Outdated Show resolved Hide resolved

davidszotten commented Jun 21, 2023

View reviewed changes

crates/ruff_python_formatter/src/statement/stmt_try.rs Show resolved Hide resolved

davidszotten force-pushed the format-stmt-try branch from ccd96e6 to 5976bd5 Compare June 21, 2023 19:57

davidszotten changed the title ~~wip Format StmtTry~~ Format StmtTry Jun 21, 2023

davidszotten marked this pull request as ready for review June 21, 2023 20:09

konstin reviewed Jun 21, 2023

View reviewed changes

MichaReiser requested changes Jun 22, 2023

View reviewed changes

davidszotten force-pushed the format-stmt-try branch from 9ea0e06 to 8d74f49 Compare June 22, 2023 16:40

MichaReiser reviewed Jun 22, 2023

View reviewed changes

crates/ruff_python_formatter/src/statement/stmt_try.rs Show resolved Hide resolved

MichaReiser reviewed Jun 23, 2023

View reviewed changes

crates/ruff_python_formatter/src/comments/placement.rs Outdated Show resolved Hide resolved

crates/ruff_python_formatter/src/other/except_handler_except_handler.rs Show resolved Hide resolved

crates/ruff_python_formatter/src/statement/stmt_try.rs Outdated Show resolved Hide resolved

davidszotten force-pushed the format-stmt-try branch 2 times, most recently from 968726f to cfe8ac1 Compare June 23, 2023 09:31

davidszotten commented Jun 23, 2023

View reviewed changes

davidszotten requested a review from MichaReiser June 23, 2023 17:45

MichaReiser reviewed Jun 26, 2023

View reviewed changes

crates/ruff_python_formatter/src/comments/placement.rs Outdated Show resolved Hide resolved

crates/ruff_python_formatter/src/comments/placement.rs Show resolved Hide resolved

crates/ruff_python_formatter/src/statement/stmt_try.rs Outdated Show resolved Hide resolved

davidszotten force-pushed the format-stmt-try branch from 279e162 to 66405db Compare June 26, 2023 10:11

davidszotten added 16 commits June 26, 2023 17:47

wip Format StmtTry

86f20ad

still working on comments

Format StmtTry: comments

fc1d071

i think there are still issues with blank lines between the sections

snapshot test

5e010f4

try IfBreaks

3609790

seems to fix some other cases but not `except (KeyError): # comment`

first attempt at empty_line fix for exc handler

cdb7e4d

empty_line fix for first exc handler

a06e686

clippy

2744c41

identifiers can now be formatted

93c4971

move to except_handler_except_handler and use leading_alternate_branc…

daea430

…h_comments TODO: now misses a blank newline before a leading comment to the first except block

fix trailing comments on except clauses sometimes causing wrapping

a1870e6

comment about fmt_dangling_comments

a36abdf

fmt_dangling_comments for excepthandler

fb9b1a0

fix leading_alternate_branc_comments for first except handler

4029566

factor out leading_alternate_branch_comments for orelse/finally

27addec

remove redundant hard_line_break

52f90fe

davidszotten force-pushed the format-stmt-try branch from 66405db to 52f90fe Compare June 26, 2023 18:21

MichaReiser added 2 commits June 28, 2023 11:47

Make it explicit that following is never None for except handler …

187f501

…placement handler

Merge branch 'main' into format-stmt-try

b728910

MichaReiser enabled auto-merge (squash) June 28, 2023 09:52

MichaReiser approved these changes Jun 28, 2023

View reviewed changes

Fix clippy warnings

56f9756

MichaReiser merged commit 1979103 into astral-sh:main Jun 28, 2023

davidszotten deleted the format-stmt-try branch July 7, 2023 20:47

	fn handle_trailing_end_of_line_condition_comment<'a>(
	comment: DecoratedComment<'a>,
	locator: &Locator,
	) -> CommentPlacement<'a> {


		return CommentPlacement::dangling(comment.enclosing_node(), comment);
		}

	#[test]
	fn if_elif_else_comments() {
	let source = r#"
	if x == y:
	pass # trailing `pass` comment
	# Root `if` trailing comment

	# Leading elif comment
	elif x < y:
	pass
	# `elif` trailing comment
	# Leading else comment
	else:
	pass
	# `else` trailing comment
	"#;
	let test_case = CommentsTestCase::from_code(source);

	let comments = test_case.to_comments();

	assert_debug_snapshot!(comments.debug(test_case.source_code));
	}

Conversation

davidszotten commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

konstin commented Jun 21, 2023

Uh oh!

github-actions bot commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

konstin Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 21, 2023

Uh oh!

Uh oh!

Uh oh!

davidszotten commented Jun 21, 2023

Uh oh!

konstin commented Jun 21, 2023

Uh oh!

davidszotten commented Jun 21, 2023

Uh oh!

konstin commented Jun 21, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 22, 2023

Uh oh!

MichaReiser commented Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser commented Jun 23, 2023

Uh oh!

davidszotten commented Jun 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidszotten commented Jun 20, 2023 •

edited

Loading

github-actions bot commented Jun 21, 2023 •

edited

Loading

konstin Jun 21, 2023 •

edited

Loading

MichaReiser commented Jun 22, 2023 •

edited

Loading

MichaReiser commented Jun 26, 2023 •

edited

Loading