feat(rome_js_formatter): Format throw and return argument (#2986)

crates/rome_js_formatter/src/js/expressions/parenthesized_expression.rs

@@ -1,5 +1,8 @@
 use crate::prelude::*;
-use crate::utils::{is_simple_expression, FormatPrecedence};
+use crate::utils::{
+    binary_argument_needs_parens, is_simple_expression, FormatPrecedence,
+    JsAnyBinaryLikeLeftExpression,
+};
 use rome_formatter::write;
 
 use crate::utils::JsAnyBinaryLikeExpression;
@@ -36,7 +39,7 @@ impl FormatNodeRule<JsParenthesizedExpression> for FormatJsParenthesizedExpressi
                 write![f, [l_paren_token.format()]]?;
             };
 
-            write![f, [expression.format(),]]?;
+            write![f, [expression.format()]]?;
 
             if parenthesis_can_be_omitted {
                 write!(f, [format_removed(&r_paren_token?)])?;
@@ -49,7 +52,7 @@ impl FormatNodeRule<JsParenthesizedExpression> for FormatJsParenthesizedExpressi
                 f,
                 [
                     format_removed(&l_paren_token?),
-                    group(&expression.format()),
+                    expression.format(),
                     format_removed(&r_paren_token?),
                 ]
             ]?;
@@ -132,6 +135,19 @@ fn parenthesis_can_be_omitted(node: &JsParenthesizedExpression) -> SyntaxResult<
     let expression = node.expression()?;
     let parent = node.syntax().parent();
 
+    if let Some(parent) = &parent {
+        match parent.kind() {
+            // The formatting of the return or throw argument takes care of adding parentheses if necessary
+            JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_THROW_STATEMENT => {
+                return Ok(true)
+            }
+            JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION => return Ok(true),
+            _ => {
+                // fall through
+            }
+        }
+    }
+
     // if expression is a StringLiteralExpression, we need to check it before precedence comparison, here is an example:
     // ```js
     // a[("test")]
@@ -156,34 +172,26 @@ fn parenthesis_can_be_omitted(node: &JsParenthesizedExpression) -> SyntaxResult<
             Some(JsSyntaxKind::JS_EXPRESSION_STATEMENT)
         ));
     }
+
     let parent_precedence = FormatPrecedence::with_precedence_for_parenthesis(parent.as_ref());
     let node_precedence = FormatPrecedence::with_precedence_for_parenthesis(Some(node.syntax()));
 
     if parent_precedence > node_precedence {
         return Ok(false);
     }
