Fix CoverInitializedName early error not raised when the object literal is not refined into a pattern

[lahma]

Fixes #46.

{ a = 0 } is only legal as a cover for an ObjectAssignmentPattern. When the enclosing left-hand side is refined into an assignment pattern the literal becomes a pattern and the shorthand-with-initializer is fine; when it is not refined — because it is the object of a member expression, which is already a valid assignment target on its own — the literal stays an ObjectLiteral and §13.2.5.1's early error must fire. All nine forms in the issue parsed clean, where V8 rejects every one.

Root cause

ReinterpretAssignmentTarget discarded the pending ShorthandAssign error whenever its position fell inside the converted left-hand side — a position test standing in for a refinement test. ToAssignable deliberately stops at a MemberExpression (it is already a valid DestructuringAssignmentTarget) and never descends into its object, so in [{a = 0}.x] = [] the literal stays an ObjectExpression while the pending error is dropped anyway.

The for-in/of heads have the same hole for a different reason: they never consult the pending error at all, so it is silently discarded when destructuringErrors goes out of scope.

Why the reset line could not simply be deleted

I built that variant to check, and it is worth recording because it is the obvious first attempt: deleting the reset breaks legal code — ({a = 0} = {}); and ({a: {b = 0}} = {});, ordinary destructuring-with-defaults, start throwing — and it still misses five of the nine forms, because the = branch of ParseMaybeAssign returns without ever calling CheckExpressionErrors, so an un-reset error is never reported, and the for-in/of heads are separate sites.

So the fix has to decide refinement from the converted node rather than from a position, raise the error explicitly, and cover the for-in/of heads. ConsumeCoverInitializedNameError does the first two; the decision procedure rests on an invariant worth stating plainly:

In an ObjectExpression, a shorthand property whose value is an AssignmentPattern can only have come from a CoverInitializedName — a refined literal is an ObjectPattern whose members are AssignmentProperty nodes.

So "was it refined?" is answered by searching the converted node for such a node.

The error is raised after CheckLValPattern/CheckLValSimple, so an invalid left-hand side still reports first — which preserves the existing ({x = yield} += 1) → Invalid left-hand side in assignment pin, and matches what V8 does in most of these cases.

Cost

Nothing is added to per-node work. ConsumeCoverInitializedNameError opens with the same ShorthandAssign < node.Start comparison the old reset used and returns immediately when nothing is pending, so the common path pays one compare-and-branch per assignment expression and per expression-headed for-in/of. ContainsCoverInitializedName runs only when a CoverInitializedName was actually recorded inside the converted node — i.e. either an about-to-be-reported error, or a destructuring assignment genuinely using shorthand defaults — and is then bounded by that left-hand side's size. It is iterative with an explicit ArrayList<Node> stack, so it adds no depth to the parser's stack-guarded recursion.

Measured on the repo's own eight benchmark files (best-of-15, 5×8 parses, alternating pre/post processes): pre median 413.7 ms, post median 399.6 ms — the post median is lower, which the change cannot cause, so the effect is below run-to-run noise. An 800-deep nested array pattern resolves correctly both ways; depth 1600 raises InsufficientExecutionStackException on both builds, which is the pre-existing StackGuard.

Verification

New theory ShouldHandleCoverInitializedNameEdgeCases, 88 rows (44 sources × script/module): the issue's 9 + 5 + 3 matrix plus 27 further forms — mixed refined/unrefined in both orders, nested arrays and objects, for-in, plain for-init, ??=, sequence, chained assignment, parenthesized, and arrow-parameter controls. Written first and run against the unfixed parser: 40 failed, 48 passed, every failure Assert.Throws() Failure: No exception was thrown — the predicted mode.

After: 88/88. Full suite 13098 (net10.0) / 13095 (net9.0, net8.0, net462); test262 99090 passing, 0 failed; source generators 2/2; and the Debug configuration run too, since the new Debug.Assert only exists there.

Differential against V8 (node 24.19.0) over 684 generated forms — 36 shapes × 19 syntactic contexts:

build	mismatches vs V8
before	165 / 684
after	0 / 684

Every one of the 165 behaviour changes is accept → reject; nothing that already agreed with V8 moved.

Two things deliberately left alone

The reported position stays Acornima's own — the = token of the shorthand property, where V8 points at the key — and in the mixed case ([{a = 0}, {b = 0}.x] = []) it is the first recorded shorthand assign, which is not necessarily the one that stayed unrefined. V8 reports the first one there too. Making it exact would mean recording every position rather than the first, growing DestructuringErrors.

