fix(interp): JSON.stringify throws real TypeError; hasOwnProperty sees setter-only accessors#967
Merged
nickna merged 1 commit intoJun 27, 2026
Conversation
…s setter-only accessors Fixes two interpreter bugs surfaced by the now-deterministic interpreted Test262 baseline (#964 follow-up). Both are interpreter-only; compiled mode already behaved correctly (separate hand-emitted IL twins). Bug 1 — JSON.stringify threw strings, not TypeError objects: Runtime/BuiltIns/JSONBuiltIns.cs detected circular structures and BigInt values but threw a bare string via `new ThrowException("TypeError: ...")`. Since the guest-error-identity change binds caught values verbatim, guest `catch` received a string, so `e instanceof TypeError` was false and every `assert.throws(TypeError, ...)` failed. Switched the 5 sites to throw a real `new SharpTSTypeError(...)`, matching the pattern already used in Interpreter.Properties and ArrayStaticBuiltIns. Bug 2 — hasOwnProperty / `in` ignored setter-only accessors: SharpTSObject.HasProperty checked _fields and _getters but not _setters, so a property defined via Object.defineProperty(o, p, {set}) (no getter) was invisible to hasOwnProperty even though its setter ran. Added the _setters check. This was masked before the realm-leak fix (#966) by Math-descriptor pollution turning such descriptors into data descriptors. Impact: 93 interpreted Test262 tests flip Fail->Pass — Bug 2 alone fixes the entire accessor-descriptor cluster (Object/defineProperty, defineProperties, create, hasOwnProperty, and Array.prototype.* accessor cases), far beyond the single Math-descriptor test that first exposed it. Validation: - Full xUnit suite: 14392 passed, 0 failed, 16 skipped. - Interpreted Test262 baseline refreshed (+93 passes) and verified 0-drift. - 9 new dual-mode/interp tests. Out of scope / noted: - The audited SharpTSObject.cs:163/212 existence checks show no observable setter-only gap (the setter-call path precedes them) — left unchanged. - Compiled-mode `in` does not yet see setter-only accessors (though compiled hasOwnProperty does) — a distinct pre-existing gap, deferred. - Promise/resolve-thenable-deferred.js is an intermittent async-timing flake (unrelated mechanism); pinned to its majority value (Fail) in the baseline and left as a separate follow-up.
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
1 participant
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.
Summary
Fixes two interpreter bugs surfaced by the now-deterministic interpreted Test262 baseline (#964 follow-up; depends on #966). Both are interpreter-only — compiled mode already behaved correctly (separate hand-emitted IL twins).
Net impact: 93 interpreted Test262 tests flip
Fail → Pass.Bug 1 —
JSON.stringifythrew strings, notTypeErrorobjects (5 tests)Runtime/BuiltIns/JSONBuiltIns.cscorrectly detected circular structures and BigInt values but threw a bare string vianew ThrowException("TypeError: …"). Since the guest-error-identity change (PR #906/#920) binds caught values verbatim, guestcatchreceived a string, soe instanceof TypeErrorwas false and everyassert.throws(TypeError, …)failed.Fix: the 5 sites now throw
new ThrowException(new SharpTSTypeError(…))— the pattern already used inInterpreter.Properties.cs:138andArrayStaticBuiltIns.cs:24. Confirmed:JSON.stringifyof a cycle / BigInt now throws a value withinstanceof TypeError === trueand.name === "TypeError".Bug 2 —
hasOwnProperty/inignored setter-only accessors (88 tests)SharpTSObject.HasPropertychecked_fieldsand_gettersbut not_setters, so a property defined viaObject.defineProperty(o, p, { set })(no getter) was invisible tohasOwnPropertyeven though its setter ran on assignment.Fix:
HasPropertynow also consults_setters. This single change fixes the entire accessor-descriptor cluster —Object/defineProperty/*,defineProperties/*,create/*,Object/prototype/hasOwnProperty/*, andArray/prototype/*accessor cases — far beyond the oneMath-descriptor test (15.2.3.6-3-253) that first exposed it. It was masked before #966 byMath-descriptor pollution turning such descriptors into data descriptors.Validation
Fail→Pass), verified 0-drift.JsonTypeErrorAndAccessorHasOwnTests): JSON circular/toJSON/BigInt throw realTypeErrorand setter-onlyhasOwnProperty/in— dual-mode where both modes agree.Out of scope / noted
SharpTSObject.cs:163/212existence checks (_fields || HasGetter) have no observable setter-only gap — the setter-call path precedes them (probed) — left unchanged.indoes not yet see setter-only accessors (though compiledhasOwnPropertydoes) — a distinct pre-existing gap, deferred (the corresponding bonus test is interp-only).Promise/resolve-thenable-deferred.jsis an intermittent async-timing flake (different mechanism than the realm leak) — pinned to its majority value (Fail) in the baseline; separate follow-up.