fix(runtime): #5587 — subclassing a Temporal.<Type> gives the instance the Temporal brand - #5672
Merged
Merged
Conversation
… the instance the Temporal brand
`class X extends Temporal.Duration { constructor(){ super(...args) } }` produced an
empty plain object: `super()` ran the native Temporal constructor (which returns a
fresh NaN-boxed cell rather than mutating the implicit `this`) and DISCARDED the
returned cell. The subclass instance then had no brand, so `instance.abs()` resolved
`abs` to `undefined` and threw `TypeError: value is not a function` — the dominant
failure (36 cases) in the test262 `built-ins/Temporal/**/subclassing-ignored.js`
cluster.
Mirror the existing `class X extends Request/Response` fetch-handle pattern: when a
`super()` parent resolves to a Temporal constructor, run it and stash the returned
cell on `this` under `__perry_temporal_cell__`. Both `super()` lowerings are covered
— `js_fetch_or_value_super` (non-spread `super(a, b)`) and `js_super_construct_apply`
(the `super(...spread)` form the helper actually uses, via the decl-time-recorded
dynamic parent value). Method-call, property-get, and `instanceof` dispatch recover
the cell from there.
Also fixes the surrounding real-cell gaps the same tests exercise:
- `Object.getPrototypeOf(temporalCell)` returned `null`; now resolves
`Temporal.<Type>.prototype` via the live namespace, so
`assert.sameValue(Object.getPrototypeOf(result), construct.prototype)` holds.
- Reading a prototype method as a value (`d.abs`, not `d.abs()`) returned
`undefined`, breaking the `instance[method](...spread)` read+apply form; now
returns a bound method (gated on a real per-kind method predicate so unknown
properties still read as `undefined`). Applies to real cells and subclass
instances.
- Construct-only Temporal ctors (`PlainDate`/`PlainTime`/`Instant`/…) threw
"requires 'new'" when invoked from `super()`; `new.target` is now set for the call.
Known remaining limitation (separate, non-Temporal bug): a class method/constructor
defined inside a function captures an enclosing `let` by value, not by cell, so the
helper's `assert.sameValue(called, 1)` still fails for in-function subclasses. This
affects all classes (reproduces with a plain user-class subclass), not just Temporal,
and is out of scope here; with it the subclassing-ignored cases now run all the way
through the Temporal logic and fail only on that `called` assertion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds Temporal subclass construction, prototype lookup, ChangesTemporal subclass support
Sequence Diagram(s)sequenceDiagram
participant js_fetch_or_value_super
participant temporal_subclass_super as crate::object::global_this::fetch_globals::temporal_subclass_super
participant temporal_constructor
participant attach_temporal_cell_to_this
js_fetch_or_value_super->>temporal_subclass_super: handle Temporal subclass super()
temporal_subclass_super->>temporal_constructor: invoke Temporal parent constructor
temporal_constructor-->>temporal_subclass_super: returned cell
temporal_subclass_super->>attach_temporal_cell_to_this: stash cell on subclass this
attach_temporal_cell_to_this-->>js_fetch_or_value_super: handled
sequenceDiagram
participant get_field_by_name_object_tail
participant temporal_subclass_cell
participant has_method as crate::temporal::dispatch::has_method
participant js_class_method_bind
get_field_by_name_object_tail->>temporal_subclass_cell: read subclass cell
temporal_subclass_cell-->>get_field_by_name_object_tail: Option<f64>
get_field_by_name_object_tail->>has_method: check Temporal method name
has_method-->>get_field_by_name_object_tail: true / false
get_field_by_name_object_tail->>js_class_method_bind: bind method name
js_class_method_bind-->>get_field_by_name_object_tail: callable
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/object/global_this/fetch_globals.rs`:
- Around line 477-486: The non-spread Temporal superclass path in
fetch_globals.rs is only checking the immediate parent_val, so aliased or stale
Temporal parents can bypass temporal_subclass_super and fall through to the
generic super handling. Update the branch in fetch_globals::global_this to
recover the dynamic parent from the subclass instance’s class id first,
mirroring the spread-path alias recovery, then pass that resolved parent into
temporal_subclass_super so Temporal new.target and stash handling are preserved
for aliased heritage values.
In `@crates/perry-runtime/src/object/instanceof.rs`:
- Around line 121-129: The Temporal brand check in instanceof logic only
recognizes the 0x7FFD pointer-tagged receiver form, so module-scoped raw I64
heap objects can incorrectly fail the subclass check. Update the receiver
handling in the instanceof path around the
value.to_bits()/temporal_subclass_cell flow to also accept the top16 == 0
raw-I64 representation, then perform the same temporal_kind(cell) comparison for
that case so Temporal.<Type> subclass instances are branded correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c93270b-4c3f-4ccd-a1be-a8c03a9efe90
📒 Files selected for processing (12)
crates/perry-runtime/src/object/class_constructors.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/global_this/fetch_globals.rscrates/perry-runtime/src/object/global_this/math_temporal.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/object_ops/prototype.rscrates/perry-runtime/src/temporal/dispatch.rscrates/perry/tests/issue_5587_temporal_subclass.rs
…rs) + rustfmt/clippy - fetch_globals: non-spread `super()` now recovers the Temporal parent from the decl-time class-id stash when the immediate heritage value is a stale alias (`const D = Temporal.Duration; class X extends D`), mirroring the Request/Response recovery. (CodeRabbit major) - instanceof: the Temporal subclass-brand check now also accepts the raw-I64 receiver form (top16 == 0), how module-level object vars are stored, not just the NaN-boxed 0x7FFD form. (CodeRabbit minor) - has_method: `matches!(x, Some(_))` → `.is_some()`; rustfmt. - New regression test for the aliased-heritage path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merged
7 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
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.
What
Fixes the dominant cluster behind #5587:
built-ins/Temporal/**/subclassing-ignored.js(36TypeError: value is not a functionfailures).class X extends Temporal.Duration { constructor(){ super(...args) } }produced an empty plain object.super()ran the native Temporal constructor — which returns a fresh NaN-boxed cell rather than mutating the implicitthis— and discarded the returned cell. The subclass instance had no Temporal brand, soinstance.abs()resolvedabstoundefinedand threw.How
Mirrors the existing
class X extends Request/Responsefetch-handle pattern: when asuper()parent resolves to a Temporal constructor, run it and stash the returned cell onthisunder__perry_temporal_cell__. Bothsuper()lowerings are covered:js_fetch_or_value_super— non-spreadsuper(a, b)js_super_construct_apply—super(...spread)(the form the test262 helper uses), via the decl-time-recorded dynamic parent valueMethod-call (
native_call_method), property-get (get_field_by_name_tail), andinstanceofdispatch recover the cell from there.Also fixes the surrounding real-cell gaps the same tests exercise:
Object.getPrototypeOf(temporalCell)returnednull; now resolvesTemporal.<Type>.prototypevia the live namespace, soassert.sameValue(Object.getPrototypeOf(result), construct.prototype)holds.d.abs, notd.abs()) returnedundefined, breaking theinstance[method](...spread)read+apply form; now returns a bound method, gated on a real per-kind method predicate so unknown properties still read asundefined. Applies to real cells and subclass instances.PlainDate/PlainTime/Instant/…) threw "requires 'new'" when invoked fromsuper();new.targetis now set for the call.Verification
crates/perry/tests/issue_5587_temporal_subclass.rs(2 tests, top-level subclasses ofTemporal.DurationandTemporal.PlainDate) — pass.issue_4908_subclass_native_member_base,issue_array_subclass_super_init) — pass (no regression).instanceof, accessor getters, methods (fused / computed / spread / read-as-value), andgetPrototypeOf.Known remaining limitation (out of scope)
A class method/constructor defined inside a function captures an enclosing
letby value, not by cell — so the test262 helper'sassert.sameValue(called, 1)still fails for the in-function subclasses it builds. This is a general class-semantics bug (reproduces with a plain user-class subclass —let c=0; class K extends B { constructor(){ c++; super() } }; new K()leavesc === 0), unrelated to Temporal, and deserves its own fix. With this PR, the subclassing-ignored cases now run all the way through the Temporal logic and fail only on thatcalledassertion (previously they threwvalue is not a functionimmediately).🤖 Generated with Claude Code
Summary by CodeRabbit
super()calls, inherited prototype resolution,instanceof, and value-based method access.Temporal.DurationandTemporal.PlainDatesubclasses, including an aliasingsuper(...)edge case.