From 8c3a819b087f05fb461e5a828dd6e63dfb4af5ca Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 7 Jun 2022 08:12:21 +0200 Subject: [PATCH] refactor(formatter): Change binary like expression to format left to right (#2640) Refactors the binary like expression formatting to lazily write the expressions rather than allocating vectors with the sub results. The basic idea remains the same. The implementation flattens an expression. What's different to before is that this is now a two-stage process: 1. Flatten the binary expression and specify for each "part" how it should be formatted (left, right, or a group) 2. Format the flattened expressions I think this also improves readability because it makes it clear what concepts exist in this algorithm (left hand side, right hand side, and a sub-group). --- .../src/utils/binary_like_expression.rs | 468 ++++++++++-------- 1 file changed, 257 insertions(+), 211 deletions(-) diff --git a/crates/rome_js_formatter/src/utils/binary_like_expression.rs b/crates/rome_js_formatter/src/utils/binary_like_expression.rs index 2f035ede93b..6ccef7bd59e 100644 --- a/crates/rome_js_formatter/src/utils/binary_like_expression.rs +++ b/crates/rome_js_formatter/src/utils/binary_like_expression.rs @@ -11,6 +11,7 @@ use rome_rowan::{AstNode, SyntaxResult}; use std::cmp::Ordering; use std::fmt::Debug; use std::iter::FusedIterator; +use std::ops::Deref; /// This function is charge to flat binaryish expressions that have the same precedence of their operators /// @@ -100,7 +101,7 @@ pub(crate) fn format_binary_like_expression( f: &mut JsFormatter, ) -> FormatResult<()> { let mut flatten_items = FlattenItems::default(); - let current_node = expression.syntax().clone(); + let current_node = expression.clone(); let post_order_binary_like_expressions = PostorderIterator::new(expression); let mut left: Option = None; @@ -109,24 +110,16 @@ pub(crate) fn format_binary_like_expression( let parent_operator = parent.operator_token()?; if let Some(left) = left { - flatten_items.format_binary_expression_right_hand_side( - left, - Some(parent_operator), - f, - )?; + flatten_items.flatten_binary_expression_right_hand_side(left, Some(parent_operator))?; } else { // Leaf binary like expression. Format the left hand side. // The right hand side gets formatted when traversing upwards in the tree. let left = parent.left()?; - let has_comments = left.syntax().has_comments_direct(); - let mut buffer = VecBuffer::new(f.state_mut()); - write!(buffer, [left])?; - - flatten_items.items.push(FlattenItem::regular( - buffer.into_element(), + flatten_items.items.push(FlattenItem::new( + FlattenedBinaryExpressionPart::Left { expression: left }, Some(parent_operator), has_comments.into(), )); @@ -137,11 +130,17 @@ pub(crate) fn format_binary_like_expression( // Format the top most binary like expression if let Some(root) = left { - flatten_items.format_binary_expression_right_hand_side(root, None, f)?; + flatten_items.flatten_binary_expression_right_hand_side(root, None)?; } - let element = flatten_items.take_format_element(¤t_node, f)?; - f.write_element(element) + let group = FlattenedBinaryExpressionPart::Group { + current: JsAnyBinaryLikeLeftExpression::JsAnyExpression(current_node.into_expression()), + expressions_start: flatten_items.current_group_start, + expressions_end: flatten_items.len(), + parenthesized: false, + }; + + group.write(f, &flatten_items) } /// Small wrapper to identify the operation of an expression and deduce their precedence @@ -170,25 +169,26 @@ enum BinaryLikeOperator { /// first `foo && bar` is computed and its result is then computed against `|| lorem`. /// /// In order to make this distinction more obvious, we wrap `foo && bar` in parenthesis. -fn format_with_or_without_parenthesis( +fn needs_parens( parent_operator: BinaryLikeOperator, - node: &JsSyntaxNode, - formatted_node: FormatElement, - f: &mut JsFormatter, -) -> FormatResult<(FormatElement, bool)> { - let compare_to = match JsAnyExpression::cast(node.clone()) { - Some(JsAnyExpression::JsLogicalExpression(logical)) => { - Some(BinaryLikeOperator::Logical(logical.operator()?)) - } - Some(JsAnyExpression::JsBinaryExpression(binary)) => { - Some(BinaryLikeOperator::Binary(binary.operator()?)) - } - Some(JsAnyExpression::JsInstanceofExpression(_)) => Some(BinaryLikeOperator::Instanceof), - Some(JsAnyExpression::JsInExpression(_)) => Some(BinaryLikeOperator::In), + node: &JsAnyBinaryLikeLeftExpression, +) -> FormatResult { + let compare_to = match node { + JsAnyBinaryLikeLeftExpression::JsAnyExpression(expression) => match expression { + JsAnyExpression::JsLogicalExpression(logical) => { + Some(BinaryLikeOperator::Logical(logical.operator()?)) + } + JsAnyExpression::JsBinaryExpression(binary) => { + Some(BinaryLikeOperator::Binary(binary.operator()?)) + } + JsAnyExpression::JsInstanceofExpression(_) => Some(BinaryLikeOperator::Instanceof), + JsAnyExpression::JsInExpression(_) => Some(BinaryLikeOperator::In), + _ => None, + }, _ => None, }; - let operation_is_higher = if let Some(compare_to) = compare_to { + let result = if let Some(compare_to) = compare_to { match (parent_operator, compare_to) { ( BinaryLikeOperator::Logical(previous_operation), @@ -209,13 +209,37 @@ fn format_with_or_without_parenthesis( false }; - let result = if operation_is_higher { - let (leading, content, trailing) = formatted_node.split_trivia(); + Ok(result) +} + +// False positive, Removing the `+ 'a` lifetime fails to compile with `hidden type for `impl Trait` captures lifetime that does not appear in bounds` +#[allow(clippy::needless_lifetimes)] +fn format_sub_expression<'a>( + parent_operator: BinaryLikeOperator, + sub_expression: &'a JsAnyBinaryLikeLeftExpression, +) -> impl Format + 'a { + format_with(move |f| { + if needs_parens(parent_operator, sub_expression)? { + write!(f, [format_parenthesized(sub_expression)]) + } else { + write!(f, [sub_expression]) + } + }) +} + +fn format_parenthesized<'a, Inner>(inner: Inner) -> impl Format +where + Inner: Format + 'a, +{ + format_with(move |f| { let mut buffer = VecBuffer::new(f.state_mut()); + write!(buffer, [inner])?; + let formatted_node = buffer.into_element(); + let (leading, content, trailing) = formatted_node.split_trivia(); - buffer.write_element(leading)?; + f.write_element(leading)?; write![ - buffer, + f, [group_elements(&format_args![ token("("), soft_block_indent(&format_once(|f| { @@ -224,24 +248,14 @@ fn format_with_or_without_parenthesis( })), token(")") ])] - ]?; - - (buffer.into_element(), true) - } else { - (formatted_node, false) - }; - - Ok(result) + ] + }) } -/// It tells if the expression can be hard grouped -fn can_hard_group(flatten_nodes: &[FlattenItem]) -> bool { +fn keep_on_same_line(flatten_nodes: &[FlattenItem]) -> bool { // We don't want to have 1 + 2 to break, for example. - // If there are any trailing comments, let's break. - flatten_nodes.len() <= 2 - && flatten_nodes - .iter() - .all(|node| !node.has_comments() && !node.is_group()) + // But if there are any trailing comments, break it. + flatten_nodes.len() <= 2 && flatten_nodes.iter().all(|node| !node.has_comments()) } fn is_inside_parenthesis(current_node: &JsSyntaxNode) -> bool { @@ -277,8 +291,8 @@ fn should_not_indent_if_parent_indents(current_node: &JsSyntaxNode) -> bool { /// these cases the decide to actually break on a new line and indent it. /// /// This function checks what the parents adheres to this behaviour -fn should_indent_if_parent_inlines(current_node: &JsSyntaxNode) -> bool { - let parent = current_node.parent(); +fn should_indent_if_parent_inlines(current_node: &JsAnyBinaryLikeLeftExpression) -> bool { + let parent = current_node.syntax().parent(); let grand_parent = parent.as_ref().and_then(|p| p.parent()); match (parent, grand_parent) { @@ -294,22 +308,32 @@ fn should_indent_if_parent_inlines(current_node: &JsSyntaxNode) -> bool { #[derive(Debug, Default)] struct FlattenItems { items: Vec, + + /// Position into `items` where the next group starts. + current_group_start: usize, +} + +impl Deref for FlattenItems { + type Target = [FlattenItem]; + + fn deref(&self) -> &Self::Target { + &self.items + } } impl FlattenItems { /// Formats the right hand side of a binary like expression - fn format_binary_expression_right_hand_side( + fn flatten_binary_expression_right_hand_side( &mut self, expression: JsAnyBinaryLikeExpression, parent_operator: Option, - f: &mut JsFormatter, ) -> FormatResult<()> { let should_flatten = expression.can_flatten()?; if should_flatten { - self.flatten_right_hand_side(expression, parent_operator, f) + self.flatten_right_hand_side(expression, parent_operator) } else { - self.format_new_binary_like_group(expression, parent_operator, f) + self.flatten_new_binary_like_group(expression, parent_operator) } } @@ -318,37 +342,29 @@ impl FlattenItems { &mut self, binary_like_expression: JsAnyBinaryLikeExpression, parent_operator: Option, - f: &mut JsFormatter, ) -> FormatResult<()> { - let right = binary_like_expression.right()?; + let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(binary_like_expression.right()?); let has_comments = right.syntax().has_comments_direct(); - let mut buffer = VecBuffer::new(f.state_mut()); - write!(buffer, [right.format()])?; - let right_formatted = buffer.into_element(); - - let (formatted_node, _) = format_with_or_without_parenthesis( - binary_like_expression.operator()?, - right.syntax(), - right_formatted, - f, - )?; - - let flatten_item = - FlattenItem::regular(formatted_node, parent_operator, has_comments.into()); + let flatten_item = FlattenItem::new( + FlattenedBinaryExpressionPart::Right { + parent: binary_like_expression, + }, + parent_operator, + has_comments.into(), + ); self.items.push(flatten_item); Ok(()) } /// The left hand-side expression and the current operator cannot be flattened. - /// Format the left hand side by its own and potentially wrap it in parentheses before formatting + /// Format the left hand side on its own and potentially wrap it in parentheses before formatting /// the right-hand side of the current expression. - fn format_new_binary_like_group( + fn flatten_new_binary_like_group( &mut self, binary_like_expression: JsAnyBinaryLikeExpression, parent_operator: Option, - f: &mut JsFormatter, ) -> FormatResult<()> { if let Some(last) = self.items.last_mut() { // Remove any line breaks and the trailing operator so that the operator/trailing aren't part @@ -361,13 +377,15 @@ impl FlattenItems { let operator = binary_like_expression.operator()?; let operator_token = binary_like_expression.operator_token()?; - let left_formatted = self.take_format_element(left.syntax(), f)?; - let (left_formatted, _) = - format_with_or_without_parenthesis(operator, left.syntax(), left_formatted, f)?; - let operator_has_trailing_comments = operator_token.has_trailing_comments(); - let mut left_item = FlattenItem::regular( - left_formatted, + let left_parenthesized = needs_parens(operator, &left)?; + let mut left_item = FlattenItem::new( + FlattenedBinaryExpressionPart::Group { + current: left, + expressions_start: self.current_group_start, + expressions_end: self.items.len(), + parenthesized: left_parenthesized, + }, Some(operator_token), operator_has_trailing_comments.into(), ); @@ -376,28 +394,23 @@ impl FlattenItems { left_item = left_item.with_terminator(TrailingTerminator::HardLineBreak); } + self.current_group_start = self.len(); self.items.push(left_item); - let right = binary_like_expression.right()?; - - let mut buffer = VecBuffer::new(f.state_mut()); - write!(buffer, [right.format()])?; - let formatted_node = buffer.into_element(); - - // Format the right node - let (formatted_right, parenthesized) = - format_with_or_without_parenthesis(operator, right.syntax(), formatted_node, f)?; + let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(binary_like_expression.right()?); + // Flatten the right node let parent_operator_has_comments = parent_operator .as_ref() .map(|operator| operator.has_leading_comments()); - let mut right_item = - if !parenthesized && matches!(right, JsAnyExpression::JsLogicalExpression(_)) { - FlattenItem::group(formatted_right, parent_operator, Comments::NoComments) - } else { - FlattenItem::regular(formatted_right, parent_operator, Comments::NoComments) - }; + let mut right_item = FlattenItem::new( + FlattenedBinaryExpressionPart::Right { + parent: binary_like_expression, + }, + parent_operator, + Comments::NoComments, + ); // Format the parent operator if let Some(parent_operator_has_comments) = parent_operator_has_comments { @@ -419,91 +432,6 @@ impl FlattenItems { Ok(()) } - - fn take_format_element( - &mut self, - current_node: &JsSyntaxNode, - f: &mut JsFormatter, - ) -> FormatResult { - let mut buffer = VecBuffer::new(f.state_mut()); - - write!( - buffer, - [format_once(|f| { - let can_hard_group = can_hard_group(&self.items); - - let mut groups = self.items.drain(..).map(|group| { - format_once(move |f| { - // groups not like ["something &&", "something &&" ] - // we want to add a space between them in case they don't break - - f.write_element(group.formatted)?; - - if let Some(operator) = group.operator { - write!(f, [space_token(), operator.format()])?; - } - - match group.terminator { - TrailingTerminator::None => (), - TrailingTerminator::HardLineBreak => write!(f, [hard_line_break()])?, - }; - - Ok(()) - }) - }); - - if can_hard_group { - // we bail early if group doesn't need to be broken. We don't need to do further checks - f.join_with(&space_token()).entries(groups).finish() - } else if is_inside_parenthesis(current_node) { - f.join_with(&soft_line_break_or_space()) - .entries(groups) - .finish() - } else if should_not_indent_if_parent_indents(current_node) { - write!( - f, - [group_elements(&format_once(|f| { - f.join_with(&soft_line_break_or_space()) - .entries(groups) - .finish() - }))] - ) - } else if should_indent_if_parent_inlines(current_node) { - // in order to correctly break, we need to check if the parent created a group - // that breaks or not. In order to do that , we need to create two conditional groups - // that behave differently depending on the situation - write!( - f, - [soft_line_indent_or_space(&group_elements(&format_once( - |f| { - f.join_with(&soft_line_break_or_space()) - .entries(groups) - .finish() - } - )))] - ) - } else { - // if none of the previous conditions is met, - // we take take out the first element from the rest of group, then we hard group the "head" - // and we indent the rest of the groups in a new line - write!(f, [groups.next().unwrap()])?; - - write!( - f, - [group_elements(&soft_line_indent_or_space(&format_once( - |f| { - f.join_with(&soft_line_break_or_space()) - .entries(groups) - .finish() - } - )))] - ) - } - })] - )?; - - Ok(buffer.into_element()) - } } #[derive(Debug)] @@ -529,10 +457,138 @@ impl From for Comments { } } +/// The left or right sub part of a binary expression. +#[derive(Debug)] +enum FlattenedBinaryExpressionPart { + /// The right hand side of a binary expression. Needs to format the parent operator and the right expression + Right { + /// The parent expression + parent: JsAnyBinaryLikeExpression, + }, + + /// The very first left hand side of a binary expression. Only formats the expression + Left { + /// The left hand side expression + expression: JsAnyBinaryLikeLeftExpression, + }, + + /// A group of expressions that can be grouped/printed together. + Group { + /// The binary expression that should be formatted now + current: JsAnyBinaryLikeLeftExpression, + + /// Start end/index into the flattened items array from where the left hand side expressions start + expressions_start: usize, + expressions_end: usize, + + /// Whether to parenthesize the expression + parenthesized: bool, + }, +} + +impl FlattenedBinaryExpressionPart { + fn write(&self, f: &mut JsFormatter, items: &[FlattenItem]) -> FormatResult<()> { + match self { + FlattenedBinaryExpressionPart::Right { parent } => { + let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(parent.right()?); + + write!(f, [format_sub_expression(parent.operator()?, &right)])?; + Ok(()) + } + FlattenedBinaryExpressionPart::Left { expression } => { + write!(f, [expression]) + } + FlattenedBinaryExpressionPart::Group { + current, + expressions_start, + expressions_end, + parenthesized, + } => { + let expressions = &items[*expressions_start..*expressions_end]; + let content = format_with(|f| { + let keep_on_same_line = keep_on_same_line(expressions); + + let mut groups = expressions.iter().map(|group| { + format_with(|f| { + group.expression.write(f, items)?; + + if let Some(operator) = &group.operator { + write!(f, [space_token(), operator.format()])?; + } + + match &group.terminator { + TrailingTerminator::None => (), + TrailingTerminator::HardLineBreak => { + write!(f, [hard_line_break()])? + } + }; + + Ok(()) + }) + }); + + if keep_on_same_line { + // we bail early if group doesn't need to be broken. We don't need to do further checks + f.join_with(space_token()).entries(groups).finish() + } else if is_inside_parenthesis(current.syntax()) { + f.join_with(soft_line_break_or_space()) + .entries(groups) + .finish() + } else if should_not_indent_if_parent_indents(current.syntax()) { + write!( + f, + [group_elements(&format_once(|f| { + f.join_with(soft_line_break_or_space()) + .entries(groups) + .finish() + }))] + ) + } else if should_indent_if_parent_inlines(current) { + write!( + f, + [soft_line_indent_or_space(&group_elements(&format_once( + |f| { + f.join_with(soft_line_break_or_space()) + .entries(groups) + .finish() + } + )))] + ) + } else { + // if none of the previous conditions is met, + // we take out the first element from the rest of the group + // and indent the rest of the groups in a new line + + // SAFETY: Safe because `keep_on_same_line` returns `true` if this is a single + // binary expression without any nested sub expressions. + write!(f, [groups.next().unwrap()])?; + + write!( + f, + [group_elements(&soft_line_indent_or_space(&format_once( + |f| { + f.join_with(soft_line_break_or_space()) + .entries(groups) + .finish() + } + )))] + ) + } + }); + + if *parenthesized { + write!(f, [format_parenthesized(content)]) + } else { + write!(f, [content]) + } + } + } + } +} + #[derive(Debug)] struct FlattenItem { - kind: FlattenItemKind, - formatted: FormatElement, + expression: FlattenedBinaryExpressionPart, operator: Option, terminator: TrailingTerminator, comments: Comments, @@ -545,38 +601,19 @@ enum TrailingTerminator { } impl FlattenItem { - fn regular( - formatted: FormatElement, + fn new( + expression: FlattenedBinaryExpressionPart, operator: Option, comments: Comments, ) -> Self { Self { - formatted, + expression, operator, - kind: FlattenItemKind::Regular, terminator: TrailingTerminator::None, comments, } } - fn group( - formatted: FormatElement, - operator: Option, - comments: Comments, - ) -> Self { - Self { - formatted, - comments, - operator, - terminator: TrailingTerminator::None, - kind: FlattenItemKind::Group, - } - } - - fn is_group(&self) -> bool { - matches!(self.kind, FlattenItemKind::Group) - } - fn has_comments(&self) -> bool { matches!(self.comments, Comments::WithComments) } @@ -592,14 +629,6 @@ impl FlattenItem { } } -#[derive(Debug)] -enum FlattenItemKind { - Regular, - /// Used when the right side of a binary/logical expression is another binary/logical. - /// When we have such cases we - Group, -} - /// The [PostorderIterator] visits every node twice. First on the way down to find the left most binary /// like expression, then on the way back up when it yields the binary like expressions. /// This enum encodes the information whatever the iterator is on its way down (`Enter`) or traversing @@ -756,6 +785,23 @@ impl JsAnyBinaryLikeExpression { JsAnyBinaryLikeExpression::JsInExpression(in_expression) => in_expression.object(), } } + + fn into_expression(self) -> JsAnyExpression { + match self { + JsAnyBinaryLikeExpression::JsLogicalExpression(logical) => { + JsAnyExpression::JsLogicalExpression(logical) + } + JsAnyBinaryLikeExpression::JsBinaryExpression(binary) => { + JsAnyExpression::JsBinaryExpression(binary) + } + JsAnyBinaryLikeExpression::JsInstanceofExpression(instanceof) => { + JsAnyExpression::JsInstanceofExpression(instanceof) + } + JsAnyBinaryLikeExpression::JsInExpression(in_expression) => { + JsAnyExpression::JsInExpression(in_expression) + } + } + } } impl AstNode for JsAnyBinaryLikeExpression {