From 8184235f93a6404e08e56e7ba7b5467112f7f330 Mon Sep 17 00:00:00 2001 From: konsti Date: Thu, 6 Jul 2023 16:07:47 +0200 Subject: [PATCH] Try statements have a body: Fix formatter instability (#5558) ## Summary The following code was previously leading to unstable formatting: ```python try: try: pass finally: print(1) # issue7208 except A: pass ``` The comment would be formatted as a trailing comment of `try` which is unstable as an end-of-line comment gets two extra whitespaces. This was originally found in https://github.com/python/cpython/blob/99b00efd5edfd5b26bf9e2a35cbfc96277fdcbb1/Lib/getpass.py#L68-L91 ## Test Plan I added a regression test --- crates/ruff_python_ast/src/node.rs | 2 + .../test/fixtures/ruff/statement/try.py | 10 ++ .../src/comments/placement.rs | 152 +++++++++--------- 3 files changed, 88 insertions(+), 76 deletions(-) diff --git a/crates/ruff_python_ast/src/node.rs b/crates/ruff_python_ast/src/node.rs index cc752a7a1f..ccbc44c723 100644 --- a/crates/ruff_python_ast/src/node.rs +++ b/crates/ruff_python_ast/src/node.rs @@ -4281,6 +4281,8 @@ impl AnyNodeRef<'_> { | AnyNodeRef::StmtFunctionDef(_) | AnyNodeRef::StmtAsyncFunctionDef(_) | AnyNodeRef::StmtClassDef(_) + | AnyNodeRef::StmtTry(_) + | AnyNodeRef::StmtTryStar(_) ) } } diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/try.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/try.py index 11d07349bc..3ec6c3ca50 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/try.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/try.py @@ -89,3 +89,13 @@ else: # before finally finally: ... + +# try and try star are statements with body +# Minimized from https://github.com/python/cpython/blob/99b00efd5edfd5b26bf9e2a35cbfc96277fdcbb1/Lib/getpass.py#L68-L91 +try: + try: + pass + finally: + print(1) # issue7208 +except A: + pass diff --git a/crates/ruff_python_formatter/src/comments/placement.rs b/crates/ruff_python_formatter/src/comments/placement.rs index 2692b0e27d..44ac6b5195 100644 --- a/crates/ruff_python_formatter/src/comments/placement.rs +++ b/crates/ruff_python_formatter/src/comments/placement.rs @@ -335,89 +335,89 @@ fn handle_in_between_bodies_end_of_line_comment<'a>( } // The comment must be between two statements... - if let (Some(preceding), Some(following)) = (comment.preceding_node(), comment.following_node()) - { - // ...and the following statement must be the first statement in an alternate body of the parent... - if !is_first_statement_in_enclosing_alternate_body(following, comment.enclosing_node()) { - // ```python - // if test: - // a - // # comment - // b - // ``` - return CommentPlacement::Default(comment); - } + let (Some(preceding), Some(following)) = (comment.preceding_node(), comment.following_node()) + else { + return CommentPlacement::Default(comment); + }; - if locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) { - // The `elif` or except handlers have their own body to which we can attach the trailing comment + // ...and the following statement must be the first statement in an alternate body of the parent... + if !is_first_statement_in_enclosing_alternate_body(following, comment.enclosing_node()) { + // ```python + // if test: + // a + // # comment + // b + // ``` + return CommentPlacement::Default(comment); + } + + if locator.contains_line_break(TextRange::new(preceding.end(), comment.slice().start())) { + // The `elif` or except handlers have their own body to which we can attach the trailing comment + // ```python + // if test: + // a + // elif c: # comment + // b + // ``` + if following.is_except_handler() { + return CommentPlacement::trailing(following, comment); + } else if following.is_stmt_if() { + // We have to exclude for following if statements that are not elif by checking the + // indentation // ```python - // if test: - // a - // elif c: # comment - // b - // ``` - if following.is_except_handler() { - return CommentPlacement::trailing(following, comment); - } else if following.is_stmt_if() { - // We have to exclude for following if statements that are not elif by checking the - // indentation - // ```python - // if True: - // pass - // else: # Comment - // if False: - // pass - // pass - // ``` - let base_if_indent = - whitespace::indentation_at_offset(locator, following.range().start()); - let maybe_elif_indent = whitespace::indentation_at_offset( - locator, - comment.enclosing_node().range().start(), - ); - if base_if_indent == maybe_elif_indent { - return CommentPlacement::trailing(following, comment); - } - } - // There are no bodies for the "else" branch and other bodies that are represented as a `Vec`. - // This means, there's no good place to attach the comments to. - // Make this a dangling comments and manually format the comment in - // in the enclosing node's formatting logic. For `try`, it's the formatters responsibility - // to correctly identify the comments for the `finally` and `orelse` block by looking - // at the comment's range. - // - // ```python - // while x == y: + // if True: + // pass + // else: # Comment + // if False: + // pass // pass - // else: # trailing - // print("nooop") // ``` - CommentPlacement::dangling(comment.enclosing_node(), comment) - } else { - // Trailing comment of the preceding statement - // ```python - // while test: - // a # comment - // else: - // b - // ``` - if preceding.is_node_with_body() { - // We can't set this as a trailing comment of the function declaration because it - // will then move behind the function block instead of sticking with the pass - // ```python - // if True: - // def f(): - // pass # a - // else: - // pass - // ``` - CommentPlacement::Default(comment) - } else { - CommentPlacement::trailing(preceding, comment) + let base_if_indent = + whitespace::indentation_at_offset(locator, following.range().start()); + let maybe_elif_indent = whitespace::indentation_at_offset( + locator, + comment.enclosing_node().range().start(), + ); + if base_if_indent == maybe_elif_indent { + return CommentPlacement::trailing(following, comment); } } + // There are no bodies for the "else" branch and other bodies that are represented as a `Vec`. + // This means, there's no good place to attach the comments to. + // Make this a dangling comments and manually format the comment in + // in the enclosing node's formatting logic. For `try`, it's the formatters responsibility + // to correctly identify the comments for the `finally` and `orelse` block by looking + // at the comment's range. + // + // ```python + // while x == y: + // pass + // else: # trailing + // print("nooop") + // ``` + CommentPlacement::dangling(comment.enclosing_node(), comment) } else { - CommentPlacement::Default(comment) + // Trailing comment of the preceding statement + // ```python + // while test: + // a # comment + // else: + // b + // ``` + if preceding.is_node_with_body() { + // We can't set this as a trailing comment of the function declaration because it + // will then move behind the function block instead of sticking with the pass + // ```python + // if True: + // def f(): + // pass # a + // else: + // pass + // ``` + CommentPlacement::Default(comment) + } else { + CommentPlacement::trailing(preceding, comment) + } } }