[robobun]

Draft on purpose: this patch is for WebKit/WebKit first, not for a merge into the fork. Found in the review of #784, not by a user.

Problem

• A bare call of a mapped parameter passes the caller's arguments object as this. So expect(new Counter().set).toThrowError(TypeError) of jasmine-core fails on Bun and passes on Node.
• delete p on a mapped parameter deletes the property p of arguments.
• emitResolveScope (bytecompiler/BytecodeGenerator.cpp:2742) returns the arguments register as the scope of a mapped parameter. The call emitters (NodesCodegen.cpp:1525) move it into this.

Fix

• emitResolveScope returns null for a mapped parameter. emitGetFromScope and emitPutToScope name the arguments register themselves. argumentsRegister() is removed.
• FunctionCallResolveNode and TaggedTemplateNode load undefined into the this register. delete p is false. ECMA-262 requires both.
• Verified: JSTests/stress/direct-arguments-parameter-call-receiver.js (new, fails on main). run-javascriptcore-tests, main against the branch: no new failure.
• Self-reviewed: 26 concerns raised, 21 addressed, 5 not taken (see Notes).

Background

• A sloppy function with simple parameters that mentions arguments and captures no parameter keeps them in a DirectArguments object: mapped parameters.
• A call by name passes the resolved scope as this, for with. op_to_this and JSValue::toThis turn a scope into undefined, but not an arguments object.
• Considered arms in the emitters only: the 44 callers of emitResolveScope keep the register.

Downsides

• Code that relies on that this now gets undefined or globalThis, as in V8.
• A later emitter that uses that register as a value crashes the bytecode generator.
• Release jsc .text: +256 bytes. Bytecode of other code blocks: 0 of 13055 differ (JetStream2).

Notes

Route. This PR does not ask for a merge into the fork as it stands.

