From 63b1c1ea8bd2b0b08661167c20492d2cb6e2889c Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Mon, 3 Nov 2025 17:06:52 -0500 Subject: [PATCH] Avoid extra parentheses for long `match` patterns with `as` captures (#21176) Summary -- This PR fixes #17796 by taking the approach mentioned in https://github.com/astral-sh/ruff/issues/17796#issuecomment-2847943862 of simply recursing into the `MatchAs` patterns when checking if we need parentheses. This allows us to reuse the parentheses in the inner pattern before also breaking the `MatchAs` pattern itself: ```diff match class_pattern: case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: pass - case ( - Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture - ): + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: pass - case ( - Class( - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - ) as capture - ): + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: pass case ( Class( @@ -685,13 +683,11 @@ match sequence_pattern_brackets: case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: pass - case ( - [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture - ): + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: pass - case ( - [ - xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx - ] as capture - ): + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: pass ``` I haven't really resolved the question of whether or not it's okay always to recurse, but I'm hoping the ecosystem check on this PR might shed some light on that. Test Plan -- New tests based on the issue and then reviewing the ecosystem check here --- .../test/fixtures/ruff/statement/match.py | 55 +++++ .../ruff_python_formatter/src/pattern/mod.rs | 20 +- crates/ruff_python_formatter/src/preview.rs | 9 + .../snapshots/format@statement__match.py.snap | 227 +++++++++++++++++- 4 files changed, 307 insertions(+), 4 deletions(-) diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py index 4067d508c0..4a403b1541 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py @@ -613,3 +613,58 @@ match guard_comments: ): pass + +# regression tests from https://github.com/astral-sh/ruff/issues/17796 +match class_pattern: + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture: + pass + +match sequence_pattern_brackets: + case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: + pass + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: + pass + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: + pass + + +match class_pattern: + # 1 + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: # 2 + # 3 + pass # 4 + # 5 + case Class( # 6 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 7 + ) as capture: # 8 + pass + case Class( # 9 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 10 + ) as capture: # 11 + pass + case Class( # 12 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 13 + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture: # 14 + pass + case Class( # 0 + # 1 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 2 + # 3 + ) as capture: + pass diff --git a/crates/ruff_python_formatter/src/pattern/mod.rs b/crates/ruff_python_formatter/src/pattern/mod.rs index 557337ddc5..a379aeb849 100644 --- a/crates/ruff_python_formatter/src/pattern/mod.rs +++ b/crates/ruff_python_formatter/src/pattern/mod.rs @@ -1,5 +1,5 @@ use ruff_formatter::{FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions}; -use ruff_python_ast::{AnyNodeRef, Expr}; +use ruff_python_ast::{AnyNodeRef, Expr, PatternMatchAs}; use ruff_python_ast::{MatchCase, Pattern}; use ruff_python_trivia::CommentRanges; use ruff_python_trivia::{ @@ -14,6 +14,7 @@ use crate::expression::parentheses::{ NeedsParentheses, OptionalParentheses, Parentheses, optional_parentheses, parenthesized, }; use crate::prelude::*; +use crate::preview::is_avoid_parens_for_long_as_captures_enabled; pub(crate) mod pattern_arguments; pub(crate) mod pattern_keyword; @@ -242,8 +243,14 @@ pub(crate) fn can_pattern_omit_optional_parentheses( Pattern::MatchValue(_) | Pattern::MatchSingleton(_) | Pattern::MatchStar(_) - | Pattern::MatchAs(_) | Pattern::MatchOr(_) => false, + Pattern::MatchAs(PatternMatchAs { pattern, .. }) => match pattern { + Some(pattern) => { + is_avoid_parens_for_long_as_captures_enabled(context) + && has_parentheses_and_is_non_empty(pattern, context) + } + None => false, + }, Pattern::MatchSequence(sequence) => { !sequence.patterns.is_empty() || context.comments().has_dangling(pattern) } @@ -318,7 +325,14 @@ impl<'a> CanOmitOptionalParenthesesVisitor<'a> { // The pattern doesn't start with a parentheses pattern, but with the class's identifier. self.first.set_if_none(First::Token); } - Pattern::MatchStar(_) | Pattern::MatchSingleton(_) | Pattern::MatchAs(_) => {} + Pattern::MatchAs(PatternMatchAs { pattern, .. }) => { + if let Some(pattern) = pattern + && is_avoid_parens_for_long_as_captures_enabled(context) + { + self.visit_sub_pattern(pattern, context); + } + } + Pattern::MatchStar(_) | Pattern::MatchSingleton(_) => {} Pattern::MatchOr(or_pattern) => { self.update_max_precedence( OperatorPrecedence::Or, diff --git a/crates/ruff_python_formatter/src/preview.rs b/crates/ruff_python_formatter/src/preview.rs index 5455fa9a12..9d307390d6 100644 --- a/crates/ruff_python_formatter/src/preview.rs +++ b/crates/ruff_python_formatter/src/preview.rs @@ -43,3 +43,12 @@ pub(crate) const fn is_remove_parens_around_except_types_enabled( pub(crate) const fn is_allow_newline_after_block_open_enabled(context: &PyFormatContext) -> bool { context.is_preview() } + +/// Returns `true` if the +/// [`avoid_parens_for_long_as_captures`](https://github.com/astral-sh/ruff/pull/21176) preview +/// style is enabled. +pub(crate) const fn is_avoid_parens_for_long_as_captures_enabled( + context: &PyFormatContext, +) -> bool { + context.is_preview() +} diff --git a/crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap index 852740fa6d..a94ee5e636 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@statement__match.py.snap @@ -1,7 +1,6 @@ --- source: crates/ruff_python_formatter/tests/fixtures.rs input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/match.py -snapshot_kind: text --- ## Input ```python @@ -620,6 +619,61 @@ match guard_comments: ): pass + +# regression tests from https://github.com/astral-sh/ruff/issues/17796 +match class_pattern: + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture: + pass + case Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture: + pass + +match sequence_pattern_brackets: + case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: + pass + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: + pass + case [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture: + pass + + +match class_pattern: + # 1 + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: # 2 + # 3 + pass # 4 + # 5 + case Class( # 6 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 7 + ) as capture: # 8 + pass + case Class( # 9 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 10 + ) as capture: # 11 + pass + case Class( # 12 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 13 + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture: # 14 + pass + case Class( # 0 + # 1 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 2 + # 3 + ) as capture: + pass ``` ## Output @@ -1285,4 +1339,175 @@ match guard_comments: # trailing own line comment ): pass + + +# regression tests from https://github.com/astral-sh/ruff/issues/17796 +match class_pattern: + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: + pass + case ( + Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture + ): + pass + case ( + Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as capture + ): + pass + case ( + Class( + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture + ): + pass + +match sequence_pattern_brackets: + case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: + pass + case ( + [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture + ): + pass + case ( + [ + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx + ] as capture + ): + pass + + +match class_pattern: + # 1 + case ( + Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture + ): # 2 + # 3 + pass # 4 + # 5 + case ( + Class( # 6 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 7 + ) as capture + ): # 8 + pass + case ( + Class( # 9 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 10 + ) as capture + ): # 11 + pass + case ( + Class( # 12 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 13 + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture + ): # 14 + pass + case ( + Class( # 0 + # 1 + xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 2 + # 3 + ) as capture + ): + pass +``` + + +## Preview changes +```diff +--- Stable ++++ Preview +@@ -665,15 +665,13 @@ + match class_pattern: + case Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture: + pass +- case ( +- Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture +- ): ++ case Class( ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ++ ) as capture: + pass +- case ( +- Class( +- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx +- ) as capture +- ): ++ case Class( ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ++ ) as capture: + pass + case ( + Class( +@@ -685,37 +683,31 @@ + match sequence_pattern_brackets: + case [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture: + pass +- case ( +- [xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] as capture +- ): ++ case [ ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ++ ] as capture: + pass +- case ( +- [ +- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx +- ] as capture +- ): ++ case [ ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ++ ] as capture: + pass + + + match class_pattern: + # 1 +- case ( +- Class(xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) as capture +- ): # 2 ++ case Class( ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx ++ ) as capture: # 2 + # 3 + pass # 4 + # 5 +- case ( +- Class( # 6 +- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 7 +- ) as capture +- ): # 8 ++ case Class( # 6 ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 7 ++ ) as capture: # 8 + pass +- case ( +- Class( # 9 +- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 10 +- ) as capture +- ): # 11 ++ case Class( # 9 ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 10 ++ ) as capture: # 11 + pass + case ( + Class( # 12 +@@ -723,11 +715,9 @@ + ) as really_really_really_really_really_really_really_really_really_really_really_really_long_capture + ): # 14 + pass +- case ( +- Class( # 0 +- # 1 +- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 2 +- # 3 +- ) as capture +- ): ++ case Class( # 0 ++ # 1 ++ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx # 2 ++ # 3 ++ ) as capture: + pass ```