Handle pattern parentheses in FormatPattern (#6800)

## Summary

This PR fixes the duplicate-parenthesis problem that's visible in the
tests from https://github.com/astral-sh/ruff/pull/6799. The issue is
that we might have parentheses around the entire match-case pattern,
like in `(1)` here:

```python
match foo:
    case (1):
        y = 0
```

In this case, the inner expression (`1`) will _think_ it's
parenthesized, but we'll _also_ detect the parentheses at the case level
-- so they get rendered by the case, then again by the expression.
Instead, if we detect parentheses at the case level, we can force-off
the parentheses for the pattern using a design similar to the way we
handle parentheses on expressions.

Closes https://github.com/astral-sh/ruff/issues/6753.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2023-08-24 23:45:49 -04:00 committed by GitHub
parent 281ce56dc1
commit 6f23469e00
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 406 additions and 124 deletions

View file

@ -263,6 +263,7 @@ match foo:
):
y = 1
match foo:
case [1, 2, *rest]:
pass
@ -299,3 +300,47 @@ match foo:
_, 1, 2]:
pass
match foo:
case (1):
pass
case ((1)):
pass
case [(1), 2]:
pass
case [( # comment
1
), 2]:
pass
case [ # outer
( # inner
1
), 2]:
pass
case [
( # outer
[ # inner
1,
]
)
]:
pass
case [ # outer
( # inner outer
[ # inner
1,
]
)
]:
pass
case [ # outer
# own line
( # inner outer
[ # inner
1,
]
)
]:
pass
case [(*rest), (a as b)]:
pass

View file

@ -212,7 +212,6 @@ fn handle_enclosed_comment<'a>(
handle_leading_class_with_decorators_comment(comment, class_def)
}
AnyNodeRef::StmtImportFrom(import_from) => handle_import_from_comment(comment, import_from),
AnyNodeRef::MatchCase(match_case) => handle_match_case_comment(comment, match_case),
AnyNodeRef::StmtWith(with_) => handle_with_comment(comment, with_),
AnyNodeRef::ExprConstant(_) => {
if let Some(AnyNodeRef::ExprFString(fstring)) = comment.enclosing_parent() {
@ -1361,28 +1360,6 @@ fn handle_import_from_comment<'a>(
}
}
/// Attach an enclosed end-of-line comment to a [`MatchCase`].
///
/// For example, given:
/// ```python
/// case ( # comment
/// pattern
/// ):
/// ...
/// ```
///
/// The comment will be attached to the [`MatchCase`] node as a dangling comment.
fn handle_match_case_comment<'a>(
comment: DecoratedComment<'a>,
match_case: &'a MatchCase,
) -> CommentPlacement<'a> {
if comment.line_position().is_end_of_line() && comment.start() < match_case.pattern.start() {
CommentPlacement::dangling(comment.enclosing_node(), comment)
} else {
CommentPlacement::Default(comment)
}
}
/// Attach an enclosed end-of-line comment to a [`ast::StmtWith`].
///
/// For example, given:

View file

