tools: add lint rule for unread benchmark n

benchmark/async_hooks/async-resource-vs-destroy.js

@@ -19,7 +19,6 @@ const bench = common.createBenchmark(main, {
   path: '/',
   connections: 500,
   duration: 5,
-  n: [1e6],
 });
 
 function buildCurrentResource(getServe) {

benchmark/eslint.config_partial.mjs

@@ -9,6 +9,7 @@ export default [
       },
     },
     rules: {
+      'node-core/benchmark-n-config': 'error',
       'prefer-arrow-callback': 'error',
     },
   },

test/parallel/test-eslint-benchmark-n-config.js

@@ -0,0 +1,82 @@
+'use strict';
+
+const common = require('../common');
+if ((!common.hasCrypto) || (!common.hasIntl)) {
+  common.skip('ESLint tests require crypto and Intl');
+}
+
+common.skipIfEslintMissing();
+
+const RuleTester = require('../../tools/eslint/node_modules/eslint').RuleTester;
+const rule = require('../../tools/eslint-rules/benchmark-n-config');
+
+const message = '`n` is configured but not read by the benchmark main ' +
+                'function (destructure `{ n }` or read `conf.n`)';
+
+new RuleTester().run('benchmark-n-config', rule, {
+  valid: [
+    `
+      const bench = common.createBenchmark(main, { len: [1], n: [100] });
+      function main({ len, n }) { bench.end(n); }
+    `,
+    `
+      const bench = common.createBenchmark(main, { n: [100] });
+      function main(conf) { bench.end(conf.n); }
+    `,
+    `
+      const bench = common.createBenchmark(main, { n: [100] });
+      function main(conf) { const { n } = conf; bench.end(n); }
+    `,
+    `
+      const bench = common.createBenchmark(({ n }) => bench.end(n), { n: [1] });
+    `,
+    `
+      const configs = { n: [100] };
+      const bench = common.createBenchmark(main, configs);
+      function main({ n }) { bench.end(n); }
+    `,
+    // Configs without `n` (e.g. duration-driven benchmarks) are not checked.
+    `
+      const bench = common.createBenchmark(main, { duration: [5] });
+      function main({ duration }) {}
+    `,
+    // Non-static configs are skipped.
+    `
+      const bench = common.createBenchmark(main, { ...base, n: [100] });
+      function main({ len }) {}
+    `,
+  ],
+  invalid: [
+    // nodejs/node#59872: the config key was renamed to `n`, but main() kept
+    // reading `num`.
+    {
+      code: `
+        const bench = common.createBenchmark(main, { len: [1], n: [100] });
+        function main({ len, num: n }) { bench.end(n); }
+      `,
+      errors: [{ message }],
+    },
+    {
+      code: `
+        const bench = common.createBenchmark(main, { n: [1e6], duration: [5] });
+        function main({ duration }) {}
+      `,
+      errors: [{ message }],
+    },
+    {
+      code: `
+        const bench = common.createBenchmark(main, { n: [100] });
+        function main(conf) { bench.end(conf.num); }
+      `,
+      errors: [{ message }],
+    },
+    {
+      code: `
+        const configs = { n: [100] };
+        const bench = common.createBenchmark(main, configs);
+        function main({ count }) { bench.end(count); }
+      `,
+      errors: [{ message }],
+    },
+  ],
+});

tools/eslint-rules/benchmark-n-config.js

