fix(calendar): replace add-mo panic with ILO-R009 on out-of-range#561
Merged
Conversation
The internal `date.year() * 12 + months` calculation in add_months_snap was i32 and overflowed when `months` approached i32::MAX or i32::MIN. In debug builds this panicked with 'attempt to add with overflow'; in release it silently wrapped to a valid (but wrong) NaiveDate, so the calendar-range guard never tripped. Widen the intermediate to i64 and narrow back via i32::try_from when constructing the chrono date. Out-of-range results now surface as the documented ILO-R009 'add-mo: result out of calendar range' via the existing None arm.
The existing add_mo_result_out_of_calendar_range test only exercised the positive i32::MAX path. Add: - add_mo_result_out_of_calendar_range_negative: symmetric i32::MIN case so an asymmetric fix can't slip through. - add_mo_large_but_valid_offset: 1000-year (12000-month) forward jump to guard against the overflow fix being too aggressive and rejecting plausible long offsets. Plus an examples/add-mo-out-of-range.ilo demonstrating the long-offset case across engines via the examples_engines harness.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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
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
add-mowas crashing the interpreter with a Rust panic in debug builds (and silently producing wrong dates in release) when called with extreme month offsets likeadd-mo 0 2147483647. The bug landed with #495 (calendar arithmetic). The runtime should surface ILO-R009 cleanly so agents can recover, not panic — and never silently return the wrong date in release.Root cause
src/interpreter/mod.rs::add_months_snapdid its year arithmetic in i32:With
months = i32::MAX,1970 * 12 + 2147483647overflows i32. Debug panics withattempt to add with overflow; release silently wraps to a valid (but wrong)NaiveDate, so thefrom_ymd_optguard never tripped and the documented "result out of calendar range" error was unreachable.The dedicated regression test
add_mo_result_out_of_calendar_rangewas passing in release (silent wrap landed in chrono's range by luck) and crashing in debug.Repro
After the fix:
What's in the diff
fix(calendar): widen add-mo month arithmetic to i64— computetotalmonths in i64, narrow to i32 viatry_fromwhen constructing the date. Overflow now propagates cleanly to the existingNone → ILO-R009arm.test(calendar): pin add-mo overflow boundaries + large-but-valid offsets— adds symmetrici32::MINnegative case, a 1000-year-forward "still valid" guard so the fix doesn't over-reject, and anexamples/add-mo-out-of-range.iloexercising the same path through the examples_engines harness.VM and Cranelift route through the tree-bridge for
add-mo, so a single fix in the interpreter covers all three engines.Test plan
cargo test --features cranelift --test regression_calendar_arithmetic— 36/36 in debugcargo test --release --features cranelift --test regression_calendar_arithmetic— 36/36 in releasecargo fmt --checkcleanilo runand the examples_engines harnessFollow-ups
None directly tied to this fix. Pre-existing clippy + JIT test failures on
main(theOP_TAILCALLmatch-arm warnings and variousvm::jit_cranelift::testscases) are not from this change and will need separate attention.