Implement AnyCalendar using a macro that improves robustness

[sffc]

This was pulled out of #7012. I considered that PR to be pushing AnyCalendar complexity over the edge to justify a new macro, but @robertbastian asked for it to be considered separately. I disagree with a process that forces authors to split stylistic changes out of otherwise-mergeable PRs, especially given time zone differences, but let's discuss the process in another thread.

Examples of bugs this can prevent:

1. Consistently applying the Hijri tabular algorithm equality check in the match
2. Matching on the same calendar identity between AnyCalendar and DateInner (for example, avoiding Buddhist AnyCalendar with Coptic DateInner)
3. Preventing calling a function with the same return type as the main function. For example, days_in_month and months_in_year have the same signature, so a compiler error wouldn't catch when they are mixed up.

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

[sffc]

In the other thread @robertbastian said

It's the same situation as the macro overload in the datetime crate.

I've seen you tweak the datetime crate and what would be a 100-line bug-prone change becomes a 3-line change with the macro. I used to dislike complex macros, too, but the extra time it takes for a developer to learn the macro is time saved in code review and bug reports.

[sffc]

This particular macro is not nearly as complex as the datetime macro, and based on @Manishearth's consistent nudging to document internal APIs, I think it's not hard to learn to use, either.

The macro could be made substantially simpler if Rust ever allows macros to generate match arms.

[sffc]

/gemini review, wdyt?

[Copilot]

Pull Request Overview

This PR refactors the AnyCalendar implementation by introducing a generalized macro system that improves code maintainability and robustness. The change replaces repetitive manual match statements with a flexible macro-based approach that reduces duplication and potential errors.

• Replaces manual match statements with a generalized match_cal_general macro system
• Introduces helper macros (pattern_cal, pattern_cal_date, pattern_cal_date_date) for pattern matching
• Removes unused DEBUG_NAME constants and related debug functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
components/calendar/src/any_calendar.rs	Implements new macro system, refactors Calendar trait methods to use macros, removes debug helper functions
components/calendar/src/cal/japanese.rs	Removes unused DEBUG_NAME constants for Japanese and JapaneseExtended calendars

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The pattern matching on Some(Err(_)) is missing, which could cause a panic if the until method returns an error. Add a case to handle Some(Err(e)) => Err(e.into()).

Suggested change

	Some(Ok(result)) => Ok(result),
	Some(Ok(result)) => Ok(result),
	Some(Err(e)) => Err(e.into()),

[sffc]

no copilot, the Err variant is uninhabited

[gemini-code-assist]

Code Review

This pull request introduces a powerful new macro, match_cal_general!, to significantly reduce boilerplate and improve the robustness of AnyCalendar's implementation. The refactoring is a great improvement, making the code more concise and maintainable.

I've found a critical issue that will prevent compilation due to a non-exhaustive match in the until method. I've also identified a couple of medium-severity maintainability issues: an unclear identifier in one of the new macros and some inaccuracies in the documentation for match_cal_general!.

Once these issues are addressed, this will be a solid improvement to the codebase.

[gemini-code-assist]

This match statement is not exhaustive. The result variable has the type Option<Result<types::DateDuration, Infallible>>, but there is no arm to handle the Some(Err(_)) case. While the error type is Infallible, meaning this arm is unreachable, the compiler requires it to be handled explicitly. This will cause a compilation error.

You can add an arm to handle this case, for example by matching on the Infallible error.

        match result {
            Some(Ok(result)) => Ok(result),
            Some(Err(never)) => match never {},
            None => Err(AnyCalendarDifferenceError::MismatchedCalendars),
        }

[sffc]

No Gemini, the Err variant is uninhabited. The code wouldn't compile otherwise.

[gemini-code-assist]

The documentation for parameters 2 and 3 of the match_cal_general! macro seems to be incorrect. It states that the first argument to the $pattern and $expr macros is $cal, but it's actually the calendar variant identifier (e.g., Buddhist).

Additionally, the description for parameter 3 is a bit confusing. It could be clarified that the $expr macro receives the same arguments as $pattern but without the algorithm identifiers for HijriTabular.

Updating the documentation would improve clarity for future maintainers of this complex but powerful macro.