@@ -0,0 +1,158 @@
+'use strict';
+
+// Requires benchmarks that declare an `n` iteration count in their configs
+// to actually read that value in the benchmark main function.
+//
+// This prevents regressions like nodejs/node#59872, where a config key was
+// renamed to `n` while main() kept reading the old key, silently producing
+// a near-zero result.
+
+function walk(node, visit) {
+  if (node === null || typeof node !== 'object') return;
+  if (Array.isArray(node)) {
+    for (const item of node) walk(item, visit);
+    return;
+  }
+  if (typeof node.type !== 'string') return;
+  visit(node);
+  for (const key of Object.keys(node)) {
+    if (key === 'parent' || key === 'loc' || key === 'range') continue;
+    walk(node[key], visit);
+  }
+}
+
+// Collects the property names read by an ObjectPattern: `{ num: n }` reads
+// `num`, `{ n }` reads `n`.
+function objectPatternNames(pattern, out) {
+  for (const prop of pattern.properties) {
+    if (prop.type !== 'Property' || prop.computed) continue;
+    if (prop.key.type === 'Identifier') out.add(prop.key.name);
+    else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') {
+      out.add(prop.key.value);
+    }
+    if (prop.value && prop.value.type === 'ObjectPattern') {
+      objectPatternNames(prop.value, out);
+    }
+  }
+}
+
+// Returns false when the object literal is not fully static.
+function objectExpressionKeys(node, out) {
+  for (const prop of node.properties) {
+    if (prop.type === 'SpreadElement') return false;
+    if (prop.type !== 'Property' || prop.computed) continue;
+    if (prop.key.type === 'Identifier') out.add(prop.key.name);
+    else if (prop.key.type === 'Literal' && typeof prop.key.value === 'string') {
+      out.add(prop.key.value);
+    }
+  }
+  return true;
+}
+
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {
+      description: 'require benchmark main to read `n` when configs declare it',
+    },
+    schema: [],
+  },
+
+  create(context) {
+    const { sourceCode } = context;
+
+    let state = null;
+
+    return {
+      CallExpression(node) {
+        if (state !== null) return;
+        const callee = node.callee;
+        if (callee.type !== 'MemberExpression' || callee.computed) return;
+        if (callee.property.name !== 'createBenchmark') return;
+        if (node.arguments.length < 2) return;
+        state = {
+          mainArg: node.arguments[0],
+          configArg: node.arguments[1],
+        };
+      },
+
+      'Program:exit'() {
+        if (state === null) return;
+        const { mainArg, configArg } = state;
+
+        // Resolve config keys.
+        const configKeys = new Set();
+        if (configArg.type === 'ObjectExpression') {
+          if (!objectExpressionKeys(configArg, configKeys)) return;
+        } else if (configArg.type === 'Identifier') {
+          let resolved = false;
+          walk(sourceCode.ast, (n) => {
+            if (resolved) return;
+            if (n.type === 'VariableDeclarator' && n.id.type === 'Identifier' &&
+                n.id.name === configArg.name && n.init &&
+                n.init.type === 'ObjectExpression') {
+              resolved = objectExpressionKeys(n.init, configKeys);
+            }
+          });
+          if (!resolved) return;
+        } else {
+          return;
+        }
+        if (!configKeys.has('n')) return;
+
+        // Resolve the main function node.
+        let fn = null;
+        if (mainArg.type === 'Identifier') {
+          walk(sourceCode.ast, (n) => {
+            if (fn !== null) return;
+            if (n.type === 'FunctionDeclaration' && n.id && n.id.name === mainArg.name) {
+              fn = n;
+            }
+            if (fn === null && n.type === 'VariableDeclarator' && n.id.type === 'Identifier' &&
+                n.id.name === mainArg.name && n.init &&
+                (n.init.type === 'FunctionExpression' ||
+                 n.init.type === 'ArrowFunctionExpression')) {
+              fn = n.init;
+            }
+          });
+        } else if (mainArg.type === 'FunctionExpression' ||
+                   mainArg.type === 'ArrowFunctionExpression') {
+          fn = mainArg;
+        }
+        if (fn === null) return;
+
+        // Collect every name through which `n` could be read in main().
+        const paramNames = new Set();
+        const readNames = new Set();
+        for (const param of fn.params) {
+          if (param.type === 'ObjectPattern') {
+            objectPatternNames(param, readNames);
+          } else if (param.type === 'Identifier') {
+            paramNames.add(param.name);
+          }
+        }
+        walk(fn.body, (n) => {
+          // `const { a, b } = conf;` inside the body.
+          if (n.type === 'VariableDeclarator' && n.id.type === 'ObjectPattern' &&
+              n.init && n.init.type === 'Identifier' && paramNames.has(n.init.name)) {
+            objectPatternNames(n.id, readNames);
+          }
+          // `conf.n` member access.
+          if (n.type === 'MemberExpression' && !n.computed &&
+              n.object.type === 'Identifier' && paramNames.has(n.object.name) &&
+              n.property.type === 'Identifier') {
+            readNames.add(n.property.name);
+          }
+        });
+
+        if (!readNames.has('n')) {
+          context.report({
+            node: fn.id ?? fn,
+            message: '`n` is configured but not read by the benchmark main ' +
+                     'function (destructure `{ n }` or read `conf.n`)',
+          });
+        }
+      },
+    };
+  },
+};