Implement DateFromFieldsError, a narrow error type for Date::try_from_fields #7100

sffc · 2025-10-14T21:01:43Z

sffc · 2025-10-14T21:10:42Z

Still one little test failure to debug but this should be mostly ready.

I also want to add additional tests to make sure that the correct MonthCode error is returned.

---- any_calendar::tests::test_any_construction stdout ----

thread 'any_calendar::tests::test_any_construction' panicked at components/calendar/src/any_calendar.rs:1648:9:
assertion `left == right` failed: Construction with Some(("am", Some(0))), 100, M14, 1 did not return UnknownMonthCode(MonthCode("M14"))
  left: Err(UnknownMonthCode(MonthCode("und")))
 right: Err(UnknownMonthCode(MonthCode("M14")))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    any_calendar::tests::test_any_construction

test result: FAILED. 133 passed; 1 failed; 5 ignored; 0 measured; 0 filtered out; finished in 7.54s

error: test failed, to rerun pass `--lib`

Manishearth

I like this approach, and fine grained errors are always good.

Manishearth · 2025-10-14T21:19:18Z

components/calendar/src/error.rs

+}
+
+/// Internal narrow error type for functions that only fail on invalid month codes
+pub(crate) struct InvalidMonthCodeError;


praise: appreciate the comment that this is internal since if this ever were to be public it should probably be an enum with a single variant

Manishearth · 2025-10-14T21:23:59Z

components/calendar/src/calendar.rs

            missing_fields_strategy: Some(MissingFieldsStrategy::Reject),
        };
