-
Couldn't load subscription status.
- Fork 230
Throw error in Date::try_from_fields for very large year values. #7062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| // called LICENSE at the top level of the ICU4X source tree | ||
| // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). | ||
|
|
||
| use crate::error::range_check_with_overflow; | ||
| use crate::error::{range_check, range_check_with_overflow}; | ||
| use crate::options::{DateAddOptions, DateDifferenceOptions}; | ||
| use crate::options::{DateFromFieldsOptions, MissingFieldsStrategy, Overflow}; | ||
| use crate::types::{DateDuration, DateDurationUnit, DateFields, DayOfYear, MonthCode}; | ||
|
|
@@ -12,8 +12,12 @@ use core::convert::TryInto; | |
| use core::fmt::Debug; | ||
| use core::hash::{Hash, Hasher}; | ||
| use core::marker::PhantomData; | ||
| use core::ops::RangeInclusive; | ||
| use tinystr::tinystr; | ||
|
|
||
| /// The range ±2²⁷. We use i32::MIN since it is -2³¹ | ||
| const VALID_YEAR_RANGE: RangeInclusive<i32> = (i32::MIN / 16)..=-(i32::MIN / 16); | ||
|
|
||
| #[derive(Debug)] | ||
| #[allow(clippy::exhaustive_structs)] // this type is stable | ||
| pub(crate) struct ArithmeticDate<C: CalendarArithmetic> { | ||
|
|
@@ -641,6 +645,23 @@ pub(crate) struct ArithmeticDateBuilder<YearInfo> { | |
| pub(crate) day: u8, | ||
| } | ||
|
|
||
| fn extended_year_as_year_info<YearInfo, C>( | ||
Manishearth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| extended_year: i32, | ||
| cal: &C, | ||
| ) -> Result<YearInfo, DateError> | ||
| where | ||
| C: DateFieldsResolver<YearInfo = YearInfo>, | ||
| { | ||
| // Check that the year is in range to avoid any arithmetic overflow. | ||
| // | ||
| // This range is currently global, but may be replaced with | ||
| // a per-calendar check in the future. | ||
| // | ||
| // <https://github.com/unicode-org/icu4x/issues/7076> | ||
| range_check(extended_year, "year", VALID_YEAR_RANGE)?; | ||
| Ok(cal.year_info_from_extended(extended_year)) | ||
| } | ||
|
|
||
| impl<YearInfo> ArithmeticDateBuilder<YearInfo> | ||
| where | ||
| YearInfo: PartialEq + Debug, | ||
|
|
@@ -654,49 +675,69 @@ where | |
| C: DateFieldsResolver<YearInfo = YearInfo>, | ||
| { | ||
| let missing_fields_strategy = options.missing_fields_strategy.unwrap_or_default(); | ||
| let maybe_year = { | ||
| let extended_year_as_year_info = fields | ||
| .extended_year | ||
| .map(|extended_year| cal.year_info_from_extended(extended_year)); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Process Discussion: This is code that I "care a lot about" because it is code that I wrote a matter of weeks ago, and it is being changed in major ways. My "care a lot about it" would decay over time, but this is still very new code that I spent a lot of time thinking about. Manish reviewed this code so I would expect that he would have been able to correctly identify that status. I left a comment on an older version of this PR. However, after I left that comment, this function changed in a large way, so it "looks like" I was OK with the PR, even though I hadn't seen these additional changes. The correct process would have been:
The fail-safe would be for Manish or Robert to flag this function as one Shane cares about, notice that Shane didn't see the latest changes, and ask for explicit LGTM. It seems the fail-safe also didn't happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In that case I don't think I have a good mental model about the type of code you would care about if you care about this, after our discussions about this PR. It's generally hard to know in advance if some code is something an author has spent a lot of time thinking about unless it that thinking happened through the review process. We explicitly had an in-person discussion about the strategy to use here, and I even wrote some of this code in front of you, checking with you on some of the stages. The code here did not change that much since that discussion, with the main change being to comments. My expectation was that we were mostly aligned on the general shape of this code and it was Robert whom I would need to get some consensus with, which is what I pursued. I think c0283f3 is the only actual change, the bulk of the work here was discussion with Robert. 1c67bd3 was a refactor I did in front of you. I think it is unreasonable to expect me to know you would want to be a blocking reviewer on this. I expected you'd want to look at it eventually, but not as a blocking review. (In this specific case, I merged once I had Robert's review because he's also working on improving the range check situation, by merging it I enabled a lot of progress on his PR #7065 during the time we were both online)
I guess when you leave a partial review it dismisses the request? Perhaps for partial reviews you should re-request review of yourself if you use that UI. This is just like re-marking an inbox item as unread/un-"done" after partially-but-not-completely consuming it. I can't know just from looking at it whether a review is a partial review or a full one. My norm is to only re-request review of reviewers whose comments I'm addressing, unless I'm pushing a large enough change that I think I need to re-request review from others. The changes I made since your review were extremely minor, mostly comments. Personally I don't use "open review requests" for this since I use notification filters. So I don't know the details of what does and doesn't work. A slight mistake I made here was starting the work for #7062 (comment) as a separate thing and then not leaving a comment saying I was planning on doing that, or waiting for a response here. That was my bad: I thought I had left a comment there or or maybe said something in person and it just hadn't happened. |
||
| let day = match fields.day { | ||
| Some(day) => day.get(), | ||
| None => match missing_fields_strategy { | ||
| MissingFieldsStrategy::Reject => return Err(DateError::NotEnoughFields), | ||
| 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); | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| if fields.month_code.is_none() && fields.ordinal_month.is_none() { | ||
| // We're returning this error early so that we return structural type | ||
| // errors before range errors, see comment in the year code below. | ||
| return Err(DateError::NotEnoughFields); | ||
| } | ||
|
|
||
| let year = { | ||
| // NOTE: The year/extendedyear range check is important to avoid arithmetic | ||
| // overflow in `year_info_from_era` and `year_info_from_extended`. It | ||
| // must happen before they are called. | ||
| // | ||
| // To better match the Temporal specification's order of operations, we try | ||
| // to return structural type errors (`NotEnoughFields`) before checking for range errors. | ||
| // This isn't behavior we *must* have, but it is not much additional work to maintain | ||
| // so we make an attempt. | ||
| match (fields.era, fields.era_year) { | ||
| (None, None) => extended_year_as_year_info, | ||
| (None, None) => match fields.extended_year { | ||
| Some(extended_year) => extended_year_as_year_info(extended_year, cal)?, | ||
| None => match missing_fields_strategy { | ||
| MissingFieldsStrategy::Reject => 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), | ||
| } | ||
| } | ||
| }, | ||
| }, | ||
| (Some(era), Some(era_year)) => { | ||
| range_check(era_year, "year", VALID_YEAR_RANGE)?; | ||
| let era_year_as_year_info = cal.year_info_from_era(era, era_year)?; | ||
| if let Some(other) = extended_year_as_year_info { | ||
| if era_year_as_year_info != other { | ||
| if let Some(extended_year) = fields.extended_year { | ||
| if era_year_as_year_info != extended_year_as_year_info(extended_year, cal)? | ||
| { | ||
| return Err(DateError::InconsistentYear); | ||
| } | ||
| } | ||
| Some(era_year_as_year_info) | ||
| era_year_as_year_info | ||
| } | ||
| // Era and Era Year must be both or neither | ||
| (Some(_), None) | (None, Some(_)) => return Err(DateError::NotEnoughFields), | ||
| } | ||
| }; | ||
| let day = match fields.day { | ||
| Some(day) => day.get(), | ||
| None => match missing_fields_strategy { | ||
| MissingFieldsStrategy::Reject => return Err(DateError::NotEnoughFields), | ||
| MissingFieldsStrategy::Ecma => match maybe_year { | ||
| // The ECMAScript strategy is to pick day 1, always, regardless of whether | ||
| // that day exists for the month/year combo | ||
| Some(_) => 1, | ||
| None => return Err(DateError::NotEnoughFields), | ||
| }, | ||
| }, | ||
| }; | ||
| let year = match maybe_year { | ||
| Some(year) => year, | ||
| None => match missing_fields_strategy { | ||
| MissingFieldsStrategy::Reject => 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), | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| let month = { | ||
| let ordinal_month_as_u8 = fields.ordinal_month.map(|x| x.get()); | ||
| match fields.month_code { | ||
|
|
@@ -711,6 +752,7 @@ where | |
| } | ||
| None => match ordinal_month_as_u8 { | ||
| Some(month) => month, | ||
| // This is technically unreachable since it's checked early above | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Then debug assert that it is unreachable, please. (this is feedback I gave orally but didn't write down in a GitHub comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I said then I really want to be careful about debug assertions in this code, but I think this case is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| None => return Err(DateError::NotEnoughFields), | ||
| }, | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add MonthDay and YearMonth tests, and missing Day tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to have gotten done. #7083
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sorry, I have a local branch for this that I didn't finish up and push. I'll do that tomorrow.