Skip to content

refactor(temporal): extract this-type-check boilerplate into a macro#5233

Open
Nakshatra480 wants to merge 1 commit intoboa-dev:mainfrom
Nakshatra480:refactor/temporal-this-type-check
Open

refactor(temporal): extract this-type-check boilerplate into a macro#5233
Nakshatra480 wants to merge 1 commit intoboa-dev:mainfrom
Nakshatra480:refactor/temporal-this-type-check

Conversation

@Nakshatra480
Copy link
Contributor

@Nakshatra480 Nakshatra480 commented Mar 23, 2026

Summary

This PR removes the repetitive this-type-check boilerplate present in almost every Temporal prototype method.

Previously, each method manually called this.as_object(), downcasted with .as_ref().and_then(JsObject::downcast_ref::<Self>), and threw a TypeError on failure - 5 lines repeated 158 times across 8 files.

I've extracted this into a single this_temporal_object! macro in temporal/mod.rs. A helper function isn't viable here because this.as_object() returns an owned JsObject that the resulting Ref<T> borrows from - the owner must live in the caller's scope. The macro solves this by emitting the let bindings directly there.

Changes

  • Replaced 158 occurrences of the type-check boilerplate across 8 Temporal modules with the new macro
  • Unified all TypeError messages to "the this object must be a {Type} object." - a few types like Duration previously used inconsistent phrasing
  • Net delta: +183 / -1,106 lines

N downcast_ref::<Self> calls in static ::from() methods are deliberately left untouched - those check item, not this.

@github-actions github-actions bot added Waiting On Review Waiting on reviews from the maintainers C-Builtins PRs and Issues related to builtins/intrinsics and removed Waiting On Review Waiting on reviews from the maintainers labels Mar 23, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 23, 2026
…oral_object! macro

Replace 163 occurrences of the 5-line this-type-check boilerplate across
8 Temporal builtin files with a single this_temporal_object! macro defined
in temporal/mod.rs.

The macro produces two let-bindings in the caller's scope: one for the
owned JsObject (to keep it alive) and one for the downcast Ref<T>.

Error messages standardized to: 'the this object must be a {Type} object.'

Files changed:
- temporal/mod.rs: macro definition
- zoneddatetime/mod.rs: 47 replacements
- plain_date_time/mod.rs: 37 replacements
- plain_time/mod.rs: 16 replacements
- plain_year_month/mod.rs: 15 replacements
- plain_date/mod.rs: 15 replacements
- duration/mod.rs: 13 replacements
- instant/mod.rs: 12 replacements
- plain_month_day/mod.rs: 8 replacements

Net delta: +188 / -1145 lines
@Nakshatra480 Nakshatra480 force-pushed the refactor/temporal-this-type-check branch from 4053092 to 4c42492 Compare March 23, 2026 05:48
@github-actions github-actions bot added the Waiting On Review Waiting on reviews from the maintainers label Mar 23, 2026
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,545 50,545 0
Ignored 1,426 1,426 0
Failed 992 992 0
Panics 2 2 0
Conformance 95.43% 95.43% 0.00%

Tested main commit: 66a1cd268ce6006918032b5d0f98d3a63c81b188
Tested PR commit: 4c42492837b7282122842a2e1773b429a53ef39a
Compare commits: 66a1cd2...4c42492

@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 3.03030% with 160 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.44%. Comparing base (6ddc2b4) to head (4c42492).
⚠️ Report is 916 commits behind head on main.

Files with missing lines Patch % Lines
.../engine/src/builtins/temporal/zoneddatetime/mod.rs 0.00% 47 Missing ⚠️
...ngine/src/builtins/temporal/plain_date_time/mod.rs 2.70% 36 Missing ⚠️
...ore/engine/src/builtins/temporal/plain_date/mod.rs 0.00% 15 Missing ⚠️
...ore/engine/src/builtins/temporal/plain_time/mod.rs 6.25% 15 Missing ⚠️
...gine/src/builtins/temporal/plain_year_month/mod.rs 0.00% 15 Missing ⚠️
core/engine/src/builtins/temporal/instant/mod.rs 0.00% 12 Missing ⚠️
core/engine/src/builtins/temporal/duration/mod.rs 23.07% 10 Missing ⚠️
...ngine/src/builtins/temporal/plain_month_day/mod.rs 0.00% 8 Missing ⚠️
core/engine/src/builtins/temporal/mod.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5233       +/-   ##
===========================================
+ Coverage   47.24%   60.44%   +13.19%     
===========================================
  Files         476      582      +106     
  Lines       46892    62806    +15914     
===========================================
+ Hits        22154    37960    +15806     
- Misses      24738    24846      +108     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Builtins PRs and Issues related to builtins/intrinsics Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant