From a56d42f1839cb9b44f0fc369c441f4d329ed868a Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 9 Mar 2024 00:40:39 +0100 Subject: [PATCH] Refactor with statement formatting to have explicit layouts (#10296) ## Summary This PR refactors the with item formatting to use more explicit layouts to make it easier to understand the different formatting cases. The benefit of the explicit layout is that it makes it easier to reasons about layout transition between format runs. For example, today it's possible that `SingleWithTarget` or `ParenthesizeIfExpands` add parentheses around the with items for `with aaaaaaaaaa + bbbbbbbbbbbb: pass`, resulting in `with (aaaaaaaaaa + bbbbbbbbbbbb): pass`. The problem with this is that the next formatting pass uses the `SingleParenthesizedContextExpression` layout that uses `maybe_parenthesize_expression` which is different from `parenthesize_if_expands(&expr)` or `optional_parentheses(&expr)`. ## Test Plan `cargo test` I ran the ecosystem checks locally and there are no changes. --- crates/ruff_python_formatter/src/builders.rs | 1 - .../src/other/arguments.rs | 6 +- .../ruff_python_formatter/src/other/commas.rs | 11 +- .../src/other/with_item.rs | 138 ++++++-- .../src/statement/stmt_with.rs | 311 +++++++++++------- 5 files changed, 303 insertions(+), 164 deletions(-) diff --git a/crates/ruff_python_formatter/src/builders.rs b/crates/ruff_python_formatter/src/builders.rs index 295d3b91edfd8..3abdedd860099 100644 --- a/crates/ruff_python_formatter/src/builders.rs +++ b/crates/ruff_python_formatter/src/builders.rs @@ -219,7 +219,6 @@ impl<'fmt, 'ast, 'buf> JoinCommaSeparatedBuilder<'fmt, 'ast, 'buf> { if let Some(last_end) = self.entries.position() { let magic_trailing_comma = has_magic_trailing_comma( TextRange::new(last_end, self.sequence_end), - self.fmt.options(), self.fmt.context(), ); diff --git a/crates/ruff_python_formatter/src/other/arguments.rs b/crates/ruff_python_formatter/src/other/arguments.rs index 62871cdf31b2d..98a0a334be4b1 100644 --- a/crates/ruff_python_formatter/src/other/arguments.rs +++ b/crates/ruff_python_formatter/src/other/arguments.rs @@ -205,11 +205,7 @@ fn is_arguments_huggable(arguments: &Arguments, context: &PyFormatContext) -> bo // If the expression has a trailing comma, then we can't hug it. if options.magic_trailing_comma().is_respect() - && commas::has_magic_trailing_comma( - TextRange::new(arg.end(), arguments.end()), - options, - context, - ) + && commas::has_magic_trailing_comma(TextRange::new(arg.end(), arguments.end()), context) { return false; } diff --git a/crates/ruff_python_formatter/src/other/commas.rs b/crates/ruff_python_formatter/src/other/commas.rs index 326daba4255b5..a102e0d59e878 100644 --- a/crates/ruff_python_formatter/src/other/commas.rs +++ b/crates/ruff_python_formatter/src/other/commas.rs @@ -1,17 +1,14 @@ +use ruff_formatter::FormatContext; use ruff_python_trivia::{SimpleToken, SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::TextRange; use crate::prelude::*; -use crate::{MagicTrailingComma, PyFormatOptions}; +use crate::MagicTrailingComma; /// Returns `true` if the range ends with a magic trailing comma (and the magic trailing comma /// should be respected). -pub(crate) fn has_magic_trailing_comma( - range: TextRange, - options: &PyFormatOptions, - context: &PyFormatContext, -) -> bool { - match options.magic_trailing_comma() { +pub(crate) fn has_magic_trailing_comma(range: TextRange, context: &PyFormatContext) -> bool { + match context.options().magic_trailing_comma() { MagicTrailingComma::Respect => { let first_token = SimpleTokenizer::new(context.source(), range) .skip_trivia() diff --git a/crates/ruff_python_formatter/src/other/with_item.rs b/crates/ruff_python_formatter/src/other/with_item.rs index 3ea82b8a9df69..38088e3b78cda 100644 --- a/crates/ruff_python_formatter/src/other/with_item.rs +++ b/crates/ruff_python_formatter/src/other/with_item.rs @@ -1,4 +1,4 @@ -use ruff_formatter::write; +use ruff_formatter::{write, FormatRuleWithOptions}; use ruff_python_ast::WithItem; use crate::comments::SourceComment; @@ -8,8 +8,66 @@ use crate::expression::parentheses::{ }; use crate::prelude::*; +#[derive(Copy, Clone, Debug, Eq, PartialEq, Default)] +pub enum WithItemLayout { + /// A with item that is the `with`s only context manager and its context expression is parenthesized. + /// + /// ```python + /// with ( + /// a + b + /// ) as b: + /// ... + /// ``` + /// + /// This layout is used independent of the target version. + SingleParenthesizedContextManager, + + /// 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. + /// + /// ```python + /// with a + b: + /// ... + /// + /// with a, b: + /// ... + /// ``` + Python38OrOlder, + + /// A with item where the `with` formatting adds parentheses around all context managers if necessary. + /// + /// ```python + /// with ( + /// a, + /// b, + /// ): pass + /// ``` + /// + /// This layout is generally used when the target version is Python 3.9 or newer, but it is used + /// for Python 3.8 if the with item has a leading or trailing comment. + /// + /// ```python + /// with ( + /// # leading + /// a + // ): ... + /// ``` + #[default] + ParenthesizedContextManagers, +} + #[derive(Default)] -pub struct FormatWithItem; +pub struct FormatWithItem { + layout: WithItemLayout, +} + +impl FormatRuleWithOptions> for FormatWithItem { + type Options = WithItemLayout; + + fn with_options(self, options: Self::Options) -> Self { + Self { layout: options } + } +} impl FormatNodeRule for FormatWithItem { fn fmt_fields(&self, item: &WithItem, f: &mut PyFormatter) -> FormatResult<()> { @@ -28,40 +86,52 @@ impl FormatNodeRule for FormatWithItem { f.context().source(), ); - // Remove the parentheses of the `with_items` if the with statement adds parentheses - if f.context().node_level().is_parenthesized() { - if is_parenthesized { - // ...except if the with item is parenthesized, then use this with item as a preferred breaking point - // or when it has comments, then parenthesize it to prevent comments from moving. - maybe_parenthesize_expression( - context_expr, - item, - Parenthesize::IfBreaksOrIfRequired, - ) - .fmt(f)?; - } else { - context_expr - .format() - .with_options(Parentheses::Never) + match self.layout { + // Remove the parentheses of the `with_items` if the with statement adds parentheses + WithItemLayout::ParenthesizedContextManagers => { + if is_parenthesized { + // ...except if the with item is parenthesized, then use this with item as a preferred breaking point + // or when it has comments, then parenthesize it to prevent comments from moving. + maybe_parenthesize_expression( + context_expr, + item, + Parenthesize::IfBreaksOrIfRequired, + ) .fmt(f)?; + } else { + context_expr + .format() + .with_options(Parentheses::Never) + .fmt(f)?; + } + } + + WithItemLayout::SingleParenthesizedContextManager => { + write!( + f, + [maybe_parenthesize_expression( + context_expr, + item, + Parenthesize::IfBreaks + )] + )?; + } + + WithItemLayout::Python38OrOlder => { + let parenthesize = if is_parenthesized { + Parenthesize::IfBreaks + } else { + Parenthesize::IfRequired + }; + write!( + f, + [maybe_parenthesize_expression( + context_expr, + item, + parenthesize + )] + )?; } - } else { - // Prefer keeping parentheses for already parenthesized expressions over - // parenthesizing other nodes. - let parenthesize = if is_parenthesized { - Parenthesize::IfBreaks - } else { - Parenthesize::IfRequired - }; - - write!( - f, - [maybe_parenthesize_expression( - context_expr, - item, - parenthesize - )] - )?; } if let Some(optional_vars) = optional_vars { diff --git a/crates/ruff_python_formatter/src/statement/stmt_with.rs b/crates/ruff_python_formatter/src/statement/stmt_with.rs index ce0af821f101d..696e7a0e0acbe 100644 --- a/crates/ruff_python_formatter/src/statement/stmt_with.rs +++ b/crates/ruff_python_formatter/src/statement/stmt_with.rs @@ -1,6 +1,6 @@ -use ruff_formatter::{format_args, write, FormatError}; -use ruff_python_ast::AstNode; +use ruff_formatter::{format_args, write, FormatContext, FormatError}; use ruff_python_ast::StmtWith; +use ruff_python_ast::{AstNode, WithItem}; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_text_size::{Ranged, TextRange}; @@ -11,9 +11,10 @@ use crate::expression::parentheses::{ is_expression_parenthesized, optional_parentheses, parenthesized, }; use crate::other::commas; +use crate::other::with_item::WithItemLayout; use crate::prelude::*; use crate::statement::clause::{clause_body, clause_header, ClauseHeader}; -use crate::{PyFormatOptions, PythonVersion}; +use crate::PythonVersion; #[derive(Default)] pub struct FormatStmtWith; @@ -61,58 +62,59 @@ impl FormatNodeRule for FormatStmtWith { ] )?; - if parenthesized_comments.is_empty() { - let format_items = format_with(|f| { - let mut joiner = - f.join_comma_separated(with_stmt.body.first().unwrap().start()); + let layout = WithItemsLayout::from_statement( + with_stmt, + f.context(), + parenthesized_comments, + )?; + + match layout { + WithItemsLayout::SingleWithTarget(single) => { + optional_parentheses(&single.format()).fmt(f) + } - for item in &with_stmt.items { - joiner.entry_with_line_separator( - item, - &item.format(), - soft_line_break_or_space(), + WithItemsLayout::SingleParenthesizedContextManager(single) => single + .format() + .with_options(WithItemLayout::SingleParenthesizedContextManager) + .fmt(f), + + WithItemsLayout::ParenthesizeIfExpands => { + parenthesize_if_expands(&format_with(|f| { + let mut joiner = f.join_comma_separated( + with_stmt.body.first().unwrap().start(), ); - } - joiner.finish() - }); - - match should_parenthesize(with_stmt, f.options(), f.context())? { - ParenthesizeWith::Optional => { - optional_parentheses(&format_items).fmt(f)?; - } - ParenthesizeWith::IfExpands => { - parenthesize_if_expands(&format_items).fmt(f)?; - } - ParenthesizeWith::UnlessCommented => { - if let [item] = with_stmt.items.as_slice() { - // This is similar to `maybe_parenthesize_expression`, but we're not - // dealing with an expression here, it's a `WithItem`. - if comments.has_leading(item) || comments.has_trailing(item) - { - parenthesized("(", &item.format(), ")").fmt(f)?; - } else { - item.format().fmt(f)?; - } - } else { - f.join_with(format_args![token(","), space()]) - .entries(with_stmt.items.iter().formatted()) - .finish()?; + + for item in &with_stmt.items { + joiner.entry_with_line_separator( + item, + &item.format(), + soft_line_break_or_space(), + ); } - } + joiner.finish() + })) + .fmt(f) } - } else { - let joined = format_with(|f: &mut PyFormatter| { - f.join_comma_separated(with_stmt.body.first().unwrap().start()) - .nodes(&with_stmt.items) - .finish() - }); - - parenthesized("(", &joined, ")") - .with_dangling_comments(parenthesized_comments) - .fmt(f)?; - } - Ok(()) + WithItemsLayout::Python38OrOlder => f + .join_with(format_args![token(","), space()]) + .entries(with_stmt.items.iter().map(|item| { + item.format().with_options(WithItemLayout::Python38OrOlder) + })) + .finish(), + + WithItemsLayout::Parenthesized => parenthesized( + "(", + &format_with(|f: &mut PyFormatter| { + f.join_comma_separated(with_stmt.body.first().unwrap().start()) + .nodes(&with_stmt.items) + .finish() + }), + ")", + ) + .with_dangling_comments(parenthesized_comments) + .fmt(f), + } }) ), clause_body(&with_stmt.body, colon_comments) @@ -130,92 +132,167 @@ impl FormatNodeRule for FormatStmtWith { } } -/// Determines whether the `with` items should be parenthesized (over parenthesizing each item), -/// and if so, which parenthesizing layout to use. -/// -/// Parenthesize `with` items if -/// * The last item has a trailing comma (implying that the with items were parenthesized in the source) -/// * There's more than one item and they're already parenthesized -/// * There's more than one item, the [`wrap_multiple_context_managers_in_parens`](is_wrap_multiple_context_managers_in_parens) preview style is enabled, -/// and the target python version is >= 3.9 -/// * There's a single non-parenthesized item. The function returns [`ParenthesizeWith::Optional`] -/// if the parentheses can be omitted if breaking around parenthesized sub-expressions is sufficient -/// to make the expression fit. It returns [`ParenthesizeWith::IfExpands`] otherwise. -/// * The only item is parenthesized and has comments. -fn should_parenthesize( - with: &StmtWith, - options: &PyFormatOptions, - context: &PyFormatContext, -) -> FormatResult { - if has_magic_trailing_comma(with, options, context) { - return Ok(ParenthesizeWith::IfExpands); - } +#[derive(Clone, Copy, Debug)] +enum WithItemsLayout<'a> { + /// The with statement's only item has a parenthesized context manager. + /// + /// ```python + /// with ( + /// a + b + /// ): + /// ... + /// + /// with ( + /// a + b + /// ) as b: + /// ... + /// ``` + /// + /// In this case, prefer keeping the parentheses around the context expression instead of parenthesizing the entire + /// with item. + SingleParenthesizedContextManager(&'a WithItem), - let can_parenthesize = options.target_version() >= PythonVersion::Py39 - || are_with_items_parenthesized(with, context)?; + /// 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: + /// ... + /// ``` + /// + /// Only used for Python 3.9+ + SingleWithTarget(&'a WithItem), - if !can_parenthesize { - return Ok(ParenthesizeWith::UnlessCommented); - } + /// The target python version doesn't support parenthesized context managers because it is Python 3.8 or older. + /// + /// In this case, never add parentheses and join the with items with spaces. + /// + /// ```python + /// with ContextManager1( + /// aaaaaaaaaaaaaaa, b + /// ), ContextManager2(), ContextManager3(), ContextManager4(): + /// pass + /// ``` + Python38OrOlder, + + /// Wrap the with items in parentheses if they don't fit on a single line and join them by soft line breaks. + /// + /// ```python + /// with ( + /// ContextManager1(aaaaaaaaaaaaaaa, b), + /// ContextManager1(), + /// ContextManager1(), + /// ContextManager1(), + /// ): + /// pass + /// ``` + /// + /// Only used for Python 3.9+. + ParenthesizeIfExpands, + + /// Always parenthesize because the context managers open-parentheses have a trailing comment: + /// + /// ```python + /// with ( # comment + /// CtxManager() as example + /// ): + /// ... + /// ``` + /// + /// Or because it is a single item with a trailing or leading comment. + /// + /// ```python + /// with ( + /// # leading + /// CtxManager() + /// # trailing + /// ): pass + /// ``` + Parenthesized, +} + +impl<'a> WithItemsLayout<'a> { + fn from_statement( + with: &'a StmtWith, + context: &PyFormatContext, + parenthesized_comments: &[SourceComment], + ) -> FormatResult { + // The with statement already has parentheses around the entire with items. Guaranteed to be Python 3.9 or newer + // ``` + // with ( # comment + // CtxManager() as example + // ): + // pass + // ``` + if !parenthesized_comments.is_empty() { + return Ok(Self::Parenthesized); + } + + // A trailing comma at the end guarantees that the context managers are parenthesized and that it is Python 3.9 or newer syntax. + // ```python + // with ( # comment + // CtxManager() as example, + // ): + // pass + // ``` + if has_magic_trailing_comma(with, context) { + return Ok(Self::ParenthesizeIfExpands); + } - if let [single] = with.items.as_slice() { - return Ok( + if let [single] = with.items.as_slice() { // If the with item itself has comments (not the context expression), then keep the parentheses + // ```python + // with ( + // # leading + // a + // ): pass + // ``` if context.comments().has_leading(single) || context.comments().has_trailing(single) { - ParenthesizeWith::IfExpands + return Ok(Self::Parenthesized); } - // If it is the only expression and it has comments, then the with statement - // as well as the with item add parentheses - else if is_expression_parenthesized( + + // Preserve the parentheses around the context expression instead of parenthesizing the entire + // with items. + if is_expression_parenthesized( (&single.context_expr).into(), context.comments().ranges(), context.source(), ) { - // Preserve the parentheses around the context expression instead of parenthesizing the entire - // with items. - ParenthesizeWith::UnlessCommented - } else if can_omit_optional_parentheses(&single.context_expr, context) { - ParenthesizeWith::Optional - } else { - ParenthesizeWith::IfExpands - }, - ); - } - - // Always parenthesize multiple items - Ok(ParenthesizeWith::IfExpands) -} + return Ok(Self::SingleParenthesizedContextManager(single)); + } + } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -enum ParenthesizeWith { - /// Don't wrap the with items in parentheses except if it is a single item - /// and it has leading or trailing comment. - /// - /// This is required because `are_with_items_parenthesized` cannot determine if - /// `with (expr)` is a parenthesized expression or a parenthesized with item. - UnlessCommented, + let can_parenthesize = context.options().target_version() >= PythonVersion::Py39 + || are_with_items_parenthesized(with, context)?; - /// Wrap the with items in optional parentheses - Optional, + // If the target version doesn't support parenthesized context managers and they aren't + // parenthesized by the user, bail out. + if !can_parenthesize { + return Ok(Self::Python38OrOlder); + } - /// Wrap the with items in parentheses if they expand - IfExpands, + Ok(match with.items.as_slice() { + [single] => { + if can_omit_optional_parentheses(&single.context_expr, context) { + Self::SingleWithTarget(single) + } else { + Self::ParenthesizeIfExpands + } + } + // Always parenthesize multiple items + [..] => Self::ParenthesizeIfExpands, + }) + } } -fn has_magic_trailing_comma( - with: &StmtWith, - options: &PyFormatOptions, - context: &PyFormatContext, -) -> bool { +fn has_magic_trailing_comma(with: &StmtWith, context: &PyFormatContext) -> bool { let Some(last_item) = with.items.last() else { return false; }; - commas::has_magic_trailing_comma( - TextRange::new(last_item.end(), with.end()), - options, - context, - ) + commas::has_magic_trailing_comma(TextRange::new(last_item.end(), with.end()), context) } fn are_with_items_parenthesized(with: &StmtWith, context: &PyFormatContext) -> FormatResult {