@ -1,13 +1,13 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::{MatchCase, Pattern, Ranged};
use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer};
use ruff_text_size::TextRange;
use ruff_python_ast::node::AstNode;
use ruff_python_ast::MatchCase;
use crate::builders::parenthesize_if_expands;
use crate::comments::SourceComment;
use crate::expression::parentheses::parenthesized;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses};
use crate::prelude::*;
use crate::statement::clause::{clause_body, clause_header, ClauseHeader};
use crate::{FormatError, FormatNodeRule, PyFormatter};
use crate::{FormatNodeRule, PyFormatter};
#[derive(Default)]
pub struct FormatMatchCase;
@ -21,13 +21,8 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
body,
} = item;
// Distinguish dangling comments that appear on the open parenthesis from those that
// appear on the trailing colon.
let comments = f.context().comments().clone();
let dangling_item_comments = comments.dangling(item);
let (open_parenthesis_comments, trailing_colon_comments) = dangling_item_comments.split_at(
dangling_item_comments.partition_point(|comment| comment.start() < pattern.start()),
);
write!(
f,
@ -38,12 +33,29 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
&format_with(|f| {
write!(f, [text("case"), space()])?;
if is_match_case_pattern_parenthesized(item, pattern, f.context())? {
parenthesized("(", &pattern.format(), ")")
.with_dangling_comments(open_parenthesis_comments)
.fmt(f)?;
let has_comments = comments.has_leading(pattern)
|| comments.has_trailing_own_line(pattern);
if has_comments {
pattern.format().with_options(Parentheses::Always).fmt(f)?;
} else {
pattern.format().fmt(f)?;
match pattern.needs_parentheses(item.as_any_node_ref(), f.context()) {
OptionalParentheses::Multiline => {
parenthesize_if_expands(
&pattern.format().with_options(Parentheses::Never),
)
.fmt(f)?;
}
OptionalParentheses::Always => {
pattern.format().with_options(Parentheses::Always).fmt(f)?;
}
OptionalParentheses::Never => {
pattern.format().with_options(Parentheses::Never).fmt(f)?;
}
OptionalParentheses::BestFit => {
pattern.format().with_options(Parentheses::Never).fmt(f)?;
}
}
}
if let Some(guard) = guard {
@ -53,7 +65,7 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
Ok(())
}),
),
clause_body(body, trailing_colon_comments),
clause_body(body, dangling_item_comments),
]
)
}
@ -67,33 +79,3 @@ impl FormatNodeRule<MatchCase> for FormatMatchCase {
Ok(())
}
}
fn is_match_case_pattern_parenthesized(
case: &MatchCase,
pattern: &Pattern,
context: &PyFormatContext,
) -> FormatResult<bool> {
let mut tokenizer = SimpleTokenizer::new(
context.source(),
TextRange::new(case.start(), pattern.start()),
)
.skip_trivia();
let case_keyword = tokenizer.next().ok_or(FormatError::syntax_error(
"Expected a `case` keyword, didn't find any token",
))?;
debug_assert_eq!(
case_keyword.kind(),
SimpleTokenKind::Case,
"Expected `case` keyword but at {case_keyword:?}"
);
match tokenizer.next() {
Some(left_paren) => {
debug_assert_eq!(left_paren.kind(), SimpleTokenKind::LParen);
Ok(true)
}
None => Ok(false),
}
}

View file

