Repository navigation
Distinguish an optional-chain short circuit from a genuine undefined - #3040
Merged
Merged
Conversation
…not at every undefined
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>
lahma
force-pushed
the
staging/optional-chain-sentinel
branch
from
August 18, 2026 17:12
bb4a383 to
c4f45ed
Compare
lahma
enabled auto-merge (squash)
August 19, 2026 10:27
7 of 8 tasks
9 of 26 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jint signalled "this optional chain short-circuited" by returning the
JsValue.Undefinedsingleton and then testing for it withReferenceEquals. That is indistinguishable from a link that legitimately producedundefined, so several expressions silently returnedundefinedwhere they must throw:Per 13.3.9.1 a chain short-circuits only at the
?.whose own base is nullish.new.target()was caught up in it becauseEngine.GetNewTarget()returnsthisEnvironment.NewTarget ?? JsValue.Undefined, which then tripped the sentinel test inJintCallExpression.There was a second, independent bug on the same code path:
JintMemberExpressionshort-circuited wheneveri.e. because the object expression merely contained a
?.somewhere, not because this link had one. That disjunct is what made({})?.a['b']and(a?.b).creturnundefined.The fix
A dedicated internal sentinel,
OptionalChainShortCircuit— aJsValuewithTypes.EmptywhoseToObject()throws, so it claims no JavaScript type and cannot masquerade as a value if it ever escaped.Whether a link emits the sentinel or a real
undefinedis decided at build time: a member/call node callsEnableShortCircuitPropagation()on its object/callee only when that child is part of the same chain (aChainExpressionchild ends the chain, which is exactly what parentheses mean). A link nobody asked returnsJsValue.Undefined, so there is no conversion step at the boundary to get wrong.Containment is structural rather than by convention: every parent that asks consumes the sentinel in the statement immediately following the child's
Evaluate, so it never travels more than one edge, is never stored, never becomes aReferencebase, and never reachesengine.GetValue. The four fast lanes that would reach a child by some route other thanEvaluateare all gated on!_objectExpressionCanShortCircuit, which is exactly complementary to the condition that enables propagation.The
IsOptional()disjunct is removed.Performance
Nothing added on the non-optional path, and the member link's own test got cheaper: an
IChainElementtype test became a bool field read, tested first. The additions are oneboolfield on each of two node types and oneinternal virtualslot called only during handler-tree construction.Tests
Jint.Tests/Runtime/OptionalChainShortCircuitTests.cs— 10 tests, 5 failing against unfixed code, plus positive cases (undefined?.a,null?.a?.b, deep mixed chains) and a test that a short-circuited chain's result is an ordinaryundefinedfortypeof,===,String(),??,JSON.stringify, spread and computed keys.language/expressions/optional-chaining/**(76 generated cases) unaffected.Frees three
staging/exclusions:class/newTargetDVG.js,expressions/optional-chain.js,extensions/extension-methods-reject-null-undefined-this.js. The exclusion comment claimed the last of these needed SpiderMonkey'stoSourceextension and was unfixable — that was wrong, and instructively so:toSourcebeing absent is precisely what makesObject.prototype.toSourceundefined, which drove(0, Class.prototype[method])()into the buggy call path. The missing extension was the trigger, not the blocker.Refs #3021.