Repository navigation
Conversation
…ody is a binding of its own
function f(a = () => arguments.length) { var arguments = 7; return a(); }
f(undefined, 1, 1, 1); // 4. Before: undefined, because the closure read 7.
When a function has parameter expressions, the body's vars live in an
environment below the one that holds the parameters and `arguments`
(FunctionDeclarationInstantiation, step 28). A var that has the name of a
parameter binding starts with that binding's value, and the two are distinct
after that. "arguments" is one of those bindings.
For a non-simple parameter list the BytecodeGenerator puts "arguments" in the
parameter scope, and it skipped the body's `var arguments`, so the body's name
resolved to the parameter scope. A store in the body changed what the
parameter expressions and their closures read.
The loop over varDeclarations() now creates the var "arguments" like any other
var. initializeDefaultParameterValuesAndSetupFunctionScopeStack() already
copies the parameter scope's value into a var that has the name of a parameter
binding, and "arguments" is in that set.
A function named `arguments` in a block of the body had the same defect:
hoistSloppyModeFunctionIfNecessary() fell back to the parameter scope when the
var scope had no "arguments". The var scope always has it now, so the fallback
is gone. Annex B.3.2.1 stores the function to the VariableEnvironment.
Two upstream tests expected the closure in the parameters to see the body's
store. They expect the arguments object now (JSTests/BUN-TEST-DIFFERENCES.md,
A10). V8 gives the same results.
|
Preview build of f2bfc5b: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reached
This review includes 3 billable files and costs up to $0.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughJavaScriptCore now creates separate body and parameter-scope ChangesArguments binding semantics
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change separates parameter and body arguments bindings with regression coverage and is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, fix, testing evidence, and changed behavior. However, it does not include the required Bugzilla URL or the required Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked whether removing the parameter-scope fallback in hoistSloppyModeFunctionIfNecessary() makes its RELEASE_ASSERT(!entry.isNull()) reachable: Parser.h finalizeSloppyModeFunctionHoisting() always adds a hoisted function's name to the function's declared variables with setIsVar() (or skips hoisting when the name is a parameter), and the var loop in the BytecodeGenerator constructor now creates every such var, so the assert is not newly reachable.
Extended reasoning...
The change touches the bytecode generator's function-scope setup (Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp) so that a body var arguments gets its own binding when a non-simple parameter list puts arguments in the parameter scope, and removes the matching fallback in sloppy-mode function hoisting; it also rewrites two upstream test expectations, adds a new stress test, and updates BUN-TEST-DIFFERENCES.md and the test count in CLAUDE.md. No security-sensitive surface is touched. It is a deliberate semantic divergence from upstream WebKit in a core engine path, and an inline finding reports a case where the new behavior disagrees with the spec, so a human should weigh it.
…at the function in a block follows The stress test also covers a parameter named `arguments` that a pattern or a rest parameter binds, a function expression named `arguments`, a direct eval in the body that declares the name again, and a store before (and a `delete` after) a function named `arguments` in a block. It also runs with the debugger's bytecode and with the profilers. For a function named `arguments` in a block, the var exists from the start and starts as the arguments object. That is what V8 does and what the open issue tc39/ecma262#991 proposes. The current text of Annex B.3.2.1 makes the var when the block runs. Only a store before the block or a `delete` after it can tell the two apart. The comment in the BytecodeGenerator and JSTests/BUN-TEST-DIFFERENCES.md now say so.
|
The bun side is oven-sh/bun#43772. It pins the preview build of the first commit here and adds a test that runs the same cases through Bun. |
There was a problem hiding this comment.
I re-reviewed after the follow-up commit and found no bugs; because this changes scoping semantics in the bytecode generator, diverges from upstream WebKit on two of its tests, and pins a deliberate V8-over-current-Annex-B choice for the block-function case, a JSC owner should still sign off on that policy.
What was reviewed:
- The var-creation path: with
argumentsin the parameter scope, the body'svar argumentsnow goes through the existing step-28 copy (parameterSetincludesargumentsat BytecodeGenerator.cpp:1278), so it starts as the arguments object and stays a separate binding. - Reachability of the remaining
RELEASE_ASSERTinhoistSloppyModeFunctionIfNecessaryafter the parameter-scope fallback was removed: every hoisted name is anisVarentry the loop now creates, and generator/async wrapper parameter names return early before the lookup; generator bodies had a simple parameter list before and are unchanged. - The follow-up commit answers the earlier inline note by documenting the block-function-named-
argumentsbehaviour in BUN-TEST-DIFFERENCES.md A10 and adding tests that pin it; the C++ diff is otherwise unchanged from the first push.
Extended reasoning...
The change removes the skip of var arguments in the BytecodeGenerator constructor's var loop and the parameter-scope fallback in hoistSloppyModeFunctionIfNecessary, so a sloppy function with parameter expressions gets two arguments bindings; it also rewrites one expectation in each of two upstream stress tests, adds a 195-line stress test, and documents the divergence in BUN-TEST-DIFFERENCES.md and CLAUDE.md. No security-sensitive surface is touched. The mechanism reuses the existing step-28 copy for vars that shadow parameters and the diff is small, but it is a language-semantics change that intentionally diverges from upstream WebKit and, for a function named arguments in a block, from the current Annex B.3.2.1 text in favour of V8, and /Source/JavaScriptCore and /JSTests are CODEOWNERS-covered, so a human decision on that policy is warranted rather than automated approval.
…arguments-parameter-expressions
Problem
var argumentsalso changes theargumentsof the parameter expressions.function f(a = () => arguments.length) { var arguments = 7; return a(); }returnsundefined. The spec and V8 return the argument count.BytecodeGeneratorconstructor (bytecompiler/BytecodeGenerator.cpp:812) keepsargumentsin the parameter scope and skips the body'svar arguments. The body then uses the parameter scope's binding.Fix
varDeclarations()creates the var like any other var.initializeDefaultParameterValuesAndSetupFunctionScopeStack()already copies a parameter binding's value into a var of the same name.hoistSloppyModeFunctionIfNecessary()loses its fallback to the parameter scope. A function namedargumentsin a block now goes to the body's var, as in V8.JSTests/stress/var-arguments-with-parameter-expressions.js(new) fails onmainand passes in everyrun-jsc-stress-testsmode. Self-reviewed: 8 concerns raised, 8 addressed (Notes).Background
arguments. The body's vars live in a second environment below it.Downsides
argumentsin a block, the var exists from the start, as in V8. The spec text makes it when the block runs (Legacy hoisting forfunction arguments() {}still not correct? tc39/ecma262#991, Notes).var argumentsgets one more variable and one copy at entry. All other bytecode is unchanged (598 stress tests compared).Notes
History. The skip and the test that expects it came with 176000@main (2016, bug 157786), which moved
argumentsto the parameter scope. Upstreammainstill has both. test262 has no test for this case: a differential run oftest/languageandtest/annexB(45,053 runs, sloppy and strict) shows no change.A function named
argumentsin a block. With this change its var exists from the start and starts as the arguments object, like avar arguments. The block stores the function to it, and the closures in the parameters keep the arguments object. V8 does the same, and the open issue tc39/ecma262#991 proposes it. The current text of Annex B.3.2.1 differs in one detail: it makes that var when the block runs. Only a store toargumentsbefore the block, or adelete argumentsafter it, can tell the two apart. The new test pins V8's results for both. Before this change the block stored the function to the binding of the parameters, which no reading of the spec gives.Two upstream tests expected the old behavior.
stress/arrow-functions-as-default-parameter-values.jsexpectedx() === argumentsaftervar arguments = 25.stress/sloppy-mode-hoist-arguments-function-non-simple-parameter-list.jsexpected the closure in the parameters to see a function namedargumentsfrom a block. Each has one new expectation and a// Bun:comment.JSTests/BUN-TEST-DIFFERENCES.mdhas the new section A10. V8 fails the same two lines of the old tests.The generated code. For
function f(a = arguments, b = arguments.length) { var arguments = b + 3; ... }the var is a register, and the copy is twomovs after the parameters. With a closure in the parameters both bindings are in environments: the parameter scope holds onearguments, the var scope holds the other, and the copy is aget_from_scopeand aput_to_scope.Same bytecode for other functions. I dumped the bytecode (
--dumpGeneratedBytecodes=true) of 598 stress tests whose names mention parameters, arguments, closures, eval, generators or hoisting, with ajscofmainand with this change. It differs in the three tests above only (and in tests that generate random source or use workers, which also differ between two runs of one binary).What I ran, with a Release
jscthat has assertions on, against ajscofmainbuilt the same way:run-jsc-stress-tests. The new test adds two runs of its own: the debugger's bytecode, and the type and control flow profilers. By hand also--validateBytecode=true --validateGraphAtEachPhase=true,--useJIT=false,--useLLInt=false,--useSourceProviderCache=falseand--useBytecodeOptimizer=true.JSTests/stresswith the default options: the only differences are the three tests above.LayoutTests/jsc-layout-tests.yaml,es6.yaml,ChakraCore.yaml,mozilla-tests.yaml,typeProfiler.yaml,controlFlowProfiler.yamlandmodules.yaml: 14,714 runs pass.with) compared with node. With this change the only differences left are the four defects below.awaitandnew Function, with up to four parameters from 16 kinds (closures that read or writearguments, eval, patterns, a rest parameter) and up to six body statements from 29 kinds. With this change all 50,000 agree with node. Ajscofmaindiffers on about 5,200. 8,000 of them also ran 60 times each with low JIT thresholds and--useConcurrentJIT=false: the result is the same on every run.Self-review. Eight concerns, all addressed. (1) Two upstream tests encode the old behavior: changed, with the reason in
BUN-TEST-DIFFERENCES.md. (2) Is the removed fallback inhoistSloppyModeFunctionIfNecessary()still reachable? No.finalizeSloppyModeFunctionHoisting()is the only place that marks a function as hoisted, and it makes the name a var ofvarDeclarations()in the same step. The loop now creates every such var. The names of a generator wrapper's parameters are the one exception, and the hoist returns before the lookup for those. TheRELEASE_ASSERTstays. (3) Does other bytecode change? No, see above. (4) The 200 ms rule: the new test takes 80 to 170 ms in its slowest mode (ftl-eager) on a build with assertions, 25 ms with the default options. (5) The first version of this text said that the function in a block follows the spec. It follows V8 and tc39/ecma262#991, see above. The comment in the code,BUN-TEST-DIFFERENCES.mdand the test now say so. (6) The change also reaches a direct eval in the body that declaresargumentsagain when the body hasvar arguments(mainmade a second binding that the body never read, also with a rest parameter only). The test pins it. (7) Shapes the first test did not pin: a parameter namedargumentsthat a pattern or a rest parameter binds (a simple parameter of that name never reached the changed line), a function expression namedarguments, a store combined with a function in a block, and the debugger and profiler modes. All added. (8) One stress test timed out once in a run under load with the debugger's bytecode. It has simple parameters only, and it passes in 1.5 s on both binaries, three times each.Found next to it, not changed here. Each has a different cause.
function f(a = 1) { eval("var a = 2"); return a; }gives 1, V8 gives 2. The body resolves the name to the parameter scope when the code is generated. The same goes foreval("var arguments = 2")and for a function in a block inside the eval.await,var arguments;with no initializer readsundefined. The body is an inner function with its own var, and nothing gives it the arguments object.function* g(a, b = () => a) { var a = 2; yield b(); }yields 2, V8 yields 1.(function (a) { return typeof arguments[0]; function a() {} })(1)gives"number", V8 gives"function". The function declaration replaces the parameter, and the mapped arguments object does not see it.arguments,function (y = () => arguments, z = y(), {arguments}) {}:zis the arguments object. The spec and V8 throw a ReferenceError, because the parameterargumentsis not initialized yet.