-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(noExtraBooleanCast): preserve parentheses to maintain operator precedence #7244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
a14fcca
4e5ffe6
57b2305
32ba1e7
27c1c87
3b2aebe
2196fdc
040defa
600d5fa
0500c6b
1617b75
d2e9574
4fedd9f
fbac80e
7a79776
3e8c850
70066e5
81ad304
e1f57f3
4f07e18
196f806
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#7225](https://github.com/biomejs/biome/issues/7225): The noExtraBooleanCast rule now preserves parentheses when removing Boolean calls inside negations. | ||
|
|
||
| ```js | ||
| // Before | ||
| !Boolean(b0 && b1) | ||
| // After | ||
| !(b0 && b1) // instead of !b0 && b1 | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,12 @@ use biome_analyze::{ | |
| }; | ||
| use biome_console::markup; | ||
| use biome_diagnostics::Severity; | ||
| use biome_js_factory::make; | ||
| use biome_js_syntax::{ | ||
| AnyJsExpression, JsCallArgumentList, JsCallArguments, JsCallExpression, JsNewExpression, | ||
| JsSyntaxNode, JsUnaryOperator, is_in_boolean_context, is_negation, | ||
| AnyJsExpression, JsAssignmentExpression, JsBinaryExpression, JsCallArgumentList, | ||
| JsCallArguments, JsCallExpression, JsConditionalExpression, JsLogicalExpression, JsNewExpression, | ||
| JsParenthesizedExpression, JsSequenceExpression, JsSyntaxNode, JsUnaryExpression, JsUnaryOperator, T, | ||
| is_in_boolean_context, is_negation, | ||
| }; | ||
| use biome_rowan::{AstNode, AstSeparatedList, BatchMutationExt}; | ||
| use biome_rule_options::no_extra_boolean_cast::NoExtraBooleanCastOptions; | ||
|
|
@@ -188,7 +191,36 @@ impl Rule for NoExtraBooleanCast { | |
| ExtraBooleanCastType::DoubleNegation => "Remove redundant double-negation", | ||
| ExtraBooleanCastType::BooleanCall => "Remove redundant `Boolean` call", | ||
| }; | ||
| mutation.replace_node(node.clone(), node_to_replace.clone()); | ||
|
|
||
| // Check if the Boolean call is inside a unary negation and the argument needs parentheses | ||
| let replacement = if matches!(extra_boolean_cast_type, ExtraBooleanCastType::BooleanCall) { | ||
| // Check if this Boolean call is inside a unary negation | ||
| if let Some(unary_expr) = node.syntax().parent().and_then(JsUnaryExpression::cast) { | ||
| if matches!(unary_expr.operator(), Ok(JsUnaryOperator::LogicalNot)) { | ||
| // Check if the argument is a complex expression that needs parentheses | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can definitely reduce the indentation here a bit. Something like: if let Some(unary_expr) = node.syntax().parent().and_then(JsUnaryExpression::cast).map(|e| e.operator()).ok().is_some_and(|op| op == JsUnaryOperator::LogicalNot) |
||
| if needs_parentheses_when_negated(node_to_replace) { | ||
| // Wrap in parentheses to preserve operator precedence | ||
| AnyJsExpression::JsParenthesizedExpression( | ||
| make::js_parenthesized_expression( | ||
| make::token(T!['(']), | ||
| node_to_replace.clone(), | ||
| make::token(T![')']), | ||
| ), | ||
| ) | ||
| } else { | ||
| node_to_replace.clone() | ||
| } | ||
| } else { | ||
| node_to_replace.clone() | ||
| } | ||
| } else { | ||
| node_to_replace.clone() | ||
| } | ||
| } else { | ||
| node_to_replace.clone() | ||
| }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And you can definitely refactor this so that these elses aren't necessary. Something like this probably: let mut replacement = ...;
if cond {
replacement = AnyJsExpression::JsParenthesizedExpression(
make::js_parenthesized_expression(
make::token(T!['(']),
replacement,
make::token(T![')']),
),
)
}
mutation.replace_node(node.clone(), replacement); |
||
|
|
||
| mutation.replace_node(node.clone(), replacement); | ||
|
|
||
| Some(JsRuleAction::new( | ||
| ctx.metadata().action_category(ctx.category(), ctx.group()), | ||
|
|
@@ -199,6 +231,27 @@ impl Rule for NoExtraBooleanCast { | |
| } | ||
| } | ||
|
|
||
| /// Determines if an expression needs parentheses when it becomes the operand of a unary negation. | ||
| /// This is needed to preserve operator precedence for expressions like binary expressions. | ||
| fn needs_parentheses_when_negated(expr: &AnyJsExpression) -> bool { | ||
| match expr { | ||
| // Binary expressions like `a + b` need parentheses in `!(a + b)` to maintain precedence | ||
| AnyJsExpression::JsBinaryExpression(_) => true, | ||
| // Logical expressions like `a && b` need parentheses in `!(a && b)` to maintain precedence | ||
| AnyJsExpression::JsLogicalExpression(_) => true, | ||
| // Conditional expressions like `a ? b : c` need parentheses | ||
| AnyJsExpression::JsConditionalExpression(_) => true, | ||
| // Assignment expressions need parentheses | ||
| AnyJsExpression::JsAssignmentExpression(_) => true, | ||
| // Sequence expressions (comma operator) need parentheses | ||
| AnyJsExpression::JsSequenceExpression(_) => true, | ||
| // Logical expressions that are already parenthesized don't need additional ones | ||
| AnyJsExpression::JsParenthesizedExpression(_) => false, | ||
| // Simple expressions like identifiers, literals, calls don't need parentheses | ||
| _ => false, | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| /// Check if the SyntaxNode is a Double Negation. Including the edge case | ||
| /// ```js | ||
| /// !(!x) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: to remove |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: to remove |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: to remove |
Uh oh!
There was an error while loading. Please reload this page.