Generators: a yield* delegation both re-delegates and keeps its place (backport of #3506 and #3518)

Jint.Tests.Test262/Test262Harness.settings.json

@@ -44,7 +44,6 @@
     "staging/sm/Date/dst-offset-caching-6-of-8.js",
     "staging/sm/Date/dst-offset-caching-7-of-8.js",
     "staging/sm/Date/dst-offset-caching-8-of-8.js",
-    "staging/sm/generators/delegating-yield-9.js",
     "staging/sm/regress/regress-610026.js",
     "staging/sm/regress/regress-619003-1.js",
     // Same shape, found by running the suite: each of these burns the whole 30-second budget and

Jint.Tests/Runtime/GeneratorTests.cs

@@ -871,6 +871,261 @@ public void ShouldRaiseAdditionChainBigIntMixErrorBeforeLaterOperandSideEffects(
         _engine.Evaluate(Script).AsString().Should().Be("[0,0]");
     }
 
+    [Fact(Timeout = 10000)]
+    public void ADelegatingYieldInsideAYieldStartsOverOnEveryLoopIteration()
+    {
+        // https://tc39.es/ecma262/#sec-generator-function-definitions-runtime-semantics-evaluation:
+        // every evaluation of `yield * AssignmentExpression` evaluates its operand and drives the
+        // resulting iterator to completion, so a loop that comes back round to the same yield* node
+        // starts a fresh delegation. Jint replays a generator body from the top on each resume and
+        // memoized what each yield node had already returned; the memo was never invalidated, so the
+        // second iteration answered the outer yield from the first iteration's value without ever
+        // evaluating the operand -- which abandoned the delegation and, because the operand carries
+        // the loop's own decrement here, left n unchanged and the loop running forever.
+        // staging/sm/generators/delegating-yield-9.js is this shape; SpiderMonkey and V8 both
+        // report eight results for countdown(3).
+        const string Script = """
+            function* countdown(n) {
+                while (n > 0) {
+                    yield (yield* countdown(--n));
+                }
+                return 34;
+            }
+
+            var results = [];
+            var it = countdown(3);
+            var result;
+            do {
+                result = it.next();
+                results.push(result.value + ':' + result.done);
+            } while (!result.done && results.length < 100);
+            return results.join(' ');
+        """;
+
+        // A regression here spins forever, and xUnit's Timeout cannot abort a synchronous test method
+        // on its own; the engine has to observe the test's token for the timeout to bite.
+        var engine = new Engine(options => options.CancellationToken(TestContext.Current.CancellationToken));
+
+        engine.Evaluate(Script).Should().Be("34:false 34:false 34:false 34:false 34:false 34:false 34:false 34:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void ADelegatingYieldInsideAYieldKeepsItsPlaceWhenTheDecrementIsElsewhere()
+    {
+        // The same defect without the runaway loop: with the decrement in its own statement the loop
+        // still terminates, but the outer yield answered from the memo instead of yielding, so two of
+        // the eight results went missing. Kept separate because a fix that only stopped the hang
+        // would leave this one silently wrong.
+        const string Script = """
+            function* countdown(n) {
+                while (n > 0) {
+                    n = n - 1;
+                    yield (yield* countdown(n));
+                }
+                return 34;
+            }
+
+            var results = [];
+            var it = countdown(3);
+            var result;
+            do {
+                result = it.next();
+                results.push(result.value + ':' + result.done);
+            } while (!result.done && results.length < 100);
+            return results.join(' ');
+        """;
+
+        var engine = new Engine(options => options.CancellationToken(TestContext.Current.CancellationToken));
+
+        engine.Evaluate(Script).Should().Be("34:false 34:false 34:false 34:false 34:false 34:false 34:false 34:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void AnAsyncDelegatingYieldInsideAYieldStartsOverOnEveryLoopIteration()
+    {
+        // The async twin of the two tests above, and a second defect underneath the memo one they
+        // pin. Nothing recorded WHERE an async generator stood while a yield* delegation was in
+        // flight: a delegation's suspension point is tracked in its own field and only an ordinary
+        // yield's was published as the resume position, so every resume-aware statement re-ran its
+        // own test on the way back in. Here `countdown(--n)` had already moved n, so the `while`
+        // that was true when the iteration began answered false on resume: the loop was abandoned
+        // mid-iteration and the outer yield it still owed was never reached -- one result lost per
+        // nesting level, half the sequence for countdown(3).
+        // https://tc39.es/ecma262/#sec-asyncgeneratoryield suspends the generator AT the yield, and
+        // https://tc39.es/ecma262/#sec-generator-function-definitions-runtime-semantics-evaluation
+        // resumes a yield* delegation with the completion its caller sent; neither re-evaluates the
+        // iteration statement the yield* sits in. V8 and SpiderMonkey both report eight results for
+        // countdown(3), as they do for the synchronous countdown above.
+        const string Script = """
+            async function* countdown(n) {
+                while (n > 0) {
+                    yield (yield* countdown(--n));
+                }
+                return 34;
+            }
+
+            function makeIterator() { return countdown(3); }
+            """;
+
+        Drain(Script, Async, TestContext.Current.CancellationToken).Should().Be("34:false 34:false 34:false 34:false 34:false 34:false 34:false 34:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void AnAsyncDelegatingYieldInsideAYieldKeepsItsPlaceWhenTheDecrementIsElsewhere()
+    {
+        // The async twin of ADelegatingYieldInsideAYieldKeepsItsPlaceWhenTheDecrementIsElsewhere.
+        // Moving the decrement out of the operand changes nothing here, because what the resume lost
+        // was its place in the loop, not the operand.
+        const string Script = """
+            async function* countdown(n) {
+                while (n > 0) {
+                    n = n - 1;
+                    yield (yield* countdown(n));
+                }
+                return 34;
+            }
+
+            function makeIterator() { return countdown(3); }
+            """;
+
+        Drain(Script, Async, TestContext.Current.CancellationToken).Should().Be("34:false 34:false 34:false 34:false 34:false 34:false 34:false 34:true");
+    }
+
+    // The three pairs below are that same missing resume position reached without any recursion, one
+    // pair per resume-aware statement. Each is written so the statement's own test is already FALSE
+    // by the time the delegation suspends, which is what makes a re-evaluated test observable: the
+    // engine dropped the rest of the iteration the generator was in the middle of. The synchronous
+    // half is pinned beside the asynchronous one because the bookkeeping is per instance type, and
+    // the two have drifted apart once already.
+
+    [Fact(Timeout = 10000)]
+    public void ADelegatingYieldSuspensionKeepsTheEnclosingWhileLoopFromRestarting()
+    {
+        Drain(WhileWithFalsifiedTest("function*"), Sync, TestContext.Current.CancellationToken).Should().Be("1:false after:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void AnAsyncDelegatingYieldSuspensionKeepsTheEnclosingWhileLoopFromRestarting()
+    {
+        Drain(WhileWithFalsifiedTest("async function*"), Async, TestContext.Current.CancellationToken).Should().Be("1:false after:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void ADelegatingYieldSuspensionKeepsTheEnclosingForLoopFromRestarting()
+    {
+        Drain(ForWithFalsifiedTest("function*"), Sync, TestContext.Current.CancellationToken).Should().Be("1:false body:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void AnAsyncDelegatingYieldSuspensionKeepsTheEnclosingForLoopFromRestarting()
+    {
+        Drain(ForWithFalsifiedTest("async function*"), Async, TestContext.Current.CancellationToken).Should().Be("1:false body:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void ADelegatingYieldSuspensionKeepsTheEnclosingIfFromTakingTheOtherBranch()
+    {
+        Drain(IfWithFlippedTest("function*"), Sync, TestContext.Current.CancellationToken).Should().Be("1:false then:true");
+    }
+
+    [Fact(Timeout = 10000)]
+    public void AnAsyncDelegatingYieldSuspensionKeepsTheEnclosingIfFromTakingTheOtherBranch()
+    {
+        Drain(IfWithFlippedTest("async function*"), Async, TestContext.Current.CancellationToken).Should().Be("1:false then:true");
+    }
+
+    private static string WhileWithFalsifiedTest(string kind) => $$"""
+        {{kind}} leaf() { yield 1; }
+        {{kind}} outer() {
+            var log = [];
+            var n = 2;
+            while (n > 0) {
+                n = 0;
+                yield* leaf();
+                log.push('after');
+            }
+            return log.join(',');
+        }
+
+        function makeIterator() { return outer(); }
+        """;
+
+    private static string ForWithFalsifiedTest(string kind) => $$"""
+        {{kind}} leaf() { yield 1; }
+        {{kind}} outer() {
+            var log = [];
+            for (var i = 0; i < 2; i++) {
+                i = 5;
+                yield* leaf();
+                log.push('body');
+            }
+            return log.join(',');
+        }
+
+        function makeIterator() { return outer(); }
+        """;
+
+    private static string IfWithFlippedTest(string kind) => $$"""
+        {{kind}} leaf() { yield 1; }
+        {{kind}} outer() {
+            var log = [];
+            var taken = true;
+            if (taken) {
+                taken = false;
+                yield* leaf();
+                log.push('then');
+            } else {
+                log.push('else');
+            }
+            return log.join(',');
+        }
+
+        function makeIterator() { return outer(); }
+        """;
+
+    private const bool Sync = false;
+    private const bool Async = true;
+
+    /// <summary>
+    /// Drives whatever the script's <c>makeIterator()</c> returns to completion, as
+    /// <c>"value:done value:done ..."</c>.
+    /// </summary>
+    /// <remarks>
+    /// The twenty-result cap is a guard rather than a limit: every sequence these tests assert is far
+    /// shorter, so a regression that never terminates comes back as a wrong string instead of as a
+    /// test run that hangs. xUnit's <c>Timeout</c> cannot abort a synchronous test method on its own,
+    /// so the engine is handed the test's token as well.
+    /// </remarks>
+    private static string Drain(string generatorSource, bool async, CancellationToken cancellationToken)
+    {
+        var driver = async
+            ? """
+              (async function () {
+                  var results = [];
+                  var it = makeIterator();
+                  var result;
+                  do {
+                      result = await it.next();
+                      results.push(result.value + ':' + result.done);
+                  } while (!result.done && results.length < 20);
+                  return results.join(' ');
+              })()
+              """
+            : """
+              var results = [];
+              var it = makeIterator();
+              var result;
+              do {
+                  result = it.next();
+                  results.push(result.value + ':' + result.done);
+              } while (!result.done && results.length < 20);
+              results.join(' ');
+              """;
+
+        var engine = new Engine(options => options.CancellationToken(cancellationToken));
+        return engine.Evaluate(generatorSource + Environment.NewLine + driver).UnwrapIfPromise().AsString();
+    }
+
     [Fact]
     public void GeneratorFunctionConstructorsInheritFromTheFunctionConstructor()
     {

Jint/Native/AsyncGenerator/AsyncGeneratorInstance.cs

@@ -70,7 +70,13 @@ bool ISuspendable.IsResuming
 
     JsValue? ISuspendable.SuspendedValue => _suspendedValue;
 
-    object? ISuspendable.LastSuspensionNode => _lastYieldNode;
+    // The same reason as GeneratorInstance's identically shaped property, plus one an async
+    // generator has on its own: a delegation that COMPLETES also resumes through a full replay of
+    // the body (ResumeAfterDelegation), because the inner iterator's step settles on a later
+    // microtask and the stack that started the delegation is long gone. ClearDelegationIterator
+    // deliberately keeps _delegatingYieldNode across that hop so the replay re-enters at the yield*,
+    // which is exactly the node the enclosing statements need to be told about as well.
+    object? ISuspendable.LastSuspensionNode => _lastYieldNode ?? _delegatingYieldNode;
 
     bool ISuspendable.ReturnRequested => _returnRequested;
 

Jint/Native/Generator/GeneratorInstance.cs

@@ -108,7 +108,19 @@ bool ISuspendable.IsResuming
 
     JsValue? ISuspendable.SuspendedValue => _suspendedValue;
 
-    object? ISuspendable.LastSuspensionNode => _lastYieldNode;
+    // A yield* delegation suspends AT the yield* expression, and its node is kept in
+    // _delegatingYieldNode rather than in _lastYieldNode: JintYieldExpression selects its delegation
+    // branch on the first field and its plain-yield branch on the second, so the two cannot share a
+    // slot. Only _lastYieldNode used to be published here, which made a resume taken in the middle
+    // of a delegation look to every resume-aware statement -- JintWhileStatement, JintForStatement,
+    // JintDoWhileStatement, JintIfStatement, JintSwitchStatement, JintTryStatement -- like a resume
+    // that had suspended nowhere. Each of them then re-ran its own test, and a test the delegated
+    // iteration had already falsified sent the generator down the other branch, abandoning the rest
+    // of the iteration it was actually in the middle of. The fallback cannot answer with a stale
+    // node: _lastYieldNode is cleared the moment the yield it names is resumed, so it is null for
+    // the whole time a delegation is in flight, and _delegatingYieldNode is cleared when the
+    // delegation ends.
+    object? ISuspendable.LastSuspensionNode => _lastYieldNode ?? _delegatingYieldNode;
 
     bool ISuspendable.ReturnRequested => _returnRequested;
 

Jint/Runtime/ISuspendable.cs

@@ -31,6 +31,13 @@ internal interface ISuspendable
     /// The AST node where execution last suspended (yield or await expression).
     /// Unified property for tracking suspension location across all suspendable types.
     /// </summary>
+    /// <remarks>
+    /// This is what tells a resume-aware statement that it is being re-entered rather than entered,
+    /// so it must not re-evaluate the test that chose the branch the suspension is inside; see
+    /// <see cref="Interpreter.Statements.JintStatement.GetSuspensionNode"/>. A generator suspended
+    /// inside a <c>yield*</c> delegation reports the <c>yield*</c> expression, which is where the
+    /// specification says it is suspended.
+    /// </remarks>
     object? LastSuspensionNode { get; }
 
     /// <summary>

Jint/Runtime/Interpreter/Expressions/JintYieldExpression.cs

@@ -172,7 +172,19 @@ protected override object EvaluateInternal(EvaluationContext context)
             // Fall through to normal yield logic
         }
 
-        // Normal yield: evaluate argument and yield the value
+        // Normal yield: evaluate argument and yield the value.
+        //
+        // Reaching here is a *fresh* evaluation of this yield node, so whatever the node's previous
+        // evaluation produced stops being an answer to it. The memo above exists only to replay one
+        // evaluation - a statement that is re-executed from the top after a later suspension must
+        // not re-run the yields it already got past - and a loop that comes back round to the same
+        // node starts a new evaluation the memo has nothing to say about. Leaving the stale entry in
+        // place made `yield (yield* g())` inside a loop answer its second iteration with the first
+        // iteration's value without ever evaluating the operand, so the delegation was abandoned
+        // mid-flight (and, when the operand carried the loop's own decrement, the loop never ended).
+        generator?._yieldNodeValues?.Remove(_expression);
+        asyncGenerator?._yieldNodeValues?.Remove(_expression);
+
         JsValue value;
         if (_argument is not null)
         {