-        self.from_fields(fields, options)
+        self.from_fields(fields, options).map_err(|e| match e {


observation: ick, not super happy about needing this

suggestion: perhaps DateFromFieldsError can have a .to_date_error_for_konth_code(code)?

I consider try_new_from_codes to be legacy at this point so I want to keep this remapping localized here

that's a good justification. refactoring the conversion into a separate method may still be nice

I added comments.

it should be fairly simple to write a ArithmeticDate::from_codes that does not use from_fields. The code will be much simpler and the errors much cleaner.

impl<C: DateFieldsResolver + CalendarArithmetic> ArithmeticDate<C> { pub(crate) fn from_codes( era: Option<&str>, year: i32, month_code: types::MonthCode, day: u8, calendar: &C ) -> Result<Self, DateError> { let year = if let Some(era) = era { calendar.year_info_from_era(era, year)? } else { calendar.year_info_from_extended(year) }; let month = calendar.ordinal_month_from_code(&year, month_code, Default::default())?; range_check(day, "day", 1..=C::days_in_provided_month(year, month))?; Ok(ArithmeticDate::new_unchecked(year, month, day)) } }

Yeah... that's nicer, but I don't see a way to do it without implementing Calendar::from_codes on every calendar. I really want to keep the number of ways to construct dates to a minimum. We already have 4 code paths (Calendar::from_fields, Calendar::from_iso, Calendar::from_rata_die, and the manual Date constructor). We don't need to add Calendar::from_codes back as a fifth code path.

I am also in favor of reusing from_fields in from_codes. Potential cleanup followup.

you would need to implement from_codes with a single call in every calendar. I don't think that's the end of the world

components/calendar/src/error.rs

components/calendar/src/types.rs

components/calendar/src/cal/chinese.rs

robertbastian

this conflicts with #7088

sffc · 2025-10-15T07:18:18Z

this conflicts with #7088

Yes and this is in the 2.1 critical path whereas #7088 is an internal refactoring that I wasn't aware you were working on.

sffc · 2025-10-15T07:58:34Z

semver says:

  variant DateError::InconsistentYear, previously in file /home/runner/work/icu4x/icu4x/semver-checks/target/semver-checks/git-origin_main/2bc4b9ddeb0fce93f5d7f6008c4f008d38dfd12e/components/calendar/src/error.rs:97
  variant DateError::InconsistentMonth, previously in file /home/runner/work/icu4x/icu4x/semver-checks/target/semver-checks/git-origin_main/2bc4b9ddeb0fce93f5d7f6008c4f008d38dfd12e/components/calendar/src/error.rs:135
  variant DateError::NotEnoughFields, previously in file /home/runner/work/icu4x/icu4x/semver-checks/target/semver-checks/git-origin_main/2bc4b9ddeb0fce93f5d7f6008c4f008d38dfd12e/components/calendar/src/error.rs:199

These variants are new since 2.0.

We should not block a 2.1-blocking PR on a non-2.1-blocking PR

sffc · 2025-10-15T08:17:27Z

autosubmit = true

robertbastian · 2025-10-15T10:05:22Z

components/calendar/src/calendar.rs

            missing_fields_strategy: Some(MissingFieldsStrategy::Reject),
        };
-        self.from_fields(fields, options)
+        self.from_fields(fields, options).map_err(|e| match e {


it should be fairly simple to write a ArithmeticDate::from_codes that does not use from_fields. The code will be much simpler and the errors much cleaner.

impl<C: DateFieldsResolver + CalendarArithmetic> ArithmeticDate<C> { pub(crate) fn from_codes( era: Option<&str>, year: i32, month_code: types::MonthCode, day: u8, calendar: &C ) -> Result<Self, DateError> { let year = if let Some(era) = era { calendar.year_info_from_era(era, year)? } else { calendar.year_info_from_extended(year) }; let month = calendar.ordinal_month_from_code(&year, month_code, Default::default())?; range_check(day, "day", 1..=C::days_in_provided_month(year, month))?; Ok(ArithmeticDate::new_unchecked(year, month, day)) } }

robertbastian · 2025-10-15T10:10:57Z

components/calendar/src/error.rs

+    /// assert!(matches!(err, DateFromFieldsError::InvalidMonthCode));
+    /// ```
+    #[displaydoc("Invalid month code syntax")]
+    InvalidMonthCode,


I don't see much value in distinguishing this from UnknownMonthCodeForCalendar. We don't do this for eras either, "b" is invalid era syntax, but we just return UnknownEra

I was unsure about this too: in my original comment I laid out the three types of invalid month code for full clarity, but I think error-wise we only need two, especially if future calendars add more types of month code.

I'm happy with having two error types. I don't really think InvalidMonthCode and UnknownMonthCodeForCalendar are differently actionable, whereas the third one very much is.

A slight difference is that I return back the unknown month code in UnknownMonthCodeForCalendar since it definitely fits in a TinyStr4, whereas InvalidMonthCode could be something that doesn't fit in a TinyStr4.

Right, when I'm talking about "differently actionable" I mean whether or not the user will have a different set of actions to take based on that. The set of actions to take are more or less the same here.

ok I removed the MonthCode field from the errors, so now there isn't a technical reason not to merge them. It's still easy in code for me to return these as different error variants. I'm mostly neutral on whether to fold them into one. I lean toward narrower variants; for example, if we merge these, should we also merge InconsistentYear and InconsistentMonth into InconsistentFields?

robertbastian · 2025-10-15T10:11:24Z

components/calendar/src/error.rs

+pub enum DateFromFieldsError {
+    /// A field is out of range for its domain.
+    #[displaydoc("{0}")]
+    Range(RangeError),


please flatten to keep consistency with DateError

Consistency with DateError is a non-goal. I think RangeError is much better to keep as a sub-error because then it has its own methods and can be used as its own variable. Also, we can't add additional fields since enum variant structs are exhaustive. We very frequently change enum variant structs to standalone structs because of these reasons.

Consistency with DateError is a non-goal

imho consistency within an API is always a goal.

"enum variant structs" can be non-exhaustive.

RangeError does not have any methods.

I think with the original RangeError the tradeoff was that matching on sub-structs is more annoying than matching on struct variants.

Though I suspect most users won't actually care to match on it?

I don't think exhaustiveness matters for this particular error.

Aside: it's unfortunate RangeError is public: I wish that field were an enum not a string.

I should clarify that the reason I see consistency with DateError as a non-goal is because I consider DateError and try_new_from_codes to be on the deprecation track.

DateError is also used by calendar-specific constructors. I also disagree about deprecating from_codes. And in any case this will only happen in the long term

components/calendar/src/error.rs

robertbastian · 2025-10-15T10:13:38Z

components/calendar/src/error.rs

+    /// assert!(matches!(err, DateFromFieldsError::UnknownMonthCodeForYear(_)));
+    /// ```
+    #[displaydoc("Month code exists in calendar, but not for this year: {0:?}")]
+    UnknownMonthCodeForYear(MonthCode),


it's not unknown, it's invalid

CC @Manishearth for feedback on naming of the error variant

robertbastian · 2025-10-15T10:14:39Z

components/calendar/src/error.rs

+    pub enum EcmaReferenceYearError {
+        InvalidMonthCode,
+        UnknownMonthCodeForCalendar(crate::types::MonthCode),
+        NotEnoughFields,


issue: this should be a range error (for days). NotEnoughFields is not an error that can happen, as that method has non-optional month and day

I thought it can happen if you attempt it with an ordinal month instead of a code?

icu4x/components/calendar/src/calendar_arithmetic.rs

Lines 683 to 689 in ef40c67

MissingFieldsStrategy::Ecma => {

if fields.extended_year.is_some() || fields.era_year.is_some() {

// The ECMAScript strategy is to pick day 1, always, regardless of whether

// that day exists for the month/year combo

1

} else {

return Err(DateError::NotEnoughFields);

icu4x/components/calendar/src/calendar_arithmetic.rs

Lines 715 to 720 in ef40c67

MissingFieldsStrategy::Ecma => {

match (fields.month_code, fields.ordinal_month) {

(Some(month_code), None) => {

cal.reference_year_from_month_day(month_code, day)?

}

_ => return Err(DateError::NotEnoughFields),

yep.

this error is for the method

fn ecma_reference_year( &self, _month_code: MonthCode, _day: u8, ) -> Result<i32, EcmaReferenceYearError>

and I guess also for reference_year_from_month_day which has the same signature - all required fields are present

I'd still like internal/unstable enums to have the correct variants, because they do influence the variants for public enums

So the only error here should be a bad month code.

even better!

I'd still like internal/unstable enums to have the correct variants, because they do influence the variants for public enums

Right, we should still strive for that, I was just observing that we don't need to perfectly design this enum just yet. We should still fix what we notice.

NotEnoughFields is reachable from the default impl of pub trait Rules.

I'll remove the InvalidMonthCode variant when adding the internal ValidMonthCode type.

For now I'll leave NotEnoughFields since it is how we signal that the function is "not implemented". Or, I could rename it to NotImplemented, or I could make the Rules function return Option<Result>.

robertbastian · 2025-10-15T10:18:31Z

components/calendar/src/cal/chinese.rs

-        };
+    fn ecma_reference_year(
+        &self,
+        month_code: types::MonthCode,


suggestion: if we allow invalid month codes to be constructed, this method should take ordinal: u8, is_leap: bool to remove the parsing error case (or introduce a new ValidMonthCode type). once we move to &str inputs we basically don't need MonthCode anymore.

this also applies to all other (internal) methods that use MonthCode. validate early so not every method needs a InvalidMonthCode error variant

i cautiously like this idea, though I think it can be a non-2.1 blocking followup as a part of graduating these traits.

it can only be non-blocking if we don't sprinkle MonthCode in other parts of the API, like error variants, and the input itself

If you both are okay with adding icu_calendar::types::ValidMonthCode, I will proceed with that.

I'm in favor of that. This issue is a mandatory 2.1 blocker so we can do things piecemeal

(Looked at the PR, not in favor of the PR, stated why there)

I think here we can have MonthCode, or a tuple, or an unstable lightweight ValidMonthCode type. The latter two are more optimized, and we can change our mind when graduating the trait. We should note this thing as a decision-to-be-made on the graduation issue and move on.

This is a decision about unstable code and does not block this PR, nor should its fix involve public stable API changes.

I can make ValidMonthCode pub(crate)/unstable in the other PR for now, and remove it from the error variant. I think in both cases where we convert to DateError, we have access to the input month code.

Okay, that works!

robertbastian · 2025-10-15T10:30:05Z

components/calendar/src/calendar_arithmetic.rs

+                DateFromFieldsOptions::from_add_options(options),
+            )
+            .map_err(|e| {
+                // TODO: Use a narrower error type here. For now, convert into DateError.


observation: this is unstable code, so it's fine to use DateError for now

sffc · 2025-10-15T21:13:07Z

components/calendar/src/lib.rs

 mod calendar_arithmetic;
 mod duration;
-mod error;
+pub mod error;


Just highlighting that I'm making this module public. It is in-scope of this PR since I am adding a new Error enum and I need a place to put it.

Manishearth

LG with the try_parse change.

sffc · 2025-10-15T21:28:29Z

There are some bikesheddy questions which I moved into #7010 (comment)

Manishearth · 2025-10-15T22:51:08Z

Shane set autosubmit

#7010 This adds ValidMonthCode as an internal type. Suggested in #7100 --------- Co-authored-by: Robert Bastian <[email protected]>

sffc added 3 commits October 14, 2025 14:01

Initial migration to DateFromFieldsError

7e6a1f6

Fix up docs tests

219caa9

ci

1c904dd

sffc marked this pull request as ready for review October 14, 2025 21:10

sffc requested a review from Manishearth as a code owner October 14, 2025 21:10

Manishearth mentioned this pull request Oct 14, 2025

Remove CalendarArithmetic trait #7088

Merged

Manishearth previously approved these changes Oct 14, 2025

View reviewed changes

Manishearth mentioned this pull request Oct 15, 2025

Consider narrower error type between try_new_from_codes and try_from_fields #7010

Open

robertbastian previously requested changes Oct 15, 2025

View reviewed changes

passing tests

37267ab

sffc dismissed Manishearth’s stale review via 37267ab October 15, 2025 07:58

sffc added 4 commits October 15, 2025 01:06

Error mapping commentary

9af0e42

try_parse

ab70a67

clippy, fmt

753bbdf

use enums for all errors

c4b2e3c

sffc requested a review from Manishearth October 15, 2025 08:15

sffc changed the title ~~Initial migration to DateFromFieldsError~~ Implement DateFromFieldsError, a narrow error type for Date::try_from_fields Oct 15, 2025

sffc requested a review from robertbastian October 15, 2025 08:17

robertbastian reviewed Oct 15, 2025

View reviewed changes

sffc mentioned this pull request Oct 15, 2025

Streamline more error cases around month code parsing #7105

Merged

sffc added 5 commits October 15, 2025 13:33

Remove MonthCode field from DateFromFieldsError

50b78f7

Merge branch 'main' into datefromfieldserror

1de767e

diplomat-coverage

ffc284c

Fix errors in FFI and in new coverage test

e78a06d

Fix rust link

b5b027f

sffc commented Oct 15, 2025

View reviewed changes

Manishearth previously approved these changes Oct 15, 2025

View reviewed changes

sffc added 2 commits October 15, 2025 14:22

Add re-exports to diplomat-coverage allowlist

0624665

diplomat-gen

f2de676

sffc dismissed Manishearth’s stale review via f2de676 October 15, 2025 21:23

sffc requested a review from Manishearth October 15, 2025 21:28

Manishearth approved these changes Oct 15, 2025

View reviewed changes

Manishearth merged commit 1fda50a into unicode-org:main Oct 15, 2025
29 of 30 checks passed

sffc deleted the datefromfieldserror branch October 15, 2025 23:14

sffc added a commit that referenced this pull request Oct 20, 2025

Streamline more error cases around month code parsing (#7105)

47ebd5e

#7010 This adds ValidMonthCode as an internal type. Suggested in #7100 --------- Co-authored-by: Robert Bastian <[email protected]>

	MissingFieldsStrategy::Ecma => {
	if fields.extended_year.is_some() \|\| fields.era_year.is_some() {
	// The ECMAScript strategy is to pick day 1, always, regardless of whether
	// that day exists for the month/year combo
	1
	} else {
	return Err(DateError::NotEnoughFields);

	MissingFieldsStrategy::Ecma => {
	match (fields.month_code, fields.ordinal_month) {
	(Some(month_code), None) => {
	cal.reference_year_from_month_day(month_code, day)?
	}
	_ => return Err(DateError::NotEnoughFields),

Implement DateFromFieldsError, a narrow error type for Date::try_from_fields #7100

Implement DateFromFieldsError, a narrow error type for Date::try_from_fields #7100

Uh oh!

Conversation

sffc commented Oct 14, 2025

Uh oh!

sffc commented Oct 14, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Oct 15, 2025

Uh oh!

sffc commented Oct 15, 2025

Uh oh!

sffc commented Oct 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth Oct 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Oct 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manishearth Oct 15, 2025 •

edited

Loading

sffc Oct 15, 2025 •

edited

Loading

Manishearth Oct 15, 2025 •

edited

Loading

Manishearth Oct 15, 2025 •

edited

Loading