@ -1,6 +1,11 @@
use ruff_formatter::{FormatOwnedWithRule, FormatRefWithRule};
use ruff_python_ast::Pattern;
use ruff_formatter::{FormatOwnedWithRule, FormatRefWithRule, FormatRule, FormatRuleWithOptions};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{Pattern, Ranged};
use ruff_python_trivia::{first_non_trivia_token, SimpleToken, SimpleTokenKind, SimpleTokenizer};
use crate::expression::parentheses::{
parenthesized, NeedsParentheses, OptionalParentheses, Parentheses,
};
use crate::prelude::*;
pub(crate) mod pattern_match_as;
@ -12,20 +17,64 @@ pub(crate) mod pattern_match_singleton;
pub(crate) mod pattern_match_star;
pub(crate) mod pattern_match_value;
#[derive(Default)]
pub struct FormatPattern;
#[derive(Copy, Clone, PartialEq, Eq, Default)]
pub struct FormatPattern {
parentheses: Parentheses,
}
impl FormatRuleWithOptions<Pattern, PyFormatContext<'_>> for FormatPattern {
type Options = Parentheses;
fn with_options(mut self, options: Self::Options) -> Self {
self.parentheses = options;
self
}
}
impl FormatRule<Pattern, PyFormatContext<'_>> for FormatPattern {
fn fmt(&self, item: &Pattern, f: &mut PyFormatter) -> FormatResult<()> {
match item {
Pattern::MatchValue(p) => p.format().fmt(f),
Pattern::MatchSingleton(p) => p.format().fmt(f),
Pattern::MatchSequence(p) => p.format().fmt(f),
Pattern::MatchMapping(p) => p.format().fmt(f),
Pattern::MatchClass(p) => p.format().fmt(f),
Pattern::MatchStar(p) => p.format().fmt(f),
Pattern::MatchAs(p) => p.format().fmt(f),
Pattern::MatchOr(p) => p.format().fmt(f),
fn fmt(&self, pattern: &Pattern, f: &mut PyFormatter) -> FormatResult<()> {
let format_pattern = format_with(|f| match pattern {
Pattern::MatchValue(pattern) => pattern.format().fmt(f),
Pattern::MatchSingleton(pattern) => pattern.format().fmt(f),
Pattern::MatchSequence(pattern) => pattern.format().fmt(f),
Pattern::MatchMapping(pattern) => pattern.format().fmt(f),
Pattern::MatchClass(pattern) => pattern.format().fmt(f),
Pattern::MatchStar(pattern) => pattern.format().fmt(f),
Pattern::MatchAs(pattern) => pattern.format().fmt(f),
Pattern::MatchOr(pattern) => pattern.format().fmt(f),
});
let parenthesize = match self.parentheses {
Parentheses::Preserve => is_pattern_parenthesized(pattern, f.context().source()),
Parentheses::Always => true,
Parentheses::Never => false,
};
if parenthesize {
let comments = f.context().comments().clone();
// Any comments on the open parenthesis.
//
// For example, `# comment` in:
// ```python
// ( # comment
// 1
// )
// ```
let open_parenthesis_comment = comments
.leading(pattern)
.first()
.filter(|comment| comment.line_position().is_end_of_line());
parenthesized("(", &format_pattern, ")")
.with_dangling_comments(
open_parenthesis_comment
.map(std::slice::from_ref)
.unwrap_or_default(),
)
.fmt(f)
} else {
format_pattern.fmt(f)
}
}
}
@ -34,7 +83,7 @@ impl<'ast> AsFormat<PyFormatContext<'ast>> for Pattern {
type Format<'a> = FormatRefWithRule<'a, Pattern, FormatPattern, PyFormatContext<'ast>>;
fn format(&self) -> Self::Format<'_> {
FormatRefWithRule::new(self, FormatPattern)
FormatRefWithRule::new(self, FormatPattern::default())
}
}
@ -42,6 +91,49 @@ impl<'ast> IntoFormat<PyFormatContext<'ast>> for Pattern {
type Format = FormatOwnedWithRule<Pattern, FormatPattern, PyFormatContext<'ast>>;
fn into_format(self) -> Self::Format {
FormatOwnedWithRule::new(self, FormatPattern)
FormatOwnedWithRule::new(self, FormatPattern::default())
}
}
fn is_pattern_parenthesized(pattern: &Pattern, contents: &str) -> bool {
// First test if there's a closing parentheses because it tends to be cheaper.
if matches!(
first_non_trivia_token(pattern.end(), contents),
Some(SimpleToken {
kind: SimpleTokenKind::RParen,
..
})
) {
let mut tokenizer =
SimpleTokenizer::up_to_without_back_comment(pattern.start(), contents).skip_trivia();
matches!(
tokenizer.next_back(),
Some(SimpleToken {
kind: SimpleTokenKind::LParen,
..
})
)
} else {
false
}
}
impl NeedsParentheses for Pattern {
fn needs_parentheses(
&self,
parent: AnyNodeRef,
context: &PyFormatContext,
) -> OptionalParentheses {
match self {
Pattern::MatchValue(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchSingleton(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchSequence(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchMapping(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchClass(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchStar(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchAs(pattern) => pattern.needs_parentheses(parent, context),
Pattern::MatchOr(pattern) => pattern.needs_parentheses(parent, context),
}
}
}

View file

@ -1,8 +1,9 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::{Pattern, PatternMatchAs};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchAs;
use crate::comments::{dangling_comments, SourceComment};
use crate::expression::parentheses::parenthesized;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::{FormatNodeRule, PyFormatter};
@ -21,18 +22,7 @@ impl FormatNodeRule<PatternMatchAs> for FormatPatternMatchAs {
if let Some(name) = name {
if let Some(pattern) = pattern {
// Parenthesize nested `PatternMatchAs` like `(a as b) as c`.
if matches!(
pattern.as_ref(),
Pattern::MatchAs(PatternMatchAs {
pattern: Some(_),
..
})
) {
parenthesized("(", &pattern.format(), ")").fmt(f)?;
} else {
pattern.format().fmt(f)?;
}
pattern.format().fmt(f)?;
if comments.has_trailing(pattern.as_ref()) {
write!(f, [hard_line_break()])?;
@ -68,3 +58,13 @@ impl FormatNodeRule<PatternMatchAs> for FormatPatternMatchAs {
Ok(())
}
}
impl NeedsParentheses for PatternMatchAs {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Multiline
}
}

View file

@ -1,6 +1,9 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchClass;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
#[derive(Default)]
@ -17,3 +20,13 @@ impl FormatNodeRule<PatternMatchClass> for FormatPatternMatchClass {
)
}
}
impl NeedsParentheses for PatternMatchClass {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -1,6 +1,9 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchMapping;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
#[derive(Default)]
@ -17,3 +20,13 @@ impl FormatNodeRule<PatternMatchMapping> for FormatPatternMatchMapping {
)
}
}
impl NeedsParentheses for PatternMatchMapping {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -1,6 +1,9 @@
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchOr;
use crate::context::PyFormatContext;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::{not_yet_implemented_custom_text, FormatNodeRule, PyFormatter};
#[derive(Default)]
@ -17,3 +20,13 @@ impl FormatNodeRule<PatternMatchOr> for FormatPatternMatchOr {
)
}
}
impl NeedsParentheses for PatternMatchOr {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Multiline
}
}

View file

@ -1,9 +1,13 @@
use ruff_formatter::prelude::format_with;
use ruff_formatter::{Format, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchSequence;
use crate::builders::PyFormatterExtensions;
use crate::expression::parentheses::{empty_parenthesized, optional_parentheses, parenthesized};
use crate::context::PyFormatContext;
use crate::expression::parentheses::{
empty_parenthesized, optional_parentheses, parenthesized, NeedsParentheses, OptionalParentheses,
};
use crate::{FormatNodeRule, PyFormatter};
#[derive(Default)]
@ -51,3 +55,13 @@ impl FormatNodeRule<PatternMatchSequence> for FormatPatternMatchSequence {
}
}
}
impl NeedsParentheses for PatternMatchSequence {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -1,6 +1,8 @@
use crate::prelude::*;
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::{Constant, PatternMatchSingleton};
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::{FormatNodeRule, PyFormatter};
#[derive(Default)]
@ -16,3 +18,13 @@ impl FormatNodeRule<PatternMatchSingleton> for FormatPatternMatchSingleton {
}
}
}
impl NeedsParentheses for PatternMatchSingleton {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -1,9 +1,10 @@
use ruff_formatter::{prelude::text, write, Buffer, FormatResult};
use ruff_formatter::{write, Buffer, FormatResult};
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchStar;
use crate::comments::{dangling_comments, SourceComment};
use crate::AsFormat;
use crate::{FormatNodeRule, PyFormatter};
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;
#[derive(Default)]
pub struct FormatPatternMatchStar;
@ -32,3 +33,13 @@ impl FormatNodeRule<PatternMatchStar> for FormatPatternMatchStar {
Ok(())
}
}
impl NeedsParentheses for PatternMatchStar {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -1,14 +1,26 @@
use ruff_python_ast::node::AnyNodeRef;
use ruff_python_ast::PatternMatchValue;
use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses, Parentheses};
use crate::prelude::*;
use crate::{AsFormat, FormatNodeRule, PyFormatter};
#[derive(Default)]
pub struct FormatPatternMatchValue;
impl FormatNodeRule<PatternMatchValue> for FormatPatternMatchValue {
fn fmt_fields(&self, item: &PatternMatchValue, f: &mut PyFormatter) -> FormatResult<()> {
// TODO(charlie): Avoid double parentheses for parenthesized top-level `PatternMatchValue`.
let PatternMatchValue { value, range: _ } = item;
value.format().fmt(f)
value.format().with_options(Parentheses::Never).fmt(f)
}
}
impl NeedsParentheses for PatternMatchValue {
fn needs_parentheses(
&self,
_parent: AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}

View file

@ -231,7 +231,7 @@ match bar1:
pass
- case 4 as d, (5 as e), (6 | 7 as g), *h:
+ case 4 as d, 5 as e, NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g, *h:
+ case 4 as d, (5 as e), (NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g), *h:
pass
@ -358,7 +358,7 @@ match something:
case 2 as b, 3 as c:
pass
case 4 as d, 5 as e, NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g, *h:
case 4 as d, (5 as e), (NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as g), *h:
pass

View file

@ -117,13 +117,13 @@ def where_is(point):
match command.split():
- case ["go", ("north" | "south" | "east" | "west")]:
+ case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf | (y)]:
+ case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf | (y))]:
current_room = current_room.neighbor(...)
# how do I know which direction to go?
match command.split():
- case ["go", ("north" | "south" | "east" | "west") as direction]:
+ case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as direction]:
+ case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf | (y)) as direction]:
current_room = current_room.neighbor(direction)
match command.split():
@ -223,12 +223,12 @@ match command.split():
... # Code for picking up the given object
match command.split():
case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf | (y)]:
case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf | (y))]:
current_room = current_room.neighbor(...)
# how do I know which direction to go?
match command.split():
case ["go", NOT_YET_IMPLEMENTED_PatternMatchOf | (y) as direction]:
case ["go", (NOT_YET_IMPLEMENTED_PatternMatchOf | (y)) as direction]:
current_room = current_room.neighbor(direction)
match command.split():