• The three files are upstream's code, and WebKit/WebKit main has the same lines (return argumentsRegister(); is at BytecodeGenerator.cpp:2716 there). The patch has no USE(BUN_JSC_ADDITIONS) guard. It applies to WebKit/WebKit main with patch -p1: 9 hunks, one with fuzz, because the fork has emitTDZCheck(target, variable) where upstream has emitTDZCheck(target). The test is a new file.
• @sosukesuzuki: the three nearest fixes are yours, [JSC] Keep the call in (a?.b)(x) outside the optional chain WebKit/WebKit#74773, [JSC] Tagged template with a parenthesized optional chain tag should keep this WebKit/WebKit#75047 and [JSC] Tagged template with a super property tag should be called with the current this WebKit/WebKit#75355. They are receivers of calls in the same emitters. I cannot file the bugs.webkit.org entry or the WebKit/WebKit PR.
• Upstream chooses the shape. The arms in NodesCodegen.cpp are enough for the behavior, with generator.emitResolveScope(nullptr, var) in place of nullptr. The BytecodeGenerator part (the null return, the removed accessor, the RELEASE_ASSERT change) is hardening on top of them.
• For the upstream report: no instruction is added (7 to 7, 9 to 8 and 5 to 5 below), so the reason for the revert of 284931@main (bug 281214) does not apply. The open re-land of bug 225397, [JSC] Don't use JSScope instances as this value when calling functions WebKit/WebKit#35527, has no delete part.
• What I expect to happen to this PR: it closes when a sync brings the upstream commit and the test of this branch passes on that main. Or it becomes a cherry-pick of that commit, as [JSC] Array profiles should not turn ordinary arrays into ArrayStorage (cherry-pick WebKit/WebKit#75376) #785 was. [JSC] (a?.b)(): a nullish a throws a TypeError, and a class field initializer makes the call #650 closed that way.
• The question for a maintainer: does the fork take this before upstream does? CI did not run: the head is a branch of a fork. The same commit is on a branch of this repository with the same name, and a run of the CI workflow on that branch builds it.

What changes. The caller is sloppy, mentions arguments, has a simple parameter list and captures no parameter. main is 87ee10d, the base of the branch. Line numbers in this description are those of that commit. Node v26.3.0 gives the right column.

Code in the caller	main	this PR
cb(), (cb)(), cb?.(), cb(...xs), strict callee	[object Arguments]	undefined
the same, sloppy callee	[object Arguments]	globalThis
cb`x`	[object Arguments]	undefined or globalThis
cb() with cb = Object.prototype.valueOf	returns the arguments object	TypeError
cb() with a Proxy, thisArgument of the apply trap	the arguments object	undefined
delete p	true, and arguments loses its property p, for example length	false

• The callee gets a live reference: a write to this[1] changes the second parameter of the caller.
• An async function with no await is affected too. Not affected: o.m(), cb.call(x), arguments[0](), a caller that does not mention arguments, a strict caller, a captured parameter, a parameter named eval, code in with.
• This is a conformance fix. ECMA-262 EvaluateCall takes the this value from refEnv.WithBaseObject(), which is undefined for a function environment. DeleteBinding returns false for a binding that cannot be deleted.

In Bun (1.4.3, .cjs file). Node gives the second value.

function victim(callback, secret) { if (arguments[5] === 1) return; callback(); return secret; }
console.log(victim(function () { if (this !== undefined && this !== globalThis) this[1] = "overwritten"; }, "the caller's own parameter"));
// Bun: overwritten                      Node: the caller's own parameter
function readFile(path, options, callback) { var args = Array.prototype.slice.call(arguments); if (args.length === 2) { callback = options; options = {}; } return callback(null, path); }
console.log(Object.prototype.toString.call(readFile("p", function () { return this; })));
// Bun: [object Arguments]               Node: [object global]

• Bun's transpiler drops a statement that is only arguments;. A repro for Bun must use the object, as arguments[5] does above.
• jasmine-core 5.4.0, toThrowError (jasmine.js:6885 to 6894): compare reads arguments and calls actual(). A method that writes this.n, passed without its object, does not throw on Bun. Node throws TypeError, and the expectation passes.
• mock.contexts[0] of bun:test is the arguments object of the caller for such a call.
• Bun's transpiler drops a "use strict" directive in a function body of a .cjs file. So a caller that is strict only by that directive is affected in Bun too.
• A search of the oven-sh/bun issues for these symptoms finds no report.

Bytecode at the changed sites (jsc --dumpGeneratedBytecodes=true).

function bare(cb, s) { arguments; return cb(); }
main:     mov loc8, loc5          get_from_arguments loc7, loc8, 0     7 instructions, 23 bytes
this PR:  mov loc8, Undefined     get_from_arguments loc7, loc5, 0     7 instructions, 23 bytes, +1 constant

• tag`x` : 9 instructions and 29 bytes become 8 and 26, +1 constant.
• delete p: 5 instructions and 13 bytes (del_by_id on the arguments register) become 5 and 11 (mov False), +1 constant.

Measurements. Two Release builds of jsc with the same flags, main 87ee10d and this branch.

• Bytecode of unaffected code blocks: 0 of 13055 differ, constant pools included. The corpus is one JetStream2 run (cli.js, testIterationCount=1, about 42600 code blocks generated, 13058 distinct). A control of main against main gives 0. The 3 blocks that differ are call, each and map of recast. Each has one bare call of a mapped parameter.
• The 155 files JSTests/stress/*arguments*.js: 0 of 2207 distinct code blocks differ.
• Release jsc .text: 28562478 to 28562734 bytes (size -A). The bytecompiler object: +259 bytes.
• Work at bytecode generation: one VarKind compare at each call, tag or delete of a name that is not a local. That is 25245 sites in the JetStream2 run. Instructions added to emitted code: 0.
• No speed change is claimed. One effect was measured: the arguments object is no longer a child of the Call node, so the arguments elimination of the FTL can remove it when the callee is not inlined. Heap bytes per call for that shape: 64.0 before, 0.0 after ($vm.heapTotalBytesAllocated() over 5000000 calls). With --useFTLJIT=0 it is 64.0 on both builds.
• The new test: 17 of 17 default configurations pass, in 34 to 99 ms each (run-jsc-stress-tests --report-execution-time). On main it fails with bareCall with a strict callee: expected undefined but got [object Arguments].
• perf, valgrind and bloaty are not installed on this machine. There is no instruction count.

A consumer that this PR missed would crash, not leak. The new test compiles the 4 places that a mapped parameter can reach. A build with one of them removed crashes on the test: the RELEASE_ASSERT change (exit 134), the arm in FunctionCallResolveNode, the arm in TaggedTemplateNode, the condition in DeleteResolveNode (exit 139 each). EvalFunctionCallNode has the same shape but only gets an ASSERT: a function that calls a name spelled eval captures all of its parameters, so that name is never a mapped parameter.

The 44 callers of emitResolveScope, by what they do with the result.

Caller	Result
FunctionCallResolveNode, TaggedTemplateNode	was the this of the call. A mapped parameter now takes a separate arm.
EvalFunctionCallNode	the this of the call. Not reachable with a mapped parameter, see above.
DeleteResolveNode	was the base of del_by_id. A mapped parameter now returns false before it.
ResolveNode, PostfixNode::emitResolve, PrefixNode::emitResolve, TypeOfResolveNode, ReadModifyResolveNode, ShortCircuitReadModifyResolveNode, ForInNode::emitLoopHeader, ForOfNode, EmptyVarExpression (type profiler only)	passed to emitGetFromScope and emitPutToScope, which do not use it for a mapped parameter
AssignResolveNode, BindingNode::bindValue, AssignmentElementNode::bindValue	the same, and passed to emitTDZCheckIfNecessary
the other 28	the variable is never a mapped parameter: a private name, a name of a class scope, a let binding, a non-local name, a parameter of a non-simple list, a function declaration

emitTDZCheckIfNecessary is reached with a null scope when a mapped parameter has the name of a let of an enclosing scope that is in its dead zone. The check is by name. The RELEASE_ASSERT there now accepts a mapped parameter.

Suites.

• Tools/Scripts/run-javascriptcore-tests as CI runs it (JSTests, LayoutTests/js, PerformanceTests, 124139 runs), on a build of main and a build of the branch: 0 runs fail only with the branch. 2326 runs in 218 files fail on both builds on that machine. Most of them are Intl tests: its ICU data cannot format a number.
• That run, the *arguments*.js comparison and the timing of the new test are from the revision before the last one. The last revision replaced an arm in EvalFunctionCallNode that cannot run by the ASSERT, and changed comments. It changed no emitted bytecode: 0 of the 13058 JetStream2 blocks differ between the two revisions.
• On the last revision: the new test passes on the Release build, and in all 17 configurations on a build with ENABLE_ASSERTS=ON. That build also ran the 7635 runs of the tests whose name has arguments, eval, tagged-template, delete, spread, call-varargs, tail-call, to-this, this-value or optional-chain: no assertion fails.
• The new test also passes with --useTypeProfiler=true, --useControlFlowProfiler=true, --forceDebuggerBytecodeGeneration=true and --useJIT=false, and in V8 with the message text of one assertion changed.
• The branch is one commit on 87ee10d. It merges with main 274ebb3 without a conflict: [JSC] The control flow profiler's basic blocks and function ranges match the text #828 changed other functions of the same files.

Self-review, not taken.

• Shorten the test to about 30 lines. Not taken: on the upstream route this test is the check that the fork got every form back.
• Pass undefined also for a callee in a closure variable. Not taken here: it is a separate change, at about 2400 call sites of the JetStream2 run against 3.
• Write the arms with emitResolveScope(nullptr, var). Not taken: it is only needed without the BytecodeGenerator part, see Route.
• Set a date for the upstream report. Not taken: a maintainer sets it.
• Close TaggedTemplateNode: call super.tag... and super[key]... with the method's this #438 and TaggedTemplateNode: call (a?.b)... with |this| = a, and super.tag... with the method's this #534, and open an issue for the other differences below. Not taken: outside this PR.

Open PR #516 rewrites the arm below each new arm in the call emitters. git merge-tree of #516 gives 8 conflict hunks against main and the same 8 against this branch (BytecodeGenerator.h 1, DFGByteCodeParser.cpp 5, JITPropertyAccess.cpp 2). NodesCodegen.cpp merges with no conflict.

Not changed here.

• A callee held in a closure variable or in a non-local variable still gets the scope as this. A JS callee that uses this converts it. A native must call toThis ([JSC] A native called with no receiver must not work on the caller's scope #784).
• A function declaration with the name of a mapped parameter does not change arguments[0]. V8 shows the function there.
• A callee can still read a copy of the parameters of a sloppy caller through caller.arguments, as in V8.
• Bun needs a test with the WEBKIT_VERSION that brings the fix: the jasmine case and mock.contexts.

[robobun]

Status: draft on purpose. This is an upstream candidate, not a request to merge into the fork (see Route in the notes of the description).

How to reproduce (jsc shell of main 87ee10d. In Bun 1.4.3 and Node.js, use console.log and a .cjs file):

function victim(callback, secret) { if (arguments[5] === 1) return; callback(); return secret; }
print(victim(function () { if (this !== undefined && this !== globalThis) this[1] = "overwritten"; }, "the caller's own parameter"));
function deleteIt(length) { return [delete length, arguments[0], arguments.length].join(); }
print(deleteIt(7));

• main and Bun 1.4.3 print overwritten and true,7,.
• This branch and Node.js v26.3.0 print the caller's own parameter and false,7,1.