From 9b6d2ce1f2aa0db3d9b388b443e70fe760ada35c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 31 May 2024 14:06:17 +0200 Subject: [PATCH] Fix incorect placement of trailing stub function comments (#11632) --- .../stub_functions_trailing_comments.py | 28 +++++++ .../src/comments/format.rs | 70 ++++++++--------- .../src/statement/stmt_class_def.rs | 4 +- .../src/statement/stmt_function_def.rs | 4 +- .../src/statement/suite.rs | 3 +- ...__stub_functions_trailing_comments.py.snap | 77 +++++++++++++++++++ 6 files changed, 142 insertions(+), 44 deletions(-) create mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/stub_functions_trailing_comments.py create mode 100644 crates/ruff_python_formatter/tests/snapshots/format@statement__stub_functions_trailing_comments.py.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/stub_functions_trailing_comments.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/stub_functions_trailing_comments.py new file mode 100644 index 0000000000000..04ab35d22e774 --- /dev/null +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/stub_functions_trailing_comments.py @@ -0,0 +1,28 @@ +# Regression tests for https://github.com/astral-sh/ruff/issues/11569 + + +# comment 1 +def foo(self) -> None: ... +def bar(self) -> None: ... +# comment 2 + +# comment 3 +def baz(self) -> None: + return None +# comment 4 + + +def foo(self) -> None: ... +# comment 5 + +def baz(self) -> None: + return None + + +def foo(self) -> None: + ... # comment 5 +def baz(self) -> None: + return None + +def foo(self) -> None: ... +# comment 5 diff --git a/crates/ruff_python_formatter/src/comments/format.rs b/crates/ruff_python_formatter/src/comments/format.rs index b22de1b05043b..8da3d1daeac2b 100644 --- a/crates/ruff_python_formatter/src/comments/format.rs +++ b/crates/ruff_python_formatter/src/comments/format.rs @@ -164,7 +164,7 @@ impl Format> for FormatTrailingComments<'_> { line_suffix( &format_args![ empty_lines(lines_before_comment), - format_comment(trailing) + format_comment(trailing), ], // Reserving width isn't necessary because we don't split // comments and the empty lines expand any enclosing group. @@ -535,22 +535,13 @@ fn strip_comment_prefix(comment_text: &str) -> FormatResult<&str> { /// ``` /// /// This builder will insert a single empty line before the comment. -pub(crate) fn empty_lines_before_trailing_comments<'a>( - f: &PyFormatter, - comments: &'a [SourceComment], +pub(crate) fn empty_lines_before_trailing_comments( + comments: &[SourceComment], node_kind: NodeKind, -) -> FormatEmptyLinesBeforeTrailingComments<'a> { - // Black has different rules for stub vs. non-stub and top level vs. indented - let empty_lines = match (f.options().source_type(), f.context().node_level()) { - (PySourceType::Stub, NodeLevel::TopLevel(_)) => 1, - (PySourceType::Stub, _) => u32::from(node_kind == NodeKind::StmtClassDef), - (_, NodeLevel::TopLevel(_)) => 2, - (_, _) => 1, - }; - +) -> FormatEmptyLinesBeforeTrailingComments { FormatEmptyLinesBeforeTrailingComments { comments, - empty_lines, + node_kind, } } @@ -558,8 +549,7 @@ pub(crate) fn empty_lines_before_trailing_comments<'a>( pub(crate) struct FormatEmptyLinesBeforeTrailingComments<'a> { /// The trailing comments of the node. comments: &'a [SourceComment], - /// The expected number of empty lines before the trailing comments. - empty_lines: u32, + node_kind: NodeKind, } impl Format> for FormatEmptyLinesBeforeTrailingComments<'_> { @@ -569,9 +559,17 @@ impl Format> for FormatEmptyLinesBeforeTrailingComments<'_> .iter() .find(|comment| comment.line_position().is_own_line()) { + // Black has different rules for stub vs. non-stub and top level vs. indented + let empty_lines = match (f.options().source_type(), f.context().node_level()) { + (PySourceType::Stub, NodeLevel::TopLevel(_)) => 1, + (PySourceType::Stub, _) => u32::from(self.node_kind == NodeKind::StmtClassDef), + (_, NodeLevel::TopLevel(_)) => 2, + (_, _) => 1, + }; + let actual = lines_before(comment.start(), f.context().source()).saturating_sub(1); - for _ in actual..self.empty_lines { - write!(f, [empty_line()])?; + for _ in actual..empty_lines { + empty_line().fmt(f)?; } } Ok(()) @@ -590,30 +588,16 @@ impl Format> for FormatEmptyLinesBeforeTrailingComments<'_> /// /// While `leading_comments` will preserve the existing empty line, this builder will insert an /// additional empty line before the comment. -pub(crate) fn empty_lines_after_leading_comments<'a>( - f: &PyFormatter, - comments: &'a [SourceComment], -) -> FormatEmptyLinesAfterLeadingComments<'a> { - // Black has different rules for stub vs. non-stub and top level vs. indented - let empty_lines = match (f.options().source_type(), f.context().node_level()) { - (PySourceType::Stub, NodeLevel::TopLevel(_)) => 1, - (PySourceType::Stub, _) => 0, - (_, NodeLevel::TopLevel(_)) => 2, - (_, _) => 1, - }; - - FormatEmptyLinesAfterLeadingComments { - comments, - empty_lines, - } +pub(crate) fn empty_lines_after_leading_comments( + comments: &[SourceComment], +) -> FormatEmptyLinesAfterLeadingComments { + FormatEmptyLinesAfterLeadingComments { comments } } #[derive(Copy, Clone, Debug)] pub(crate) struct FormatEmptyLinesAfterLeadingComments<'a> { /// The leading comments of the node. comments: &'a [SourceComment], - /// The expected number of empty lines after the leading comments. - empty_lines: u32, } impl Format> for FormatEmptyLinesAfterLeadingComments<'_> { @@ -624,6 +608,14 @@ impl Format> for FormatEmptyLinesAfterLeadingComments<'_> { .rev() .find(|comment| comment.line_position().is_own_line()) { + // Black has different rules for stub vs. non-stub and top level vs. indented + let empty_lines = match (f.options().source_type(), f.context().node_level()) { + (PySourceType::Stub, NodeLevel::TopLevel(_)) => 1, + (PySourceType::Stub, _) => 0, + (_, NodeLevel::TopLevel(_)) => 2, + (_, _) => 1, + }; + let actual = lines_after(comment.end(), f.context().source()).saturating_sub(1); // If there are no empty lines, keep the comment tight to the node. if actual == 0 { @@ -632,12 +624,12 @@ impl Format> for FormatEmptyLinesAfterLeadingComments<'_> { // If there are more than enough empty lines already, `leading_comments` will // trim them as necessary. - if actual >= self.empty_lines { + if actual >= empty_lines { return Ok(()); } - for _ in actual..self.empty_lines { - write!(f, [empty_line()])?; + for _ in actual..empty_lines { + empty_line().fmt(f)?; } } Ok(()) diff --git a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs index e7fd973e22832..063199131ecd6 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_class_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_class_def.rs @@ -55,7 +55,7 @@ impl FormatNodeRule for FormatStmtClassDef { // newline between the comment and the node, but we _require_ two newlines. If there are // _no_ newlines between the comment and the node, we don't insert _any_ newlines; if there // are more than two, then `leading_comments` will preserve the correct number of newlines. - empty_lines_after_leading_comments(f, comments.leading(item)).fmt(f)?; + empty_lines_after_leading_comments(comments.leading(item)).fmt(f)?; write!( f, @@ -152,7 +152,7 @@ impl FormatNodeRule for FormatStmtClassDef { // // # comment // ``` - empty_lines_before_trailing_comments(f, comments.trailing(item), NodeKind::StmtClassDef) + empty_lines_before_trailing_comments(comments.trailing(item), NodeKind::StmtClassDef) .fmt(f)?; Ok(()) diff --git a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs index 93f89bf6f1ece..24a578414fad9 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_function_def.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_function_def.rs @@ -52,7 +52,7 @@ impl FormatNodeRule for FormatStmtFunctionDef { // newline between the comment and the node, but we _require_ two newlines. If there are // _no_ newlines between the comment and the node, we don't insert _any_ newlines; if there // are more than two, then `leading_comments` will preserve the correct number of newlines. - empty_lines_after_leading_comments(f, comments.leading(item)).fmt(f)?; + empty_lines_after_leading_comments(comments.leading(item)).fmt(f)?; write!( f, @@ -86,7 +86,7 @@ impl FormatNodeRule for FormatStmtFunctionDef { // // # comment // ``` - empty_lines_before_trailing_comments(f, comments.trailing(item), NodeKind::StmtFunctionDef) + empty_lines_before_trailing_comments(comments.trailing(item), NodeKind::StmtFunctionDef) .fmt(f) } } diff --git a/crates/ruff_python_formatter/src/statement/suite.rs b/crates/ruff_python_formatter/src/statement/suite.rs index 844fcc0bec5e6..7137558c500b3 100644 --- a/crates/ruff_python_formatter/src/statement/suite.rs +++ b/crates/ruff_python_formatter/src/statement/suite.rs @@ -240,7 +240,8 @@ impl FormatRule> for FormatSuite { preceding_stub.end(), f.context().source(), ) < 2 - }); + }) + && !preceding_comments.has_trailing_own_line(); if !is_preceding_stub_function_without_empty_line { match self.kind { diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__stub_functions_trailing_comments.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__stub_functions_trailing_comments.py.snap new file mode 100644 index 0000000000000..39b3ecb94b1a8 --- /dev/null +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__stub_functions_trailing_comments.py.snap @@ -0,0 +1,77 @@ +--- +source: crates/ruff_python_formatter/tests/fixtures.rs +input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/stub_functions_trailing_comments.py +--- +## Input +```python +# Regression tests for https://github.com/astral-sh/ruff/issues/11569 + + +# comment 1 +def foo(self) -> None: ... +def bar(self) -> None: ... +# comment 2 + +# comment 3 +def baz(self) -> None: + return None +# comment 4 + + +def foo(self) -> None: ... +# comment 5 + +def baz(self) -> None: + return None + + +def foo(self) -> None: + ... # comment 5 +def baz(self) -> None: + return None + +def foo(self) -> None: ... +# comment 5 +``` + +## Output +```python +# Regression tests for https://github.com/astral-sh/ruff/issues/11569 + + +# comment 1 +def foo(self) -> None: ... +def bar(self) -> None: ... + + +# comment 2 + + +# comment 3 +def baz(self) -> None: + return None + + +# comment 4 + + +def foo(self) -> None: ... + + +# comment 5 + + +def baz(self) -> None: + return None + + +def foo(self) -> None: ... # comment 5 +def baz(self) -> None: + return None + + +def foo(self) -> None: ... + + +# comment 5 +```