Add ParserOptions.AllowDirectSuperOutsideMethod - #44
Conversation
|
Even though this is still a draft, let me add my initial thoughts: First of all, I see this feature is needed for Jint to fully support eval, so I have no objections. At first glance, the implementation looks good too. However, once we touch this area, I'd prefer to align the behavior of the existing I mean that currently So, could you update But before doing anything, please rebase this PR onto master so we can see whether it passes the tests I added for some edge cases here. FWIW, this commit of mine also fixes a parser bug: class C extends Object {
constructor() {
class X { p = super() }
}
}This edge case was incorrectly accepted since v1.1.1. BTW, this will be an interesting edge case for Jint too: new (class extends Object {
constructor() {
class X { static p = eval('super()') }
}
})And also: new (class extends Object {
constructor() {
class X { static [eval('super()')]() { } }
}
})This evil eval one is syntactically and semantically valid! 🤯 |
ECMA-262 PerformEval (https://tc39.es/ecma262/#sec-performeval) passes inDerivedConstructor into the parse of the eval code, so a direct eval called from the constructor of a derived class may contain a super call: class B { } class D extends B { constructor() { eval("super()") } } There was no way to ask the parser for that. AllowSuperOutsideMethod enables super property accesses only, and AllowDirectSuper was derived from the scope flags alone, so any host embedding the parser as the front end of a JS engine (this came up in Jint, which computes inDerivedConstructor correctly but cannot pass it on) reported a SyntaxError for every eval containing a super call. The new option is implemented by seeding the root scope with ScopeFlags.Super | ScopeFlags.DirectSuper rather than by short-circuiting the AllowDirectSuper getter. That way the option follows the this binding for free: EnterScope propagates the current this scope through arrow function scopes but not through ordinary function scopes, so `super()`, `(() => super())()` and `(() => eval("super()"))()` are accepted while `function f() { super() }` remains a SyntaxError, which is exactly what the spec prescribes for eval code in a derived constructor. Seeding Super alongside DirectSuper mirrors what ParseMethod does for the constructor of a derived class (a derived constructor is a method, so super property accesses are allowed there as well). The behavior of AllowSuperOutsideMethod and AllowNewTargetOutsideFunction is left untouched; with the new option disabled, nothing changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e AllowDirectSuperOutsideMethod
59c77fa to
08c6a63
Compare
|
Rebased onto The rebase changed two expectations, and 33a8c34 is why. With Both flips are right: §15.7.1 makes it a Syntax Error if a
Suites at head Verified downstream as well, which is what this option exists for. A local Acornima built from |
…anket permission The last five entries under the STAGING EXCLUSIONS banner were all blocked on the parser, not on Jint. Three of them - the derived-constructor arrow eval files and bug1587574 - needed Acornima to have a switch for a direct super() at all, which adams85/acornima#44 adds; the other two are closed by adams85/acornima#48 and sebastienros#49 with no Jint change beyond deleting their entries. The wiring is the interesting half. Jint already parses eval code with AllowSuperOutsideMethod, which covers SuperProperty only, so eval("super()") in a derived constructor failed to parse instead of running. PerformEval (https://tc39.es/ecma262/#sec-performeval) permits a direct super() exactly when inDerivedConstructor is true, and EvalFunction already computes that flag for its own post-parse early error - so the flag is passed through to the parser rather than the option being switched on for every eval. That is why the adjusted ParserOptions are memoized in two slots now: the adjustment is no longer constant, and the variant that admits a super() must never reach an eval the specification forbids one in. The eval cache already validates the ParserOptions an entry was parsed with, so one source evaluated from both contexts gets one parse each. The option is scoped to the eval'd unit's this binding, so it reaches arrow functions declared in the eval but not ordinary ones - which is what lets Jint enable it for the whole eval and still have the parser refuse eval("(function () { super() })"), as V8 does. Acornima 1.8.0 does not exist yet: the three pull requests are open, and the version here is the expected next one. Everything was verified against a local Acornima built from master plus those three, which takes test262 from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
…anket permission The last five entries under the STAGING EXCLUSIONS banner were all blocked on the parser, not on Jint. Three of them - the derived-constructor arrow eval files and bug1587574 - needed Acornima to have a switch for a direct super() at all, which adams85/acornima#44 adds; the other two are closed by adams85/acornima#48 and sebastienros#49 with no Jint change beyond deleting their entries. The wiring is the interesting half. Jint already parses eval code with AllowSuperOutsideMethod, which covers SuperProperty only, so eval("super()") in a derived constructor failed to parse instead of running. PerformEval (https://tc39.es/ecma262/#sec-performeval) permits a direct super() exactly when inDerivedConstructor is true, and EvalFunction already computes that flag for its own post-parse early error - so the flag is passed through to the parser rather than the option being switched on for every eval. That is why the adjusted ParserOptions are memoized in two slots now: the adjustment is no longer constant, and the variant that admits a super() must never reach an eval the specification forbids one in. The eval cache already validates the ParserOptions an entry was parsed with, so one source evaluated from both contexts gets one parse each. The option is scoped to the eval'd unit's this binding, so it reaches arrow functions declared in the eval but not ordinary ones - which is what lets Jint enable it for the whole eval and still have the parser refuse eval("(function () { super() })"), as V8 does. Acornima 1.8.0 does not exist yet: the three pull requests are open, and the version here is the expected next one. Everything was verified against a local Acornima built from master plus those three, which takes test262 from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
|
I'd suggest naming the new option Does this work for you? |
f1b91b6 renamed the option but left the test method names, the theory parameter and one comment on the old AllowDirectSuperOutsideMethod spelling, so the tests no longer said what they cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGoP6H8jUJ26uj6U5mt8Fn
|
Yes, Your Suites at that head, all green: The PR title and the opening description still use the old name; I'll leave them as they are unless you'd like them rewritten, since the discussion above is easier to follow with the original wording intact. |
…anket permission The last five entries under the STAGING EXCLUSIONS banner were all blocked on the parser, not on Jint. Three of them - the derived-constructor arrow eval files and bug1587574 - needed Acornima to have a switch for a direct super() at all, which adams85/acornima#44 adds; the other two are closed by adams85/acornima#48 and sebastienros#49 with no Jint change beyond deleting their entries. The wiring is the interesting half. Jint already parses eval code with AllowSuperOutsideMethod, which covers SuperProperty only, so eval("super()") in a derived constructor failed to parse instead of running. PerformEval (https://tc39.es/ecma262/#sec-performeval) permits a direct super() exactly when inDerivedConstructor is true, and EvalFunction already computes that flag for its own post-parse early error - so the flag is passed through to the parser rather than the option being switched on for every eval. That is why the adjusted ParserOptions are memoized in two slots now: the adjustment is no longer constant, and the variant that admits a super() must never reach an eval the specification forbids one in. The eval cache already validates the ParserOptions an entry was parsed with, so one source evaluated from both contexts gets one parse each. The option is scoped to the eval'd unit's this binding, so it reaches arrow functions declared in the eval but not ordinary ones - which is what lets Jint enable it for the whole eval and still have the parser refuse eval("(function () { super() })"), as V8 does. Acornima 1.8.0 does not exist yet: the three pull requests are open, and the version here is the expected next one. Everything was verified against a local Acornima built from master plus those three, which takes test262 from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
…Constructor adams85/acornima#44 merged as 09c08b3, and the option we contributed as ParserOptions.AllowDirectSuperOutsideMethod shipped under a different name: ParserOptions.AllowSuperCallOutsideConstructor. The new name says what the option admits (a direct super *call*) and where the code it is parsing came from (the *constructor* of a derived class, PerformEval's inDerivedConstructor) rather than naming the parser-internal AllowDirectSuper flag it seeds, which also reads better beside the AllowSuperOutsideMethod it implies. Nothing but the name changed: the option still defaults to false, still seeds the root scope with Super | DirectSuper, and so still follows the eval'd unit's this binding - reaching arrow functions declared in the eval but not ordinary ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
…anket permission The last five entries under the STAGING EXCLUSIONS banner were all blocked on the parser, not on Jint. Three of them - the derived-constructor arrow eval files and bug1587574 - needed Acornima to have a switch for a direct super() at all, which adams85/acornima#44 adds; the other two are closed by adams85/acornima#48 and sebastienros#49 with no Jint change beyond deleting their entries. The wiring is the interesting half. Jint already parses eval code with AllowSuperOutsideMethod, which covers SuperProperty only, so eval("super()") in a derived constructor failed to parse instead of running. PerformEval (https://tc39.es/ecma262/#sec-performeval) permits a direct super() exactly when inDerivedConstructor is true, and EvalFunction already computes that flag for its own post-parse early error - so the flag is passed through to the parser rather than the option being switched on for every eval. That is why the adjusted ParserOptions are memoized in two slots now: the adjustment is no longer constant, and the variant that admits a super() must never reach an eval the specification forbids one in. The eval cache already validates the ParserOptions an entry was parsed with, so one source evaluated from both contexts gets one parse each. The option is scoped to the eval'd unit's this binding, so it reaches arrow functions declared in the eval but not ordinary ones - which is what lets Jint enable it for the whole eval and still have the parser refuse eval("(function () { super() })"), as V8 does. Acornima 1.8.0 does not exist yet: the three pull requests are open, and the version here is the expected next one. Everything was verified against a local Acornima built from master plus those three, which takes test262 from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
…Constructor adams85/acornima#44 merged as 09c08b3, and the option we contributed as ParserOptions.AllowDirectSuperOutsideMethod shipped under a different name: ParserOptions.AllowSuperCallOutsideConstructor. The new name says what the option admits (a direct super *call*) and where the code it is parsing came from (the *constructor* of a derived class, PerformEval's inDerivedConstructor) rather than naming the parser-internal AllowDirectSuper flag it seeds, which also reads better beside the AllowSuperOutsideMethod it implies. Nothing but the name changed: the option still defaults to false, still seeds the root scope with Super | DirectSuper, and so still follows the eval'd unit's this binding - reaching arrow functions declared in the eval but not ordinary ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd
…r the last five staging exclusions (#3277) * Eval: hand PerformEval's inDerivedConstructor to the parser, not a blanket permission The last five entries under the STAGING EXCLUSIONS banner were all blocked on the parser, not on Jint. Three of them - the derived-constructor arrow eval files and bug1587574 - needed Acornima to have a switch for a direct super() at all, which adams85/acornima#44 adds; the other two are closed by adams85/acornima#48 and #49 with no Jint change beyond deleting their entries. The wiring is the interesting half. Jint already parses eval code with AllowSuperOutsideMethod, which covers SuperProperty only, so eval("super()") in a derived constructor failed to parse instead of running. PerformEval (https://tc39.es/ecma262/#sec-performeval) permits a direct super() exactly when inDerivedConstructor is true, and EvalFunction already computes that flag for its own post-parse early error - so the flag is passed through to the parser rather than the option being switched on for every eval. That is why the adjusted ParserOptions are memoized in two slots now: the adjustment is no longer constant, and the variant that admits a super() must never reach an eval the specification forbids one in. The eval cache already validates the ParserOptions an entry was parsed with, so one source evaluated from both contexts gets one parse each. The option is scoped to the eval'd unit's this binding, so it reaches arrow functions declared in the eval but not ordinary ones - which is what lets Jint enable it for the whole eval and still have the parser refuse eval("(function () { super() })"), as V8 does. Acornima 1.8.0 does not exist yet: the three pull requests are open, and the version here is the expected next one. Everything was verified against a local Acornima built from master plus those three, which takes test262 from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd * Eval: follow Acornima's rename of the option to AllowSuperCallOutsideConstructor adams85/acornima#44 merged as 09c08b3, and the option we contributed as ParserOptions.AllowDirectSuperOutsideMethod shipped under a different name: ParserOptions.AllowSuperCallOutsideConstructor. The new name says what the option admits (a direct super *call*) and where the code it is parsing came from (the *constructor* of a derived class, PerformEval's inDerivedConstructor) rather than naming the parser-internal AllowDirectSuper flag it seeds, which also reads better beside the AllowSuperOutsideMethod it implies. Nothing but the name changed: the option still defaults to false, still seeds the root scope with Super | DirectSuper, and so still follows the eval'd unit's this binding - reaching arrow functions declared in the eval but not ordinary ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FHnbgQGW5QgfatxL6DgGEd * Test262: move the pin to 14e8c908, the upstream tip Thirteen commits since 3655e746, eleven of them CI tables and tooling. The two that touch test/ are ac7b5f8c, which adds negative parse tests for a braced quantifier whose lower bound exceeds its upper bound (/a{2,1}/ and /a{2,1}/u), and 14e8c908, which lists testTypedArray.js once instead of twice in four includes lines. The parser already rejects the quantifier at parse time, and the RegExp constructor rejects it under every flag, so nothing in the engine moves: +4 passed, +4 total, skipped unchanged. features.txt is unchanged. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Updated [Acornima](https://github.com/adams85/acornima) from 1.7.0 to 1.8.0. <details> <summary>Release notes</summary> _Sourced from [Acornima's releases](https://github.com/adams85/acornima/releases)._ ## 1.8.0 New features: - Introduce `ParserOptions.AllowSuperCallOutsideConstructor` to support [PerformEval](https://tc39.es/ecma262/#sec-performeval) (see #44). Bug fixes: - Fix `ParserOptions.AllowSuperOutsideMethod` to reject `super` property access in function contexts outside of classes (see adams85/acornima#44 (comment)). - Fix regression that incorrectly allows `super()` in class fields when the class is declared inside another class's constructor (see adams85/acornima#44 (comment)). - Fix legacy octal constructs not rejected in the token following an ASI-terminated "use strict" directive (see #49). - Fix legacy octal constructs being rejected right after a strict function body (see #51). - Fix early errors not raised in assignment patterns when the object literal is not refined into a pattern (see #52). Commits viewable in [compare view](adams85/acornima@v1.7.0...v1.8.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
Motivation
ECMA-262's PerformEval passes
inDerivedConstructorinto the parse of the eval code, so a direct eval called from the constructor of a derived class may legally contain a super call:There is currently no way to ask the parser for that.
AllowSuperOutsideMethodenables super property accesses only, andAllowDirectSuperis derived from the scope flags alone with no option behind it — unlike its siblingsAllowSuperandAllowNewDotTarget. So an embedder that computesinDerivedConstructorcorrectly still cannot pass it on, and everyevalcontaining a super call comes back as aSyntaxError.This came up in Jint, which uses Acornima as its front end and already tracks
inDerivedConstructorcorrectly where it parses eval code — the missing piece is purely the parser option. Three test262 conformance tests fail on this today (staging/sm/class/derivedConstructorArrowEvalSuperCall.js,derivedConstructorArrowEvalNestedSuperCall.js,staging/sm/fields/bug1587574.js).What this adds
ParserOptions.AllowDirectSuperOutsideMethod(defaultfalse), declared next toAllowSuperOutsideMethodand following the same pattern — backing field,get/init, copy-constructor entry.Design: seeding the root scope, not short-circuiting the getter
The obvious implementation —
AllowDirectSuper => _options._allowDirectSuperOutsideMethod || …— would be wrong. It would allowat the top level of the parsed unit, which the spec does not: eval code in a derived constructor gets the constructor's this binding, and an ordinary function introduces one of its own.
Instead the option seeds the root scope with the flag pair
ParseMethodalready uses for a derived constructor:Because
EnterScopepropagates the current this scope through arrow function scopes but not through ordinary function scopes, the spec semantics fall out of the existing machinery with no new logic:super()SyntaxError(() => super())()SyntaxError() => () => super()SyntaxErrorfunction f() { super() }SyntaxErrorSyntaxError(function () { super() })SyntaxErrorSyntaxError({ m() { super() } })SyntaxErrorSyntaxErrorclass A extends B { m() { super() } }SyntaxErrorSyntaxErrorScopeFlags.Superis seeded alongsideDirectSuperfor two reasons:AllowSuperis checked first inParseExprAtom, soDirectSuperalone would leavesuper()rejected; and a derived constructor is a method, so super property accesses are legal there anyway — which is exactly whyParseMethodsets the same pair.AllowSuperOutsideMethodandAllowNewTargetOutsideFunctionare deliberately untouched: with the new option disabled nothing changes at all.Tests
ShouldHandleDirectSuperOutsideMethod(37 cases, next toShouldHandleSuperKeywordEdgeCasesand in its style) covers both options in all four combinations across script/module/expression: the top-level and arrow cases, the ordinary-function and method cases that must keep failing,super.xunder each option, and class bodies.AllowDirectSuperOutsideMethodShouldDefaultToFalsepins the default and that awith-expression copy keeps the flag.Green:
Acornima.Testson net462/net8.0/net9.0/net10.0 (13,025–13,028 each),Acornima.Tests.Test262(99,090),Acornima.Tests.SourceGenerators, solution build with 0 warnings.Two things noticed while doing this, not fixed here
_allowTopLevelUsingis missing from theParserOptionscopy constructor — it is the only bool option that is, so(ParserOptions.Default with { AllowTopLevelUsing = true }) with { EcmaVersion = … }silently drops the flag. Happy to send that as its own one-line PR.class A extends B { constructor() { class C { x = super() } } }already parses on master (V8 rejects it), and consequentlyclass C { x = super() }parses when this option is on. Same quirk, inherited from acorn — both rows are pinned in the new theory with a comment rather than changed.