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 4, 2023
1 parent 034551d commit 4ee9068
Show file tree
Hide file tree
Showing 28 changed files with 554 additions and 759 deletions.
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)
}
13 changes: 4 additions & 9 deletions crates/rome_js_analyze/src/analyzers/suspicious/no_unsafe_negation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make;
use rome_js_syntax::{AnyJsExpression, JsInExpression, JsInstanceofExpression, T};
use rome_js_syntax::{AnyJsExpression, JsInExpression, JsInstanceofExpression};
use rome_rowan::{declare_node_union, AstNode, AstNodeExt, BatchMutationExt};

declare_rule! {
Expand Down Expand Up @@ -100,10 +100,8 @@ impl Rule for NoUnsafeNegation {
let next_expr = expr
.clone()
.replace_node_discard_trivia(left.clone(), argument)?;
let next_parenthesis_expression = make::js_parenthesized_expression(
make::token(T!['(']),
let next_parenthesis_expression = make::parenthesized(
rome_js_syntax::AnyJsExpression::JsInstanceofExpression(next_expr),
make::token(T![')']),
);
let next_unary_expression = make::js_unary_expression(
unary_expression.operator_token().ok()?,
Expand All @@ -122,11 +120,8 @@ impl Rule for NoUnsafeNegation {
left.clone(),
rome_js_syntax::AnyJsInProperty::AnyJsExpression(argument),
)?;
let next_parenthesis_expression = make::js_parenthesized_expression(
make::token(T!['(']),
rome_js_syntax::AnyJsExpression::JsInExpression(next_expr),
make::token(T![')']),
);
let next_parenthesis_expression =
make::parenthesized(rome_js_syntax::AnyJsExpression::JsInExpression(next_expr));
let next_unary_expression = make::js_unary_expression(
unary_expression.operator_token().ok()?,
AnyJsExpression::JsParenthesizedExpression(next_parenthesis_expression),
Expand Down
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 4ee9068

Please sign in to comment.