More documentation for min_months_from_inner by Manishearth · Pull Request #7746 · unicode-org/icu4x

Manishearth · 2026-03-06T22:51:39Z

Improving docs from #7739

Changelog: N/A

components/calendar/src/cal/east_asian_traditional.rs

components/calendar/src/calendar_arithmetic.rs

components/calendar/src/cal/hebrew.rs

robertbastian · 2026-03-09T10:14:12Z

components/calendar/src/cal/hebrew.rs

+        // The Hebrew Metonic cycle has leap years in year 3, 6, 8, 11, 14, 17, and 19,
+        // i.e., leap year gaps of +3, +3, +2, +3, +3, +3, +2.


be explicit about the boundary, either add year 0 or year 22

Year 19 is the boundary. This is every 19 years.

components/calendar/src/cal/hebrew.rs

poulsbo

@robertbastian should your comments re: the EastAsianTraditional calendar $12y + floor(y/3)$ be included into the source code comment, in the TODO block starting on line 598 of east_asian_traditional.rs?

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

robertbastian · 2026-03-09T20:00:01Z

components/calendar/src/cal/east_asian_traditional.rs

 ///
 /// This crate currently provides [`Rules`] for [`China`] and [`Korea`].
 ///
+/// Currently, implementations of this trait must produce calendars that


ok, now show that China and Korea obey this invariant

No.

I am mostly convinced the code currently upholds this property. I am not convinced enough to "prove" it, I am convinced from a read through of the code, enough that I am satisfied with landing that code.

My original plan was to properly sit down and figure out China and Korea (such that there is a constant time error). I haven't had time to do that (and didn't plan to do that for 2.2), but Alan opened his PR in the meantime. I am doing followups for his PR because I have thought about the Hebrew stuff and wanted to write it down. Me choosing to do those followups is not me signing up for more work here, if you want to disagree with that PR feel free to disagree with it and discuss further, or open a PR to undo the Chinese part of it.

I already recommended another route above: instead of debug asserting after computing min_months, we can set min_months to sign when that assertion fails. That is the only thing that needs to change to turn this from a hard requirement into a soft "optimal behavior" one. If you care about this, go ahead and do that.

Well you merged Alan's PR before it was thoroughly reviewed, so you're kind of on the hook for this now.

Now you're adding an invariant to the type without showing that it holds. The safe thing to do would be to remove the invariant, go back to returning 12 * years and figure this out later.

I already recommended another route above: instead of debug asserting after computing min_months, we can set min_months to sign when that assertion fails. That is the only thing that needs to change to turn this from a hard requirement into a soft "optimal behavior" one.

Cool, we could have discussed that on the PR that added this code. I don't want to do this work either, so let's revert #7739.

I am fine doing small changes here. Please do not ask me to do large changes as a part of a followup PR that I am making as a courtesy.

I'm quite confident that Pinqi follows this invariant, because it adds a leap month with an exact period, I think every 33 months.

I'm quite confident that Pinqi follows this invariant, because it adds a leap month with an exact period, I think every 33 months.

Please don't call the simple approximation Pinqi, it's not that because it's based on the Gregorian year.

I'm quite confident too, but I don't want to find out when Temporal is experiencing panics in half a year. We use several approximations in that code.

components/calendar/src/cal/east_asian_traditional.rs

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

sffc

This improves the status quo by documenting the invariant. We should also test the invariant against built-in calendars, and the debug assertions should catch it for user calendars.

sffc

No, the last commit pushed a few seconds ago regresses performance relative to what landed on main.

robertbastian · 2026-03-09T20:42:25Z

No, the last commit pushed a few seconds ago regresses performance relative to what landed on main.

Yes but it's provably correct. Please let's not go back to yoloing code with debug assertions, that has been painful enough to clean up in the past.

Manishearth · 2026-03-09T20:42:47Z

I am going to undo my undoing so that this PR is purely a docs fix. We should land this, and we can separately discuss the chinese invariant. I'll make a separate PR for that.

I am rather frustrated with this ping ponging on a small courtesy PR.

Manishearth · 2026-03-09T20:43:08Z

Yes but it's provably correct. Please let's not go back to yoloing code with debug assertions, that has been painful enough to clean up in the past.

And it's scope creep for this PR.

Manishearth · 2026-03-09T20:43:51Z

Especially since the fix to avoid the debug assertions is not that big a deal.

robertbastian · 2026-03-09T20:47:17Z

I am rather frustrated with this ping ponging on a small courtesy PR.

I'm rather frustrated with #7739 being merged without my review. When I left work on Friday that PR was still actively being worked on, and I had drafted review comments. Now I'm the bad guy for asking for these changes to be reverted.

no

Manishearth · 2026-03-09T20:49:44Z

