[JSC] new.target in a class field initializer or a class static block throws a ReferenceError when the class is in an arrow function - #647
Conversation
… does not make the code around the class a user of new.target
A class field initializer and a class static block are functions of their own
at run time (ClassFieldInitializerMode, ClassStaticBlockMode). They run as a
method call, so new.target is undefined in them.
parseClass() parses the source of both in place, with the scopes and the tree
builder of the code around the class: parseAssignmentExpression(context) for a
field initializer and parseBlockStatement(context, BlockType::StaticBlock) for
a static block. That parse checks the syntax and finds the end. Each function
is parsed again, on its own, when it is compiled.
For new.target, parseMemberExpression() calls context.createNewTargetExpr(),
which sets NewTargetFeature on the code that the tree builder builds. In the
in-place parse that is the code around the class. It also calls
setInnerArrowFunctionUsesNewTarget() on the current scope when the scope
inherited isArrowFunction, which is true for a class scope in an arrow function.
BytecodeGenerator emits emitLoadNewTargetFromArrowFunctionLexicalEnvironment()
on entry to an arrow function, and to eval code, that
needsNewTargetRegisterForThisScope(). It resolves @newTargetLocal with
ThrowIfNotFound. So this throws "ReferenceError: Can't find private variable:
PrivateSymbol.newTargetLocal" when the arrow function is called:
(() => { class A { x = new.target; } })();
No function is around the arrow function, so nothing stored @newTargetLocal.
The same happens for a static field, for a static block, for an async arrow
function (the call throws, the promise does not reject), and for
(0, eval)("class A { x = new.target }"). With a function around the arrow
function, a field works by accident: the class scope passes the inner arrow
function feature up to the function, and the function stores its new.target
for the arrow function to load. A static block scope is a function boundary
and does not pass the feature up, so this throws too:
function f() { (() => { class A { static { new.target; } } })(); }
Add Parser::isDirectlyInClassElementParsedInPlace(). It walks up from the
current scope to the first function boundary. It returns true for a class
scope with the new isParsingFieldInitializerInPlace flag, which parseClass()
sets while it parses a field initializer, and for a static block scope that is
not the outermost scope of the parser. When it returns true,
parseMemberExpression() does not call setInnerArrowFunctionUsesNewTarget() and
createNewTargetExpr() does not record NewTargetFeature.
The parse of the class element as a function of its own is not in place, so it
records the feature on that function as before. new.target in an arrow
function in a class element is not changed either: the arrow function records
the inner arrow function feature, and the function of the class element
stores its (undefined) new.target for it.
|
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 (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe parser now distinguishes class elements parsed in place from enclosing code when handling ChangesClass element
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No merge-blocking issue has been identified from the available evidence. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 checked that the new m_isParsingFieldInitializerInPlace bit is the 31st one-bit Scope member, so the packed group stays within 4 bytes and Scope::verifyLayout() offsets do not shift. I also confirmed the flag is cleared before failIfFalse(initializer, ...), so a parse error in the initializer does not leave it set on the class scope, and that isDirectlyInClassElementParsedInPlace() stops at an arrow-function boundary and only treats a static-block boundary as in-place when it has a containing scope — so the reparse-as-own-function pass and arrows nested inside a field initializer keep their existing attribution.
Extended reasoning...
The posted inline finding is a test-convention nit (hard-coded loop count vs testLoopCount), not a correctness concern with the parser change itself. While reading the diff I separately checked three things a JSC reviewer would likely want confirmed: (1) the new one-bit Scope member does not overflow the packed bit-field group and shift m_lexicalVariables past the cache-line assertion in Scope::verifyLayout(); (2) the manual set/reset of the flag around parseAssignmentExpression is safe on the error path because the reset precedes failIfFalse; (3) the scope walk in isDirectlyInClassElementParsedInPlace() returns false for an arrow function nested inside a field initializer (function boundary that is not a static-block boundary) and for a static block that is the outermost scope of its own reparse (containingScope() is null), so only the genuinely in-place cases suppress the NewTargetFeature. None of these turned up a problem, so I noted them as ruled out rather than staying silent, since the inline nit alone would not convey that the parser logic was examined.
Preview Builds
|
The change is in the parser, so a case passes or fails on its first run. No loop count is needed.
There was a problem hiding this comment.
Thanks for dropping the hard-coded iteration loops in the stress test — that resolves the earlier nit (this bug reproduces at parse/first-execution, so single-shot assertions are fine). I re-reviewed after the new push and didn't find any bugs; because the change alters new.target feature-bit propagation in the parser scope chain, a JSC reviewer should still give it a look.
What was reviewed:
isDirectlyInClassElementParsedInPlace()scope walk — stops at arrow-function boundaries (isFunctionBoundary()is true for arrows), sox = () => new.targetstill marks the class element correctly on re-parse.- Static-block detection via
containingScope()— outermost re-parse scope has no parent, in-place scope does; matches the intent. SetForScope-free toggle ofm_isParsingFieldInitializerInPlace— reset happens before anyfailIfFalseearly return, so no stale state on parse failure.- Computed keys / heritage still use the surrounding code's
new.target(flag is only set around the initializer expression, not the whole class element).
Extended reasoning...
Overview
The PR fixes a JSC parser bug where new.target inside a class field initializer or static block incorrectly marks the surrounding code (arrow function / eval) as a new.target user, causing a ReferenceError on @ newTargetLocal load. It adds a per-Scope bit (m_isParsingFieldInitializerInPlace), a scope-walk helper isDirectlyInClassElementParsedInPlace(), and threads a usesNewTargetOfThisCode flag into ASTBuilder::createNewTargetExpr / SyntaxChecker::createNewTargetExpr. A comprehensive stress test covers instance/static/private/computed fields, static blocks, nested arrows/classes, eval, async arrows, non-simple parameters, and computed keys/heritage. Since the prior review, only the test file changed: the three for (let i = 0; i < 100; ++i) loops were removed in favor of single-shot assertions, which addresses the earlier testLoopCount nit and is appropriate given the bug is parse-time, not tier-dependent.
Security risks
None identified. This is a correctness fix in the parser's feature-bit bookkeeping; it does not touch auth, crypto, permissions, or memory management. The new Scope bit is the 31st single-bit member and does not perturb the verifyLayout() invariant per the PR notes. No untrusted input handling changes — the parser already handled these tokens; only the propagation of a boolean feature flag changes.
Level of scrutiny
Moderate-to-high. Parser scope-chain semantics in JSC are subtle: the fix relies on the invariant that field initializers and static blocks are re-parsed as standalone functions at compile time (so suppressing the feature bit during the in-place parse is safe), and on isFunctionBoundary() returning true for arrow functions so the walk stops before reaching the class scope when inside a nested arrow. I traced these and they hold, but this is exactly the kind of change where a JSC-domain reviewer's confirmation is valuable. The changed paths (Source/JavaScriptCore/**, JSTests/**) are also listed under @ WebKit/jsc-reviewers in .github/CODEOWNERS.
Other factors
The stress test is thorough and, per the author, verified to fail before the fix and pass after (including under --useJIT=0, --validateBytecode=1, etc.) and cross-checked against V8. The setIsParsingFieldInitializerInPlace(false) reset occurs before the failIfFalse(initializer, ...) bailout, so a failed initializer parse does not leave the class scope in a stale state. The SyntaxChecker stub was updated in lockstep with ASTBuilder. No outstanding third-party CHANGES_REQUESTED reviews are on the timeline.
|
The bun side is oven-sh/bun#42653. It substitutes |
1 participant
Problem
(() => { class A { x = new.target } })()in global or module code throwsReferenceError: Can't find private variable: PrivateSymbol.newTargetLocalon entry to the arrow function. The same for a static field, a static block, an async arrow function and indirect eval. The value must beundefined.parseClass()parses a field initializer and a static block in place, with the tree builder of the code around the class (parser/Parser.cpp:3373,:3390).new.targetthere setsNewTargetFeatureon that code (:5490). An arrow function or eval code with the feature loads@newTargetLocalon entry, and no function stored it. Upstream has the same code.Fix
Parser::isDirectlyInClassElementParsedInPlace()walks up to the first function boundary. It is true for a class scope with the newisParsingFieldInitializerInPlaceflag, and for a static block scope that is not the outermost scope.parseMemberExpression()skipssetInnerArrowFunctionUsesNewTarget()andcreateNewTargetExpr()does not record the feature.JSTests/stress/new-target-in-class-field-initializer-and-static-block.jsfails before and passes after. All 35 stress tests that containnew.targetpass.Background
new.targetisundefinedin them.new.target. It loads@newTargetLocalfrom the scope of the closest function, which stores it when an inner arrow function needs it.NewTargetFeaturecomes from theASTBuilderof the code that is compiled.Notes
How each case fails before this change:
isArrowFunction, soparseMemberExpression()callssetInnerArrowFunctionUsesNewTarget()on it, andcreateNewTargetExpr()setsNewTargetFeatureon the arrow function. With a function around the arrow function the inner arrow function feature reaches that function, it stores its ownnew.target, and the arrow function loads a value that it never uses. Without a function, the load throws (emitLoadNewTargetFromArrowFunctionLexicalEnvironment()resolves withThrowIfNotFound).parseBlockStatement(context, BlockType::StaticBlock)pushes a scope withsetSourceParseMode(ClassStaticBlockMode). That scope is a function boundary, so it is not an arrow function scope and it does not pass inner arrow function features up. The arrow function still getsNewTargetFeature, and no function stores@newTargetLocal.function f() { (() => { class A { static { new.target } } })() }throws.BytecodeGenerator(EvalNode*)loads@newTargetLocalwhenevalNode->needsNewTargetRegisterForThisScope(). Indirect eval, and direct eval in global code, have no function that stored it.ProgramNodeandModuleProgramNodeignore the feature, so a class at the top level works. That is whyJSTests/stress/class-fields-harmony.js(class C { c = new.target }) passes.An arrow function in a class element (
x = () => new.target) is not the in-place case: the walk stops at the arrow function scope. It keeps the inner arrow function feature in itsSourceProviderCacheItem, and the function of the class element stores itsnew.targetfor it, as before.The new
Scopeflag is the 31st one-bit member, so the layout thatScope::verifyLayout()checks does not change.Test runs (linux x64,
-O1,ENABLE_ASSERTS=ON, this change oncf1b36ec8703): the new test also passes with--useJIT=0,--useDFGJIT=0, low tier-up thresholds with--useConcurrentJIT=0,--validateBytecode=1,--forceDebuggerBytecodeGeneration=1and--useBytecodeOptimizer=1. 224 stress tests with class, field, static-block, arrow or new-target in the name give the same result on thecf1b36ec8703releasejscand on this build, except the new test. The expectations of the new test also hold on V8 (node 26.3.0, withdrainMicrotasksreplaced).bun also substitutes
undefinedfor thesenew.targetin its transpiler, because a bundle has to run on a JavaScriptCore without this change too. The PR is linked in a comment.