From 4bce80106545af8d1edd028c7085a9e9d703fc6b Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 9 Mar 2024 00:48:47 +0100 Subject: [PATCH] Fix unstable with-items formatting (#10274) ## Summary Fixes https://github.com/astral-sh/ruff/issues/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 --- .../test/fixtures/ruff/statement/with.py | 11 ++++++ .../test/fixtures/ruff/statement/with_39.py | 6 ++- .../src/other/with_item.rs | 20 +++++++++- .../src/statement/stmt_with.rs | 33 ++++++++++++++-- .../snapshots/format@statement__with.py.snap | 38 +++++++++++++++++-- .../format@statement__with_39.py.snap | 15 ++++++-- 6 files changed, 111 insertions(+), 12 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py index b222747733fe9..5b92ed3f7f766 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with.py @@ -304,6 +304,17 @@ with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext(): pass +if True: + with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as c: + pass with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(aaaaa, bbbbbbbbbb, ddddddddddddd): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/10267 +with ( + open( + "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization + ) +): + pass diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with_39.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with_39.py index e1b2da74e4ea0..0e6e384fa3b09 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with_39.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/with_39.py @@ -85,4 +85,8 @@ with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c): pass - +with ( # outer comment + CtxManager1(), + CtxManager2(), +): + pass diff --git a/crates/ruff_python_formatter/src/other/with_item.rs b/crates/ruff_python_formatter/src/other/with_item.rs index 38088e3b78cda..fdd4cb54c855d 100644 --- a/crates/ruff_python_formatter/src/other/with_item.rs +++ b/crates/ruff_python_formatter/src/other/with_item.rs @@ -22,6 +22,23 @@ pub enum WithItemLayout { /// This layout is used independent of the target version. SingleParenthesizedContextManager, + /// A with item that is the `with`s only context manager and it has no `target`. + /// + /// ```python + /// with a + b: + /// ... + /// ``` + /// + /// In this case, use [`maybe_parenthesize_expression`] to get the same formatting as when + /// formatting any other statement with a clause header. + /// + /// This layout is only used for Python 3.9+. + /// + /// Be careful that [`Self::SingleParenthesizedContextManager`] and this layout are compatible because + /// removing optional parentheses or adding parentheses will make the formatter pick the opposite layout + /// the second time the file gets formatted. + SingleWithoutTarget, + /// This layout is used when the target python version doesn't support parenthesized context managers and /// it's either a single, unparenthesized with item or multiple items. /// @@ -106,7 +123,8 @@ impl FormatNodeRule for FormatWithItem { } } - WithItemLayout::SingleParenthesizedContextManager => { + WithItemLayout::SingleParenthesizedContextManager + | WithItemLayout::SingleWithoutTarget => { write!( f, [maybe_parenthesize_expression( diff --git a/crates/ruff_python_formatter/src/statement/stmt_with.rs b/crates/ruff_python_formatter/src/statement/stmt_with.rs index 696e7a0e0acbe..00d2fe31c4602 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_with.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_with.rs @@ -73,6 +73,11 @@ impl FormatNodeRule for FormatStmtWith { optional_parentheses(&single.format()).fmt(f) } + WithItemsLayout::SingleWithoutTarget(single) => single + .format() + .with_options(WithItemLayout::SingleWithoutTarget) + .fmt(f), + WithItemsLayout::SingleParenthesizedContextManager(single) => single .format() .with_options(WithItemLayout::SingleParenthesizedContextManager) @@ -150,15 +155,35 @@ enum WithItemsLayout<'a> { /// /// In this case, prefer keeping the parentheses around the context expression instead of parenthesizing the entire /// with item. + /// + /// Ensure that this layout is compatible with [`Self::SingleWithoutTarget`] because removing the parentheses + /// results in the formatter taking that layout when formatting the file again SingleParenthesizedContextManager(&'a WithItem), + /// The with statement's only item has no target. + /// + /// ```python + /// with a + b: + /// ... + /// ``` + /// + /// In this case, use [`maybe_parenthesize_expression`] to format the context expression + /// to get the exact same formatting as when formatting an expression in any other clause header. + /// + /// Only used for Python 3.9+ + /// + /// Be careful that [`Self::SingleParenthesizedContextManager`] and this layout are compatible because + /// adding parentheses around a [`WithItem`] will result in the context expression being parenthesized in + /// the next formatting pass. + SingleWithoutTarget(&'a WithItem), + /// It's a single with item with a target. Use the optional parentheses layout (see [`optional_parentheses`]) /// to mimic the `maybe_parenthesize_expression` behavior. /// /// ```python /// with ( - /// a + b - /// ) as b: + /// a + b as b + /// ): /// ... /// ``` /// @@ -275,7 +300,9 @@ impl<'a> WithItemsLayout<'a> { Ok(match with.items.as_slice() { [single] => { - if can_omit_optional_parentheses(&single.context_expr, context) { + if single.optional_vars.is_none() { + Self::SingleWithoutTarget(single) + } else if can_omit_optional_parentheses(&single.context_expr, context) { Self::SingleWithTarget(single) } else { Self::ParenthesizeIfExpands diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap index fb2a0b081c264..4b3dc56389a95 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__with.py.snap @@ -310,9 +310,20 @@ if True: with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext(): pass +if True: + with anyio.CancelScope(shield=True) if get_running_loop() else contextlib.nullcontext() as c: + pass with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(aaaaa, bbbbbbbbbb, ddddddddddddd): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/10267 +with ( + open( + "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization + ) +): + pass ``` ## Outputs @@ -661,11 +672,22 @@ if True: ) if get_running_loop() else contextlib.nullcontext(): pass +if True: + with anyio.CancelScope( + shield=True + ) if get_running_loop() else contextlib.nullcontext() as c: + pass with Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document( aaaaa, bbbbbbbbbb, ddddddddddddd ): pass + +# Regression test for https://github.com/astral-sh/ruff/issues/10267 +with open( + "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization +): + pass ``` @@ -1052,13 +1074,23 @@ if True: ): pass +if True: + with ( + anyio.CancelScope(shield=True) + if get_running_loop() + else contextlib.nullcontext() as c + ): + pass with ( Child(aaaaaaaaa, bbbbbbbbbbbbbbb, cccccc), Document(aaaaa, bbbbbbbbbb, ddddddddddddd), ): pass -``` - - +# Regression test for https://github.com/astral-sh/ruff/issues/10267 +with open( + "/etc/hosts" # This is an incredibly long comment that has been replaced for sanitization +): + pass +``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__with_39.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__with_39.py.snap index 5319f46100e14..7dd1f089bbf05 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__with_39.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__with_39.py.snap @@ -91,7 +91,11 @@ if True: with (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c): pass - +with ( # outer comment + CtxManager1(), + CtxManager2(), +): + pass ``` ## Outputs @@ -217,7 +221,10 @@ with ( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb as c ): pass -``` - - +with ( # outer comment + CtxManager1(), + CtxManager2(), +): + pass +```