Skip to content

Commit

Permalink
Fix zulip unstable formatting with end-of-line comments (#6386)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
konstin committed Aug 8, 2023
1 parent 2bd3453 commit 90ba40c
Show file tree
Hide file tree
Showing 11 changed files with 73 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
() - () #
)
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,7 @@

del ( # dangling end of line comment
)

del ( # dangling end of line comment
# dangling own line comment
) # trailing statement comment
10 changes: 10 additions & 0 deletions crates/ruff_python_formatter/src/builders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,16 @@ impl<'ast> Format<PyFormatContext<'ast>> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 = (
() - () #
)
```


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ a = {
3: True,
}
x = {} # dangling end of line comment
x = { # dangling end of line comment
}
```


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ a = (
# Regression test: lambda empty arguments ranges were too long, leading to unstable
# formatting
(
lambda: (), #
lambda: ( #
),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
]
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
```


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -845,7 +845,8 @@ def f( # first
...


def f(): # first # second
def f( # first
): # second
...


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ raise aksjdhflsakhdflkjsadlfajkslhfdkjsaldajlahflashdfljahlfk < (
) # the other end
# sneaky comment
raise () # another comment
raise ( # another comment
)
raise () # what now
Expand Down

0 comments on commit 90ba40c

Please sign in to comment.