-    // Here we handle cases where we have binary/logical expressions.
-    // We want to remove the parenthesis only in cases where `left` and `right` are not other
-    // binary/logical expressions.
-    //
-    // From another point of view, logical/binary expressions with the same operator can stay without
-    // parenthesis.
-    match expression {
-        JsAnyExpression::JsBinaryExpression(expression) => {
-            let left = expression.left()?;
-            let right = expression.right()?;
-
-            Ok(!JsAnyBinaryLikeExpression::can_cast(left.syntax().kind())
-                && !JsAnyBinaryLikeExpression::can_cast(right.syntax().kind()))
-        }
 
-        JsAnyExpression::JsLogicalExpression(expression) => {
-            let left = expression.left()?;
-            let right = expression.right()?;
+    if let Some(parent) = parent {
+        if let Some(binary_like) = JsAnyBinaryLikeExpression::cast(parent) {
+            let operator = binary_like.operator()?;
 
-            Ok(!JsAnyBinaryLikeExpression::can_cast(left.syntax().kind())
-                && !JsAnyBinaryLikeExpression::can_cast(right.syntax().kind()))
+            if !binary_argument_needs_parens(
+                operator,
+                &JsAnyBinaryLikeLeftExpression::from(expression),
+            )? {
+                return Ok(true);
+            }
         }
-        _ => Ok(false),
     }
+
+    Ok(false)
 }

crates/rome_js_formatter/src/js/expressions/sequence_expression.rs

@@ -1,7 +1,7 @@
 use crate::prelude::*;
 
 use rome_formatter::write;
-use rome_js_syntax::JsSyntaxKind::JS_SEQUENCE_EXPRESSION;
+use rome_js_syntax::JsSyntaxKind::{JS_PARENTHESIZED_EXPRESSION, JS_SEQUENCE_EXPRESSION};
 use rome_js_syntax::{JsSequenceExpression, JsSequenceExpressionFields, JsSyntaxKind};
 use rome_rowan::AstNode;
 
@@ -10,61 +10,59 @@ pub struct FormatJsSequenceExpression;
 
 impl FormatNodeRule<JsSequenceExpression> for FormatJsSequenceExpression {
     fn fmt_fields(&self, node: &JsSequenceExpression, f: &mut JsFormatter) -> FormatResult<()> {
-        let first_non_sequence_parent = node
-            .syntax()
-            .ancestors()
-            .find(|p| p.kind() != JS_SEQUENCE_EXPRESSION);
-
-        let is_nested = first_non_sequence_parent != node.syntax().parent();
-
-        let has_already_indentation = first_non_sequence_parent.map_or(false, |parent| {
-            match parent.kind() {
-                // Return statement already does the indentation for us
-                // Arrow function body can't have a sequence expression unless it's parenthesized, otherwise
-                // would be a syntax error
-                JsSyntaxKind::JS_RETURN_STATEMENT => true,
-                JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION => {
-                    // In case we are inside a sequence expression, we have to go up a level and see the great parent.
-                    // Arrow function body and return statements applying indentation for us, so we signal the
-                    // sequence expression to not add other indentation levels
-                    let great_parent = parent.parent().map(|gp| gp.kind());
-
-                    matches!(
-                        great_parent,
-                        Some(
-                            JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION
-                                | JsSyntaxKind::JS_RETURN_STATEMENT
-                                | JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER
-                        )
-                    )
-                }
-                _ => false,
-            }
-        });
-
         let JsSequenceExpressionFields {
             left,
             comma_token,
             right,
         } = node.as_fields();
 
-        let format_content = format_with(|f| {
-            write!(f, [left.format(), comma_token.format()])?;
+        let mut is_nested = false;
+        let mut first_non_sequence_or_paren_parent = None;
 
-            let format_right =
-                format_with(|f| write!(f, [soft_line_break_or_space(), right.format()]));
+        // Skip 1 because ancestor starts with the current node but we're interested in the parent
+        for parent in node.syntax().ancestors().skip(1) {
+            if parent.kind() == JS_SEQUENCE_EXPRESSION {
+                is_nested = true;
+            } else if parent.kind() != JS_PARENTHESIZED_EXPRESSION {
+                first_non_sequence_or_paren_parent = Some(parent);
+                break;
+            }
+        }
 
-            if has_already_indentation {
-                write!(f, [format_right])
-            } else {
-                write!(f, [indent(&format_right)])
+        let format_inner = format_with(|f| {
+            if let Some(parent) = &first_non_sequence_or_paren_parent {
+                if matches!(
+                    parent.kind(),
+                    JsSyntaxKind::JS_EXPRESSION_STATEMENT | JsSyntaxKind::JS_FOR_STATEMENT
+                ) {
+                    return write!(
+                        f,
+                        [
+                            left.format(),
+                            comma_token.format(),
+                            line_suffix_boundary(),
+                            soft_line_indent_or_space(&right.format())
+                        ]
+                    );
+                }
             }
+
+            write!(
+                f,
+                [
+                    left.format(),
+                    comma_token.format(),
+                    line_suffix_boundary(),
+                    soft_line_break_or_space(),
+                    right.format()
+                ]
+            )
         });
 
         if is_nested {
-            write!(f, [format_content])
+            write!(f, [format_inner])
         } else {
-            write!(f, [group(&format_content)])
+            write!(f, [group(&format_inner)])
         }
     }
 }

crates/rome_js_formatter/src/js/statements/return_statement.rs

@@ -1,8 +1,12 @@
 use crate::prelude::*;
-use crate::utils::FormatWithSemicolon;
+use crate::utils::{FormatWithSemicolon, JsAnyBinaryLikeExpression};
 
-use rome_formatter::write;
-use rome_js_syntax::{JsAnyExpression, JsReturnStatement, JsReturnStatementFields};
+use rome_formatter::{format_args, write};
+
+use rome_js_syntax::{
+    JsAnyExpression, JsReturnStatement, JsReturnStatementFields, JsSequenceExpression,
+};
+use rome_rowan::SyntaxResult;
 
 #[derive(Debug, Clone, Default)]
 pub struct FormatJsReturnStatement;
@@ -15,32 +19,96 @@ impl FormatNodeRule<JsReturnStatement> for FormatJsReturnStatement {
             semicolon_token,
         } = node.as_fields();
 
+        let format_inner = format_with(|f| {
+            write!(f, [return_token.format()])?;
+
+            if let Some(argument) = &argument {
+                write!(f, [space(), FormatReturnOrThrowArgument(argument)])?;
+            }
+
+            Ok(())
+        });
+
         write!(
             f,
             [FormatWithSemicolon::new(
-                &format_with(|f| {
-                    write!(f, [return_token.format()])?;
-
-                    if let Some(argument) = &argument {
-                        write!(f, [space()])?;
-
-                        if let JsAnyExpression::JsSequenceExpression(_expression) = argument {
-                            format_parenthesize(
-                                argument.syntax().first_token().as_ref(),
-                                &argument.format(),
-                                argument.syntax().last_token().as_ref(),
-                            )
-                            .grouped_with_soft_block_indent()
-                            .fmt(f)?;
-                        } else {
-                            write![f, [argument.format()]]?;
-                        }
-                    }
-
-                    Ok(())
-                }),
+                &format_inner,
                 semicolon_token.as_ref()
             )]
         )
     }
 }
+
+pub(super) struct FormatReturnOrThrowArgument<'a>(&'a JsAnyExpression);
+
+impl<'a> FormatReturnOrThrowArgument<'a> {
+    pub fn new(argument: &'a JsAnyExpression) -> Self {
+        Self(argument)
+    }
+}
+
+impl Format<JsFormatContext> for FormatReturnOrThrowArgument<'_> {
+    fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
+        let argument = self.0;
+
+        if has_argument_leading_comments(argument)? {
+            let syntax = argument.syntax();
+            let first_token = syntax.first_token();
+            let last_token = syntax.last_token();
+            write!(
+                f,
+                [format_parenthesize(
+                    first_token.as_ref(),
+                    &block_indent(&argument.format()),
+                    last_token.as_ref()
+                )]
+            )
+        } else if is_binary_or_sequence_argument(argument)? {
+            write!(
+                f,
+                [group(&format_args![
+                    if_group_breaks(&text("(")),
+                    soft_block_indent(&argument.format()),
+                    if_group_breaks(&text(")"))
+                ])]
+            )
+        } else {
+            write!(f, [argument.format()])
+        }
+    }
+}
+
+/// Tests if the passed in argument has any leading comments. This is the case if
+/// * the argument's first token has a leading comment
+/// * the argument is a parenthesized expression and the inner expression has a leading comment.
+fn has_argument_leading_comments(argument: &JsAnyExpression) -> SyntaxResult<bool> {
+    if matches!(argument, JsAnyExpression::JsxTagExpression(_)) {
+        // JSX formatting takes care of adding parens
+        return Ok(false);
+    }
+
+    if argument.syntax().has_leading_comments() {
+        return Ok(true);
+    }
+
+    let result = match argument {
+        JsAnyExpression::JsParenthesizedExpression(inner) => {
+            has_argument_leading_comments(&inner.expression()?)?
+        }
+        _ => false,
+    };
+
+    Ok(result)
+}
+
+fn is_binary_or_sequence_argument(argument: &JsAnyExpression) -> SyntaxResult<bool> {
+    if JsSequenceExpression::can_cast(argument.syntax().kind())
+        || JsAnyBinaryLikeExpression::can_cast(argument.syntax().kind())
+    {
+        Ok(true)
+    } else if let JsAnyExpression::JsParenthesizedExpression(inner) = argument {
+        is_binary_or_sequence_argument(&inner.expression()?)
+    } else {
+        Ok(false)
+    }
+}

crates/rome_js_formatter/src/js/statements/throw_statement.rs

@@ -3,6 +3,7 @@ use rome_formatter::{format_args, write};
 
 use crate::utils::FormatWithSemicolon;
 
+use crate::js::statements::return_statement::FormatReturnOrThrowArgument;
 use rome_js_syntax::JsThrowStatement;
 use rome_js_syntax::JsThrowStatementFields;
 
@@ -17,13 +18,14 @@ impl FormatNodeRule<JsThrowStatement> for FormatJsThrowStatement {
             semicolon_token,
         } = node.as_fields();
 
-        let throw_token = throw_token.format();
-        let exception = argument.format();
-
         write!(
             f,
             [FormatWithSemicolon::new(
-                &format_args!(throw_token, space(), exception),
+                &format_args![
+                    throw_token.format(),
+                    space(),
+                    FormatReturnOrThrowArgument::new(&argument?)
+                ],
                 semicolon_token.as_ref()
             )]
         )