Skip to content

Commit 4bce801

Browse files
authored
Fix unstable with-items formatting (#10274)
## Summary Fixes #10267 The issue with the current formatting is that the formatter flips between the `SingleParenthesizedContextManager` and `ParenthesizeIfExpands` or `SingleWithTarget` because the layouts use incompatible formatting ( `SingleParenthesizedContextManager`: `maybe_parenthesize_expression(context)` vs `ParenthesizeIfExpands`: `parenthesize_if_expands(item)`, `SingleWithTarget`: `optional_parentheses(item)`. The fix is to ensure that the layouts between which the formatter flips when adding or removing parentheses are the same. I do this by introducing a new `SingleWithoutTarget` layout that uses the same formatting as `SingleParenthesizedContextManager` if it has no target and prefer `SingleWithoutTarget` over using `ParenthesizeIfExpands` or `SingleWithTarget`. ## Formatting change The downside is that we now use `maybe_parenthesize_expression` over `parenthesize_if_expands` for expressions where `can_omit_optional_parentheses` returns `false`. This can lead to stable formatting changes. I only found one formatting change in our ecosystem check and, unfortunately, this is necessary to fix the instability (and instability fixes are okay to have as part of minor changes according to our versioning policy) The benefit of the change is that `with` items with a single context manager and without a target are now formatted identically to how the same expression would be formatted in other clause headers. ## Test Plan I ran the ecosystem check locally
1 parent a56d42f commit 4bce801

File tree

6 files changed

+111
-12
lines changed

6 files changed

+111
-12
lines changed

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py

+11
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,17 @@
304304
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext():
305305
pass
306306

307+
if True:
308+
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as c:
309+
pass
307310

308311
with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(aaaaa, bbbbbbbbbb, ddddddddddddd):
309312
pass
313+
314+
# Regression test for https://github.com/astral-sh/ruff/issues/10267
315+
with (
316+
open(
317+
"/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization
318+
)
319+
):
320+
pass

crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with_39.py

+5-1
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,8 @@
8585
with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c):
8686
pass
8787

88-
88+
with ( # outer comment
89+
CtxManager1(),
90+
CtxManager2(),
91+
):
92+
pass

crates/ruff_python_formatter/src/other/with_item.rs

+19-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,23 @@ pub enum WithItemLayout {
2222
/// This layout is used independent of the target version.
2323
SingleParenthesizedContextManager,
2424

25+
/// A with item that is the `with`s only context manager and it has no `target`.
26+
///
27+
/// ```python
28+
/// with a + b:
29+
/// ...
30+
/// ```
31+
///
32+
/// In this case, use [`maybe_parenthesize_expression`] to get the same formatting as when
33+
/// formatting any other statement with a clause header.
34+
///
35+
/// This layout is only used for Python 3.9+.
36+
///
37+
/// Be careful that [`Self::SingleParenthesizedContextManager`] and this layout are compatible because
38+
/// removing optional parentheses or adding parentheses will make the formatter pick the opposite layout
39+
/// the second time the file gets formatted.
40+
SingleWithoutTarget,
41+
2542
/// This layout is used when the target python version doesn't support parenthesized context managers and
2643
/// it's either a single, unparenthesized with item or multiple items.
2744
///
@@ -106,7 +123,8 @@ impl FormatNodeRule<WithItem> for FormatWithItem {
106123
}
107124
}
108125

