refactor(linter): improve unicorn/new-for-builtins

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() {