From 4c6e0f12119286dd5d3eaea7c602629220640221 Mon Sep 17 00:00:00 2001 From: azjezz Date: Sun, 9 Mar 2025 02:36:18 +0100 Subject: [PATCH] feat(formatter): add preserve_breaking_conditional_expression option This commit introduces a new formatter option, `preserve_breaking_conditional_expression`, which allows users to preserve line breaks in conditional (ternary) expressions. When enabled, the formatter will not collapse multi-line conditional expressions into a single line, even if they fit within the print width. This provides more control over the formatting of complex or lengthy conditional expressions, allowing users to maintain their preferred layout for readability. The default value for this option is `false`, maintaining the existing behavior of collapsing conditional expressions when possible. Signed-off-by: azjezz --- crates/formatter/src/internal/format/array.rs | 13 +--- .../src/internal/format/assignment.rs | 5 +- .../src/internal/format/call_arguments.rs | 35 +-------- .../src/internal/format/expression.rs | 73 ++++++++++++++----- .../src/internal/format/member_access.rs | 4 + crates/formatter/src/internal/format/misc.rs | 5 +- crates/formatter/src/internal/utils.rs | 38 ++++++++++ crates/formatter/src/lib.rs | 2 - crates/formatter/src/settings.rs | 11 +++ .../after.php | 30 ++++++++ .../before.php | 27 +++++++ .../settings.inc | 4 + .../after.php | 34 +++++++++ .../before.php | 37 ++++++++++ .../settings.inc | 4 + crates/formatter/tests/mod.rs | 2 + docs/formatter/settings.md | 16 ++++ 17 files changed, 277 insertions(+), 63 deletions(-) create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression/after.php create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression/before.php create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression/settings.inc create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/after.php create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/before.php create mode 100644 crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/settings.inc diff --git a/crates/formatter/src/internal/format/array.rs b/crates/formatter/src/internal/format/array.rs index 0a8febcc..796b2213 100644 --- a/crates/formatter/src/internal/format/array.rs +++ b/crates/formatter/src/internal/format/array.rs @@ -395,6 +395,9 @@ fn is_table_style<'a>(f: &mut FormatterState<'a>, array_like: &ArrayLike<'a>) -> | Expression::LegacyArray(LegacyArray { elements, .. }) = element.value.as_ref() { let size = elements.len(); + if 0 == size { + return false; // Empty row + } // Check if row size is consistent row_size = row_size.max(size); @@ -438,19 +441,11 @@ fn is_table_style<'a>(f: &mut FormatterState<'a>, array_like: &ArrayLike<'a>) -> // Check if row size is within reasonable bounds (3-10 columns) if !(3..=12).contains(&row_size) { - println!("Row size: {}", row_size); - return false; } // At least 60% of the rows should have the same size - let is = (sizes.iter().filter(|size| **size == row_size).count() as f64) / (sizes.len() as f64) >= 0.6; - - println!("Row size: {}", row_size); - println!("Sizes: {:?}", sizes); - println!("Is: {}", is); - - is + (sizes.iter().filter(|size| **size == row_size).count() as f64) / (sizes.len() as f64) >= 0.6 } fn calculate_column_widths<'a>(f: &mut FormatterState<'a>, array_like: &ArrayLike<'a>) -> Option> { diff --git a/crates/formatter/src/internal/format/assignment.rs b/crates/formatter/src/internal/format/assignment.rs index 55ecfc40..3e3e7970 100644 --- a/crates/formatter/src/internal/format/assignment.rs +++ b/crates/formatter/src/internal/format/assignment.rs @@ -89,7 +89,10 @@ pub(super) fn print_assignment<'a>( Document::Group(Group::new(vec![lhs])), Document::space(), operator, - Document::Group(Group::new(vec![Document::Indent(vec![Document::Line(Line::default()), rhs])])), + Document::Group(Group::new(vec![Document::IndentIfBreak(IndentIfBreak::new(vec![ + Document::Line(Line::default()), + rhs, + ]))])), ])), Layout::NeverBreakAfterOperator => Document::Group(Group::new(vec![ Document::Group(Group::new(vec![lhs])), diff --git a/crates/formatter/src/internal/format/call_arguments.rs b/crates/formatter/src/internal/format/call_arguments.rs index e5d6fef9..cb04a0ec 100644 --- a/crates/formatter/src/internal/format/call_arguments.rs +++ b/crates/formatter/src/internal/format/call_arguments.rs @@ -10,6 +10,7 @@ use crate::internal::format::misc; use crate::internal::format::misc::is_simple_expression; use crate::internal::format::misc::is_string_word_type; use crate::internal::format::misc::should_hug_expression; +use crate::internal::utils::could_expand_value; use crate::internal::utils::will_break; pub(super) fn print_call_arguments<'a>(f: &mut FormatterState<'a>, expression: &CallLikeNode<'a>) -> Document<'a> { @@ -384,7 +385,7 @@ fn should_expand_first_arg<'a>(f: &FormatterState<'a>, argument_list: &'a Argume match second_argument.value() { Expression::Array(_) | Expression::List(_) | Expression::LegacyArray(_) => false, Expression::Closure(_) | Expression::ArrowFunction(_) | Expression::Conditional(_) => false, - expression => is_hopefully_short_call_argument(expression) && !could_expand_argument_value(expression, false), + expression => is_hopefully_short_call_argument(expression) && !could_expand_value(expression, false), } } @@ -407,7 +408,7 @@ fn should_expand_last_arg<'a>(f: &FormatterState<'a>, argument_list: &'a Argumen .map(|a| f.has_comment(a.span(), CommentFlags::Leading | CommentFlags::Trailing)) .unwrap_or(false); - could_expand_argument_value(last_argument_value, false) + could_expand_value(last_argument_value, false) // If the last two arguments are of the same type, // disable last element expansion. && (penultimate_argument.is_none() @@ -552,33 +553,3 @@ fn is_simple_call_argument<'a>(node: &'a Expression, depth: usize) -> bool { _ => false, } } - -fn could_expand_argument_value(argument_value: &Expression, arrow_chain_recursion: bool) -> bool { - match argument_value { - Expression::Array(expr) => !expr.elements.is_empty(), - Expression::LegacyArray(expr) => !expr.elements.is_empty(), - Expression::List(expr) => !expr.elements.is_empty(), - Expression::Closure(_) => true, - Expression::Match(m) => !m.arms.is_empty(), - Expression::Binary(operation) => could_expand_argument_value(&operation.lhs, arrow_chain_recursion), - Expression::ArrowFunction(arrow_function) => match arrow_function.expression.as_ref() { - Expression::Array(_) | Expression::List(_) | Expression::LegacyArray(_) => { - could_expand_argument_value(&arrow_function.expression, true) - } - Expression::Call(_) | Expression::Conditional(_) => !arrow_chain_recursion, - _ => false, - }, - Expression::Instantiation(instantiation) => { - let Expression::Identifier(_) = instantiation.class.as_ref() else { - return false; - }; - - let Some(arguments) = instantiation.arguments.as_ref() else { - return false; - }; - - arguments.arguments.len() > 1 && arguments.arguments.iter().all(|a| matches!(a, Argument::Named(_))) - } - _ => false, - } -} diff --git a/crates/formatter/src/internal/format/expression.rs b/crates/formatter/src/internal/format/expression.rs index aa940338..e55ab5d3 100644 --- a/crates/formatter/src/internal/format/expression.rs +++ b/crates/formatter/src/internal/format/expression.rs @@ -27,6 +27,7 @@ use crate::internal::format::misc::print_modifiers; use crate::internal::format::return_value::format_return_value; use crate::internal::format::string::print_string; use crate::internal::utils; +use crate::internal::utils::could_expand_value; use crate::settings::*; use crate::wrap; @@ -837,27 +838,65 @@ impl<'a> Format<'a> for Match { impl<'a> Format<'a> for Conditional { fn format(&'a self, f: &mut FormatterState<'a>) -> Document<'a> { wrap!(f, self, Conditional, { + let must_break = f.settings.preserve_breaking_conditional_expression && { + misc::has_new_line_in_range( + f.source_text, + self.condition.span().end.offset, + self.question_mark.start.offset, + ) || self.then.as_ref().is_some_and(|t| { + misc::has_new_line_in_range(f.source_text, self.question_mark.start.offset, t.span().start.offset) + }) + }; + match &self.then { - Some(then) => Document::Group(Group::new(vec![ - self.condition.format(f), - Document::Indent(vec![ - Document::Line(Line::default()), - Document::String("? "), - then.format(f), - Document::Line(Line::default()), - Document::String(": "), - self.r#else.format(f), - ]), - ])), + Some(then) => { + let inline_colon = !misc::has_new_line_in_range( + f.source_text, + then.span().end.offset, + self.r#else.span().start.offset, + ) && could_expand_value(then, false); + + let conditional_id = f.next_id(); + let then_id = f.next_id(); + + Document::Group( + Group::new(vec![ + self.condition.format(f), + Document::Indent(vec![ + Document::Line(if must_break { Line::hard() } else { Line::default() }), + Document::String("? "), + Document::Group(Group::new(vec![then.format(f)]).with_id(then_id)), + { + if inline_colon { + if must_break { + Document::space() + } else { + Document::IfBreak( + IfBreak::new(Document::space(), { + Document::IfBreak( + IfBreak::new(Document::Line(Line::hard()), Document::space()) + .with_id(conditional_id), + ) + }) + .with_id(then_id), + ) + } + } else { + Document::Line(if must_break { Line::hard() } else { Line::default() }) + } + }, + Document::String(": "), + self.r#else.format(f), + ]), + ]) + .with_id(conditional_id), + ) + } None => Document::Group(Group::new(vec![ self.condition.format(f), Document::Indent(vec![ - Document::space(), - Document::Group(Group::new(vec![ - Document::String("?:"), - Document::Line(Line::default()), - self.r#else.format(f), - ])), + Document::Line(if must_break { Line::hard() } else { Line::default() }), + Document::Group(Group::new(vec![Document::String("?: "), self.r#else.format(f)])), ]), ])), } diff --git a/crates/formatter/src/internal/format/member_access.rs b/crates/formatter/src/internal/format/member_access.rs index 2074a1d3..6bf73208 100644 --- a/crates/formatter/src/internal/format/member_access.rs +++ b/crates/formatter/src/internal/format/member_access.rs @@ -115,6 +115,10 @@ impl MemberAccessChain<'_> { #[inline] fn must_break(&self, f: &FormatterState) -> bool { + if self.is_first_link_static_method_call() && self.accesses.len() > 3 { + return true; + } + let must_break = match self.base { Expression::Instantiation(_) => { self.accesses.iter().all(|access| { diff --git a/crates/formatter/src/internal/format/misc.rs b/crates/formatter/src/internal/format/misc.rs index bb8ac9f0..53ef405b 100644 --- a/crates/formatter/src/internal/format/misc.rs +++ b/crates/formatter/src/internal/format/misc.rs @@ -4,6 +4,7 @@ use mago_span::Span; use crate::document::Document; use crate::document::Group; +use crate::document::IndentIfBreak; use crate::document::Line; use crate::document::Separator; use crate::internal::FormatterState; @@ -355,10 +356,10 @@ pub(super) fn adjust_clause<'a>( pub(super) fn print_condition<'a>(f: &mut FormatterState<'a>, condition: &'a Expression) -> Document<'a> { Document::Group(Group::new(vec![ Document::String("("), - Document::Indent(vec![ + Document::IndentIfBreak(IndentIfBreak::new(vec![ Document::Line(if f.settings.control_space_parens { Line::default() } else { Line::soft() }), condition.format(f), - ]), + ])), Document::Line(if f.settings.control_space_parens { Line::default() } else { Line::soft() }), Document::String(")"), ])) diff --git a/crates/formatter/src/internal/utils.rs b/crates/formatter/src/internal/utils.rs index cbadd64d..968fa303 100644 --- a/crates/formatter/src/internal/utils.rs +++ b/crates/formatter/src/internal/utils.rs @@ -6,6 +6,7 @@ use crate::document::IndentIfBreak; use crate::document::Separator; use crate::internal::FormatterState; +#[inline] pub const fn has_naked_left_side(expression: &Expression) -> bool { matches!( expression, @@ -21,6 +22,7 @@ pub const fn has_naked_left_side(expression: &Expression) -> bool { ) } +#[inline] pub const fn get_left_side(expression: &Expression) -> Option<&Expression> { match expression { Expression::Binary(binary) => Some(&binary.lhs), @@ -50,6 +52,7 @@ pub const fn get_left_side(expression: &Expression) -> Option<&Expression> { } } +#[inline] pub fn is_at_call_like_expression(f: &FormatterState<'_>) -> bool { let Some(grant_parent) = f.grandparent_node() else { return false; @@ -76,6 +79,7 @@ pub const fn unwrap_parenthesized(mut expression: &Expression) -> &Expression { expression } +#[inline] pub fn is_at_callee(f: &FormatterState<'_>) -> bool { let Node::Expression(expression) = f.parent_node() else { return false; @@ -97,6 +101,7 @@ pub fn is_at_callee(f: &FormatterState<'_>) -> bool { } } +#[inline] pub fn will_break(document: &mut Document<'_>) -> bool { let check_array = |array: &mut Vec>| array.iter_mut().rev().any(|doc| will_break(doc)); @@ -125,6 +130,7 @@ pub fn will_break(document: &mut Document<'_>) -> bool { } } +#[inline] pub fn replace_end_of_line(document: Document<'_>, replacement: Separator) -> Document<'_> { let Document::String(text) = document else { return document; @@ -132,3 +138,35 @@ pub fn replace_end_of_line(document: Document<'_>, replacement: Separator) -> Do Document::Array(Document::join(text.split("\n").map(Document::String).collect(), replacement)) } + +#[inline] +pub fn could_expand_value(value: &Expression, arrow_chain_recursion: bool) -> bool { + match value { + Expression::Array(expr) => !expr.elements.is_empty(), + Expression::LegacyArray(expr) => !expr.elements.is_empty(), + Expression::List(expr) => !expr.elements.is_empty(), + Expression::Closure(_) => true, + Expression::Match(m) => !m.arms.is_empty(), + Expression::Binary(operation) => could_expand_value(&operation.lhs, arrow_chain_recursion), + Expression::ArrowFunction(arrow_function) if !arrow_chain_recursion => match arrow_function.expression.as_ref() + { + Expression::Array(_) | Expression::List(_) | Expression::LegacyArray(_) => { + could_expand_value(&arrow_function.expression, true) + } + Expression::Call(_) | Expression::Conditional(_) => true, + _ => false, + }, + Expression::Instantiation(instantiation) => { + let Expression::Identifier(_) = instantiation.class.as_ref() else { + return false; + }; + + let Some(arguments) = instantiation.arguments.as_ref() else { + return false; + }; + + arguments.arguments.len() > 2 + } + _ => false, + } +} diff --git a/crates/formatter/src/lib.rs b/crates/formatter/src/lib.rs index 9b7bc0b9..aee7e610 100644 --- a/crates/formatter/src/lib.rs +++ b/crates/formatter/src/lib.rs @@ -104,8 +104,6 @@ impl<'a> Formatter<'a> { pub fn format(&self, source: &'a Source, program: &'a Program) -> String { let document = self.build(source, program); - // println!("Document: {}", document); - self.print(document, Some(source.size)) } diff --git a/crates/formatter/src/settings.rs b/crates/formatter/src/settings.rs index 413a6e05..35599b6c 100644 --- a/crates/formatter/src/settings.rs +++ b/crates/formatter/src/settings.rs @@ -716,6 +716,16 @@ pub struct FormatSettings { /// Default: `false` #[serde(default = "default_false")] pub preserve_breaking_attribute_list: bool, + + /// Controls whether to preserve line breaks in conditional ( ternary ) expressions, even if they could fit on a single line. + /// + /// If enabled, the formatter will not attempt to collapse conditional expressions onto a single line if they + /// were originally written with line breaks. This can be useful for preserving the original formatting + /// of complex or lengthy conditional expressions. + /// + /// Default: `false` + #[serde(default = "default_false")] + pub preserve_breaking_conditional_expression: bool, } impl Default for FormatSettings { @@ -763,6 +773,7 @@ impl Default for FormatSettings { preserve_breaking_array_like: true, preserve_breaking_parameter_list: false, preserve_breaking_attribute_list: false, + preserve_breaking_conditional_expression: false, } } } diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression/after.php b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/after.php new file mode 100644 index 00000000..2c9dedd2 --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/after.php @@ -0,0 +1,30 @@ +multiple + ? ($this->search)($query) + : [self::CANCEL, self::SEARCH_AGAIN, ...($this->search)($query)]; + +$this->now = ($now instanceof DateTimeInterface) + ? DateTimeImmutable::createFromInterface($now) + : new DateTimeImmutable($now); + +function getControls(): array +{ + return [ + ...( + $this->bufferEnabled + ? [ + 'esc' => 'select', + ] : [ + '/' => 'filter', + 'space' => 'select', + ] + ), + '↑' => 'up', + '↓' => 'down', + 'enter' => $this->options->getSelectedOptions() === [] + ? 'skip' + : 'confirm', + 'ctrl+c' => 'cancel', + ]; +} diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression/before.php b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/before.php new file mode 100644 index 00000000..21c57324 --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/before.php @@ -0,0 +1,27 @@ +multiple + ? ($this->search)($query) + : [self::CANCEL, self::SEARCH_AGAIN, ...($this->search)($query)]; + + $this->now = $now instanceof DateTimeInterface + ? DateTimeImmutable::createFromInterface($now) + : new DateTimeImmutable($now); + + function getControls(): array + { + return [ + ...($this->bufferEnabled ? [ + 'esc' => 'select', + ] : [ + '/' => 'filter', + 'space' => 'select', + ]), + '↑' => 'up', + '↓' => 'down', + 'enter' => $this->options->getSelectedOptions() === [] + ? 'skip' + : 'confirm', + 'ctrl+c' => 'cancel', + ]; + } diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression/settings.inc b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/settings.inc new file mode 100644 index 00000000..8abe9cef --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression/settings.inc @@ -0,0 +1,4 @@ +FormatSettings { + preserve_breaking_conditional_expression: true, + ..Default::default() +} diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/after.php b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/after.php new file mode 100644 index 00000000..6ec1af63 --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/after.php @@ -0,0 +1,34 @@ +multiple ? ($this->search)($query) : [self::CANCEL, self::SEARCH_AGAIN, ...($this->search)($query)]; + +$this->now = + ($now instanceof DateTimeInterface) ? DateTimeImmutable::createFromInterface($now) : new DateTimeImmutable($now); + +function getControls(): array +{ + return [ + ...( + $this->bufferEnabled + ? [ + 'esc' => 'select', + ] : [ + '/' => 'filter', + 'space' => 'select', + ] + ), + ...( + $this->bufferEnabled + ? ['esc' => 'select'] + : [ + '/' => 'filter', + 'space' => 'select', + ] + ), + ...($this->bufferEnabled ? ['esc' => 'select'] : ['/' => 'filter', 'space' => 'select']), + '↑' => 'up', + '↓' => 'down', + 'enter' => $this->options->getSelectedOptions() === [] ? 'skip' : 'confirm', + 'ctrl+c' => 'cancel', + ]; +} diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/before.php b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/before.php new file mode 100644 index 00000000..952dee61 --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/before.php @@ -0,0 +1,37 @@ +multiple + ? ($this->search)($query) + : [self::CANCEL, self::SEARCH_AGAIN, ...($this->search)($query)]; + + $this->now = $now instanceof DateTimeInterface + ? DateTimeImmutable::createFromInterface($now) + : new DateTimeImmutable($now); + + function getControls(): array + { + return [ + ...($this->bufferEnabled ? [ + 'esc' => 'select', + ] : [ + '/' => 'filter', + 'space' => 'select', + ]), + ...( + $this->bufferEnabled + ? ['esc' => 'select'] : [ + '/' => 'filter', + 'space' => 'select', + ] + ), + ...( + $this->bufferEnabled ? ['esc' => 'select'] : ['/' => 'filter', 'space' => 'select'] + ), + '↑' => 'up', + '↓' => 'down', + 'enter' => $this->options->getSelectedOptions() === [] + ? 'skip' + : 'confirm', + 'ctrl+c' => 'cancel', + ]; + } diff --git a/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/settings.inc b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/settings.inc new file mode 100644 index 00000000..4e622aec --- /dev/null +++ b/crates/formatter/tests/cases/preserve_breaking_conditional_expression_disabled/settings.inc @@ -0,0 +1,4 @@ +FormatSettings { + preserve_breaking_conditional_expression: false, + ..Default::default() +} diff --git a/crates/formatter/tests/mod.rs b/crates/formatter/tests/mod.rs index f162fe5d..cf429db9 100644 --- a/crates/formatter/tests/mod.rs +++ b/crates/formatter/tests/mod.rs @@ -122,6 +122,8 @@ test_case!(preserve_breaking_parameter_list); test_case!(preserve_breaking_parameter_list_disabled); test_case!(preserve_breaking_attribute_list); test_case!(preserve_breaking_attribute_list_disabled); +test_case!(preserve_breaking_conditional_expression); +test_case!(preserve_breaking_conditional_expression_disabled); test_case!(hooks_always_break); // GitHub issue test cases diff --git a/docs/formatter/settings.md b/docs/formatter/settings.md index dd4d0de3..626b94c5 100644 --- a/docs/formatter/settings.md +++ b/docs/formatter/settings.md @@ -533,3 +533,19 @@ If enabled, the formatter will not attempt to collapse attribute lists onto a si ```toml preserve_breaking_attribute_list = true ``` + +### `preserve_breaking_conditional_expression` + +Controls whether to preserve line breaks in conditional ( ternary ) expressions, even if they could fit on a single line. + +If enabled, the formatter will not attempt to collapse conditional expressions onto a single line if they +were originally written with line breaks. This can be useful for preserving the original formatting +of complex or lengthy conditional expressions. + +- Default: `false` +- Type: `boolean` +- Example: + + ```toml + preserve_breaking_conditional_expression = true + ```