Distinguish an optional-chain short circuit from a genuine undefined (#3040)

Jint signalled "this optional chain short-circuited" by returning the
`JsValue.Undefined` singleton and testing for it with `ReferenceEquals`, which
is indistinguishable from a value that simply is `undefined`. Three consumers
read that signal, and each of them swallowed a `TypeError` the spec owes:

    (0, undefined)()                       // returned undefined
    new.target()                           // returned undefined
    ({})?.a['b']                           // returned undefined
    (({a:{b:()=>undefined}}).a.b?.())()    // returned undefined

`new.target()` is the same bug seen from `Engine.GetNewTarget`, which hands
back the singleton for a [[Call]]ed function; `({})?.a['b']` is the member
path's second half of it, where a base that merely *contains* a `?.`
(`_objectExpression._expression.IsOptional()`) short-circuited the link after
it. Per ECMA-262 13.3.9.1 the short circuit is decided only at the `?.` whose
own base is nullish; a link that produces `undefined` continues normally and
the next link throws.

The signal is now a dedicated sentinel, and it is opt-in per parent/child edge:
a member or call node calls `EnableShortCircuitPropagation()` on its
object/callee at build time, and only a link whose parent asked ever produces
the sentinel. Every such parent tests for it in the statement that follows the
evaluation and returns instead of using it, so the sentinel never travels
farther than one edge and can never become a value. A link nobody asked — the
outermost one, i.e. whatever a `ChainExpression` wraps, which is also what
parentheses create — returns a real `undefined`, which is exactly the chain's
value. A `ChainExpression` object/callee is deliberately excluded from the
opt-in: `(a?.b).c` must throw.

The sentinel is an internal JsValue with no JavaScript type whose `ToObject`
throws, so a future change that broke the invariant would fail loudly rather
than hand script a stray value.

No cost on the non-optional path: the two consumption sites are the same single
reference compare they were, the fast lanes are unchanged, and the member link's
own test got cheaper (a readonly bool where an `IChainElement` type test used to
be). The new field and the build-time virtual are paid once per node, never per
evaluation.

Frees three test262 staging files, which now pass in every mode they declare:
`staging/sm/class/newTargetDVG.js`, `staging/sm/expressions/optional-chain.js`
and `staging/sm/extensions/extension-methods-reject-null-undefined-this.js` —
the last of which the exclusion banner claimed needed SpiderMonkey's `toSource`
extension and could not be fixed. It never did: `toSource` being absent is what
makes `(0, Object.prototype.toSource)()` reach the call in the first place.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: lahma

Jint.Tests.Test262/Test262Harness.settings.json

@@ -172,15 +172,12 @@
     "staging/sm/Function/arguments-parameter-shadowing.js",
     "staging/sm/regress/regress-552432.js",
 
-    // Argument validation raising no error, or the wrong one, on built-ins called with a bad this
-    // or a bad index. Includes SpiderMonkey's own toSource extension, which Jint does not implement
-    // at all -- that half is removed by the test moving, not by Jint changing.
+    // Argument validation raising no error, or the wrong one: a bad this or a bad index reaching a
+    // built-in, a NUL-bearing string reaching one, and the order in which a super reference is
+    // evaluated against its side effects.
     "staging/sm/TypedArray/constructor-byteoffsets-bounds.js",
     "staging/sm/TypedArray/iterator-next-with-detached.js",
-    "staging/sm/class/newTargetDVG.js",
     "staging/sm/class/superPropOrdering.js",
-    "staging/sm/expressions/optional-chain.js",
-    "staging/sm/extensions/extension-methods-reject-null-undefined-this.js",
     "staging/sm/extensions/quote-string-for-nul-character.js",
     "staging/sm/misc/builtin-methods-reject-null-undefined-this.js",
 

Jint.Tests/Runtime/OptionalChainShortCircuitTests.cs

@@ -0,0 +1,197 @@
+using Jint.Native;
+
+namespace Jint.Tests.Runtime;
+
+/// <summary>
+/// Pins ECMA-262 §13.3.9.1 (https://tc39.es/ecma262/#sec-optional-chaining-evaluation): an optional
+/// chain short-circuits at the <c>?.</c> whose <em>own</em> base is nullish, and nowhere else. A link
+/// that merely produces <c>undefined</c> — because the property is absent, because the callee returned
+/// it, or because the value simply is <c>undefined</c> — continues the chain, and the next link throws.
+/// <para>
+/// The interpreter signals the short circuit by returning a marker up the chain. These tests exist
+/// because that marker used to be <see cref="JsValue.Undefined"/> itself, compared with
+/// <c>ReferenceEquals</c> — indistinguishable from a genuine <c>undefined</c>, so every expression that
+/// happened to evaluate to the singleton was treated as a short circuit and swallowed the
+/// <c>TypeError</c> it owed.
+/// </para>
+/// </summary>
+public class OptionalChainShortCircuitTests
+{
+    /// <summary>
+    /// Runs <paramref name="source"/> and reports the constructor name of whatever it threw, so a test
+    /// pins the error <em>type</em> rather than a message that is free to change.
+    /// </summary>
+    private static string Throws(string source)
+    {
+        var engine = new Engine();
+        return engine.Evaluate($$"""
+            (function () {
+                try {
+                    {{source}};
+                } catch (e) {
+                    return e.constructor.name;
+                }
+                return 'did not throw';
+            })()
+            """).AsString();
+    }
+
+    private static JsValue Evaluate(string source) => new Engine().Evaluate(source);
+
+    [Fact]
+    public void CallingAValueThatIsUndefinedThrowsRatherThanShortCircuiting()
+    {
+        // The sequence expression yields the JsValue.Undefined singleton; nothing here is an optional
+        // chain, so the call must throw.
+        // (staging/sm/extensions/extension-methods-reject-null-undefined-this.js)
+        Throws("(0, undefined)()").Should().Be("TypeError");
+    }
+
+    [Fact]
+    public void CallingAnUndefinedNewTargetThrows()
+    {
+        // Engine.GetNewTarget hands back the JsValue.Undefined singleton for a [[Call]]ed function.
+        // (staging/sm/class/newTargetDVG.js)
+        Throws("(function () { new.target(); })()").Should().Be("TypeError");
+    }
+
+    [Fact]
+    public void ANonOptionalLinkAfterAnOptionalOneStillThrows()
+    {
+        // `({})?.a` does not short-circuit — {} is not nullish — so it is a legitimate undefined and
+        // the non-optional `['b']` that follows must throw.
+        Throws("({})?.a['b']").Should().Be("TypeError");
+        Throws("({})?.['a'].b").Should().Be("TypeError");
+        Throws("({ a: { b: undefined } }).a?.b.b.c").Should().Be("TypeError");
+    }
+
+    [Fact]
+    public void CallingTheUndefinedResultOfACompletedOptionalCallThrows()
+    {
+        // `b?.()` does not short-circuit: b is callable. It simply returns undefined, and calling that
+        // undefined must throw.
+        Throws("(({ a: { b: () => undefined } }).a.b?.())()").Should().Be("TypeError");
+    }
+
+    [Fact]
+    public void ParenthesesEndTheChainSoTheNextLinkThrows()
+    {
+        // `(a?.b)` is a complete ChainExpression: its short circuit produces a real undefined, and the
+        // member access outside the parentheses is not part of that chain.
+        Throws("var a = null; (a?.b).c").Should().Be("TypeError");
+        Throws("var a = null; (a?.b)()").Should().Be("TypeError");
+    }
+
+    [Fact]
+    public void ShortCircuitStopsTheWholeChain()
+    {
+        Evaluate("undefined?.a").Should().BeUndefined();
+        Evaluate("null?.a?.b").Should().BeUndefined();
+        Evaluate("var a; a?.()").Should().BeUndefined();
+        Evaluate("null?.a['b']().c").Should().BeUndefined();
+        Evaluate("null?.['a'].b()['c']").Should().BeUndefined();
+        Evaluate("null?.()().a['b']").Should().BeUndefined();
+        Evaluate("({ a: { b: undefined } }).a.b?.()()()").Should().BeUndefined();
+    }
+
+    [Fact]
+    public void OrdinaryChainsKeepResolving()
+    {
+        Evaluate("({ a: 1 })?.a").Should().Be(1);
+        Evaluate("({ a: { b: [10, 20] } })?.a.b[1]").Should().Be(20);
+
+        // A deep chain mixing optional and non-optional links, calls and computed reads.
+        var deep = Evaluate("""
+            (function () {
+                var o = {
+                    a: {
+                        b: function () { return this._b.bind(this); },
+                        _b: function () { return this.__b; },
+                        __b: { c: 42 }
+                    }
+                };
+                return [
+                    o?.a?.['b']?.()?.()?.c,
+                    o.a.b()().c,
+                    o?.a.b?.()().c,
+                    o?.missing?.['x']?.()?.()?.y
+                ].join(',');
+            })()
+            """);
+
+        deep.AsString().Should().Be("42,42,42,");
+    }
+
+    [Fact]
+    public void ShortCircuitSkipsTheRestOfTheChainWithoutEvaluatingIt()
+    {
+        // `y` is not declared: reaching the computed property expression at all would raise a
+        // ReferenceError, so `true` proves the whole tail was skipped.
+        Evaluate("delete undefined?.x[y + 1]").Should().Be(true);
+
+        // Every base is evaluated exactly once, and only up to the short circuit.
+        var counts = Evaluate("""
+            (function () {
+                var calls = 0;
+                var a = { b: { c: { d: function () { calls++; return a; } } } };
+                a.b.c.d?.()?.b?.c?.d;
+
+                var reads = 0;
+                var g = { get b() { reads++; return { c: {} }; } };
+                g.b?.c?.d;
+                g.b?.c?.d;
+
+                return calls + ',' + reads;
+            })()
+            """);
+
+        counts.AsString().Should().Be("1,2");
+    }
+
+    [Fact]
+    public void DeleteOfAShortCircuitedChainIsTrue()
+    {
+        Evaluate("delete undefined?.foo").Should().Be(true);
+        Evaluate("delete null?.['foo']").Should().Be(true);
+        Evaluate("delete null?.()").Should().Be(true);
+        Evaluate("delete ({ a: { b: undefined } }).a?.b?.b").Should().Be(true);
+    }
+
+    [Fact]
+    public void TheVerdictSurvivesHandlerTreeReuseAndPreparedScripts()
+    {
+        // Whether a link propagates its short circuit is decided once, when the handler tree is built, so
+        // it has to hold for a re-run on a warmed engine and for a Prepared<Script> shared across engines.
+        var prepared = Engine.PrepareScript("""
+            (function () {
+                var results = [];
+                results.push(String(null?.a['b']().c));
+                try { ({})?.a['b']; results.push('did not throw'); }
+                catch (e) { results.push(e.constructor.name); }
+                return results.join(',');
+            })()
+            """);
+
+        var engine = new Engine();
+        for (var i = 0; i < 3; i++)
+        {
+            engine.Evaluate(prepared).AsString().Should().Be("undefined,TypeError");
+        }
+
+        new Engine().Evaluate(prepared).AsString().Should().Be("undefined,TypeError");
+    }
+
+    [Fact]
+    public void ShortCircuitedChainStaysAnOrdinaryUndefinedForItsConsumers()
+    {
+        // The short-circuit marker must never reach a consumer of the chain's value.
+        Evaluate("typeof null?.a").Should().Be("undefined");
+        Evaluate("null?.a === undefined").Should().Be(true);
+        Evaluate("String(null?.a)").Should().Be("undefined");
+        Evaluate("null?.a ?? 'fallback'").Should().Be("fallback");
+        Evaluate("JSON.stringify({ x: null?.a })").Should().Be("{}");
+        Evaluate("[null?.a].length").Should().Be(1);
+        Evaluate("({ undefined: 3 })?.[null?.a]").Should().Be(3);
+        Evaluate("((x) => typeof x)(null?.a)").Should().Be("undefined");
+    }
+}

Jint/Runtime/Interpreter/Expressions/JintCallExpression.cs

@@ -82,6 +82,11 @@ internal sealed class JintCallExpression : JintExpression
     /// </summary>
     private const int MaxRegisterArguments = 4;
 
+    // Set by the next link of the same optional chain (see JintExpression.EnableShortCircuitPropagation):
+    // true means this node's short circuit is consumed by its parent link, false means this node is the
+    // outermost link and its short circuit is the chain's value, i.e. a real undefined.
+    private bool _propagatesShortCircuit;
+
     public JintCallExpression(CallExpression expression) : base(expression)
     {
         _arguments.Initialize(expression.Arguments.AsSpan());
@@ -90,11 +95,27 @@ public JintCallExpression(CallExpression expression) : base(expression)
         _calleeMember = _calleeExpression as JintMemberExpression;
         _isTailPosition = ReferenceEquals(expression.UserData, TailCallMarker.Instance);
 
+        // A ChainExpression callee is a chain of its own whose value has already been taken
+        // (`(a?.b)()`), so its short circuit is a real undefined and this call must run against it.
+        // Anything else is the same chain continuing.
+        if (expression.Callee.Type != NodeType.ChainExpression && CanShortCircuit(expression.Callee))
+        {
+            _calleeExpression.EnableShortCircuitPropagation();
+        }
+
         _argCount = expression.Arguments.Count;
         _fastArgsEligible = !_arguments.HasSpreads && _argCount <= 2;
         _regLaneEligible = !_arguments.HasSpreads && _argCount is >= 1 and <= MaxRegisterArguments;
     }
 
+    internal override void EnableShortCircuitPropagation() => _propagatesShortCircuit = true;
+
+    /// <summary>
+    /// What this link returns when the chain short-circuits here or below: the signal when the next link
+    /// is waiting for it, and the chain's actual value — <c>undefined</c> — when this is the outermost link.
+    /// </summary>
+    private JsValue ShortCircuit() => _propagatesShortCircuit ? ShortCircuited : JsValue.Undefined;
+
     protected override object EvaluateInternal(EvaluationContext context)
     {
 
@@ -176,17 +197,21 @@ protected override object EvaluateInternal(EvaluationContext context)
                 return calleeReference as JsValue ?? JsValue.Undefined;
             }
 
-            if (ReferenceEquals(calleeReference, JsValue.Undefined))
+            if (ReferenceEquals(calleeReference, ShortCircuited))
             {
-                return JsValue.Undefined;
+                // The callee link short-circuited, so this call is skipped along with the rest of the
+                // chain — its arguments are never evaluated. A callee that merely evaluated to undefined
+                // is *not* this case and falls through to the "not a function" throw below.
+                return ShortCircuit();
             }
 
             func = engine.GetValue(calleeReference, false);
 
+            // This link's own `?.(`: the value being called is nullish, so the chain short-circuits here.
             if (func.IsNullOrUndefined() && _expression.IsOptional())
             {
                 engine._referencePool.Return(referenceRecord);
-                return JsValue.Undefined;
+                return ShortCircuit();
             }
 
             if (ReferenceEquals(func, engine.Realm.Intrinsics.Eval)

Jint/Runtime/Interpreter/Expressions/JintExpression.cs

@@ -10,11 +10,62 @@ internal abstract class JintExpression
 {
     protected internal readonly Expression _expression;
 
+    /// <summary>
+    /// The signal one optional-chain link hands the next when the chain short-circuits — that is, when a
+    /// <c>?.</c> found its <em>own</em> base nullish (https://tc39.es/ecma262/#sec-optional-chaining-evaluation).
+    /// It exists so that a short circuit is distinguishable from a link that legitimately produced
+    /// <c>undefined</c>: <c>({})?.a['b']</c> must throw because <c>({})?.a</c> did not short-circuit, and
+    /// <c>(0, undefined)()</c> must throw because nothing in it is a chain at all.
+    /// <para>
+    /// It cannot reach a script. A link produces it only when the node that evaluates it asked to receive
+    /// it — <see cref="EnableShortCircuitPropagation"/>, called at build time by the member/call node that
+    /// owns it as its object/callee — and every such node tests for it in the statement immediately
+    /// following that evaluation and returns instead of using it. A link nobody asked (the outermost link
+    /// of the chain, i.e. whatever a <c>ChainExpression</c> wraps) returns a real
+    /// <see cref="JsValue.Undefined"/>, which is precisely what the chain's value is.
+    /// </para>
+    /// </summary>
+    private protected static readonly JsValue ShortCircuited = OptionalChainShortCircuit.Instance;
+
     protected JintExpression(Expression expression)
     {
         _expression = expression;
     }
 
+    /// <summary>
+    /// Tells this node that whoever built it will evaluate it as the base of the next link in the same
+    /// optional chain, and so will consume <see cref="ShortCircuited"/> rather than mistake it for a value.
+    /// Only the two link types — <see cref="JintMemberExpression"/> and <see cref="JintCallExpression"/> —
+    /// can short-circuit at all, so every other node ignores the request. Build time only; nothing calls
+    /// this during evaluation.
+    /// </summary>
+    internal virtual void EnableShortCircuitPropagation()
+    {
+    }
+
+    /// <summary>
+    /// Whether evaluating <paramref name="expression"/> can end in an optional chain's short circuit: it is
+    /// itself a <c>?.</c> link, or it is a link whose own base can. A <c>ChainExpression</c> is deliberately
+    /// transparent here — the predicate gates fast paths, where over-approximating costs a fast lane and
+    /// never correctness; the chain <em>boundary</em> a <c>ChainExpression</c> marks is honoured where
+    /// propagation is enabled instead.
+    /// </summary>
+    private protected static bool CanShortCircuit(Expression expression)
+    {
+        if (expression.IsOptional())
+        {
+            return true;
+        }
+
+        return expression switch
+        {
+            ChainExpression chainExpression => CanShortCircuit(chainExpression.Expression),
+            CallExpression callExpression => CanShortCircuit(callExpression.Callee),
+            MemberExpression memberExpression => CanShortCircuit(memberExpression.Object),
+            _ => false
+        };
+    }
+
     /// <summary>
     /// Resolves the underlying value for this expression.
     /// By default uses the Engine for resolving.
@@ -574,3 +625,27 @@ protected static bool AreIntegerOperands(JsValue left, JsValue right)
         return left._type == right._type && left._type == InternalTypes.Integer;
     }
 }
+
+/// <summary>
+/// The single instance behind <see cref="JintExpression.ShortCircuited"/>. A dedicated type rather than a
+/// reused <see cref="JsValue.Undefined"/> (which is exactly what made the short circuit indistinguishable
+/// from a value) or a marker string (which would masquerade as one if it ever escaped): it claims no
+/// JavaScript type at all, so nothing coerces or compares it into looking like a value, and the one member
+/// through which a host could observe it throws instead of answering.
+/// </summary>
+internal sealed class OptionalChainShortCircuit : JsValue
+{
+    internal static readonly OptionalChainShortCircuit Instance = new();
+
+    private OptionalChainShortCircuit() : base(Types.Empty)
+    {
+    }
+
+    public override object? ToObject()
+    {
+        Throw.InvalidOperationException("The optional chain short-circuit signal escaped the chain that produced it.");
+        return null;
+    }
+
+    public override string ToString() => "[[optional chain short-circuit]]";
+}