Two pre-existing ordering divergences from V8 are preserved rather than "fixed": [{a = 0}.x] += 1 and ({a = 0} += 1) report Invalid left-hand side in assignment where V8 reports the shorthand error, and ({a = 0}.x) => 0 reports Illegal property in declaration context. Changing them would contradict the existing ShouldHandleVariableAssignmentEdgeCases pins. All three carry // V8 reports "…" comments in the new theory.

Note the issue's finding that acorn 8.18.0 shares this defect, so this is not a port of an upstream fix — happy to adjust the approach if you would rather it track acorn.

Found while working on Jint; it is one of the two parser gaps blocking test262 staging/ there (sebastienros/jint#3021). I hand-checked staging/sm/destructuring/bug1396261.js: all 10 must-parse statements parse and all 4 must-throw sources now throw. Acornima's own Test262Harness.settings.json does not include staging in SubDirectories, which is why the suite stayed green on this — adding those ~2,800 cases felt like a separate change with its own triage rather than something to fold in here.

[adams85]

Damn, the JS grammar is full of surprises...😅 Not only is this issue present in the upstream project, but the latest @babel/parser has it too. (As a matter of fact, while tracking this down, I even found a bug in V8: async () => { for await ({__proto__: x, __proto__: y}() of []) { } } is incorrectly accepted there. This, however, seems related to another fun feature, Annex B.3.9.)

Back to the matter at hand, the fix seems correct to me (at least, I couldn't find a construct that breaks it so far), but it also feels kind of brute-force and suboptimal performance-wise.

I suspect there is another way to approach this problem where you catch these errors on the fly, in ParseExprSubscripts. I did some experimenting in that direction, but that road looked pretty bumpy. Probably, it would take a lot of effort to make it work.

So, let's stick with this solution for now. I reported the issue in the upstream project. In case they come up with something better, we'll get back to this.

[adams85]

The proposed solution had one major shortcoming, though: this whole problem also applies to object expressions with duplicate __proto__ properties.

So, I made an attempt at fixing also this here: af572dc

Could you (make Claude) take a look at it?

[lahma]

Thanks for extending this — the __proto__ half is a genuine gap we missed, and the unified CheckAssignmentLhsErrors is a better shape than our two helpers.

I ran a differential against V8 (node 24 / V8 13.6) over 692 cases: 72 hand-written __proto__/shorthand rows, 40 nesting/edge rows, 560 generated payload×target×context combinations, and 20 unary/sequence probes. Accept/reject disagreements with V8:

	disagreements
upstream master	162
our 647ec25	47
this PR (head 32482de)	1

The one remaining is for await ({__proto__:1,__proto__:2}() of []), which V8 wrongly accepts and you already flagged in a test comment. Every legal refined pattern parses, every spec-uncovered form (computed / shorthand / method / accessor / spread) parses, and the child is ObjectProperty guard does exactly the right thing — confirmed behaviourally, not just structurally. Suites at head are green: Acornima.Tests 13141 / 13141 / 13144 / 13141 across net8.0 / net9.0 / net10.0 / net462, test262 99090/99090.

Two things I'd like to raise, one of which I think is a blocker.

1. CheckForErrors recursion can kill the process

The walk recurses per child and isn't behind StackGuard. The parser's own guard doesn't cover it, because it isn't the parser that goes deep: ParseSubscripts is a for(;;) loop, so a member chain builds an arbitrarily deep AST at constant parser recursion depth.

var n = 4000;                                   // ~8 kB of source
var src = "[{a = 0}" + string.Concat(Enumerable.Repeat(".x", n)) + "] = []";
new Parser().ParseScript(src);                  // process dies, exit code 0xC00000FD

Stack overflow.
   at Acornima.Ast.Node.get_ChildNodes()
   at Acornima.Parser.<CheckAssignmentLhsErrors>g__CheckForErrors|121_0(...)   [repeat 6013 times]

Bisected thresholds:

thread stack	survives	crashes	source size at crash
256 KB	773	789	~1.6 kB
512 KB	1803	1819	~3.6 kB
1 MB (typical pool/request thread)	3848	3864	~7.7 kB
1.5 MB (default main thread)	5820	5859	~11.7 kB

The same shape with {a: 0} — no pending error, so no walk — parses fine at 50,000 links (100 kB), so the depth itself is not a parser problem. Nested arrays (the shape from #46) do hit StackGuard cleanly at n≈1000; member chains sail straight past it. It reproduces through =, for-in, for-of and for await-of, for both the shorthand and the __proto__ variant.

That matters for embedders parsing untrusted script (Jint does), because it is a process kill rather than a catchable exception.

I have a verified iterative rewrite that keeps your reported positions exactly: byte-identical output on all 672 differential rows, survives 500,000 links, survives 50,000 links on a 256 KB stack, and keeps Acornima.Tests (all four TFMs) and test262 green. The shape is the obvious one — an explicit ArrayList<Node> stack, sawProto still per node, and the offending properties collected so the earliest-in-source one is raised at the end (which is what the pre-order recursion found first, and it removes the dependence on traversal order). Happy to push it to this branch or open it against yours, whichever you prefer.

2. RaiseAtNext — the goal is right, but Raise(property.Start, …) gets there more cheaply

Reporting at the actual location is a real improvement over our first-recorded-position approximation, and for duplicate __proto__ it lands perfectly: 184 of 185 rejected rows match V8's position exactly. But for the shorthand error it is 0 of 177 — V8 reports at the start of the offending property, and AssignmentPattern.Left.End re-scans to the =, two characters later. The two paths also disagree with each other: ({a = 0}) reports at } (via CheckExpressionErrors, since c3f1663 moved the ShorthandAssign recording past the initializer) while [{a = 0}.x] = [] reports at =.

I tried replacing that one call with Raise(property.Start, InvalidCoverInitializedName): 79 rows change, 75 of them to an exact V8 match, none move away, no error-class or accept/reject changes, exact V8 agreement goes 456/671 → 531/671, and the full suite passes unchanged on all four TFMs (the theories assert ex.Description only, so positions aren't pinned). It also matches what the sibling __proto__ branch already does — and RaiseAtNext has exactly one caller, so the whole helper goes away.

That's worth doing on its own, because RaiseAtNext has two side effects I don't think are intended:

• It clears collected errors in tolerant mode. ResetInternal ends with _options._errorHandler.Reset(), which on ParseErrorCollector is _errors.Clear(). With Tolerant = true and a collector, "use strict"; delete x; [{a = 0}.x] = []; ends with Errors.Count == 0, where the same source ending in ({a = 0}); or a __proto__ pair ends with 1.
• It mutates a shared, user-owned TokenizerOptions. Parser takes options.GetTokenizerOptions() without cloning, and new Parser() shares the process-wide ParserOptions.Default, so nulling _onComment/_onToken/_onRegExp is visible to any other parser built from the same options. With one thread parsing [{a = 0}.x] = [] in a loop, another observed OnToken == null on 0.59% of 123 M reads.

Smaller notes

• The NOTE: above CheckAssignmentLhsErrors is now stale — it says the reported position is the first recorded one and that V8 agrees, but the walk deliberately reports the offending occurrence. [{__proto__: a, __proto__: a, x = 0}, {__proto__: a, __proto__: a}.x] = [] reports @53 where V8 reports @16, which is the better answer and worth saying so explicitly.
• Nothing asserts the reported position anywhere, so the improvement RaiseAtNext exists for has no regression net. A handful of Assert.Equal(n, ex.Error.Index) rows would lock it in.
• c3f1663 restoring the early return in ParseExprSubscripts (and dropping CheckExpressionErrors from ParseMaybeUnary's prefix branch) moves ~10 ++/-- rows away from V8's message where master matched it — ++{a = 0}, ({a = 0}++), [{a = 0} ++x] and the __proto__ equivalents now report the early error instead of "Invalid left-hand side expression in prefix/postfix operation". You updated four test rows for this, so I assume it's deliberate; it also reverses the deviation the removed comment recorded as intentional. Both are errors either way, so it's cosmetic — just checking it's the trade you meant.
• [{a = 0}, {__proto__:1, __proto__:2}.x] = [] is another row where acornima is arguably more correct than V8 (V8 reports the shorthand from the object it did refine); might be worth a row alongside your E-series ones.
• Debug.Assert(!Unsafe.IsNullRef(ref destructuringErrors)) is genuinely unnecessary today, but CheckAssignmentLhsErrors dereferences unconditionally on its first line now, so restoring it is cheap insurance against a future caller.

Everything else looks right to me — the >= convertedNode.Start gate correctly generalises acorn's oldDoubleProto save/restore, the CheckKeyName and KeywordOperator edits are behaviour-neutral, and all 88 rows of ShouldHandleCoverInitializedNameEdgeCases still pass verbatim.

[lahma]

Verified downstream, since this is one of three PRs Jint is waiting on.

A local Acornima built from master (7a4e5a1) plus this PR at ae3b66c, #49 and #44 takes Jint's test262 run from 102,495 passed / 189 skipped to 102,505 passed / 179 skipped, 0 failed — five previously excluded staging/sm files now pass, destructuring/bug1396261.js among them, and nothing else moved.

The stack-overflow blocker is gone: "[{a = 0}" + ".x".repeat(4000) + "] = []" now reports Invalid shorthand property initializer at the right position instead of killing the process. Thanks for taking the StackGuard point and for the __proto__ half.

[adams85]

Closing in favor of #52