diff --git a/crates/oxc_linter/src/rules/unicorn/new_for_builtins.rs b/crates/oxc_linter/src/rules/unicorn/new_for_builtins.rs index 4ddad5e877f09..fe4f72ae9877d 100644 --- a/crates/oxc_linter/src/rules/unicorn/new_for_builtins.rs +++ b/crates/oxc_linter/src/rules/unicorn/new_for_builtins.rs @@ -1,7 +1,4 @@ -use oxc_ast::{ - AstKind, - ast::{Expression, match_member_expression}, -}; +use oxc_ast::{AstKind, ast::Expression}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; @@ -24,16 +21,18 @@ pub struct NewForBuiltins; declare_oxc_lint!( /// ### What it does /// - /// Enforces the use of `new` for following builtins: `Object`, `Array`, `ArrayBuffer`, `BigInt64Array`, `BigUint64Array`, `DataView`, `Date`, `Error`, `Float32Array`, `Float64Array`, `Function`, `Int8Array`, `Int16Array`, `Int32Array`, `Map`, `WeakMap`, `Set`, `WeakSet`, `Promise`, `RegExp`, `Uint8Array`, `Uint16Array`, `Uint32Array`, `Uint8ClampedArray`, `SharedArrayBuffer`, `Proxy`, `WeakRef`, `FinalizationRegistry`. - /// - /// Disallows the use of `new` for following builtins: `String`, `Number`, `Boolean`, `Symbol`, `BigInt`. + /// Enforces the use of `new` for the following builtins: `Object`, `Array`, `ArrayBuffer`, `BigInt64Array`, + /// `BigUint64Array`, `DataView`, `Date`, `Error`, `Float32Array`, `Float64Array`, `Function`, `Int8Array`, + /// `Int16Array`, `Int32Array`, `Map`, `WeakMap`, `Set`, `WeakSet`, `Promise`, `RegExp`, `Uint8Array`, + /// `Uint16Array`, `Uint32Array`, `Uint8ClampedArray`, `SharedArrayBuffer`, `Proxy`, `WeakRef`, `FinalizationRegistry`. /// - /// These should not use `new` as that would create object wrappers for the primitive values, which is not what you want. However, without `new` they can be useful for coercing a value to that type. + /// Disallows the use of `new` for the following builtins: `String`, `Number`, `Boolean`, `Symbol`, `BigInt`. /// /// ### Why is this bad? /// - /// They work the same, but `new` should be preferred for consistency with other constructors. - /// + /// Using `new` inconsistently can cause confusion. Constructors like `Array` and `RegExp` should always use `new` + /// to ensure the expected instance type. Meanwhile, `String`, `Number`, `Boolean`, `Symbol`, and `BigInt` should not use `new`, + /// as they create object wrappers instead of primitive values. /// /// ### Examples /// @@ -57,32 +56,27 @@ impl Rule for NewForBuiltins { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { match node.kind() { AstKind::NewExpression(new_expr) => { - let callee = new_expr.callee.without_parentheses(); - - let Some(builtin_name) = is_expr_global_builtin(callee, ctx) else { + let Some(builtin_name) = is_expr_global_builtin(&new_expr.callee, ctx) else { return; }; - if DISALLOW_NEW_FOR_BUILTINS.contains(builtin_name) { + if DISALLOW_NEW_FOR_BUILTINS.contains(&builtin_name) { ctx.diagnostic(disallow(new_expr.span, builtin_name)); } } AstKind::CallExpression(call_expr) => { - let Some(builtin_name) = - is_expr_global_builtin(call_expr.callee.without_parentheses(), ctx) - else { + let Some(builtin_name) = is_expr_global_builtin(&call_expr.callee, ctx) else { return; }; if ENFORCE_NEW_FOR_BUILTINS.contains(builtin_name) { if builtin_name == "Object" { - if let Some(parent) = ctx.nodes().parent_node(node.id()) { - if let AstKind::BinaryExpression(bin_expr) = parent.kind() { - if bin_expr.operator == BinaryOperator::StrictEquality - || bin_expr.operator == BinaryOperator::StrictInequality - { - return; - } + let parent_kind = ctx.nodes().parent_kind(node.id()); + if let Some(AstKind::BinaryExpression(bin_expr)) = parent_kind { + if bin_expr.operator == BinaryOperator::StrictEquality + || bin_expr.operator == BinaryOperator::StrictInequality + { + return; } } } @@ -99,33 +93,26 @@ fn is_expr_global_builtin<'a, 'b>( expr: &'b Expression<'a>, ctx: &'b LintContext<'a>, ) -> Option<&'b str> { - match expr { - Expression::Identifier(ident) => { - if !ctx.scoping().root_unresolved_references().contains_key(ident.name.as_str()) { - return None; - } - - if !ENFORCE_NEW_FOR_BUILTINS.contains(&ident.name) - && !DISALLOW_NEW_FOR_BUILTINS.contains(&ident.name) - { - return None; - } - - Some(ident.name.as_str()) + let expr = expr.without_parentheses(); + if let Expression::Identifier(ident) = expr { + let name = ident.name.as_str(); + if !ctx.scoping().root_unresolved_references().contains_key(name) { + return None; } - match_member_expression!(Expression) => { - let member_expr = expr.to_member_expression(); - let Expression::Identifier(ident) = member_expr.object() else { - return None; - }; - if !GLOBAL_OBJECT_NAMES.contains(ident.name.as_str()) { - return None; - } + Some(name) + } else { + let member_expr = expr.as_member_expression()?; - member_expr.static_property_name() + let Expression::Identifier(ident) = member_expr.object() else { + return None; + }; + + if !GLOBAL_OBJECT_NAMES.contains(&ident.name) { + return None; } - _ => None, + + member_expr.static_property_name() } } @@ -160,13 +147,7 @@ const ENFORCE_NEW_FOR_BUILTINS: phf::Set<&'static str> = phf_set! { "FinalizationRegistry", }; -const DISALLOW_NEW_FOR_BUILTINS: phf::Set<&'static str> = phf_set! { - "BigInt", - "Boolean", - "Number", - "Symbol", - "String", -}; +const DISALLOW_NEW_FOR_BUILTINS: [&str; 5] = ["BigInt", "Boolean", "Number", "Symbol", "String"]; #[test] fn test() {