Skip to content

fix(interp): collapse drifted boxed EvaluateGetOnRecord onto the RV implementation#1267

Merged
nickna merged 1 commit into
mainfrom
worktree-getonrecord-drift
Jul 11, 2026
Merged

fix(interp): collapse drifted boxed EvaluateGetOnRecord onto the RV implementation#1267
nickna merged 1 commit into
mainfrom
worktree-getonrecord-drift

Conversation

@nickna

@nickna nickna commented Jul 11, 2026

Copy link
Copy Markdown
Owner

One of the fix-first duplication findings from the 2026-07-01 audit (re-verified today): EvaluateGetOnRecord (boxed) and EvaluateGetOnRecordRV were a ~68-line near-duplicate that had already drifted — the boxed __proto__ walk stopped at a SharpTSInstance prototype and dispatched getters via legacy .Call, while the RV loop handles the instance arm and uses CallV2.

Observable bug fixed

Boxed callers are the named-CJS-import interop reads (Interpreter.cs import/re-export paths) and the for-of iterator-protocol done/value reads. With a CJS module whose exports object is Object.create(new Point()):

// point.cjs: class Point { constructor() { this.x = 42; } }
//            module.exports = Object.create(new Point());
import { x } from "./point.cjs";
console.log(x); // main: undefined  →  this PR: 42

Change

The boxed method is now a one-line .ToObject() adapter over EvaluateGetOnRecordRV (−65 lines; RuntimeValue.ToObject maps Undefined back to the SharpTSUndefined.Instance sentinel, so miss behavior is identical). Rationale docs moved onto the RV method, which is now the single implementation.

Verification

  • Live before/after repro above (pre-fix binaries print undefined, post-fix 42), plus a for-of protocol sanity check
  • Full unit suite: 15,452/15,452 pass
  • Test262 interpreter baseline diff clean (subset incl. built-ins/Object, property-accessors — no regressions, no unexpected new-passes)
  • TS-conformance unaffected (type-checker only; this is a runtime change)

…mplementation

The boxed copy had drifted from EvaluateGetOnRecordRV: its __proto__
walk only continued through SharpTSObject prototypes, so a property
living on a SharpTSInstance prototype (Object.create(new Point())-style
chains) resolved to undefined for boxed callers — named ESM imports
from CJS modules, CJS re-exports, and for-of iterator-protocol reads.
It also still dispatched getters via legacy .Call instead of CallV2.

The boxed method is now a one-line .ToObject() adapter over the RV
implementation (RuntimeValue.ToObject maps Undefined back to the
SharpTSUndefined.Instance sentinel the old copy returned).

Repro fixed: point.cjs with 'module.exports = Object.create(new
Point())' + 'import { x } from "./point.cjs"' — was undefined, now
reads the instance field.

Verified: full suite 15452/0; Test262 interpreter baseline diff clean.
@nickna
nickna merged commit 4aa1aba into main Jul 11, 2026
2 checks passed
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