tools: add new ESLint rule: prefer-primordials

I added a new custom ESLint rule to fix these problems. We have a lot of replaceable codes with primordials. Accessing built-in objects is restricted by existing rule (no-restricted-globals), but accessing property in the built-in objects is not restricted right now. We manually review codes that can be replaced by primordials, but there's a lot of code that actually needs to be fixed. We have often made pull requests to replace the primordials with. Restrict accessing global built-in objects such as `Promise`. Restrict calling static methods such as `Array.from` or `Symbol.for`. Don't restrict prototype methods to prevent false-positive. PR-URL: #35448 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ben Coe <[email protected]>
nodejs · Nov 7, 2020 · cef1444 · cef1444
1 parent 1d525f5
commit cef1444
Show file tree

Hide file tree

Showing 17 changed files with 477 additions and 128 deletions.
diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml
@@ -7,105 +7,80 @@ rules:
   no-mixed-operators:
     - error
     - groups: [[ "&&", "||" ]]
-  no-restricted-globals:
+  no-restricted-syntax:
+    # Config copied from .eslintrc.js
+    - error
+    - selector: "CallExpression[callee.object.name='assert']:not([callee.property.name='ok']):not([callee.property.name='fail']):not([callee.property.name='ifError'])"
+      message: "Please only use simple assertions in ./lib"
+    - selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
+      message: "setTimeout() must be invoked with at least two arguments."
+    - selector: "CallExpression[callee.name='setInterval'][arguments.length<2]"
+      message: "setInterval() must be invoked with at least 2 arguments."
+    - selector: "ThrowStatement > CallExpression[callee.name=/Error$/]"
+      message: "Use new keyword when throwing an Error."
+    # Config specific to lib
+    - selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
+      message: "Use an error exported by the internal/errors module."
+    - selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"
+      message: "Please use `require('internal/errors').hideStackFrames()` instead."
+    - selector: "AssignmentExpression:matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace'])"
+      message: "Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'."
+  # Custom rules in tools/eslint-rules
+  node-core/lowercase-name-for-primitive: error
+  node-core/non-ascii-character: error
+  node-core/prefer-primordials:
     - error
     - name: Array
-      message: "Use `const { Array } = primordials;` instead of the global."
     - name: ArrayBuffer
-      message: "Use `const { ArrayBuffer } = primordials;` instead of the global."
     - name: BigInt
-      message: "Use `const { BigInt } = primordials;` instead of the global."
     - name: BigInt64Array
-      message: "Use `const { BigInt64Array } = primordials;` instead of the global."
     - name: BigUint64Array
-      message: "Use `const { BigUint64Array } = primordials;` instead of the global."
     - name: Boolean
-      message: "Use `const { Boolean } = primordials;` instead of the global."
+    - name: DataView
+    - name: Date
     - name: Error
-      message: "Use `const { Error } = primordials;` instead of the global."
+      ignore:
+        - stackTraceLimit
+        - captureStackTrace
+        - prepareStackTrace
+        - isPrototypeOf
     - name: EvalError
-      message: "Use `const { EvalError } = primordials;` instead of the global."
     - name: Float32Array
-      message: "Use `const { Float32Array } = primordials;` instead of the global."
     - name: Float64Array
-      message: "Use `const { Float64Array } = primordials;` instead of the global."
+    - name: Function
     - name: Int16Array
-      message: "Use `const { Int16Array } = primordials;` instead of the global."
     - name: Int32Array
-      message: "Use `const { Int32Array } = primordials;` instead of the global."
     - name: Int8Array
-      message: "Use `const { Int8Array } = primordials;` instead of the global."
+    - name: isFinite
+      into: Number
+    - name: isNaN
+      into: Number
     - name: JSON
-      message: "Use `const { JSON } = primordials;` instead of the global."
     - name: Map
-      message: "Use `const { Map } = primordials;` instead of the global."
     - name: Math
-      message: "Use `const { Math } = primordials;` instead of the global."
     - name: Number
-      message: "Use `const { Number } = primordials;` instead of the global."
     - name: Object
-      message: "Use `const { Object } = primordials;` instead of the global."
+    - name: parseFloat
+      into: Number
+    - name: parseInt
+      into: Number
     - name: Promise
