From 90ba40c23c4a179637da81f5a38f50b2f4b4951c Mon Sep 17 00:00:00 2001 From: konsti Date: Tue, 8 Aug 2023 11:15:35 +0200 Subject: [PATCH] Fix zulip unstable formatting with end-of-line comments (#6386) ## Bug Given ```python x = () - (# ) ``` the comment is a dangling comment of the empty tuple. This is an end-of-line comment so it may move after the expression. It still expands the parent, so the operator breaks: ```python x = ( () - () # ) ``` In the next formatting pass, the comment is not a trailing tuple but a trailing bin op comment, so the bin op doesn't break anymore. The comment again expands the parent, so we still add the superfluous parentheses ```python x = ( () - () # ) ``` ## Fix The new formatting is to keep the comment on the empty tuple. This is a log uglier and again has additional outer parentheses, but it's stable: ```python x = ( () - ( # ) ) ``` ## Alternatives Black formats all the examples above as ```python x = () - () # ``` which i find better. I would be happy about any suggestions for better solutions than the current one. I'd mainly need a workaround for expand parent having an effect on the bin op instead of first moving the comment to the end and then applying expand parent to the assign statement. --- .../test/fixtures/ruff/expression/binary.py | 11 +++++++++ .../test/fixtures/ruff/statement/delete.py | 4 ++++ crates/ruff_python_formatter/src/builders.rs | 10 ++++++++ .../format@expression__binary.py.snap | 24 +++++++++++++++++++ .../snapshots/format@expression__call.py.snap | 3 ++- .../snapshots/format@expression__dict.py.snap | 3 ++- .../format@expression__lambda.py.snap | 3 ++- .../snapshots/format@expression__list.py.snap | 6 +++-- .../format@statement__delete.py.snap | 11 ++++++++- .../format@statement__function.py.snap | 3 ++- .../snapshots/format@statement__raise.py.snap | 3 ++- 11 files changed, 73 insertions(+), 8 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py index 30cf4c4465f47..572518635ea86 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/binary.py @@ -211,3 +211,14 @@ log(self.price / self.strike) + (self.risk_free - self.div_cont + 0.5 * (self.sigma**2)) * self.exp_time ) / self.sigmaT + +# Stability with end-of-line comments between empty tuples and bin op +x = () - (# +) +x = ( + () + - () # +) +x = ( + () - () # +) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/delete.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/delete.py index c8ca7da9d70e1..a86c65dde8f04 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/delete.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/delete.py @@ -73,3 +73,7 @@ del ( # dangling end of line comment ) + +del ( # dangling end of line comment + # dangling own line comment +) # trailing statement comment diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 8acf77004400b..5d7e0c2c3f98f 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -253,6 +253,16 @@ impl<'ast> Format> for EmptyWithDanglingComments<'_> { self.opening, // end-of-line comments trailing_comments(&self.comments[..end_of_line_split]), + // Avoid unstable formatting with + // ```python + // x = () - (# + // ) + // ``` + // Without this the comment would go after the empty tuple first, but still expand + // the bin op. In the second formatting pass they are trailing bin op comments + // so the bin op collapse. Suboptimally we keep parentheses around the bin op in + // either case. + (!self.comments[..end_of_line_split].is_empty()).then_some(hard_line_break()), // own line comments, which need to be indented soft_block_indent(&dangling_comments(&self.comments[end_of_line_split..])), self.closing diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap index 61499e20e362e..172b318812e4c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__binary.py.snap @@ -217,6 +217,17 @@ for user_id in set(target_user_ids) - {u.user_id for u in updates}: log(self.price / self.strike) + (self.risk_free - self.div_cont + 0.5 * (self.sigma**2)) * self.exp_time ) / self.sigmaT + +# Stability with end-of-line comments between empty tuples and bin op +x = () - (# +) +x = ( + () + - () # +) +x = ( + () - () # +) ``` ## Output @@ -488,6 +499,19 @@ for user_id in set(target_user_ids) - {u.user_id for u in updates}: log(self.price / self.strike) + (self.risk_free - self.div_cont + 0.5 * (self.sigma**2)) * self.exp_time ) / self.sigmaT + +# Stability with end-of-line comments between empty tuples and bin op +x = ( + () + - ( # + ) +) +x = ( + () - () # +) +x = ( + () - () # +) ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap index 45bcc8a76ab86..86880bd396c5e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__call.py.snap @@ -213,7 +213,8 @@ f( a.very_long_function_function_that_is_so_long_that_it_expands_the_parent_but_its_only_a_single_argument() ) -f() # abc +f( # abc +) f( # abc # abc diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap index e27a63126d6f1..a5b6c5e896034 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__dict.py.snap @@ -129,7 +129,8 @@ a = { 3: True, } -x = {} # dangling end of line comment +x = { # dangling end of line comment +} ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap index 6937a6e5e160c..2d126b34084a2 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__lambda.py.snap @@ -165,7 +165,8 @@ a = ( # Regression test: lambda empty arguments ranges were too long, leading to unstable # formatting ( - lambda: (), # + lambda: ( # + ), ) diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap index 04f9c816e3229..ff883a8f45d2b 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__list.py.snap @@ -62,7 +62,8 @@ c1 = [ # trailing open bracket ```py # Dangling comment placement in empty lists # Regression test for https://github.com/python/cpython/blob/03160630319ca26dcbbad65225da4248e54c45ec/Tools/c-analyzer/c_analyzer/datafiles.py#L14-L16 -a1 = [] # a +a1 = [ # a +] a2 = [ # a # b ] @@ -93,7 +94,8 @@ c1 = [ # trailing open bracket ] # trailing close bracket -[] # end-of-line comment +[ # end-of-line comment +] [ # end-of-line comment # own-line comment diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap index 39a4afbc07410..ce4add0b9462c 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__delete.py.snap @@ -79,6 +79,10 @@ del ( del ( # dangling end of line comment ) + +del ( # dangling end of line comment + # dangling own line comment +) # trailing statement comment ``` ## Output @@ -215,7 +219,12 @@ del ( ) # Completed # Done -del () # dangling end of line comment +del ( # dangling end of line comment +) + +del ( # dangling end of line comment + # dangling own line comment +) # trailing statement comment ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap index 8a7676fce96f6..7f77fc37abe04 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__function.py.snap @@ -845,7 +845,8 @@ def f( # first ... -def f(): # first # second +def f( # first +): # second ... diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap index db7ed62b7382d..1c9746b12aa5d 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__raise.py.snap @@ -192,7 +192,8 @@ raise aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfk < ( ) # the other end # sneaky comment -raise () # another comment +raise ( # another comment +) raise () # what now