View file

@ -269,6 +269,7 @@ match foo:
):
y = 1
match foo:
case [1, 2, *rest]:
pass
@ -305,6 +306,50 @@ match foo:
_, 1, 2]:
pass
match foo:
case (1):
pass
case ((1)):
pass
case [(1), 2]:
pass
case [( # comment
1
), 2]:
pass
case [ # outer
( # inner
1
), 2]:
pass
case [
( # outer
[ # inner
1,
]
)
]:
pass
case [ # outer
( # inner outer
[ # inner
1,
]
)
]:
pass
case [ # outer
# own line
( # inner outer
[ # inner
1,
]
)
]:
pass
case [(*rest), (a as b)]:
pass
```
## Output
@ -440,7 +485,7 @@ match pattern_comments:
match pattern_comments:
case (no_comments):
case no_comments:
pass
@ -504,9 +549,7 @@ match pattern_singleton:
# trailing own 2
):
pass
case (
True # trailing
):
case True: # trailing
...
case False:
...
@ -524,7 +567,7 @@ match foo:
pass
case ["a", "b"]:
pass
case (["a", "b"]):
case ["a", "b"]:
pass
@ -545,29 +588,28 @@ match foo:
match foo:
case 1:
y = 0
case ((1)):
case 1:
y = 1
case (("a")):
case "a":
y = 1
case ( # comment
(1)
1
):
y = 1
case (
# comment
(1)
1
):
y = 1
case (
(1) # comment
):
case 1: # comment
y = 1
case (
(1)
1
# comment
):
y = 1
match foo:
case [1, 2, *rest]:
pass
@ -627,6 +669,62 @@ match foo:
2,
]:
pass
match foo:
case 1:
pass
case 1:
pass
case [(1), 2]:
pass
case [
( # comment
1
),
2,
]:
pass
case [
( # outer
# inner
1
),
2,
]:
pass
case [
( # outer
[
# inner
1,
]
)
]:
pass
case [
( # outer
# inner outer
[
# inner
1,
]
)
]:
pass
case [
( # outer
# own line
# inner outer
[
# inner
1,
]
)
]:
pass
case [(*rest), (a as b)]:
pass
```