From 8678148467156fdb0f7b59fe8b9552aefa790e7f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 2 Jun 2022 10:30:47 +0200 Subject: [PATCH 1/5] Refactor `needs_paren`, `parenthesize` --- .../src/utils/binary_like_expression.rs | 129 +++++++++++------- 1 file changed, 78 insertions(+), 51 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..e13bdf12156 100644 --- a/crates/rome_js_formatter/src/utils/binary_like_expression.rs +++ b/crates/rome_js_formatter/src/utils/binary_like_expression.rs @@ -170,25 +170,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 +210,35 @@ fn format_with_or_without_parenthesis( false }; - let result = if operation_is_higher { - let (leading, content, trailing) = formatted_node.split_trivia(); + Ok(result) +} + +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,14 +247,8 @@ 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 @@ -320,20 +337,21 @@ impl FlattenItems { 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, + write!( + buffer, + [format_sub_expression( + binary_like_expression.operator()?, + &right + )] )?; + let formatted_node = buffer.into_element(); + let flatten_item = FlattenItem::regular(formatted_node, parent_operator, has_comments.into()); self.items.push(flatten_item); @@ -342,7 +360,7 @@ impl FlattenItems { } /// 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( &mut self, @@ -361,9 +379,14 @@ 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 mut left_formatted = self.take_format_element(left.syntax(), f)?; + + if needs_parens(operator, &left)? { + let mut buffer = VecBuffer::new(f.state_mut()); + let format_left = format_once(|f| f.write_element(left_formatted)); + write!(buffer, [format_parenthesized(format_left)])?; + left_formatted = buffer.into_element(); + } let operator_has_trailing_comments = operator_token.has_trailing_comments(); let mut left_item = FlattenItem::regular( @@ -378,26 +401,30 @@ impl FlattenItems { self.items.push(left_item); - let right = binary_like_expression.right()?; + let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(binary_like_expression.right()?); - let mut buffer = VecBuffer::new(f.state_mut()); - write!(buffer, [right.format()])?; - let formatted_node = buffer.into_element(); + let parenthesized = needs_parens(operator, &right)?; // Format the right node - let (formatted_right, parenthesized) = - format_with_or_without_parenthesis(operator, right.syntax(), formatted_node, f)?; + let mut buffer = VecBuffer::new(f.state_mut()); + write!(buffer, [format_sub_expression(operator, &right)])?; + let formatted_right = buffer.into_element(); 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 = if !parenthesized + && matches!( + right, + JsAnyBinaryLikeLeftExpression::JsAnyExpression( + JsAnyExpression::JsLogicalExpression(_) + ) + ) { + FlattenItem::group(formatted_right, parent_operator, Comments::NoComments) + } else { + FlattenItem::regular(formatted_right, parent_operator, Comments::NoComments) + }; // Format the parent operator if let Some(parent_operator_has_comments) = parent_operator_has_comments { From bc008afdddecb7a9aed1d890c9b063b93298f43d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 2 Jun 2022 11:52:40 +0200 Subject: [PATCH 2/5] Lazy write binary like expressions --- .../src/utils/binary_like_expression.rs | 378 +++++++++--------- 1 file changed, 189 insertions(+), 189 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 e13bdf12156..577e73198d4 100644 --- a/crates/rome_js_formatter/src/utils/binary_like_expression.rs +++ b/crates/rome_js_formatter/src/utils/binary_like_expression.rs @@ -100,7 +100,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 +109,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( + FlattenedExpression::Left { expression: left }, Some(parent_operator), has_comments.into(), )); @@ -137,11 +129,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) + write!( + f, + [FlattenedExpression::Group { + current: JsAnyBinaryLikeLeftExpression::JsAnyExpression(current_node.into_expression()), + expressions: flatten_items.items, + parenthesized: false + }] + ) } /// Small wrapper to identify the operation of an expression and deduce their precedence @@ -254,11 +252,8 @@ where /// It tells if the expression can be hard grouped fn can_hard_group(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 { @@ -294,8 +289,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) { @@ -315,18 +310,17 @@ struct FlattenItems { 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) } } @@ -335,25 +329,17 @@ impl FlattenItems { &mut self, binary_like_expression: JsAnyBinaryLikeExpression, parent_operator: Option, - f: &mut JsFormatter, ) -> FormatResult<()> { 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, - [format_sub_expression( - binary_like_expression.operator()?, - &right - )] - )?; - - let formatted_node = buffer.into_element(); - - let flatten_item = - FlattenItem::regular(formatted_node, parent_operator, has_comments.into()); + let flatten_item = FlattenItem::new( + FlattenedExpression::Right { + parent: binary_like_expression, + }, + parent_operator, + has_comments.into(), + ); self.items.push(flatten_item); Ok(()) @@ -362,11 +348,10 @@ impl FlattenItems { /// The left hand-side expression and the current operator cannot be flattened. /// 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 @@ -379,18 +364,14 @@ impl FlattenItems { let operator = binary_like_expression.operator()?; let operator_token = binary_like_expression.operator_token()?; - let mut left_formatted = self.take_format_element(left.syntax(), f)?; - - if needs_parens(operator, &left)? { - let mut buffer = VecBuffer::new(f.state_mut()); - let format_left = format_once(|f| f.write_element(left_formatted)); - write!(buffer, [format_parenthesized(format_left)])?; - left_formatted = buffer.into_element(); - } - 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( + FlattenedExpression::Group { + current: left, + expressions: std::mem::take(&mut self.items), + parenthesized: left_parenthesized, + }, Some(operator_token), operator_has_trailing_comments.into(), ); @@ -403,28 +384,18 @@ impl FlattenItems { let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(binary_like_expression.right()?); - let parenthesized = needs_parens(operator, &right)?; - - // Format the right node - let mut buffer = VecBuffer::new(f.state_mut()); - write!(buffer, [format_sub_expression(operator, &right)])?; - let formatted_right = buffer.into_element(); - + // 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, - JsAnyBinaryLikeLeftExpression::JsAnyExpression( - 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( + FlattenedExpression::Right { + parent: binary_like_expression, + }, + parent_operator, + Comments::NoComments, + ); // Format the parent operator if let Some(parent_operator_has_comments) = parent_operator_has_comments { @@ -446,91 +417,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)] @@ -556,10 +442,134 @@ impl From for Comments { } } +/// The left or right sub part of a binary expression. +#[derive(Debug)] +enum FlattenedExpression { + /// The right hand sie 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, + + /// The left hand side expression the current node + expressions: Vec, + parenthesized: bool, + }, +} + +impl Format for FlattenedExpression { + fn fmt(&self, f: &mut JsFormatter) -> FormatResult<()> { + match self { + FlattenedExpression::Right { parent } => { + let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(parent.right()?); + + write!(f, [format_sub_expression(parent.operator()?, &right)])?; + Ok(()) + } + FlattenedExpression::Left { expression } => { + write!(f, [expression]) + } + FlattenedExpression::Group { + current, + expressions: left_expressions, + parenthesized, + } => { + // take format element + let content = format_with(|f| { + let can_hard_group = can_hard_group(&left_expressions); + + let mut groups = left_expressions.iter().map(|group| { + format_with(|f| { + write!(f, [&group.expression])?; + + 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.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) { + // 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() + } + )))] + ) + } + }); + + if *parenthesized { + write!(f, [format_parenthesized(content)]) + } else { + write!(f, [content]) + } + } + } + } +} + #[derive(Debug)] struct FlattenItem { - kind: FlattenItemKind, - formatted: FormatElement, + expression: FlattenedExpression, operator: Option, terminator: TrailingTerminator, comments: Comments, @@ -572,38 +582,19 @@ enum TrailingTerminator { } impl FlattenItem { - fn regular( - formatted: FormatElement, + fn new( + expression: FlattenedExpression, 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) } @@ -619,14 +610,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 @@ -783,6 +766,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 { From 5bdaae92f9252424262493d51b5ca63cf64ad9f8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 2 Jun 2022 12:09:35 +0200 Subject: [PATCH 3/5] Use slice over vec --- .../src/utils/binary_like_expression.rs | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 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 577e73198d4..fc8200d88e7 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 /// @@ -132,14 +133,14 @@ pub(crate) fn format_binary_like_expression( flatten_items.flatten_binary_expression_right_hand_side(root, None)?; } - write!( - f, - [FlattenedExpression::Group { - current: JsAnyBinaryLikeLeftExpression::JsAnyExpression(current_node.into_expression()), - expressions: flatten_items.items, - parenthesized: false - }] - ) + let group = FlattenedExpression::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 @@ -306,6 +307,17 @@ fn should_indent_if_parent_inlines(current_node: &JsAnyBinaryLikeLeftExpression) #[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 { @@ -369,7 +381,8 @@ impl FlattenItems { let mut left_item = FlattenItem::new( FlattenedExpression::Group { current: left, - expressions: std::mem::take(&mut self.items), + expressions_start: self.current_group_start, + expressions_end: self.items.len(), parenthesized: left_parenthesized, }, Some(operator_token), @@ -380,6 +393,7 @@ impl FlattenItems { left_item = left_item.with_terminator(TrailingTerminator::HardLineBreak); } + self.current_group_start = self.len(); self.items.push(left_item); let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(binary_like_expression.right()?); @@ -462,14 +476,17 @@ enum FlattenedExpression { /// The binary expression that should be formatted now current: JsAnyBinaryLikeLeftExpression, - /// The left hand side expression the current node - expressions: Vec, + /// 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 Format for FlattenedExpression { - fn fmt(&self, f: &mut JsFormatter) -> FormatResult<()> { +impl FlattenedExpression { + fn write(&self, f: &mut JsFormatter, items: &[FlattenItem]) -> FormatResult<()> { match self { FlattenedExpression::Right { parent } => { let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(parent.right()?); @@ -482,16 +499,17 @@ impl Format for FlattenedExpression { } FlattenedExpression::Group { current, - expressions: left_expressions, + expressions_start, + expressions_end, parenthesized, } => { - // take format element + let expressions = &items[*expressions_start..*expressions_end]; let content = format_with(|f| { - let can_hard_group = can_hard_group(&left_expressions); + let can_hard_group = can_hard_group(expressions); - let mut groups = left_expressions.iter().map(|group| { + let mut groups = expressions.iter().map(|group| { format_with(|f| { - write!(f, [&group.expression])?; + group.expression.write(f, items)?; if let Some(operator) = &group.operator { write!(f, [space_token(), operator.format()])?; From 6e8a3cb5899f2017332d4e58bfaff63cf9c99bc6 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 6 Jun 2022 13:11:57 +0200 Subject: [PATCH 4/5] Suppress incorrect clippy warning --- crates/rome_js_formatter/src/utils/binary_like_expression.rs | 2 ++ 1 file changed, 2 insertions(+) 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 fc8200d88e7..1bcc35e751a 100644 --- a/crates/rome_js_formatter/src/utils/binary_like_expression.rs +++ b/crates/rome_js_formatter/src/utils/binary_like_expression.rs @@ -212,6 +212,8 @@ fn needs_parens( 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, From 6a4ab2e64064d8a631f2eea093bd91952769bd71 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 6 Jun 2022 14:37:00 +0200 Subject: [PATCH 5/5] address code review feedback --- .../src/utils/binary_like_expression.rs | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 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 1bcc35e751a..6ccef7bd59e 100644 --- a/crates/rome_js_formatter/src/utils/binary_like_expression.rs +++ b/crates/rome_js_formatter/src/utils/binary_like_expression.rs @@ -119,7 +119,7 @@ pub(crate) fn format_binary_like_expression( let has_comments = left.syntax().has_comments_direct(); flatten_items.items.push(FlattenItem::new( - FlattenedExpression::Left { expression: left }, + FlattenedBinaryExpressionPart::Left { expression: left }, Some(parent_operator), has_comments.into(), )); @@ -133,7 +133,7 @@ pub(crate) fn format_binary_like_expression( flatten_items.flatten_binary_expression_right_hand_side(root, None)?; } - let group = FlattenedExpression::Group { + let group = FlattenedBinaryExpressionPart::Group { current: JsAnyBinaryLikeLeftExpression::JsAnyExpression(current_node.into_expression()), expressions_start: flatten_items.current_group_start, expressions_end: flatten_items.len(), @@ -252,8 +252,7 @@ where }) } -/// 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. // But if there are any trailing comments, break it. flatten_nodes.len() <= 2 && flatten_nodes.iter().all(|node| !node.has_comments()) @@ -348,7 +347,7 @@ impl FlattenItems { let has_comments = right.syntax().has_comments_direct(); let flatten_item = FlattenItem::new( - FlattenedExpression::Right { + FlattenedBinaryExpressionPart::Right { parent: binary_like_expression, }, parent_operator, @@ -381,7 +380,7 @@ impl FlattenItems { let operator_has_trailing_comments = operator_token.has_trailing_comments(); let left_parenthesized = needs_parens(operator, &left)?; let mut left_item = FlattenItem::new( - FlattenedExpression::Group { + FlattenedBinaryExpressionPart::Group { current: left, expressions_start: self.current_group_start, expressions_end: self.items.len(), @@ -406,7 +405,7 @@ impl FlattenItems { .map(|operator| operator.has_leading_comments()); let mut right_item = FlattenItem::new( - FlattenedExpression::Right { + FlattenedBinaryExpressionPart::Right { parent: binary_like_expression, }, parent_operator, @@ -460,8 +459,8 @@ impl From for Comments { /// The left or right sub part of a binary expression. #[derive(Debug)] -enum FlattenedExpression { - /// The right hand sie of a binary expression. Needs to format the parent operator and the right expression +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, @@ -487,19 +486,19 @@ enum FlattenedExpression { }, } -impl FlattenedExpression { +impl FlattenedBinaryExpressionPart { fn write(&self, f: &mut JsFormatter, items: &[FlattenItem]) -> FormatResult<()> { match self { - FlattenedExpression::Right { parent } => { + FlattenedBinaryExpressionPart::Right { parent } => { let right = JsAnyBinaryLikeLeftExpression::JsAnyExpression(parent.right()?); write!(f, [format_sub_expression(parent.operator()?, &right)])?; Ok(()) } - FlattenedExpression::Left { expression } => { + FlattenedBinaryExpressionPart::Left { expression } => { write!(f, [expression]) } - FlattenedExpression::Group { + FlattenedBinaryExpressionPart::Group { current, expressions_start, expressions_end, @@ -507,7 +506,7 @@ impl FlattenedExpression { } => { let expressions = &items[*expressions_start..*expressions_end]; let content = format_with(|f| { - let can_hard_group = can_hard_group(expressions); + let keep_on_same_line = keep_on_same_line(expressions); let mut groups = expressions.iter().map(|group| { format_with(|f| { @@ -528,7 +527,7 @@ impl FlattenedExpression { }) }); - if can_hard_group { + 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()) { @@ -545,9 +544,6 @@ impl FlattenedExpression { }))] ) } else if should_indent_if_parent_inlines(current) { - // 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( @@ -560,8 +556,11 @@ impl FlattenedExpression { ) } 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 + // 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!( @@ -589,7 +588,7 @@ impl FlattenedExpression { #[derive(Debug)] struct FlattenItem { - expression: FlattenedExpression, + expression: FlattenedBinaryExpressionPart, operator: Option, terminator: TrailingTerminator, comments: Comments, @@ -603,7 +602,7 @@ enum TrailingTerminator { impl FlattenItem { fn new( - expression: FlattenedExpression, + expression: FlattenedBinaryExpressionPart, operator: Option, comments: Comments, ) -> Self {