109-
WithItemLayout::SingleParenthesizedContextManager => {
126+
WithItemLayout::SingleParenthesizedContextManager
127+
| WithItemLayout::SingleWithoutTarget => {
110128
write!(
111129
f,
112130
[maybe_parenthesize_expression(

crates/ruff_python_formatter/src/statement/stmt_with.rs

+30-3
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,11 @@ impl FormatNodeRule<StmtWith> for FormatStmtWith {
7373
optional_parentheses(&single.format()).fmt(f)
7474
}
7575

76+
WithItemsLayout::SingleWithoutTarget(single) => single
77+
.format()
78+
.with_options(WithItemLayout::SingleWithoutTarget)
79+
.fmt(f),
80+
7681
WithItemsLayout::SingleParenthesizedContextManager(single) => single
7782
.format()
7883
.with_options(WithItemLayout::SingleParenthesizedContextManager)
@@ -150,15 +155,35 @@ enum WithItemsLayout<'a> {
150155
///
151156
/// In this case, prefer keeping the parentheses around the context expression instead of parenthesizing the entire
152157
/// with item.
158+
///
159+
/// Ensure that this layout is compatible with [`Self::SingleWithoutTarget`] because removing the parentheses
160+
/// results in the formatter taking that layout when formatting the file again
153161
SingleParenthesizedContextManager(&'a WithItem),
154162

163+
/// The with statement's only item has no target.
164+
///
165+
/// ```python
166+
/// with a + b:
167+
/// ...
168+
/// ```
169+
///
170+
/// In this case, use [`maybe_parenthesize_expression`] to format the context expression
171+
/// to get the exact same formatting as when formatting an expression in any other clause header.
172+
///
173+
/// Only used for Python 3.9+
174+
///
175+
/// Be careful that [`Self::SingleParenthesizedContextManager`] and this layout are compatible because
176+
/// adding parentheses around a [`WithItem`] will result in the context expression being parenthesized in
177+
/// the next formatting pass.
178+
SingleWithoutTarget(&'a WithItem),
179+
155180
/// It's a single with item with a target. Use the optional parentheses layout (see [`optional_parentheses`])
156181
/// to mimic the `maybe_parenthesize_expression` behavior.
157182
///
158183
/// ```python
159184
/// with (
160-
/// a + b
161-
/// ) as b:
185+
/// a + b as b
186+
/// ):
162187
/// ...
163188
/// ```
164189
///
@@ -275,7 +300,9 @@ impl<'a> WithItemsLayout<'a> {
275300

276301
Ok(match with.items.as_slice() {
277302
[single] => {
278-
if can_omit_optional_parentheses(&single.context_expr, context) {
303+
if single.optional_vars.is_none() {
304+
Self::SingleWithoutTarget(single)
305+
} else if can_omit_optional_parentheses(&single.context_expr, context) {
279306
Self::SingleWithTarget(single)
280307
} else {
281308
Self::ParenthesizeIfExpands

crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap

+35-3
Original file line numberDiff line numberDiff line change
@@ -310,9 +310,20 @@ if True:
310310
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext():
311311
pass
312312
313+
if True:
314+
with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as c:
315+
pass
313316
314317
with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(aaaaa, bbbbbbbbbb, ddddddddddddd):
315318
pass
319+
320+
# Regression test for https://github.com/astral-sh/ruff/issues/10267
321+
with (
322+
open(
323+
"/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization
324+
)
325+
):
326+
pass
316327
```
317328

318329
## Outputs
@@ -661,11 +672,22 @@ if True:
661672
) if get_running_loop() else contextlib.nullcontext():
662673
pass
663674
675+
if True:
676+
with anyio.CancelScope(
677+
shield=True
678+
) if get_running_loop() else contextlib.nullcontext() as c:
679+
pass
664680
665681
with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(
666682
aaaaa, bbbbbbbbbb, ddddddddddddd
667683
):
668684
pass
685+
686+
# Regression test for https://github.com/astral-sh/ruff/issues/10267
687+
with open(
688+
"/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization
689+
):
690+
pass
669691
```
670692

671693

@@ -1052,13 +1074,23 @@ if True:
10521074
):
10531075
pass
10541076
1077+
if True:
1078+
with (
1079+
anyio.CancelScope(shield=True)
1080+
if get_running_loop()
1081+
else contextlib.nullcontext() as c
1082+
):
1083+
pass
10551084
10561085
with (
10571086
Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc),
10581087
Document(aaaaa, bbbbbbbbbb, ddddddddddddd),
10591088
):
10601089
pass
1061-
```
1062-
1063-
10641090
1091+
# Regression test for https://github.com/astral-sh/ruff/issues/10267
1092+
with open(
1093+
"/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization
1094+
):
1095+
pass
1096+
```

crates/ruff_python_formatter/tests/snapshots/format@statement__with_39.py.snap

+11-4
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,11 @@ if True:
9191
with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c):
9292
pass
9393
94-
94+
with ( # outer comment
95+
CtxManager1(),
96+
CtxManager2(),
97+
):
98+
pass
9599
```
96100

97101
## Outputs
@@ -217,7 +221,10 @@ with (
217221
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c
218222
):
219223
pass
220-
```
221-
222-
223224
225+
with ( # outer comment
226+
CtxManager1(),
227+
CtxManager2(),
228+
):
229+
pass
230+
```

0 commit comments

Comments
 (0)