fix: expand spread args to Math.max/min/hypot (#951)#957
Merged
Conversation
Compiled `Math.max(...arr)` / `min` / `hypot` returned NaN: the variadic Math emitters emitted each arg via EmitExpression, but a spread `...arr` is an Expr.Spread whose emit yields the array object, so ToNumber(array) → NaN. Route spread calls through the variadic object[] adapters with a spread-expanded args array (reusing the f(...arr) expansion path, extracted as EmitArgsArrayWithSpread on IEmitterContext). The interpreter was also affected (contrary to the issue text): the built-in static call path (CallBuiltInSync / CallBuiltInWithPooledArgs) never expanded spreads, leaking the array as a single Object-kind arg so AsNumber() threw. Both paths now expand spreads before dispatch. Also fixes two adjacent interpreter spec bugs surfaced by differential testing: Math.max/min ignored NaN args (now propagate NaN), and Math.hypot lacked the ECMA-262 Infinity-before-NaN check (now returns Infinity). The compiled hypot adapter gains the same Infinity-first check so value-form and spread paths agree. Tests: MathSpreadArgumentTests covers basic/mixed/empty/boxed-array/ NaN-Infinity/async spreads across interpreter and compiled modes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #951.
Problem
In compiled mode,
Math.max(...arr)/Math.min(...arr)/Math.hypot(...arr)returnedNaN. The variadic Math emitters iteratedargumentsand emitted each viaEmitExpression, but a spread argument...arris anExpr.Spreadwhose emit just yields the inner array object — soToNumber(array)→NaN.While fixing it, differential-vs-Node testing showed the bug was broader than the issue described (the "compiled-only; interpreter unaffected" note was wrong): the interpreter also failed, and there were two adjacent
Math.*spec bugs.Changes
Compiled (the reported bug)
MathStaticEmitter—min/max/hypotdetect spread args and route to the variadicobject[]adapters with a spread-expanded argument array. The hypot guard runs before the generalToNumberloop so it doesn't corrupt the stack.ExpressionEmitterBase.CallHelpers— extracted the existingf(...arr)arg-array-with-spread logic into a reusableEmitArgsArrayWithSpread(exposed onIEmitterContext), keeping the working generic-call path as the single source of truth.RuntimeEmitter.MathAdapters— added the ECMA-262 21.3.2.16 Infinity-first check to the hypot adapter so value-form (const h = Math.hypot) and spread paths agree.Interpreter (also affected)
Interpreter.Calls— the built-in static call path (CallBuiltInSync+ asyncCallBuiltInWithPooledArgs) never expanded spreads, leaking the array through as a singleObject-kind arg soAsNumber()threw. Both now expand spreads into a flat arg list before dispatch.MathBuiltIns— pre-existing bugs surfaced by testing:Math.max/min(1, NaN, 3)returned1/3(nowNaN);Math.hypot(NaN, Infinity)returnedNaN(nowInfinity).Tests
MathSpreadArgumentTests— basic / mixed / empty / boxedany[]/ NaN-Infinity / async spreads, run in both interpreter and compiled modes (interpreter as the Node-matching oracle).Verification
Test262baseline tests (untracked local scaffolding with a stale baseline — fails identically on cleanmain) and one load-flaky unit test (differs each run, passes in isolation; the known .NET 10 tier-0 QuickJit issue).