Add ESLint rule: no-math-minmax-array-spread - #53653
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a custom ESLint rule to detect potentially unsafe array spreading into Math.min and Math.max.
Changes:
- Implements and tests the new rule.
- Registers and enables it as a warning.
- Documents detected forms and alternatives.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-math-minmax-array-spread.ts |
Implements detection and diagnostics. |
eslint-factory/src/rules/no-math-minmax-array-spread.test.ts |
Tests supported rule branches. |
eslint-factory/src/index.ts |
Registers the rule. |
eslint-factory/eslint.config.cjs |
Enables the rule as a warning. |
eslint-factory/README.md |
Documents the rule. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (3)
eslint-factory/src/rules/no-math-minmax-array-spread.ts:73
- Fixed arguments do not bound the spread:
Math.max(0, ...values)still passes every element as a call argument and can hit the same engine limit. This early return leaves a common vulnerable form unreported. The rule should inspect every spread argument and report any unbounded one; the mixed-form test and README exclusion should be updated accordingly.
// Only the single-argument spread form is reported: fixed arguments such as
// `Math.max(0, ...arr)` suggest an intentional, likely bounded call shape.
if (node.arguments.length !== 1) return;
eslint-factory/src/rules/no-math-minmax-array-spread.ts:60
- An inline array literal is not necessarily bounded:
Math.max(...[0, ...values])has anArrayExpressionargument but can still expand an unbounded number of elements, so this predicate skips it. Treat array literals as bounded only when their own spread elements are also statically bounded, and add a regression test for nested spread arrays.
* Returns true when the spread argument has a size that is not statically bounded by
* the source itself. Inline array literals are always bounded, so they are excluded.
*/
function isUnboundedSpreadArgument(node: TSESTree.Node): boolean {
return node.type === AST_NODE_TYPES.Identifier || node.type === AST_NODE_TYPES.MemberExpression || node.type === AST_NODE_TYPES.CallExpression;
eslint-factory/src/rules/no-math-minmax-array-spread.ts:60
- This treats every identifier and call result as unbounded, producing a concrete false positive in the configured scan:
actions/setup/js/patch_path_helpers.cjs:26-28spreads a value derived from filtering a two-element literal, so its maximum length is two. Resolve immutable identifier initializers and preserve cardinality through bounded operations such asfilterbefore reporting; the repository already has a safe scope-resolution pattern inno-child-process-interpolated-command.ts:38-58.
function isUnboundedSpreadArgument(node: TSESTree.Node): boolean {
return node.type === AST_NODE_TYPES.Identifier || node.type === AST_NODE_TYPES.MemberExpression || node.type === AST_NODE_TYPES.CallExpression;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| }, | ||
| schema: [], | ||
| messages: { | ||
| noMathMinMaxArraySpread: "Avoid Math.{{method}}(...{{arg}}) — spreading an array of unknown size can throw `RangeError: Maximum call stack size exceeded`. Use `{{arg}}.reduce((a, b) => Math.{{method}}(a, b))` instead.", |
There was a problem hiding this comment.
Fixed in d98e5c9 — the reduce fix now includes the identity value as the initializer (Infinity for Math.min, -Infinity for Math.max), so empty-array behavior matches the spread form. Updated the message text, tests, and README accordingly.
|
@copilot Please refresh this PR branch if needed, review for any unresolved feedback, and run the
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. The PR adds a single well-scoped ESLint rule (no-math-minmax-array-spread) with tests and docs — no unnecessary complexity, no speculative abstraction, nothing to cut.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel: TypeScript tests (.test.ts) are not in the analyzed scope. This PR adds an ESLint rule with TypeScript tests in eslint-factory/src/rules/no-math-minmax-array-spread.test.ts. Only Go (_test.go) and JavaScript (*.test.cjs, *.test.js) tests are scored by this tool.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (default_business_additions=0, threshold=100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This rule currently recommends a transformation that changes Math.min/Math.max empty-input semantics and can introduce a new runtime throw.
Blocking theme
- The diagnostic message and README both steer callers from
Math.min(...values)/Math.max(...values)tovalues.reduce(...), but that replacement throws on[]where the built-ins returnInfinity/-Infinity. - Because the rule targets arrays derived from runtime data, empty inputs are not rare edge cases here.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.47 AIC · ⌖ 6.9 AIC · ⊞ 7K
Comment /review to run again
| }, | ||
| schema: [], | ||
| messages: { | ||
| noMathMinMaxArraySpread: "Avoid Math.{{method}}(...{{arg}}) — spreading an array of unknown size can throw `RangeError: Maximum call stack size exceeded`. Use `{{arg}}.reduce((a, b) => Math.{{method}}(a, b))` instead.", |
There was a problem hiding this comment.
This warning hard-codes a broken replacement: Math.max(...[]) returns -Infinity and Math.min(...[]) returns Infinity, but values.reduce((a, b) => Math.max(a, b)) throws on empty arrays, so the rule will push users toward a semantic regression and potential runtime crash.
💡 Why this needs fixing
The rule message is part of the API here, and right now it recommends a transformation that is only valid when the array is known non-empty. Many of the flagged cases are explicitly about data coming from workflow runs, API responses, or file scans, so emptiness is a realistic edge case rather than a theoretical one.
At minimum, the warning text and README need to call out the empty-array behavior and suggest a safe pattern, for example:
const largest = values.length === 0
? -Infinity
: values.reduce((a, b) => Math.max(a, b));Likewise for Math.min, preserving Infinity on empty input.
There was a problem hiding this comment.
Fixed in d98e5c9 — the reduce fix now includes the identity value as the initializer (Infinity for Math.min, -Infinity for Math.max), so empty-array behavior matches the spread form. Updated the message text, tests, and README accordingly.
There was a problem hiding this comment.
The rule is well-scoped and the tests cover the documented cases. One non-blocking issue was found, and there is an existing comment from Copilot about the empty-array behavior difference between Math.max(...[]) → -Infinity and [].reduce(...) → RangeError.
Additional non-blocking issue: The arguments.length !== 1 guard passes Math.max(...a, ...b) (multiple unbounded spreads, no fixed arg), which has the same stack-overflow risk the rule is designed to prevent. The exemption intent is "at least one fixed argument is present", not "more than one argument total".
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.7 AIC · ⌖ 8.9 AIC · ⊞ 5.7K
| if (argument.type !== AST_NODE_TYPES.SpreadElement) return; | ||
| if (!isUnboundedSpreadArgument(argument.argument)) return; | ||
|
|
||
| context.report({ |
There was a problem hiding this comment.
The arguments.length !== 1 guard silently passes Math.max(...a, ...b) — two unbounded spreads with no fixed argument — which is equally risky. Consider only exempting the case where at least one argument is not a spread element (i.e. a fixed value is present), rather than exempting all calls with more than one argument:
// current: any call with != 1 argument is skipped
if (node.arguments.length !== 1) return;
// suggested: only skip when a non-spread (fixed) arg is present
const hasFixedArg = node.arguments.some(a => a.type !== AST_NODE_TYPES.SpreadElement);
if (hasFixedArg) return; // at least one fixed arg → intentional bounded shape
// then check each spread argument individuallyThis would also flag Math.max(...a, ...b) where all arguments are unbounded spreads.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on three focused issues.
📋 Key Themes & Highlights
Issues found
- Empty-array behavior gap in docs (
README.mdline 175) — the suggestedreducealternative throws on empty arrays, whileMath.max(...[])returns-Infinity. The safe forms with initial values (-Infinity/Infinity) should be documented. - Missing empty-array test case (test file line 47) — no test pins the deliberate decision to exclude
Math.max(...[])from flagging; easy regression target. - Fragile hardcoded message string (test file line 93) — one test case asserts the full rendered message string while all others use
messageId. Makes the test brittle to wording changes.
Positive Highlights
- ✅ Excellent scope discipline: skips mixed forms, inline literals, and shadowed
Math— all edge cases handled correctly. - ✅ Computed access (
Math["max"]) is covered — easy to miss. - ✅ Rule wiring, README index, and parity test all updated in the same PR.
- ✅
hasLocalBindingscope-chain walk is a clean, reusable pattern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.3 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
| invalid: [ | ||
| { | ||
| code: `const largest = Math["max"](...values);`, | ||
| errors: [ |
There was a problem hiding this comment.
[/tdd] Hardcoded message string creates fragile coupling — if the message template changes, this test silently diverges from the messageId-based tests above.
💡 Use messageId for consistency
All other invalid test cases use { messageId: "noMathMinMaxArraySpread" }. This one case uses a raw string:
// fragile — breaks silently when message wording changes
errors: [{ message: "Avoid Math.max(...values) — ..." }]
// preferred — consistent with the rest of the suite
errors: [{ messageId: "noMathMinMaxArraySpread" }]If the goal is to assert exact rendered text (e.g. to guard against template regressions), add a dedicated snapshot test for the formatted message separately.
@copilot please address this.
| **Out of scope:** | ||
| - `Math.max(0, ...values)` and `Math.min(a, b, ...values)` — fixed arguments alongside the spread suggest an intentional, likely bounded call shape. | ||
| - `Math.max(...[1, 2, 3])` — inline array literals are statically bounded by the source. | ||
| - Calls where `Math` is shadowed by a local declaration. |
There was a problem hiding this comment.
[/grill-with-docs] The documented safe alternative silently swaps one failure mode for another: [].reduce((a, b) => Math.max(a, b)) throws TypeError: Reduce of empty array with no initial value, while Math.max(...[]) returns -Infinity.
💡 Show the fully safe form with an initial value
Update the README's Safe alternative section to:
**Safe alternative:**
- `values.reduce((a, b) => Math.max(a, b), -Infinity)` — folds the array without expanding into arguments; safe for empty arrays.
- `values.reduce((a, b) => Math.min(a, b), Infinity)` — equivalent safe form for `Math.min`.Without the initial value, users who follow the docs literally introduce a new crash on empty inputs.
@copilot please address this.
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] No test documents the empty-array boundary: Math.max(...[]) returns -Infinity but the suggested [].reduce(...) throws. A test case anchors the deliberate design decision and guards against silent future changes.
💡 Suggested test case
it("valid: spreading an empty inline array literal is accepted (statically bounded)", () => {
cjsRuleTester.run("no-math-minmax-array-spread", noMathMinMaxArraySpreadRule, {
valid: [`const x = Math.max(...[]);`],
invalid: [],
});
});This makes the current out-of-scope decision explicit and forces any future change to be a deliberate diff.
@copilot please address this.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
PR TriageCategory: chore | Risk: low | Priority: low (score 35)
Recommended action: defer — resolve lint-rule review feedback
|
|
🎉 This pull request is included in a new release. Release: |
Math.max(...arr)expands every element into call arguments, so an array whose size depends on runtime data can throwRangeError: Maximum call stack size exceeded(limit is engine-dependent, commonly tens of thousands of elements). Three instances of this pattern exist inactions/setup/js, all over arrays derived from workflow-run or file-scan data.Rule (
eslint-factory/src/rules/no-math-minmax-array-spread.ts)Math.min/Math.maxcalls whose sole argument is a spread of an identifier, member expression, or call expression. Handles computed access (Math["max"]).Math.max(0, ...arr)) — fixed arguments imply an intentional, bounded call shape.Math.max(...[1, 2, 3])) — statically bounded by the source.Mathis shadowed by a local binding.Wiring
src/index.ts; enabled aswarnineslint.config.cjs.index-readme-paritytest).no-math-minmax-array-spread.test.tscovering each valid/invalid branch above.Scope is limited to
eslint-factory/**; no source changes to the flaggedactions/setup/jscall sites, which surface as warnings.