Skip to content

Commit

Permalink
refactor(lint/useExponentiationOperator): avoid incorrect code fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@Conaclos
Conaclos committed Sep 2, 2023
1 parent e2b368f commit 27537a2
Show file tree
Hide file tree
Showing 15 changed files with 345 additions and 533 deletions.
183 changes: 69 additions & 114 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,10 +6,11 @@ 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::{AstNode, AstSeparatedList, BatchMutationExt};
use rome_rowan::{AstNode, AstSeparatedList, BatchMutationExt, SyntaxResult};

declare_rule! {
/// Disallow the use of `Math.pow` in favor of the `**` operator.
Expand Down Expand Up @@ -58,45 +59,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 +82,38 @@ 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)? {
if !should_suggest_fix(node).ok()? {
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 Some(AnyJsCallArgument::AnyJsExpression(base)) = base else {
return None;
};

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

let Some(AnyJsCallArgument::AnyJsExpression(exponent)) = exponent else {
return None;
};
let mut new_node = AnyJsExpression::from(make::js_binary_expression(
make_base(base).ok()?,
make::token_decorated_with_space(T![**]),
make_exponent(exponent).ok()?,
));
let mut mutation = ctx.root().begin();
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()?, parenthesize_any_js_expression(parent?));
}

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

mutation.replace_node(AnyJsExpression::from(node.clone()), new_node);
Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
Expand All @@ -175,31 +123,25 @@ impl Rule for UseExponentiationOperator {
}
}

/// Verify if the autofix is safe to be applied and won't remove comments.
/// Verify if the auto-fix 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()
}),
)
fn should_suggest_fix(node: &JsCallExpression) -> SyntaxResult<bool> {
let arguments = node.arguments()?;
Ok(arguments.args().len() == 2
&& !arguments.l_paren_token()?.has_trailing_comments()
&& !arguments.r_paren_token()?.has_leading_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 {
/// Wraps a [AnyJsExpression] in parentheses
fn parenthesize_any_js_expression(expr: AnyJsExpression) -> AnyJsExpression {
AnyJsExpression::from(make::js_parenthesized_expression(
make::token(T!['(']),
expr.clone(),
expr,
make::token(T![')']),
))
}
Expand All @@ -216,15 +158,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 @@ -240,39 +180,29 @@ fn does_expression_need_parens(
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();
Expand All @@ -286,6 +216,31 @@ fn does_expression_need_parens(
| AnyJsExpression::JsTemplateExpression(_) => true,
_ => false,
};

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

fn make_base(base: AnyJsExpression) -> SyntaxResult<AnyJsExpression> {
// '**' is right-associative, parens are needed when Math.pow(a ** b, c) is converted to (a ** b) ** c
let needs_parens = 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();
Ok(if needs_parens {
parenthesize_any_js_expression(base)
} else {
base
})
}

fn make_exponent(exponent: AnyJsExpression) -> SyntaxResult<AnyJsExpression> {
let needs_parens = exponent.precedence()? < OperatorPrecedence::Exponential;
Ok(if needs_parens {
parenthesize_any_js_expression(exponent)
} else {
exponent
})
}
8 changes: 0 additions & 8 deletions crates/rome_js_analyze/tests/specs/style/useExponentiationOperator/invalid.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,6 @@ foo + (Math.pow(a, b));
(Math.pow(a, b)) + foo;
`${(Math.pow(a, b))}`;

// doesn't preserve unnecessary parens around base and exponent
Math.pow((a), (b))
Math.pow(((a)), ((b)))
Math.pow((a.foo), b)
Math.pow(a, (b.foo))
Math.pow((a()), b)
Math.pow(a, (b()))

// Optional chaining
Math.pow?.(a, b)
Math?.pow(a, b)
Expand Down
Loading

0 comments on commit 27537a2

Please sign in to comment.