Skip to content

Consult the iterator's done flag before stepping it again - #3048

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:staging/destructuring-iter
Aug 18, 2026
Merged

Consult the iterator's done flag before stepping it again#3048
lahma merged 1 commit into
sebastienros:mainfrom
lahma:staging/destructuring-iter

Conversation

@lahma

@lahma lahma commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Four defects in array destructuring and the array iterator.

1. done was tracked but never consulted before stepping again

DestructuringPatternAssignmentExpression maintained a done flag and correctly suppressed the close when it was set, but nothing guarded the next step. Per §13.15.5.5 each element only steps If iteratorRecord.[[Done]] is false. Six call sites were unguarded — elision, Identifier, MemberExpression, nested pattern, rest element, AssignmentPattern — and one of them set done = true unconditionally before re-stepping.

[a, b, c] = iterableOf([]);   // "next,done,next,done,next,done" -> "next,done"

2. An elision terminated the whole pattern

The elision site did if (!ConsumeFromIterator(...)) break;, so every element after an elision was skipped once the iterator ran out:

var b; [, b = 5] = nonArrayIterable;   // b was undefined, must be 5

This one was not in the issue's description of the group at all.

3. A nested pattern received the iterator result object, not result.value

The nested-pattern branch assigned temp (the {value, done} object) where its three sibling branches all read CommonProperties.Value. This is a shipping bug beyond test262 — let [[a]] = someNonArrayIterable was broken, masked whenever the RHS is a real array because that takes an ArrayOperations fast path.

4. A replaced %ArrayIteratorPrototype%.next lost the iterated object

ArrayIteratorPrototype.Construct dropped its array and kind arguments and returned an iterator over the prototype when next was not pristine, so a replacement that delegates to the original got TypeError.

Now Construct always returns the real array iterator (that object is script-visible — it is what [][Symbol.iterator]() returns), and JsValue.TryGetIterator wraps it when next is not native. The check reads next off the instance, which also closes a related gap: §7.4.4 GetIterator step 3 reads next unconditionally, so an own next on an array-iterator instance was previously ignored.

Performance

Destructuring pays one perfectly-predicted branch per element on the iterator path, and it is saved work when taken; the real-array fast path is untouched. The Get(next) did not appear — it moved: Construct used to perform it on every call and no longer does, HasNativeNext performs it instead, so it is the same one read per iterator record. arr.values() called straight from script now performs zero reads where it used to perform one. Other iterator kinds pay one virtual call returning a constant, once per loop entry.

All 20 GetIterator call sites were checked; none casts to a concrete iterator type, and every other IteratorInstance subclass keeps the default.

Tests

Jint.Tests/Runtime/DestructuringIteratorProtocolTests.cs — 10 tests, 6 failing against unfixed code, asserting spec-exact next counts for 0/1/2/3-element iterables and full operation logs.

test262: 102,334 passed, 0 failed (+8; four files × two modes, verified in both sloppy and strict).

Frees Array/for_of_2.js, destructuring/order.js, destructuring/order-super.js, expressions/destructuring-array-done.js.

Two pre-existing gaps left alone and noted: for-of bypasses GetIterator when its RHS is already an IteratorInstance, and the array destructuring/spread fast paths identity-check @@iterator only, so a replaced %ArrayIteratorPrototype%.next is still skipped for a real array.

Refs #3021.

Array destructuring tracked an iterator record's [[Done]] and let the
finally suppress the close when it was set, but nothing guarded the next
step. Every element form in 13.15.5.5 IteratorDestructuringAssignmentEvaluation
and 8.6.2 IteratorBindingInitialization steps only "If iteratorRecord.[[Done]]
is false", so `[a, b, c] = oneElementIterable` performed three next() calls
where the spec performs two, and the rest element even set done unconditionally
before re-stepping. ConsumeFromIterator becomes StepValue, which is
IteratorStepValue under that guard: an exhausted iterator is never stepped
again, the element takes undefined, and every abrupt completion (the next
call, the done read, the value read) marks the record done so the finally
does not close it. The rest element's loop becomes the spec's "Repeat, while
iteratorRecord.[[Done]] is false" and grows the done bookkeeping it never had.

An elision no longer ends the pattern either. It used to break out of the
element loop when its step reported done, so `[, a = 5] = iterableOf([])`
left a undefined instead of running its initializer, and a member-expression
target after an elision was never assigned at all.