-      message: "Use `const { Promise } = primordials;` instead of the global."
     - name: RangeError
-      message: "Use `const { RangeError } = primordials;` instead of the global."
     - name: ReferenceError
-      message: "Use `const { ReferenceError } = primordials;` instead of the global."
     - name: Reflect
-      message: "Use `const { Reflect } = primordials;` instead of the global."
     - name: RegExp
-      message: "Use `const { RegExp } = primordials;` instead of the global."
     - name: Set
-      message: "Use `const { Set } = primordials;` instead of the global."
     - name: String
-      message: "Use `const { String } = primordials;` instead of the global."
     - name: Symbol
-      message: "Use `const { Symbol } = primordials;` instead of the global."
     - name: SyntaxError
-      message: "Use `const { SyntaxError } = primordials;` instead of the global."
     - name: TypeError
-      message: "Use `const { TypeError } = primordials;` instead of the global."
-    - name: URIError
-      message: "Use `const { URIError } = primordials;` instead of the global."
     - name: Uint16Array
-      message: "Use `const { Uint16Array } = primordials;` instead of the global."
     - name: Uint32Array
-      message: "Use `const { Uint32Array } = primordials;` instead of the global."
     - name: Uint8Array
-      message: "Use `const { Uint8Array } = primordials;` instead of the global."
     - name: Uint8ClampedArray
-      message: "Use `const { Uint8ClampedArray } = primordials;` instead of the global."
+    - name: URIError
     - name: WeakMap
-      message: "Use `const { WeakMap } = primordials;` instead of the global."
     - name: WeakSet
-      message: "Use `const { WeakSet } = primordials;` instead of the global."
-    - name: parseFloat
-      message: "Use `const { NumberParseFloat } = primordials;` instead of the global."
-    - name: parseInt
-      message: "Use `const { NumberParseInt } = primordials;` instead of the global."
-  no-restricted-syntax:
-    # Config copied from .eslintrc.js
-    - error
-    - selector: "CallExpression[callee.object.name='assert']:not([callee.property.name='ok']):not([callee.property.name='fail']):not([callee.property.name='ifError'])"
-      message: "Please only use simple assertions in ./lib"
-    - selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
-      message: "setTimeout() must be invoked with at least two arguments."
-    - selector: "CallExpression[callee.name='setInterval'][arguments.length<2]"
-      message: "setInterval() must be invoked with at least 2 arguments."
-    - selector: "ThrowStatement > CallExpression[callee.name=/Error$/]"
-      message: "Use new keyword when throwing an Error."
-    # Config specific to lib
-    - selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
-      message: "Use an error exported by the internal/errors module."
-    - selector: "CallExpression[callee.object.name='Error'][callee.property.name='captureStackTrace']"
-      message: "Please use `require('internal/errors').hideStackFrames()` instead."
-    - selector: "AssignmentExpression:matches([left.name='prepareStackTrace'], [left.property.name='prepareStackTrace'])"
-      message: "Use 'overrideStackTrace' from 'lib/internal/errors.js' instead of 'Error.prepareStackTrace'."
-    - selector: "CallExpression[callee.name='isNaN']"
-      message: "Use NumberIsNaN() primordial instead of the global isNaN() function."
-  # Custom rules in tools/eslint-rules
-  node-core/lowercase-name-for-primitive: error
-  node-core/non-ascii-character: error
 globals:
   Intl: false
   # Parameters passed to internal modules

diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -36,7 +36,7 @@ const kEvents = Symbol('kEvents');
 const kStop = Symbol('kStop');
 const kTarget = Symbol('kTarget');
 
-const kHybridDispatch = Symbol.for('nodejs.internal.kHybridDispatch');
+const kHybridDispatch = SymbolFor('nodejs.internal.kHybridDispatch');
 const kCreateEvent = Symbol('kCreateEvent');
 const kNewListener = Symbol('kNewListener');
 const kRemoveListener = Symbol('kRemoveListener');

diff --git a/lib/internal/freeze_intrinsics.js b/lib/internal/freeze_intrinsics.js
@@ -20,21 +20,61 @@
 // https://github.com/tc39/proposal-ses/blob/e5271cc42a257a05dcae2fd94713ed2f46c08620/shim/src/freeze.js
 
 /* global WebAssembly, SharedArrayBuffer, console */
-/* eslint-disable no-restricted-globals */
 'use strict';
 
+const {
+  Array,
+  ArrayBuffer,
+  ArrayPrototypeForEach,
+  BigInt,
+  BigInt64Array,
+  BigUint64Array,
+  Boolean,
+  DataView,
+  Date,
+  Error,
+  EvalError,
+  Float32Array,
+  Float64Array,
+  Function,
+  Int16Array,
+  Int32Array,
+  Int8Array,
+  JSON,
+  Map,
+  Math,
+  Number,
+  Object,
+  ObjectDefineProperty,
+  ObjectFreeze,
+  ObjectGetOwnPropertyDescriptor,
+  ObjectGetOwnPropertyDescriptors,
+  ObjectGetOwnPropertyNames,
+  ObjectGetOwnPropertySymbols,
+  ObjectGetPrototypeOf,
+  ObjectPrototypeHasOwnProperty,
+  Promise,
+  RangeError,
+  ReferenceError,
+  Reflect,
+  ReflectOwnKeys,
+  RegExp,
+  Set,
+  String,
+  Symbol,
+  SymbolIterator,
+  SyntaxError,
+  TypeError,
+  Uint16Array,
+  Uint32Array,
+  Uint8Array,
+  Uint8ClampedArray,
+  URIError,
+  WeakMap,
+  WeakSet,
+} = primordials;
+
 module.exports = function() {
-  const {
-    defineProperty,
-    freeze,
-    getOwnPropertyDescriptor,
-    getOwnPropertyDescriptors,
-    getOwnPropertyNames,
-    getOwnPropertySymbols,
-    getPrototypeOf,
-  } = Object;
-  const objectHasOwnProperty = Object.prototype.hasOwnProperty;
-  const { ownKeys } = Reflect;
   const {
     clearImmediate,
     clearInterval,
@@ -47,25 +87,25 @@ module.exports = function() {
   const intrinsicPrototypes = [
     // Anonymous Intrinsics
     // IteratorPrototype
-    getPrototypeOf(
-      getPrototypeOf(new Array()[Symbol.iterator]())
+    ObjectGetPrototypeOf(
+      ObjectGetPrototypeOf(new Array()[SymbolIterator]())
     ),
     // ArrayIteratorPrototype
-    getPrototypeOf(new Array()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Array()[SymbolIterator]()),
     // StringIteratorPrototype
-    getPrototypeOf(new String()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new String()[SymbolIterator]()),
     // MapIteratorPrototype
-    getPrototypeOf(new Map()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Map()[SymbolIterator]()),
     // SetIteratorPrototype
-    getPrototypeOf(new Set()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Set()[SymbolIterator]()),
     // GeneratorFunction
-    getPrototypeOf(function* () {}),
+    ObjectGetPrototypeOf(function* () {}),
     // AsyncFunction
-    getPrototypeOf(async function() {}),
+    ObjectGetPrototypeOf(async function() {}),
     // AsyncGeneratorFunction
-    getPrototypeOf(async function* () {}),
+    ObjectGetPrototypeOf(async function* () {}),
     // TypedArray
-    getPrototypeOf(Uint8Array),
+    ObjectGetPrototypeOf(Uint8Array),
 
     // 19 Fundamental Objects
     Object.prototype, // 19.1
@@ -129,33 +169,37 @@ module.exports = function() {
   const intrinsics = [
     // Anonymous Intrinsics
     // ThrowTypeError
-    getOwnPropertyDescriptor(Function.prototype, 'caller').get,
+    ObjectGetOwnPropertyDescriptor(Function.prototype, 'caller').get,
     // IteratorPrototype
-    getPrototypeOf(
-      getPrototypeOf(new Array()[Symbol.iterator]())
+    ObjectGetPrototypeOf(
+      ObjectGetPrototypeOf(new Array()[SymbolIterator]())
     ),
     // ArrayIteratorPrototype
-    getPrototypeOf(new Array()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Array()[SymbolIterator]()),
     // StringIteratorPrototype
-    getPrototypeOf(new String()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new String()[SymbolIterator]()),
     // MapIteratorPrototype
-    getPrototypeOf(new Map()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Map()[SymbolIterator]()),
     // SetIteratorPrototype
-    getPrototypeOf(new Set()[Symbol.iterator]()),
+    ObjectGetPrototypeOf(new Set()[SymbolIterator]()),
     // GeneratorFunction
-    getPrototypeOf(function* () {}),
+    ObjectGetPrototypeOf(function* () {}),
     // AsyncFunction
-    getPrototypeOf(async function() {}),
+    ObjectGetPrototypeOf(async function() {}),
     // AsyncGeneratorFunction
-    getPrototypeOf(async function* () {}),
+    ObjectGetPrototypeOf(async function* () {}),
     // TypedArray
-    getPrototypeOf(Uint8Array),
+    ObjectGetPrototypeOf(Uint8Array),
 
     // 18 The Global Object
     eval,
+    // eslint-disable-next-line node-core/prefer-primordials
     isFinite,
+    // eslint-disable-next-line node-core/prefer-primordials
     isNaN,
+    // eslint-disable-next-line node-core/prefer-primordials
     parseFloat,
+    // eslint-disable-next-line node-core/prefer-primordials
     parseInt,
     decodeURI,
     decodeURIComponent,
@@ -289,16 +333,16 @@ module.exports = function() {
         // Object are verified before being enqueued,
         // therefore this is a valid candidate.
         // Throws if this fails (strict mode).
-        freeze(obj);
+        ObjectFreeze(obj);
 
         // We rely upon certain commitments of Object.freeze and proxies here
 
         // Get stable/immutable outbound links before a Proxy has a chance to do
         // something sneaky.
-        const proto = getPrototypeOf(obj);
-        const descs = getOwnPropertyDescriptors(obj);
+        const proto = ObjectGetPrototypeOf(obj);
+        const descs = ObjectGetOwnPropertyDescriptors(obj);
         enqueue(proto);
-        ownKeys(descs).forEach((name) => {
+        ArrayPrototypeForEach(ReflectOwnKeys(descs), (name) => {
           // TODO: Uncurried form
           // TODO: getOwnPropertyDescriptors is guaranteed to return well-formed
           // descriptors, but they still inherit from Object.prototype. If
@@ -378,10 +422,10 @@ module.exports = function() {
             `Cannot assign to read only property '${prop}' of object '${obj}'`
           );
         }
-        if (objectHasOwnProperty.call(this, prop)) {
+        if (ObjectPrototypeHasOwnProperty(this, prop)) {
           this[prop] = newValue;
         } else {
-          defineProperty(this, prop, {
+          ObjectDefineProperty(this, prop, {
             value: newValue,
             writable: true,
             enumerable: true,
@@ -390,7 +434,7 @@ module.exports = function() {
         }
       }
 
-      defineProperty(obj, prop, {
+      ObjectDefineProperty(obj, prop, {
         get: getter,
         set: setter,
         enumerable: desc.enumerable,
@@ -403,14 +447,14 @@ module.exports = function() {
     if (!obj) {
       return;
     }
-    const descs = getOwnPropertyDescriptors(obj);
+    const descs = ObjectGetOwnPropertyDescriptors(obj);
     if (!descs) {
       return;
     }
-    getOwnPropertyNames(obj).forEach((prop) => {
+    ArrayPrototypeForEach(ObjectGetOwnPropertyNames(obj), (prop) => {
       return enableDerivedOverride(obj, prop, descs[prop]);
     });
-    getOwnPropertySymbols(obj).forEach((prop) => {
+    ArrayPrototypeForEach(ObjectGetOwnPropertySymbols(obj), (prop) => {
       return enableDerivedOverride(obj, prop, descs[prop]);
     });
   }

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
@@ -3,11 +3,13 @@
 const {
   ArrayIsArray,
   BigInt,
+  Date,
   DateNow,
   ErrorCaptureStackTrace,
   ObjectPrototypeHasOwnProperty,
   Number,
   NumberIsFinite,
+  NumberIsInteger,
   MathMin,
   ObjectSetPrototypeOf,
   ReflectOwnKeys,
@@ -785,7 +787,7 @@ const getValidMode = hideStackFrames((mode, type) => {
   if (mode == null) {
     return def;
   }
-  if (Number.isInteger(mode) && mode >= min && mode <= max) {
+  if (NumberIsInteger(mode) && mode >= min && mode <= max) {
     return mode;
   }
   if (typeof mode !== 'number') {