[JSC] Parser: accept arguments and super() in the parameters of a function in a class field initializer - #711
Conversation
… function in a class field initializer
A class field initializer has two early errors: "ContainsArguments of Initializer is true" and "Initializer Contains SuperCall is true". Both rules look into an arrow function and stop at any other function, because such a function has its own `arguments` and its own constructor kind. The parser keeps m_parserState.isParsingClassFieldInitializer for these checks. Only parseFunctionBody() cleared the flag, so it was still set while parseFunctionInfo() parsed the parameters. `class A { f = function (p = arguments.length) { return p; }; }` was a SyntaxError ("Cannot reference 'arguments' in class field initializer"). So was `super()` in the parameters of the constructor of a derived class in the initializer ("super call is not valid in class field initializer context").
parseFunctionInfo() now clears the flag for the parameters and the body of every function that is not an arrow function. An arrow function keeps the flag, as before.
|
Preview build of 0b6d919: |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 preserves class-field initializer state only for nested arrow functions. A stress test covers valid parsing, expected syntax errors, parameter defaults, and repeated construction across base and derived classes. ChangesClass-field initializer parsing
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The parser accepts the intended nested-function parameter forms while preserving class-field restrictions, with regression coverage described for valid and invalid cases. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed problem, cause, fix, testing, and compatibility information. It does not include the required Bugzilla bug title and link, the Reviewed by NOBODY line, or the template-style changed-file entries.
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 |
WEBKIT_VERSION is autobuild-preview-pr-711-57818ea1: the current pin (63a807e88c) plus the one commit of oven-sh/WebKit#711. The test comment names that PR. Before this merges, the tag must become the merge commit of oven-sh/WebKit#711.
After a function with `arguments` or `super()` in its parameters, the rest of the initializer has the checks of the initializer again: `[function (p = arguments) { }, arguments]` is still a SyntaxError, and `new.target` in an arrow function after the function is still valid in global code.
Problem
class A { f = function (p = arguments.length) { return p; }; }throwsSyntaxError: Unexpected identifier 'arguments'. Cannot reference 'arguments' in class field initializer.Node accepts it.super()in the parameters of a derived class constructor there fails:super call is not valid in class field initializer context.parseClass()setsm_parserState.isParsingClassFieldInitializerfor the initializer (parser/Parser.cpp:3370). OnlyparseFunctionBody()cleared it (Parser.cpp:2367), so the parameters still had it.Fix
parseFunctionInfo()clears the flag for the parameters and the body of every function that is not an arrow function. Both early errors of a field initializer (ContainsArguments, Contains SuperCall) stop at such a function.JSTests/stress/class-field-initializer-nested-function-parameters.js. Of 20,535,680 generated programs, 1,663,758 change from a SyntaxError to the result of node.argumentsand someawaitparameter names in an arrow function in a class static block bun#43477, which the flag hid in these parameters. With [JSC] Parser: rejectargumentsin an arrow function in a class static block, andawaitas an arrow function parameter name there #704 the number is 0, so land [JSC] Parser: rejectargumentsin an arrow function in a class static block, andawaitas an arrow function parameter name there #704 first or together.Background
argumentsin a class field initializer". It looks into an arrow function and stops at any other function, which has its ownarguments.arguments,super()andyieldread it.parseFunctionInfo()parses the name, the parameters and the body of every function.Notes
Repro (
jsc, orbunwith plaineval). node 26 accepts all but the last one.jscof63a807e88caccepts only the first and the sixth, with and without--useSourceProviderCache=false:Why one place. The flag was cleared in
parseFunctionBody(), which sees only the body. The rule is about the whole function, so the clear moves toparseFunctionInfo(), which parses the parameters and the body. A computed key and a class heritage are parsed beforeparseFunctionInfo(), so they stay part of the initializer, as ContainsArguments says (f = { [arguments]() { } }is still an error). For an arrow function the new condition is the old one (bodyType != StandardFunctionBodyBlockis true exactly for the two arrow function modes). The SourceProviderCache is not involved: the flag at a source position does not depend on which pass parses it, and--useSourceProviderCache=falsegives the same results.Messages that change for code that is still an error.
super()in the parameters of a function that cannot call it, in a field initializer:super is not valid in this context.The old message wasUnexpected token '('. super call is not valid in class field initializer context.The new one is what the same function gets outside a class field, and in its body. Example:(class extends Object { f = { m(p = super()) { } } }). No test inJSTestsorLayoutTests/jshas the old message.yield,new.target, direct eval. No result changes.yieldin these parameters fails on an earlier check (Cannot use yield expression within parameters, orout of generator).new.targetis valid in these parameters throughcurrentScope()->isFunction(). Code from a direct eval in these parameters is checked throughclosestScopeOwningArguments(), which already stops at the function. The test has all three.Generated corpora. Each program goes to an indirect
evalinjscof63a807e88c, injscwith this change, and in node 26. A program is a class field context with an initializer made from 54 expression templates with a hole (functions, generators, async functions, arrow functions, methods, getters, setters, constructors, class heritage, computed keys, fields, static blocks, patterns) and 16 leaves (arguments,arguments.length,{ arguments },super(),super.x,new.target,yield,await,this,eval, ...).argumentsin an arrow function in a static block of a class in such parameters:(class { f = function (p = class { static { () => arguments; } }) { }; }). That is JavaScriptCore acceptsargumentsand someawaitparameter names in an arrow function in a class static block bun#43477:jscacceptsfunction (p = class { static { () => arguments; } }) { }everywhere else, and the flag hid it in this one place. With [JSC] Parser: rejectargumentsin an arrow function in a class static block, andawaitas an arrow function parameter name there #704 and this change together, the same corpus has 0 programs that change away from node, and 1,797,295 that change to it.--useSourceProviderCache=falsethe results are the same, except for 9 programs of [JSC] Parser: in a generator, rejectyieldin the parameters of an arrow function nested in another's parameters #702 that this change does not touch.arguments,new.target,this, direct eval orsuper()in the parameters, each in 24 places (instance, static, private and computed fields, classes in functions, generators, methods, static blocks,eval,new Function). Each is called 3,000 times. All 1,032 results are those of the same function outside a class, and those of node. The old parser rejects 828 of them.Tests run. The local build is Release with assertions. The new test passes in all 17 modes of
run-javascriptcore-testsand fails to parse with the oldjsc. It also passes with #704 applied, and node accepts and rejects the same programs.run-javascriptcore-tests --filter 'class|arguments|super|field|static-block|arrow|syntax|parser|eval|new-target|newtarget|default-param|destructur|reparse|source-provider|method|getter|setter|constructor': 15,613 runs of 1,563 tests, no failure. test262: every test undertest/languageandtest/annexB/languagein the strict, default, module and raw scenarios, compared by exit code and output between the two builds: 45,388 runs, one test differs,grammar-private-environment-on-class-heritage-chained-usage.js. Its message names one of three undeclared private names, and which one changes from run to run in both builds.Self-review. Two reviewers asked for the same thing: the first version of the test did not check the flag after the function. A parser that clears the flag and never restores it passed that test, and so did one that does not restore it when the function comes from the SourceProviderCache. The second commit adds
[function (p = arguments) { }, arguments],(a = function (p = arguments) { }, b = arguments) => a,[class extends Object { constructor(p = super()) { } }, super()]and ten more, and both of those parsers fail it. The two concerns that I rejected are about programs that have a second, separate error thatjscdoes not see, with and without this change: the shorthand{ arguments }(#704),static f = await, and[super.x]below. The old parser rejected them only because of the function next to it. Withp = 1in place ofp = arguments, the old parser accepts every one of them.Found during this work, not fixed here.
function f(p) { for (arguments.length of [5]); return arguments.length; }throwsReferenceError: arguments is not definedwhen it is called. node returns 5. Thearguments.lengthfast path of the parser does not count the head of afor-oforfor-inas a write. No class is needed.(async function () { class C { f = (p = await) => p; } })is a SyntaxError (Cannot use 'await' within a parameter default expression.). node accepts it. Both acceptf = awaitthere, whereawaitis an identifier.(function () { class C { [super.x]() { } } })is accepted. node rejects it: the function has no home object.(class { f = new.target; })in global code throwsReferenceError: Can't find private variable: PrivateSymbol.newTargetLocalwhen the class is evaluated. [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 changes the code fornew.targetin a field initializer. I did not test this program with it.With other open PRs. This change applies on top of #704 without a conflict, and both tests pass with the two together. #430 adds a line next to the one this change removes from
parseFunctionBody(), so the second one to land needs a rebase.