A nested pattern was handed the iterator *result object* rather than
result.value, while its three sibling branches all went through
ConsumeFromIterator. That is a shipping bug beyond conformance:
`let [[a]] = someNonArrayIterable` threw "The value is not iterable" and
`let [{p}] = ...` silently bound undefined. It is masked whenever the
right-hand side is a real array, because that takes the ArrayOperations
fast path.

Finally, %ArrayIteratorPrototype%.Construct dropped both of its arguments
when `next` was not pristine, returning an ObjectIterator over the
*prototype* — so a replacement delegating to the original got a TypeError,
since the prototype is not an IteratorInstance. CreateArrayIterator now
always returns the real array iterator (that object is script-visible: it is
what `[][Symbol.iterator]()` evaluates to), and the decision moves to where
the iterator record is built. JsValue.TryGetIterator asks the new
IteratorInstance.HasNativeNext before adopting an instance as the record and
otherwise wraps it, which also closes GetIterator step 3: an own `next` on an
array-iterator instance is now honoured instead of ignored. The check is one
read of `next` off the instance, exactly the read Construct used to perform
on the prototype, so a pristine iteration pays nothing new -- and `arr.values()`
called straight from script now pays nothing at all.

Frees staging/sm/Array/for_of_2.js, staging/sm/destructuring/order.js,
staging/sm/destructuring/order-super.js and
staging/sm/expressions/destructuring-array-done.js.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lahma
lahma merged commit a6501f6 into sebastienros:main Aug 18, 2026
5 checks passed
@lahma
lahma deleted the staging/destructuring-iter branch August 18, 2026 16:22
legrab added a commit to legrab/pocok that referenced this pull request Aug 25, 2026
Updated [Jint](https://github.com/sebastienros/jint) from 4.16.0 to
4.16.1.

<details>
<summary>Release notes</summary>

_Sourced from [Jint's
releases](https://github.com/sebastienros/jint/releases)._

## 4.16.1

Jint 4.16.1 is the **first release from the new `4.x` maintenance
branch**, and it marks the point where the two lines separate: `main` is
now **5.0.0 development**, and `4.x` is where the 4.16.x line continues.

**What that means for you.** If you are on 4.16.0, this is a drop-in
update — it is correctness and conformance work only, **no API change
and no changed default**. Every public signature is the same one 4.16.0
shipped, on all five target frameworks. If you want the 4.x line, take
it from `4.x` and expect fixes rather than features. If you want to
follow where the engine is going, watch `main` — v5 brings breaking API
changes, an opt-in WHATWG web API surface, Web Workers, Node
compatibility and a raised .NET Framework floor, and every one of them
is recorded as it lands in
[`docs/v5-migration.md`](https://github.com/sebastienros/jint/blob/main/docs/v5-migration.md).

From this release onward the 4.x public surface is snapshotted per
target framework in `Jint.Tests.PublicInterface/Verify/`, so "did the
API move?" is a diff rather than a judgement call — on this branch a
diff there is a bug, and comparing those files against `main`'s is the
v4→v5 delta.

### Highlights

**Conformance, from a suite that now runs more of test262.** The
`staging/` directory is generated and executed for the first time
(#​3016), which is roughly 2,800 additional cases — largely
SpiderMonkey's own suite contributed upstream, covering behaviour the
stable directories never reach. Much of the work below is what it found.

**Built-ins do what the spec says, step by step.** The array built-ins
perform the internal methods they name rather than equivalents (#​3066);
`Array.from` honours `IsConstructor` and a typed array's `length` write
throws (#​3043); an array truncation walks downwards and the generics
report the writes they fail (#​3072); argument validation and evaluation
order are corrected in five built-ins (#​3069); `Map` and `Set` get the
`[[SetData]]` tombstone their traversals are specified over (#​3073);
`Date.prototype.setTime` stores the clipped time value (#​3042); and
`Array.prototype.values`/`keys`/`entries` no longer gate on an
array-like receiver (#​3236).

**Iterators and control flow.** A throw from the iterator step no longer
closes the iterator (#​3047); the `done` flag is consulted before
stepping again (#​3048); a rejected `return()` propagates out of an
abandoned `for await` loop (#​3113); an optional-chain short circuit is
distinguished from a genuine `undefined` (#​3040); a computed property
key is evaluated even when spelled as a literal (#​3039) and survives an
`await` or `yield` intact (#​3144, #​3150); and destructuring the rest
of an exhausted array yields an empty array rather than 2³² elements
(#​3263).

**Numeric and string accuracy.** `Math.acosh`, `asinh`, `atanh`, `cbrt`,
`expm1` and `log1p` are ported from fdlibm for correctly-rounded results
across every target framework (#​3050); `toFixed` formats from the
double's exact value and reads `this` from `[[NumberData]]` (#​3071);
`String.prototype` case conversion derives from Jint's own Unicode
tables rather than the host's culture data (#​3068); and the regex
engine is chosen per subject, with `RegExp.prototype.replace` no longer
rewriting `lastIndex` (#​3070).

**Bounds that hold.** JavaScript strings have a maximum length instead
of a wrapped array rent (#​3015); a JSON document too long to become a
string is refused while it is being built (#​3028); a frame displaced by
a proper tail call keeps counting while its trampoline runs, so
`MaxRecursionDepth` cannot be evaded by leaving and re-entering the
trampoline (#​3022); and an `Atomics` waiter is released when nothing
can ever notify it again (#​3029).

**Error messages no longer run user JavaScript** (#​3041) — rendering a
message for a value with a script-supplied `toString` used to invoke it,
from inside the failure path.

**Internationalization.** The five Temporal members the proposal removed
are dropped (#​3014), and `u`-extension options are canonicalized with
every date format the spec allows (#​3018).

Two fixes in this release come from **@​svenrog** — a sloppy function
answering its own `arguments` (#​3061) and the outer link on a parked
`Function`-constructor environment (#​3063).

## What's Changed
* Drop the five Temporal members the proposal removed by @​lahma in
sebastienros/jint#3014
* Canonicalize u-extension options and format every date the spec allows
by @​lahma in sebastienros/jint#3018
* Mark a global created by an unresolvable assignment, and stop a
waitAsync timeout outliving its engine by @​lahma in
sebastienros/jint#3019
* Run test262's staging/ directory too by @​lahma in
sebastienros/jint#3016
* Give JavaScript strings a maximum length instead of a wrapped array
rent by @​lahma in sebastienros/jint#3015
* Let a for-of frame decline the unwind it can only rethrow by @​lahma
in sebastienros/jint#3017
* Keep counting a frame a tail call replaced while its trampoline runs
by @​lahma in sebastienros/jint#3022
* Unpark staging/Temporal/removed-methods.js, which #​3014 already fixed
by @​lahma in sebastienros/jint#3023
* Drop the Islamic date conversions no calendar path reaches by @​lahma
in sebastienros/jint#3027
* Let an Atomics waiter go when nothing can ever notify it again by
@​lahma in sebastienros/jint#3029
* Refuse a JSON document too long to be a string while it is being built
by @​lahma in sebastienros/jint#3028
* Bump the microsoft group with 3 updates by @​dependabot[bot] in
sebastienros/jint#3033
* Bump the analyzers group with 1 update by @​dependabot[bot] in
sebastienros/jint#3031
* Add initial threat model for untrusted scripts by @​sebastienros in
sebastienros/jint#3030
* Stop ClassBenchmark rebuilding its engine per iteration by @​lahma in
sebastienros/jint#3053
* createRealm installs a full $262 on the new realm and returns it by
@​lahma in sebastienros/jint#3044
* Give the benchmark suite a measurement environment by @​lahma in
sebastienros/jint#3055
* Evaluate a computed property key even when it is spelled as a literal
by @​lahma in sebastienros/jint#3039
* Stop error messages from running user JavaScript by @​lahma in
sebastienros/jint#3041
* Array.from honours IsConstructor, and a typed array's length write
throws by @​lahma in sebastienros/jint#3043
* Consult the iterator's done flag before stepping it again by @​lahma
in sebastienros/jint#3048
* Date.prototype.setTime must store the clipped time value by @​lahma in
sebastienros/jint#3042
* Answer a sloppy function's own arguments instead of throwing by
@​svenrog in sebastienros/jint#3061
* Keep the outer link on a parked Function-constructor environment by
@​svenrog in sebastienros/jint#3063
* A throw from the iterator step must not close the iterator by @​lahma
in sebastienros/jint#3047
 ... (truncated)

Commits viewable in [compare
view](sebastienros/jint@v4.16.0...v4.16.1).
</details>

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=Jint&package-manager=nuget&previous-version=4.16.0&new-version=4.16.1)](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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant