fix(dayjs): arguments in seq exprs + computed Date-method dispatch (#5133)#5147
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughVersion bumped from Changesdayjs compatibility fixes and version bump
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/object/native_call_method.rs (1)
2074-2099:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDate receiver path needs a terminal fallback to avoid unsafe fallthrough
At Line 2074, this branch documents that
DateCellmust not hit generic object dispatch, but when a method is not found (and not"toString"), execution currently falls through anyway. That reopens the same invalidDateCell→ObjectHeaderreinterpretation risk this block is meant to prevent.Suggested fix
if crate::date::is_date_value(object) { let ctor = crate::object::js_get_global_this_builtin_value(b"Date".as_ptr(), 4); let ctor_ptr = crate::value::js_nanbox_get_pointer(ctor) as usize; if ctor_ptr != 0 { let proto = crate::closure::closure_get_dynamic_prop(ctor_ptr, "prototype"); if let Some(proto_ptr) = object_ptr_from_value(proto) { let key = crate::string::js_string_from_bytes( method_name_ptr as *const u8, method_name_len as u32, ); let value = crate::object::js_object_get_field_by_name(proto_ptr, key); if !value.is_undefined() { let value_f64 = f64::from_bits(value.bits()); let prev_this = IMPLICIT_THIS.with(|c| c.replace(object.to_bits())); let result = crate::closure::js_native_call_value(value_f64, args_ptr, args_len); IMPLICIT_THIS.with(|c| c.set(prev_this)); return result; } } } if method_name == "toString" { let string = crate::date::js_date_to_string(object); return f64::from_bits(JSValue::string_ptr(string).bits()); } + crate::error::js_throw_type_error_not_a_function( + std::ptr::null(), + 0, + method_name.as_ptr(), + method_name.len(), + ); }🤖 Prompt for 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. In `@crates/perry-runtime/src/object/native_call_method.rs` around lines 2074 - 2099, The Date receiver branch starting with the `is_date_value(object)` check needs a terminal fallback to prevent unsafe execution fallthrough. When a Date method is not found in the prototype and is not "toString", the code currently falls through to generic object dispatch, which would unsafely reinterpret a DateCell as an ObjectHeader. Add an explicit fallback handler (such as returning an undefined value or error) at the end of the Date branch, after the toString check, to ensure that Date objects never reach the generic object dispatch path when their requested method is not available.
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/object/native_call_method.rs`:
- Around line 2074-2099: The Date receiver branch starting with the
`is_date_value(object)` check needs a terminal fallback to prevent unsafe
execution fallthrough. When a Date method is not found in the prototype and is
not "toString", the code currently falls through to generic object dispatch,
which would unsafely reinterpret a DateCell as an ObjectHeader. Add an explicit
fallback handler (such as returning an undefined value or error) at the end of
the Date branch, after the toString check, to ensure that Date objects never
reach the generic object dispatch path when their requested method is not
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d0448a3-dddd-4893-957d-1c5029b9fba9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdCLAUDE.mdCargo.tomlcrates/perry-hir/src/lower_decl/helpers.rscrates/perry-runtime/src/object/native_call_method.rs
…5133) - expr_uses_arguments descends into Seq/Await/Yield/OptChain/computed SuperProp/TaggedTpl so synthetic-arguments pre-scan catches minified dayjs wrappers (return n.date=t, n.args=arguments, ...). - Route any method on a date receiver through Date.prototype (not just toString) so computed/dynamic calls (date[m](...)) no longer drop setter mutations. v0.5.1171
proggeramlug
force-pushed
the
fix/dayjs-arguments-seq
branch
from
31dd2d1 to
37e22f0
Compare
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.
Fixes #5133.
compilePackages: dayjsthrewReferenceError: arguments is not defined, and once that was resolved.add()/.date(n)silently no-op'd. Two distinct, general root causes:1.
argumentshidden inside a sequence expressionMinified dayjs builds the wrapper as:
The
argumentsreference lives inside a comma/sequence expression in thereturn. The synthetic-arguments pre-scanexpr_uses_arguments(crates/perry-hir/src/lower_decl/helpers.rs) had noExpr::Seqarm, fell through its_ => falsecatch-all, so the enclosing function never synthesized its hidden raw-argumentsparam andExpr::Ident("arguments")resolved to nothing.Fix: descend into
Expr::Seq, plus the other operand-bearing forms the catch-all skipped —Await,Yield,OptChain, computedSuperProp,TaggedTpl.2. Computed/dynamic Date-method calls dropped to generic dispatch
dayjs mutates dates via
this.$d[l]($)(e.g.date["setDate"](44)). ADateCellis a NaN-boxed pointer but not anObjectHeader;js_native_call_methodspecial-cased onlytoStringfor date receivers, so every other computed/dynamic call fell through to generic object dispatch, returned[object Object]and dropped the mutation. Direct.setDate(44)worked (codegen fast path), butdate[m](44)did not.Fix: route any method on a date receiver through
Date.prototype(where all getter/setter/toISOString/toJSONthunks are installed and readIMPLICIT_THIS), withthisbound to the cell.Validation
Issue repro now matches Node byte-for-byte:
-p perry-runtimedate tests: 23 passed / 0 failed.Known residual
A SIGSEGV in dayjs's month arithmetic (
.endOf('month')/.set('month', …)— a number-as-pointer deref injs_object_get_field_by_name, unrelated to either fix here) is left for a follow-up.Summary by CodeRabbit
argumentsusage inside complex expression forms (sequence/comma,await/yield, optional chaining, computedsuper, tagged templates) to ensure it’s recognized reliably.Datemethod calls (e.g.,date["setDate"](...)) so the correct receiver is used and mutations are no longer dropped; also improvedtoStringdispatch.