Skip to content

Commit

Permalink
fix(lint/useExponentiationOperator): avoid incorrect code fixes (#121)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos authored Sep 4, 2023
1 parent 86b5fdc commit 9649616
Show file tree
Hide file tree
Showing 23 changed files with 492 additions and 639 deletions.
28 changes: 28 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,18 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
+ `\`${v}\``;
```

- [useExponentiationOperator](https://biomejs.dev/linter/rules/use_exponentiation_operator/) suggests better code fixes.

The rule now preserves any comment preceding the exponent,
and it preserves any parenthesis around the base or the exponent.
It also adds spaces around the exponentiation operator `**`,
and always adds parentheses for pre- and post-updates.

```diff
- Math.pow(a++, /**/ (2))
+ (a++) ** /**/ (2)
```

#### Bug fixes

- Fix [#80](https://github.com/biomejs/biome/issues/95), making [noDuplicateJsxProps](https://biomejs.dev/linter/rules/noDuplicateJsxProps/) case-insensitive.
Expand Down Expand Up @@ -99,6 +111,22 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom

As a sideeffect, the rule also suggests the removal of any inner comments.

- Fix [rome#3850](https://github.com/rome/tools/issues/3850)

Previously [useExponentiationOperator](https://biomejs.dev/linter/rules/use_exponentiation_operator/) suggested invalid code in a specific edge case:

```diff
- 1 +Math.pow(++a, 2)
+ 1 +++a**2
```

Now, the rule properly adds parentheses:

```diff
- 1 +Math.pow(++a, 2)
+ 1 +(++a) ** 2
```

- Fix [#106](https://github.com/biomejs/biome/issues/106)

[noUndeclaredVariables](https://biomejs.dev/linter/rules/noUndeclaredVariables/) now correctly recognizes some TypeScript types such as `Uppercase`.
Expand Down
7 changes: 1 addition & 6 deletions crates/rome_js_analyze/src/analyzers/complexity/use_optional_chain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,7 @@ impl Rule for UseOptionalChain {
let need_parenthesis =
left.precedence().ok()? < OperatorPrecedence::LeftHandSide;
if need_parenthesis {
left = make::js_parenthesized_expression(
make::token(T!['(']),
left,
make::token(T![')']),
)
.into();
left = make::parenthesized(left).into();
}
let next_member = match member.clone() {
AnyJsMemberExpression::JsStaticMemberExpression(expression) => {
Expand Down
4 changes: 1 addition & 3 deletions crates/rome_js_analyze/src/analyzers/complexity/use_simplified_logic_expression.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,8 @@ fn simplify_de_morgan(node: &JsLogicalExpression) -> Option<JsUnaryExpression> {
next_logic_expression = next_logic_expression.with_right(right.argument().ok()?);
Some(make::js_unary_expression(
make::token(T![!]),
AnyJsExpression::JsParenthesizedExpression(make::js_parenthesized_expression(
make::token(T!['(']),
AnyJsExpression::JsParenthesizedExpression(make::parenthesized(
AnyJsExpression::JsLogicalExpression(next_logic_expression),
make::token(T![')']),
)),
))
}
Expand Down
9 changes: 1 addition & 8 deletions crates/rome_js_analyze/src/analyzers/nursery/use_arrow_function.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,14 +279,7 @@ fn to_arrow_body(body: JsFunctionBody) -> AnyJsFunctionBody {
};
if first_token.kind() == T!['{'] {
// () => ({ ... })
result = AnyJsFunctionBody::AnyJsExpression(
make::js_parenthesized_expression(
make::token(T!['(']),
return_arg,
make::token(T![')']),
)
.into(),
);
result = AnyJsFunctionBody::AnyJsExpression(make::parenthesized(return_arg).into());
}
result
}
208 changes: 78 additions & 130 deletions crates/rome_js_analyze/src/analyzers/style/use_exponentiation_operator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,19 @@ use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::{make, syntax::T};
use rome_js_syntax::{
global_identifier, AnyJsExpression, AnyJsMemberExpression, JsBinaryOperator, JsCallExpression,
JsClassDeclaration, JsClassExpression, JsExtendsClause, JsInExpression, OperatorPrecedence,
global_identifier, AnyJsCallArgument, AnyJsExpression, AnyJsMemberExpression, JsBinaryOperator,
JsCallExpression, JsClassDeclaration, JsClassExpression, JsExtendsClause, JsInExpression,
OperatorPrecedence,
};
use rome_rowan::{
trim_leading_trivia_pieces, AstNode, AstSeparatedList, BatchMutationExt, SyntaxResult,
};
use rome_rowan::{AstNode, AstSeparatedList, BatchMutationExt};

declare_rule! {
/// Disallow the use of `Math.pow` in favor of the `**` operator.
///
/// > Introduced in ES2016, the infix exponentiation operator ** is an alternative for the standard Math.pow function.
/// > Infix notation is considered to be more readable and thus more preferable than the function notation.
/// Introduced in ES2016, the infix exponentiation operator `**` is an alternative for the standard `Math.pow` function.
/// Infix notation is considered to be more readable and thus more preferable than the function notation.
///
/// Source: https://eslint.org/docs/latest/rules/prefer-exponentiation-operator
///
Expand Down Expand Up @@ -58,45 +61,6 @@ declare_rule! {
}
}

pub struct MathPowCall {
base: AnyJsExpression,
exponent: AnyJsExpression,
}

impl MathPowCall {
fn make_base(&self) -> Option<AnyJsExpression> {
Some(if self.does_base_need_parens()? {
parenthesize_any_js_expression(&self.base)
} else {
self.base.clone()
})
}

fn make_exponent(&self) -> Option<AnyJsExpression> {
Some(if self.does_exponent_need_parens()? {
parenthesize_any_js_expression(&self.exponent)
} else {
self.exponent.clone()
})
}

/// Determines whether the base expression needs parens in an exponentiation binary expression.
fn does_base_need_parens(&self) -> Option<bool> {
Some(
// '**' is right-associative, parens are needed when Math.pow(a ** b, c) is converted to (a ** b) ** c
self.base.precedence().ok()? <= OperatorPrecedence::Exponential
// An unary operator cannot be used immediately before an exponentiation expression
|| self.base.as_js_unary_expression().is_some()
|| self.base.as_js_await_expression().is_some(),
)
}

/// Determines whether the exponent expression needs parens in an exponentiation binary expression.
fn does_exponent_need_parens(&self) -> Option<bool> {
Some(self.exponent.precedence().ok()? < OperatorPrecedence::Exponential)
}
}

impl Rule for UseExponentiationOperator {
type Query = Semantic<JsCallExpression>;
type State = ();
Expand All @@ -120,52 +84,63 @@ impl Rule for UseExponentiationOperator {
}

fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
let diagnostic = RuleDiagnostic::new(
Some(RuleDiagnostic::new(
rule_category!(),
ctx.query().range(),
"Use the '**' operator instead of 'Math.pow'.",
);

Some(diagnostic)
))
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
let node = ctx.query();

if !should_suggest_fix(node)? {
let args = node.arguments().ok()?;
let [Some(AnyJsCallArgument::AnyJsExpression(base)), Some(AnyJsCallArgument::AnyJsExpression(exponent)), None] =
node.get_arguments_by_index([0, 1, 2])
else {
return None;
}

let mut mutation = ctx.root().begin();
let [base, exponent] = node.get_arguments_by_index([0, 1]);

let math_pow_call = MathPowCall {
base: base?.as_any_js_expression()?.clone().omit_parentheses(),
exponent: exponent?.as_any_js_expression()?.clone().omit_parentheses(),
};

let new_node = make::js_binary_expression(
math_pow_call.make_base()?,
make::token(T![**]),
math_pow_call.make_exponent()?,
);

if let Some((needs_parens, parent)) = does_exponentiation_expression_need_parens(node) {
let base = if does_base_need_parens(&base).ok()? {
make::parenthesized(base).into()
} else {
base
};
let exponent = if does_exponent_need_parens(&exponent).ok()? {
make::parenthesized(exponent).into()
} else {
exponent
};
let comma_separator = args.args().separators().next()?.ok()?;
// Transfer comments before and after `base` and `exponent`
// which are associated with the comma or a paren.
let base = base
.prepend_trivia_pieces(args.l_paren_token().ok()?.trailing_trivia().pieces())?
.append_trivia_pieces(comma_separator.leading_trivia().pieces())?;
let exponent = exponent
.prepend_trivia_pieces(trim_leading_trivia_pieces(
comma_separator.trailing_trivia().pieces(),
))?
.append_trivia_pieces(args.r_paren_token().ok()?.leading_trivia().pieces())?;
let mut mutation = ctx.root().begin();
let new_node = AnyJsExpression::from(make::js_binary_expression(
base,
make::token_decorated_with_space(T![**]),
exponent,
));
let new_node = if let Some((needs_parens, parent)) =
does_exponentiation_expression_need_parens(node)
{
if needs_parens && parent.is_some() {
mutation.replace_node(parent.clone()?, parenthesize_any_js_expression(&parent?));
mutation.replace_node(parent.clone()?, make::parenthesized(parent?).into());
}

mutation.replace_node(
AnyJsExpression::from(node.clone()),
parenthesize_any_js_expression(&AnyJsExpression::from(new_node)),
);
make::parenthesized(new_node).into()
} else {
mutation.replace_node(
AnyJsExpression::from(node.clone()),
AnyJsExpression::from(new_node),
);
}

new_node
};
// Transfer leading and trailing comments
let new_node = new_node
.prepend_trivia_pieces(node.syntax().first_leading_trivia()?.pieces())?
.append_trivia_pieces(node.syntax().last_trailing_trivia()?.pieces())?;
mutation.replace_node_discard_trivia(AnyJsExpression::from(node.clone()), new_node);
Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
Expand All @@ -175,35 +150,6 @@ impl Rule for UseExponentiationOperator {
}
}

/// Verify if the autofix is safe to be applied and won't remove comments.
/// Argument list is considered valid if there's no spread arg and leading/trailing comments.
fn should_suggest_fix(node: &JsCallExpression) -> Option<bool> {
let arguments = node.arguments().ok()?;
let args_count = arguments.args().len();

Some(
args_count == 2
&& !arguments.l_paren_token().ok()?.has_leading_comments()
&& !arguments.l_paren_token().ok()?.has_trailing_comments()
&& !arguments.r_paren_token().ok()?.has_leading_comments()
&& !arguments.r_paren_token().ok()?.has_trailing_comments()
&& arguments.args().into_iter().flatten().all(|arg| {
!arg.syntax().has_leading_comments()
&& !arg.syntax().has_trailing_comments()
&& arg.as_js_spread().is_none()
}),
)
}

/// Wraps a [AnyJsExpression] in paretheses
fn parenthesize_any_js_expression(expr: &AnyJsExpression) -> AnyJsExpression {
AnyJsExpression::from(make::js_parenthesized_expression(
make::token(T!['(']),
expr.clone(),
make::token(T![')']),
))
}

/// Determines whether the given parent node needs parens if used as the exponent in an exponentiation binary expression.
fn does_exponentiation_expression_need_parens(
node: &JsCallExpression,
Expand All @@ -216,15 +162,13 @@ fn does_exponentiation_expression_need_parens(
if extends_clause.parent::<JsClassDeclaration>().is_some() {
return Some((true, None));
}

if let Some(class_expr) = extends_clause.parent::<JsClassExpression>() {
let class_expr = AnyJsExpression::from(class_expr);
if does_expression_need_parens(node, &class_expr)? {
return Some((true, Some(class_expr)));
}
}
}

None
}

Expand All @@ -235,48 +179,37 @@ fn does_expression_need_parens(
) -> Option<bool> {
let needs_parentheses = match &expression {
// Skips already parenthesized expressions
AnyJsExpression::JsParenthesizedExpression(_) => return None,
AnyJsExpression::JsParenthesizedExpression(_) => return Some(false),
AnyJsExpression::JsBinaryExpression(bin_expr) => {
if bin_expr.parent::<JsInExpression>().is_some() {
return Some(true);
}

let binding = bin_expr.right().ok()?;
let call_expr = binding.as_js_call_expression();

bin_expr.operator().ok()? != JsBinaryOperator::Exponent
|| call_expr.is_none()
|| call_expr? != node
}
AnyJsExpression::JsCallExpression(call_expr) => !call_expr
AnyJsExpression::JsCallExpression(call_expr) => call_expr
.arguments()
.ok()?
.args()
.iter()
.filter_map(|arg| {
let binding = arg.ok()?;
return binding
.as_any_js_expression()?
.as_js_call_expression()
.cloned();
.find_map(|arg| {
Some(arg.ok()?.as_any_js_expression()?.as_js_call_expression()? == node)
})
.any(|arg| &arg == node),
AnyJsExpression::JsNewExpression(new_expr) => !new_expr
.is_none(),
AnyJsExpression::JsNewExpression(new_expr) => new_expr
.arguments()?
.args()
.iter()
.filter_map(|arg| {
let binding = arg.ok()?;
return binding
.as_any_js_expression()?
.as_js_call_expression()
.cloned();
.find_map(|arg| {
Some(arg.ok()?.as_any_js_expression()?.as_js_call_expression()? == node)
})
.any(|arg| &arg == node),
.is_none(),
AnyJsExpression::JsComputedMemberExpression(member_expr) => {
let binding = member_expr.member().ok()?;
let call_expr = binding.as_js_call_expression();

call_expr.is_none() || call_expr? != node
}
AnyJsExpression::JsInExpression(_) => return Some(true),
Expand All @@ -286,6 +219,21 @@ fn does_expression_need_parens(
| AnyJsExpression::JsTemplateExpression(_) => true,
_ => false,
};

Some(needs_parentheses && expression.precedence().ok()? >= OperatorPrecedence::Exponential)
}

fn does_base_need_parens(base: &AnyJsExpression) -> SyntaxResult<bool> {
// '**' is right-associative, parens are needed when Math.pow(a ** b, c) is converted to (a ** b) ** c
Ok(base.precedence()? <= OperatorPrecedence::Exponential
// An unary operator cannot be used immediately before an exponentiation expression
|| base.as_js_unary_expression().is_some()
|| base.as_js_await_expression().is_some()
// Parenthesis could be avoided in the following cases.
// However, this improves readability.
|| base.as_js_pre_update_expression().is_some()
|| base.as_js_post_update_expression().is_some())
}

fn does_exponent_need_parens(exponent: &AnyJsExpression) -> SyntaxResult<bool> {
Ok(exponent.precedence()? < OperatorPrecedence::Exponential)
}
Loading

0 comments on commit 9649616

Please sign in to comment.