Now I'm the bad guy for asking for these changes to be reverted.

No. You could still post the review comments.

I'm just frustrated that I'm being asked to fix these in this PR.

robertbastian · 2026-03-09T20:53:09Z

I'm just frustrated that I'm being asked to fix these in this PR.

You're adding invariants that might not actually be true, thereby legitimizing problematic code. I'm happy to approve this without the east asian changes.

Manishearth · 2026-03-09T20:56:30Z

You're adding invariants that might not actually be true, thereby legitimizing problematic code.

You said: "We either need to add a requirement that the rules insert a leap month at least every three years".

I followed that feedback. Turned out you wanted the associated proof as well, which I do not have right now. Okay, fine, I'll undo that change there.

robertbastian

Turned out you wanted the associated proof as well

Yes I generally expect that when adding an invariant.

sffc · 2026-03-09T21:02:21Z

I think the version of the PR that documented the invariant was better, because we should write down our assumptions, but the PR here is still an improvement over the status quo so I approve

Improving docs from unicode-org#7739 --------- Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

More documentation for min_months_from_inner

459c94a

Manishearth requested a review from sffc as a code owner March 6, 2026 22:51

Manishearth mentioned this pull request Mar 6, 2026

Improve the performance of ArithmeticDate.until() #7739

Merged

Manishearth requested review from poulsbo and robertbastian March 7, 2026 03:07

robertbastian reviewed Mar 9, 2026

View reviewed changes

poulsbo approved these changes Mar 9, 2026

View reviewed changes

Manishearth and others added 6 commits March 9, 2026 10:52

Update components/calendar/src/calendar_arithmetic.rs

a311bcb

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Update components/calendar/src/calendar_arithmetic.rs

68b22b7

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Update components/calendar/src/calendar_arithmetic.rs

c9fb1c5

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Update components/calendar/src/cal/hebrew.rs

157cedb

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Update components/calendar/src/cal/hebrew.rs

72976d7

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Update components/calendar/src/cal/hebrew.rs

c209736

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Manishearth force-pushed the min-months-docs branch 2 times, most recently from 5530d53 to a60b9bc Compare March 9, 2026 17:55

approx

1374201

Manishearth force-pushed the min-months-docs branch from a60b9bc to 1374201 Compare March 9, 2026 17:56

Manishearth added 2 commits March 9, 2026 11:05

Comment on EAT about leap year cadence

d7dfacb

fractions

73673f0

robertbastian reviewed Mar 9, 2026

View reviewed changes

Update components/calendar/src/cal/east_asian_traditional.rs

8aade84

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Manishearth requested a review from robertbastian March 9, 2026 20:11

Manishearth added 2 commits March 9, 2026 13:12

fmt

132791d

undo chinese

f004be7

sffc approved these changes Mar 9, 2026

View reviewed changes

sffc requested changes Mar 9, 2026

View reviewed changes

undo undo

136d206

robertbastian previously approved these changes Mar 9, 2026

View reviewed changes

remove documented invariant

f29a76e

robertbastian approved these changes Mar 9, 2026

View reviewed changes

sffc approved these changes Mar 9, 2026

View reviewed changes

Manishearth merged commit 392546a into unicode-org:main Mar 9, 2026
32 of 33 checks passed

Manishearth deleted the min-months-docs branch March 9, 2026 21:10

sffc mentioned this pull request Mar 10, 2026

Create testing_and_documentation.md #7754

Merged

poulsbo pushed a commit to poulsbo/icu4x that referenced this pull request Mar 10, 2026

More documentation for min_months_from_inner (unicode-org#7746)

7689416

Improving docs from unicode-org#7739 --------- Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

		// The Hebrew Metonic cycle has leap years in year 3, 6, 8, 11, 14, 17, and 19,
		// i.e., leap year gaps of +3, +3, +2, +3, +3, +3, +2.

Conversation

Manishearth commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog: N/A

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertbastian Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Manishearth Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poulsbo left a comment

Choose a reason for hiding this comment

Uh oh!

robertbastian Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Manishearth Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbastian Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Manishearth Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

sffc Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

robertbastian Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

sffc left a comment

Choose a reason for hiding this comment

Uh oh!

robertbastian commented Mar 9, 2026

Uh oh!

Manishearth commented Mar 9, 2026

Uh oh!

Manishearth commented Mar 9, 2026

Uh oh!

Manishearth commented Mar 9, 2026

Uh oh!

robertbastian commented Mar 9, 2026

Uh oh!

Manishearth commented Mar 9, 2026

Uh oh!

robertbastian commented Mar 9, 2026

Uh oh!

Manishearth commented Mar 9, 2026

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Mar 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Manishearth commented Mar 6, 2026 •

edited

Loading

Manishearth Mar 9, 2026 •

edited

Loading