Skip to content

Commit

Permalink
fix(useArrayLiterals): check all expressions and add a fix
Browse files Browse the repository at this point in the history
  • Loading branch information
Conaclos committed Oct 29, 2024
1 parent d780164 commit 2cf143b
Show file tree
Hide file tree
Showing 7 changed files with 457 additions and 107 deletions.
26 changes: 26 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,32 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b
#### Enhancements

- `useExportType` and `useImportType` now ignore TypeScript declaration files ([#4416](https://github.com/biomejs/biome/pull/4416)). Contributed by @Conaclos
- [useArrayLiterals](https://biomejs.dev/linter/rules/use-array-literals/) now provides a code fix.

```diff
- const xs = new Array();
+ const xs = [];
```

The code fix is currently marked as unsafe.
We plan to make it safe in a future release of Biome.

Contributed by @Conaclos

#### Bug fixes

- [useArrayLiterals](https://biomejs.dev/linter/rules/use-array-literals/) now reports all expressions using the `Array` constructors.

Previously, the rule reported only use of the `Array` constructor in expressions statements.

```js
// This was reported
new Array();
// This was not reported
const xs = new Array();
```

Contributed by @Conaclos

### Parser

Expand Down
133 changes: 88 additions & 45 deletions crates/biome_js_analyze/src/lint/correctness/use_array_literals.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
use biome_analyze::{
context::RuleContext, declare_lint_rule, Ast, FixKind, Rule, RuleDiagnostic, RuleSource,
context::RuleContext, declare_lint_rule, ActionCategory, FixKind, Rule, RuleDiagnostic,
RuleSource,
};
use biome_console::markup;
use biome_js_syntax::{AnyJsCallArgument, AnyJsExpression, JsCallArguments, JsExpressionStatement};
use biome_rowan::AstNode;
use biome_js_factory::make;
use biome_js_syntax::{
global_identifier, AnyJsCallArgument, AnyJsExpression, JsNewOrCallExpression, JsSyntaxKind, T,
};
use biome_rowan::{AstNode, AstSeparatedList, BatchMutationExt};

use crate::{services::semantic::Semantic, JsRuleAction};

declare_lint_rule! {
/// Disallow Array constructors.
Expand All @@ -16,29 +22,29 @@ declare_lint_rule! {
/// ### Invalid
///
/// ```js,expect_diagnostic
/// Array();
/// const xs = Array();
/// ```
///
/// ```js,expect_diagnostic
/// Array(0, 1, 2);
/// const xs = Array(0, 1, 2);
/// ```
///
/// ```js,expect_diagnostic
/// new Array(0, 1, 2);
/// const xs = new Array(0, 1, 2);
/// ```
///
/// ```js,expect_diagnostic
/// Array(...args);
/// const xs = Array(...args);
/// ```
///
/// ### Valid
///
/// ```js
/// Array(500);
/// const xs = Array(65000);
/// ```
///
/// ```js
/// [0, 1, 2];
/// const xs = [0, 1, 2];
/// ```
///
pub UseArrayLiterals {
Expand All @@ -52,29 +58,41 @@ declare_lint_rule! {
}

impl Rule for UseArrayLiterals {
type Query = Ast<JsExpressionStatement>;
type Query = Semantic<JsNewOrCallExpression>;
type State = ();
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let expression_statement = ctx.query();

let expression = expression_statement.expression().ok()?;

match expression {
AnyJsExpression::JsCallExpression(call_expression) => {
let callee = call_expression.callee().ok()?;
let arguments = call_expression.arguments().ok()?;
validate(&callee, &arguments)
}
AnyJsExpression::JsNewExpression(new_expression) => {
let callee = new_expression.callee().ok()?;
let arguments = new_expression.arguments()?;
validate(&callee, &arguments)
}
_ => None,
let node = ctx.query();
let callee = node.callee().ok()?.omit_parentheses();
let (reference, name) = global_identifier(&callee)?;
if name.text() != "Array" || ctx.model().binding(&reference).is_some() {
return None;
}
if callee.syntax() != reference.syntax()
&& !reference
.value_token()
.is_ok_and(|name| matches!(name.text_trimmed(), "globalThis" | "window" | "Array"))
{
return None;
}
let Some(arguments) = node.arguments() else {
return if matches!(node, JsNewOrCallExpression::JsNewExpression(_)) {
// Report `new Array`
Some(())
} else {
// ignore `Array`
None
};
};
let [arg1, arg2] = arguments.get_arguments_by_index([0, 1]);
if arg1.is_some() && arg2.is_none() && !matches!(arg1?, AnyJsCallArgument::JsSpread(_)) {
// Ignore `Array(length)`
return None;
}
// Report `Array()`, `Array(x, y)`, and `Array(...xs)`
Some(())
}

fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> {
Expand All @@ -84,31 +102,56 @@ impl Rule for UseArrayLiterals {
rule_category!(),
node.range(),
markup! {
"Don't use Array constructors."
"Use an array literal instead of the "<Emphasis>"Array"</Emphasis>" constructor."
},
)
.note(markup! {
"Use of the Array constructor is not allowed except creating sparse arrays of a specified size by giving a single numeric argument."
})
.note(markup! {
"The array literal notation [] is preferable."
"The "<Emphasis>"Array"</Emphasis>" constructor is misleading because it can be used to preallocate an array of a given length or to create an array with a given list of elements."
}),
)
}
}

fn validate(callee: &AnyJsExpression, arguments: &JsCallArguments) -> Option<()> {
if callee.text() != "Array" {
return None;
}
let mut args_iter = arguments.args().into_iter();
let first_arg = args_iter.next();
let second_arg = args_iter.next();
if first_arg.is_some()
&& second_arg.is_none()
&& !matches!(first_arg?.ok()?, AnyJsCallArgument::JsSpread(_))
{
return None;
fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
let node = ctx.query();
if node
.syntax()
.parent()
.is_some_and(|parent| parent.kind() == JsSyntaxKind::JS_EXPRESSION_STATEMENT)
{
// Ignore useless expression statements.
// This avoids issues with missing semicolons.
return None;
}
let mut mutation = ctx.root().begin();
let new_node = if let Some(args) = node.arguments() {
let l_paren_trailing_trivia = args.l_paren_token().ok()?.trailing_trivia().pieces();
let r_paren_leading_trivia = args.r_paren_token().ok()?.leading_trivia().pieces();
let args = args.args();
let items = args
.elements()
.flat_map(|item| item.into_node())
.map(|item| item.into())
.collect::<Vec<_>>();
let separators = args.separators().flatten().collect::<Vec<_>>();
make::js_array_expression(
make::token(T!['[']).append_trivia_pieces(l_paren_trailing_trivia),
make::js_array_element_list(items, separators),
make::token(T![']']).prepend_trivia_pieces(r_paren_leading_trivia),
)
} else {
// `new Array` -> `[]`
make::js_array_expression(
make::token(T!['[']),
make::js_array_element_list([], []),
make::token(T![']']),
)
};
mutation.replace_node::<AnyJsExpression>(node.clone().into(), new_node.into());
Some(JsRuleAction::new(
ActionCategory::QuickFix,
ctx.metadata().applicability(),
markup! { "Use an array literal." }.to_owned(),
mutation,
))
}
Some(())
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
Array();
var xs = Array();

Array(0, 1, 2);
var xs = Array(0, 1, 2);

Array(...args);
var xs = Array(...args);

new Array();
var xs = new Array;

new Array(0, 1, 2);
var xs = new Array();

new Array(...args);
var xs = new Array(0, 1, 2);

var xs = new Array(...args);

var xs = /**A*/ new /**B*/ Array /**C*/ ( /**D*/ 0 /**E*/, /**F*/ 1 /**G*/) /**H*/;

var xs = (Array)(
/* foo */ a,
b = c() // bar
);

var xs = Array?.();

// ASI
foo
new Array

var xs = globalThis.Array();

var xs = window.Array();
Loading

0 comments on commit 2cf143b

Please sign in to comment.