fix(js_formatter): keep parens for some optional chains (#897)

crates/biome_js_analyze/src/analyzers/correctness/no_unsafe_optional_chaining.rs

@@ -1,11 +1,11 @@
 use biome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
 use biome_console::markup;
 use biome_js_syntax::{
-    AnyJsAssignmentPattern, AnyJsBindingPattern, AnyJsExpression, JsAssignmentExpression,
-    JsAssignmentWithDefault, JsAwaitExpression, JsCallExpression, JsComputedMemberExpression,
-    JsConditionalExpression, JsExtendsClause, JsForOfStatement, JsInExpression,
-    JsInitializerClause, JsInstanceofExpression, JsLogicalExpression, JsLogicalOperator,
-    JsNewExpression, JsObjectAssignmentPatternProperty, JsObjectMemberList,
+    AnyJsAssignmentPattern, AnyJsBindingPattern, AnyJsOptionalChainExpression,
+    JsAssignmentExpression, JsAssignmentWithDefault, JsAwaitExpression, JsCallExpression,
+    JsComputedMemberExpression, JsConditionalExpression, JsExtendsClause, JsForOfStatement,
+    JsInExpression, JsInitializerClause, JsInstanceofExpression, JsLogicalExpression,
+    JsLogicalOperator, JsNewExpression, JsObjectAssignmentPatternProperty, JsObjectMemberList,
     JsParenthesizedExpression, JsSequenceExpression, JsSpread, JsStaticMemberExpression,
     JsTemplateExpression, JsVariableDeclarator, JsWithStatement,
 };
@@ -69,19 +69,19 @@ declare_rule! {
 }
 
 impl Rule for NoUnsafeOptionalChaining {
-    type Query = Ast<QueryNode>;
+    type Query = Ast<AnyJsOptionalChainExpression>;
     type State = TextRange;
     type Signals = Option<Self::State>;
     type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Self::Signals {
-        let query_node = ctx.query();
+        let node = ctx.query();
 
         // need to check only optional chain nodes
-        if !query_node.is_optional() {
+        if !node.is_optional() {
             return None;
         }
-        let mut node: RuleNode = RuleNode::cast_ref(query_node.syntax())?;
+        let mut node: RuleNode = RuleNode::cast_ref(node.syntax())?;
         let mut parent = node.parent::<RuleNode>();
         // parentheses limit the scope of short-circuiting in chains
         // (a?.b).c // here we have an error
@@ -288,12 +288,11 @@ impl Rule for NoUnsafeOptionalChaining {
     }
 
     fn diagnostic(ctx: &RuleContext<Self>, range: &Self::State) -> Option<RuleDiagnostic> {
-        let query_node = ctx.query();
-
+        let node = ctx.query();
         Some(
             RuleDiagnostic::new(
                 rule_category!(),
-                query_node.range(),
+                node.optional_chain_token()?.text_trimmed_range(),
                 markup! {
                     "Unsafe usage of optional chaining."
                 },
@@ -306,54 +305,6 @@ impl Rule for NoUnsafeOptionalChaining {
     }
 }
 
-declare_node_union! {
-    pub(crate) QueryNode = JsCallExpression | JsStaticMemberExpression | JsComputedMemberExpression
-}
-
-impl QueryNode {
-    pub fn is_optional(&self) -> bool {
-        match self {
-            QueryNode::JsCallExpression(expression) => expression.is_optional(),
-            QueryNode::JsStaticMemberExpression(expression) => expression.is_optional(),
-            QueryNode::JsComputedMemberExpression(expression) => expression.is_optional(),
-        }
-    }
-
-    pub fn range(&self) -> Option<TextRange> {
-        let token = match self {
-            QueryNode::JsCallExpression(expression) => expression.optional_chain_token(),
-            QueryNode::JsStaticMemberExpression(expression) => expression.operator_token().ok(),
-            QueryNode::JsComputedMemberExpression(expression) => expression.optional_chain_token(),
-        };
-
-        Some(token?.text_trimmed_range())
-    }
-}
-
-impl From<QueryNode> for AnyJsExpression {
-    fn from(node: QueryNode) -> AnyJsExpression {
-        match node {
-            QueryNode::JsCallExpression(expression) => expression.into(),
-            QueryNode::JsStaticMemberExpression(expression) => expression.into(),
-            QueryNode::JsComputedMemberExpression(expression) => expression.into(),
-        }
-    }
-}
-
-impl From<QueryNode> for RuleNode {
-    fn from(node: QueryNode) -> RuleNode {
-        match node {
-            QueryNode::JsCallExpression(expression) => RuleNode::JsCallExpression(expression),
-            QueryNode::JsStaticMemberExpression(expression) => {
-                RuleNode::JsStaticMemberExpression(expression)
-            }
-            QueryNode::JsComputedMemberExpression(expression) => {
-                RuleNode::JsComputedMemberExpression(expression)
-            }
-        }
-    }
-}
-
 declare_node_union! {
     /// Only these variants of the union can be part of an unsafe optional chain.
     pub(crate) RuleNode =
@@ -377,3 +328,19 @@ declare_node_union! {
     | JsInstanceofExpression
     | JsAssignmentWithDefault
 }
+
+impl From<AnyJsOptionalChainExpression> for RuleNode {
+    fn from(node: AnyJsOptionalChainExpression) -> RuleNode {
+        match node {
+            AnyJsOptionalChainExpression::JsCallExpression(expression) => {
+                RuleNode::JsCallExpression(expression)
+            }
+            AnyJsOptionalChainExpression::JsStaticMemberExpression(expression) => {
+                RuleNode::JsStaticMemberExpression(expression)
+            }
+            AnyJsOptionalChainExpression::JsComputedMemberExpression(expression) => {
+                RuleNode::JsComputedMemberExpression(expression)
+            }
+        }
+    }
+}

crates/biome_js_formatter/src/js/expressions/computed_member_expression.rs

@@ -75,7 +75,7 @@ impl Format<JsFormatContext> for FormatComputedMemberLookup<'_> {
 
 impl NeedsParentheses for JsComputedMemberExpression {
     fn needs_parentheses_with_parent(&self, parent: &JsSyntaxNode) -> bool {
-        if self.is_optional_chain() && matches!(parent.kind(), JsSyntaxKind::JS_NEW_EXPRESSION) {
+        if matches!(parent.kind(), JsSyntaxKind::JS_NEW_EXPRESSION) && self.is_optional_chain() {
             return true;
         }
 
@@ -99,7 +99,6 @@ mod tests {
         );
         assert_needs_parentheses!("new (test()![member])()", JsComputedMemberExpression);
 
-        assert_needs_parentheses!("new (a?.b[c])()", JsComputedMemberExpression);
         assert_not_needs_parentheses!("new (test[a])()", JsComputedMemberExpression);
     }
 }

crates/biome_js_formatter/src/js/expressions/static_member_expression.rs

@@ -172,7 +172,7 @@ impl AnyJsStaticMemberLike {
 
 impl NeedsParentheses for JsStaticMemberExpression {
     fn needs_parentheses_with_parent(&self, parent: &JsSyntaxNode) -> bool {
-        if self.is_optional_chain() && matches!(parent.kind(), JsSyntaxKind::JS_NEW_EXPRESSION) {
+        if matches!(parent.kind(), JsSyntaxKind::JS_NEW_EXPRESSION) && self.is_optional_chain() {
             return true;
         }
 
@@ -217,8 +217,6 @@ mod tests {
         assert_needs_parentheses!("new (test()`template`.length)()", JsStaticMemberExpression);
         assert_needs_parentheses!("new (test()!.member)()", JsStaticMemberExpression);
 
-        assert_needs_parentheses!("new (foo?.bar)();", JsStaticMemberExpression);
-
         assert_not_needs_parentheses!("new (test.a)()", JsStaticMemberExpression);
     }
 }

crates/biome_js_formatter/src/syntax_rewriter.rs

@@ -2,8 +2,8 @@ use crate::comments::is_type_comment;
 use crate::parentheses::AnyJsParenthesized;
 use biome_formatter::{TransformSourceMap, TransformSourceMapBuilder};
 use biome_js_syntax::{
-    AnyJsAssignment, AnyJsExpression, AnyTsType, JsLanguage, JsLogicalExpression, JsSyntaxKind,
-    JsSyntaxNode,
+    AnyJsAssignment, AnyJsExpression, AnyJsOptionalChainExpression, AnyTsType, JsLanguage,
+    JsLogicalExpression, JsSyntaxKind, JsSyntaxNode,
 };
 use biome_rowan::syntax::SyntaxTrivia;
 use biome_rowan::{
@@ -140,7 +140,6 @@ impl JsFormatSyntaxRewriter {
         ) {
             (Ok(l_paren), Ok(inner), Ok(r_paren)) => {
                 let prev_token = l_paren.prev_token();
-
                 // Keep parentheses around unknown expressions. Biome can't know the precedence.
                 if inner.kind().is_bogus()
                     // Don't remove parentheses if the expression is a decorator
@@ -150,6 +149,9 @@ impl JsFormatSyntaxRewriter {
                     || has_type_cast_comment_or_skipped(&l_paren.leading_trivia())
                     || prev_token.map_or(false, |prev_token| has_type_cast_comment_or_skipped(&prev_token.trailing_trivia()))
                     || r_paren.leading_trivia().has_skipped()
+                    // Don't remove parentheses if it is an optional chain inside a chain that doesn't start by an optional token
+                    // (a?.b).c
+                    || (parenthesized.syntax().parent().and_then(AnyJsOptionalChainExpression::cast).is_some_and(|chain| chain.optional_chain_token().is_none()) && AnyJsOptionalChainExpression::cast_ref(&inner).is_some_and(|x| x.is_optional_chain()))
                 {
                     return VisitNodeSignal::Traverse(parenthesized.into_syntax());
                 } else {