Skip to content
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

fix(lint/useExponentiationOperator): avoid incorrect code fixes #121

Merged
merged 1 commit into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CHANGELOG.md
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
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
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
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
}
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> {
Conaclos marked this conversation as resolved.
Show resolved Hide resolved
// '**' 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