/// 2. `$pattern`: The name of a macro that generates a pattern for the match statement. It is passed as arguments:
///     - The calendar variant ident (e.g. `Buddhist`).
///     - An ident to be used for matching the `AnyCalendar` variant, `c`.
///     - A list of identifiers for `AnyDateInner` variants. For `HijriTabular`, this list also includes identifiers for the algorithm.
/// 3. `$expr`: The name of a macro that generates an expression for the match statement. It is passed as arguments:
///     - The calendar variant ident (e.g. `Buddhist`).
///     - An ident for the `AnyCalendar` variant, `c`.
///     - A list of identifiers for `AnyDateInner` variants.

[sffc]

Good suggestion

[gemini-code-assist]

The identifier blah is used here for the Hijri algorithm parameter. This is a poor choice for a variable name as it's typically used for placeholder code and can be confusing for future readers. In other parts of this file, like in the add and until methods, alg or alg1/alg2 are used for this purpose. For consistency and clarity, please rename blah to alg.

        match_cal_general!($cal, pattern_cal_date, helper, [d, alg], {

[sffc]

Good suggestion

[sffc]

The refactoring is a great improvement, making the code more concise and maintainable.

Once these issues are addressed, this will be a solid improvement to the codebase.

Gemini wants to be my friend, clearly. I wonder if it would say the same thing for the opposite PR. We can test it later.

[robertbastian]

I've spent a good 10 minutes trying to understand how this works and have given up. This change is an immense readability hurdle; this code should be kept simple enough for any contributor to update.

I also don't see the type of bug that you're saying this avoids. Please update the PR description with why this PR is justified, not just where this code was previously discussed.

[Manishearth]

I plan to look at this, but not until this afternoon unfortunately.

[Manishearth]

So I've been thinking about this.

We basically have a macro complexity-utility tradeoff.

Most of our macros, including the current-on-main AnyCalendar match macro have very little complexity: they are basically just "copy paste this template" potentially with some very minor bells and whistles. Those tend to be easy to accept.

Beyond such simple macros, I think "complexity" comes in a couple flavors (labels are not intended to be precise):

• Use-site comprehension: Is it hard to understand what the macro is doing when you see a use site? This can be ameliorated by docs. Complex macros should pair with a sufficient amount of docs so that someone does not need to reach into the macro def to understand a use site.
• Def-site comprehension (Debuggability): Is it hard to understand what the macro is doing, regardless of docs, because you are tracking down a bug and need to reach into the macro guts?
• Reuse: Is it hard to reuse the macro in a new situation? It should not be the case that someone intending to use the macro needs to dig into the macro implementation to figure out how to use it.
• Upgradability: Is it possible to tweak the macro when requirements change? This depends on how the requirements may change, too. Maybe the macro implements a trait that will eventually get more methods. Maybe the macro produces a match statement that will eventually get more arms. Maybe we don't expect the macro to ever be edited again. Judging this requires having a good ability to predict that.

Utility also comes in a couple flavors:

• Boilerplate reduction: Large piles of repetitive code are just generally dispreferred. Getting rid of that is great. Most macros do this.
• Readability: A macro can make use sites much more readable: sometimes the important bits can often be hidden behind all the boilerplate, and it's hard to see what is and isn't really key to a particular chunk of code. A macro can be used to bring that out.
• Mistake prevention: Centralizing things in a macro makes it easier to avoid copy-paste errors and other such things.

Okay, so let's look at, say, the field_type! (use) macro.

• Use-site comprehension: Okayish: you can basically guess what is happening here by looking at the use site, but ideally field_type!() would have more docs that help here.
• Def-site comprehension: It's got some messy bits but it's mostly just a template.
• Reuse: Super easy to reuse. Wish it had docs, again, but it's easy enough to tease out what it means
• Upgradability: This macro has been refactored a million times and has changed the precise boilerplate it generates a bunch. Pretty straightforward

And in the utility column, it ticks all of the boxes too: the short macro call makes it easy to see what's important (Readability), and probably prevents mistakes (Mistake prevention).

What about a more complex macro, impl_date_marker! (use)? This does a lot of stuff

• Use-site comprehension: Okayish. It's hard to know what exactly is happening uder the hood. This would be ameliorated by docs. It's mostly possible to squint at a couple use sites and figure out what you need to do.
• Def-site comprehension: Not great. It's got a lot of "optional" options that are messy to handle in macros.
• Reuse: Okayish, again ameliorated by docs. Copying an existing use and tweaking it works well.
• Upgradability: Unsure! It's mostly a pile of more macros, which have similar complexity. I think you would have to spend some time looking through this macro to make most of the types of changes we can expect here.

And utility wise, it's reducing a lot of boilerplate, and is amazing at bringing the important bits to the forefront both for readability and mistake prevention.

---

Alright, let's apply that here. Note that I'm not trying to come up with a rubric that "gives us an answer", I'm just trying to come up with a rubric that brings to the forefront the key points about a particular macro to aid this discussion.

• Use-site comprehension: The shortcut macros are quite easy to understand, and again some docs on them would make this even better. match_cal_general is decently documented.
• Def-site comprehension: Not great. It's a higher-order macro which already complicates things
• Reuse: Super easy to reuse. Copy paste a use site.
• Upgradability: The most common upgrade will be adding a calendar, which is easy. There's a chance we will need matches that have natures not supported by this macro, that would be much harder, but I also don't really think that's likely.

And from the utility column, it's again reducing a lot of boilerplate. However I'm not sure if there's much of a readability delta: the code it's replacing is, to me, not super hard to understand at a glance. The thing I mentioned before of using rustfmt::skip so we're not wrapping overlong lines would also be sufficient to fix that problem for me.

I think it has a slight benefit in Mistake prevention since it would prevent getting the match arms wrong, but for only two additional callsites compared to the status quo I'm not too worried, and Rust forces us to add the right match arms anyway because of crate-internal exhaustiveness. There's also a slight benefit with making sure everyone handles the HijriTabular sighting type.

With this framing, personally I think I see this macro as having a fair amount of additional complexity without much additional utility. I weakly believe that the utility does not justify the added complexity.

I appreciate being able to review this as a separate change, it's easier to see the now-more-advanced add/until code with and without the macro.

[sffc]

I've spent a good 10 minutes trying to understand how this works and have given up. This change is an immense readability hurdle; this code should be kept simple enough for any contributor to update.

My expectation was 3-5 minutes to learn how to use the macro. The macro is intended to be general enough that you won't normally need to modify it. If you do need to modify it, 10-15 minutes would be my expectation.

For example, I wouldn't dare to understand how std::format! works under the hood, but it is easy for me to use, given sufficient documentation.

I also don't see the type of bug that you're saying this avoids. Please update the PR description with why this PR is justified, not just where this code was previously discussed.

Added.

We basically have a macro complexity-utility tradeoff.

Yes, I agree with @Manishearth's characterizations.

[sffc]

Also, we should normalize passing macros as arguments to other macros. It's a powerful technique that, while not the first thing you learn in Rust, greatly improves macro brevity and readability for those who are similarly initiated. We shouldn't discourage its use due to unfamiliarity.

(no promises, but I have that topic flagged as material for a blog post.)

[Manishearth]

this is a test

[Manishearth]

Also, we should normalize passing macros as arguments to other macros. It's a powerful technique that, while not the first thing you learn in Rust, greatly improves macro brevity and readability for those who are similarly initiated. We shouldn't discourage its use due to unfamiliarity.

I think it's often hard to keep track of the "signature" of macros in your head, and higher-order macros make this worse since you now need to keep track of that signature while keeping track of other stuff.

It's still a powerful and useful trick, though.

[robertbastian]

Also, we should normalize passing macros as arguments to other macros.

No we should not. It requires uncoditional trust in the macro, because it's impossible to follow what's happening.

I think it's often hard to keep track of the "signature" of macros in your head, and higher-order macros make this worse since you now need to keep track of that signature while keeping track of other stuff.

Many macros don't even have a single signature, they have dozens.

[robertbastian]

If you do need to modify it, 10-15 minutes would be my expectation.

That is not a reasonable time to expect someone to onboard onto this otherwise trivial code.

Examples of bugs this can prevent:

• Consistently applying the Hijri tabular algorithm equality check in the match

ok

• Matching on the same calendar identity between AnyCalendar and DateInner (for example, avoiding Buddhist AnyCalendar with Coptic DateInner)
• Preventing calling a function with the same return type as the main function. For example, days_in_month and months_in_year have the same signature, so a compiler error wouldn't catch when they are mixed up.

both of these things we have successfully managed for 5 years. we have code reviews, tests, and we're not stupid