Skip to content
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

Merge ParseError with Error, convert Parsed_set* #1511

Open
wants to merge 3 commits into
base: 0.5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/datetime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use std::time::{SystemTime, UNIX_EPOCH};
#[cfg(all(feature = "unstable-locales", feature = "alloc"))]
use crate::format::Locale;
use crate::format::{
parse, parse_and_remainder, parse_rfc3339, Fixed, Item, ParseError, ParseResult, Parsed,
StrftimeItems, TOO_LONG,
parse, parse_and_remainder, parse_rfc3339, Fixed, Item, ParseResult, Parsed, StrftimeItems,
TOO_LONG,
};
#[cfg(feature = "alloc")]
use crate::format::{write_rfc2822, write_rfc3339, DelayedFormat, SecondsFormat};
Expand Down Expand Up @@ -1633,10 +1633,10 @@ where
/// "2012-12-12 12:12:12Z".parse::<DateTime<Utc>>()?;
/// "2012-12-12 12:12:12+0000".parse::<DateTime<Utc>>()?;
/// "2012-12-12 12:12:12+00:00".parse::<DateTime<Utc>>()?;
/// # Ok::<(), chrono::ParseError>(())
/// # Ok::<(), chrono::Error>(())
/// ```
impl str::FromStr for DateTime<Utc> {
type Err = ParseError;
type Err = Error;

fn from_str(s: &str) -> ParseResult<DateTime<Utc>> {
s.parse::<DateTime<FixedOffset>>().map(|dt| dt.with_timezone(&Utc))
Expand All @@ -1654,11 +1654,11 @@ impl str::FromStr for DateTime<Utc> {
/// "2012-12-12 12:12:12Z".parse::<DateTime<Local>>()?;
/// "2012-12-12 12:12:12+0000".parse::<DateTime<Local>>()?;
/// "2012-12-12 12:12:12+00:00".parse::<DateTime<Local>>()?;
/// # Ok::<(), chrono::ParseError>(())
/// # Ok::<(), chrono::Error>(())
/// ```
#[cfg(feature = "clock")]
impl str::FromStr for DateTime<Local> {
type Err = ParseError;
type Err = Error;

fn from_str(s: &str) -> ParseResult<DateTime<Local>> {
s.parse::<DateTime<FixedOffset>>().map(|dt| dt.with_timezone(&Local))
Expand Down
34 changes: 34 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ use core::fmt;
#[non_exhaustive]
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum Error {
/// There is not enough information to create a date/time.
///
/// An example is parsing a string with not enough date/time fields, or the result of a
/// time that is ambiguous during a time zone transition (due to for example DST).
Ambiguous,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an extra note: the intention of this commit by merging ParseError into Error is to make intermediate states possible.

With this we can work on methods that currently return ParseError one by one without needing commits of many 100 lines and weird workarounds.

The addition of the Ambiguous and Inconsistent variants are the only parts I do not intent to change later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these error variants seem pretty specific to parsing, so I wonder if there's a different way of modeling these that would avoid having this in the top-level Error variant (since they couldn't occur in most of the places where Error happens).

Like, maybe ParseError gains an Invalid(Error) variant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ambiguous was mostly planned as an error variant that corresponds to MappedLocalTime::Ambiguous, similar to how DoesNotExist is planned to correspond to the current MappedLocalTime::None. It is nice that Ambiguous also makes sense for what is currently ParseError::NotEnough.

Inconsistent does not feel entirely unreasonable to have. I must admit its only use is with the Parsed type currently, which is of course closely related to parsing.

Errors during parsing have an overlap with our general errors, only InvalidArgument would not be returned by it.
Personally I see no problem in including three or four parsing-specific variants in our Error enum. I very much like to see them all in a single type with a single level (not nested) because that seems like the most convenient for our users.

As I currently see it there is going to be one more variant in the Error enum after this to wrap the error type from #1448 (comment).

(since they couldn't occur in most of the places where Error happens).

Errors related to time zone conversion don't happen with the naive types, and only OutOfRange applies to TimeDelta and FixedOffset. Still it seems helpful to have them return the same type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(since they couldn't occur in most of the places where Error happens).

Errors related to time zone conversion don't happen with the naive types, and only OutOfRange applies to TimeDelta and FixedOffset. Still it seems helpful to have them return the same type.

My point here is that there's a balance to strike here, and also there's some asymmetry here:

  • If, say, 40% of the top-level Error variants can only be triggered by 10% of functions returning it, that doesn't seem great -- I'm not sure if the parsing-specific variants for chrono's Error would be like that, but that would be my impression?
  • On the other hand, it's fine to have a small number of functions that can only trigger a small percentage of the Error variants (because the alternative of introducing a large number of error types is worse)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What advantages do separate type for parse errors bring?

  • The extra errors that can arise when parsing don't pollute the regular enum with three or four extra variants.
  • Users that never use the parsing functionality of chrono don't have to write a conversion method to their own error type for parsing error variants.

What advantages does it bring to have one error type?

  • We consistently return one error type.

All in all the advantages of either one or two errors are very tiny and theoretical. The types can have the same size (if we care, I do). Users can rarely if ever exhaustively match on the Error variants of any method, I'm not sure in the end we will have even one method than can return all variants. And we don't mark the enum as [non_exhaustive] for nothing, users are not expected to do exhaustive matching.

So it comes down to preference. Then my preference is for simplicity: one error type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay: let's do single error type for now, but I think we should reconsider splitting it before the 0.5.0 release.


/// A date or datetime does not exist.
///
/// Examples are:
Expand All @@ -14,25 +20,53 @@ pub enum Error {
/// - a leap second on a non-minute boundary.
DoesNotExist,

/// Some of the date or time components are not consistent with each other.
///
/// An example is parsing 'Sunday 2023-04-21', while that date is a Friday.
Inconsistent,

/// One or more of the arguments to a function are invalid.
///
/// An example is creating a `NaiveTime` with 25 as the hour value.
InvalidArgument,

/// Character does not match with the expected format (during parsing).
InvalidCharacter,

/// The result, or an intermediate value necessary for calculating a result, would be out of
/// range.
///
/// An example is a date for the year 500.000, which is out of the range supported by chrono's
/// types.
OutOfRange,

/// All formatting items have been read but there is a remaining input.
TooLong,

/// The input string has been prematurely ended.
// TEMPORARY
TooShort,

/// The format string contains a formatting specifier that is not supported.
UnsupportedSpecifier,
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Error::Ambiguous => write!(f, "not enough information for a unique date and time"),
Error::DoesNotExist => write!(f, "date or datetime does not exist"),
Error::Inconsistent => {
write!(f, "some of the date or time components are not consistent with each other")
}
Error::InvalidArgument => write!(f, "invalid parameter"),
Error::InvalidCharacter => write!(f, "input doesn't match with the expected format"),
Error::OutOfRange => write!(f, "date outside of the supported range"),
Error::TooLong => write!(f, "trailing input"),
Error::TooShort => write!(f, "premature end of input"),
Error::UnsupportedSpecifier => {
write!(f, "format string contains a formatting specifier that is not supported")
}
}
}
}
Expand Down
90 changes: 11 additions & 79 deletions src/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,15 @@
//! let parsed = NaiveDateTime::parse_from_str(&formatted, "%Y-%m-%d %H:%M:%S")?.in_utc();
//! assert_eq!(parsed, date_time);
//! # }
//! # Ok::<(), chrono::ParseError>(())
//! # Ok::<(), chrono::Error>(())
//! ```

#[cfg(all(feature = "alloc", not(feature = "std"), not(test)))]
use alloc::boxed::Box;
use core::fmt;
use core::str::FromStr;
#[cfg(feature = "std")]
use std::error::Error;

use crate::{Month, ParseMonthError, ParseWeekdayError, Weekday};
use crate::{Error, Month, ParseMonthError, ParseWeekdayError, Weekday};

mod formatting;
mod parsed;
Expand Down Expand Up @@ -381,83 +379,17 @@ impl<'a> Item<'a> {
}
}

/// An error from the `parse` function.
#[derive(Debug, Clone, PartialEq, Eq, Copy, Hash)]
pub struct ParseError(ParseErrorKind);

impl ParseError {
/// The category of parse error
pub const fn kind(&self) -> ParseErrorKind {
self.0
}
}

/// The category of parse error
#[non_exhaustive]
#[derive(Debug, Clone, PartialEq, Eq, Copy, Hash)]
pub enum ParseErrorKind {
/// Given field is out of permitted range.
OutOfRange,

/// There is no possible date and time value with given set of fields.
///
/// This does not include the out-of-range conditions, which are trivially invalid.
/// It includes the case that there are one or more fields that are inconsistent to each other.
Impossible,

/// Given set of fields is not enough to make a requested date and time value.
///
/// Note that there *may* be a case that given fields constrain the possible values so much
/// that there is a unique possible value. Chrono only tries to be correct for
/// most useful sets of fields however, as such constraint solving can be expensive.
NotEnough,

/// The input string has some invalid character sequence for given formatting items.
Invalid,

/// The input string has been prematurely ended.
TooShort,

/// All formatting items have been read but there is a remaining input.
TooLong,

/// There was an error on the formatting string, or there were non-supported formating items.
BadFormat,
}

/// Same as `Result<T, ParseError>`.
pub type ParseResult<T> = Result<T, ParseError>;

impl fmt::Display for ParseError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self.0 {
ParseErrorKind::OutOfRange => write!(f, "input is out of range"),
ParseErrorKind::Impossible => write!(f, "no possible date and time matching input"),
ParseErrorKind::NotEnough => write!(f, "input is not enough for unique date and time"),
ParseErrorKind::Invalid => write!(f, "input contains invalid characters"),
ParseErrorKind::TooShort => write!(f, "premature end of input"),
ParseErrorKind::TooLong => write!(f, "trailing input"),
ParseErrorKind::BadFormat => write!(f, "bad or unsupported format string"),
}
}
}

#[cfg(feature = "std")]
impl Error for ParseError {
#[allow(deprecated)]
fn description(&self) -> &str {
"parser error, see to_string() for details"
}
}
/// Same as `Result<T, Error>`.
pub type ParseResult<T> = Result<T, Error>;

// to be used in this module and submodules
pub(crate) const OUT_OF_RANGE: ParseError = ParseError(ParseErrorKind::OutOfRange);
const IMPOSSIBLE: ParseError = ParseError(ParseErrorKind::Impossible);
const NOT_ENOUGH: ParseError = ParseError(ParseErrorKind::NotEnough);
const INVALID: ParseError = ParseError(ParseErrorKind::Invalid);
const TOO_SHORT: ParseError = ParseError(ParseErrorKind::TooShort);
pub(crate) const TOO_LONG: ParseError = ParseError(ParseErrorKind::TooLong);
const BAD_FORMAT: ParseError = ParseError(ParseErrorKind::BadFormat);
pub(crate) const OUT_OF_RANGE: Error = Error::InvalidArgument;
const IMPOSSIBLE: Error = Error::Inconsistent;
const NOT_ENOUGH: Error = Error::Ambiguous;
const INVALID: Error = Error::InvalidCharacter;
const TOO_SHORT: Error = Error::TooShort;
pub(crate) const TOO_LONG: Error = Error::TooLong;
const BAD_FORMAT: Error = Error::UnsupportedSpecifier;

// this implementation is here only because we need some private code from `scan`

Expand Down
32 changes: 16 additions & 16 deletions src/format/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ use core::str;
use core::usize;

use super::scan;
use super::ParseResult;
use super::{Fixed, InternalFixed, InternalInternal, Item, Numeric, Pad, Parsed};
use super::{ParseError, ParseResult};
use super::{BAD_FORMAT, INVALID, OUT_OF_RANGE, TOO_LONG, TOO_SHORT};
use crate::{DateTime, FixedOffset, Weekday};
use crate::{DateTime, Error, FixedOffset, Weekday};

fn set_weekday_with_num_days_from_sunday(p: &mut Parsed, v: i64) -> ParseResult<&mut Parsed> {
fn set_weekday_with_num_days_from_sunday(p: &mut Parsed, v: i64) -> Result<&mut Parsed, Error> {
p.set_weekday(match v {
0 => Weekday::Sun,
1 => Weekday::Mon,
Expand All @@ -23,11 +23,11 @@ fn set_weekday_with_num_days_from_sunday(p: &mut Parsed, v: i64) -> ParseResult<
4 => Weekday::Thu,
5 => Weekday::Fri,
6 => Weekday::Sat,
_ => return Err(OUT_OF_RANGE),
_ => return Err(Error::InvalidArgument),
})
}

fn set_weekday_with_number_from_monday(p: &mut Parsed, v: i64) -> ParseResult<&mut Parsed> {
fn set_weekday_with_number_from_monday(p: &mut Parsed, v: i64) -> Result<&mut Parsed, Error> {
p.set_weekday(match v {
1 => Weekday::Mon,
2 => Weekday::Tue,
Expand All @@ -36,7 +36,7 @@ fn set_weekday_with_number_from_monday(p: &mut Parsed, v: i64) -> ParseResult<&m
5 => Weekday::Fri,
6 => Weekday::Sat,
7 => Weekday::Sun,
_ => return Err(OUT_OF_RANGE),
_ => return Err(Error::InvalidArgument),
})
}

Expand Down Expand Up @@ -288,7 +288,7 @@ fn parse_internal<'a, 'b, I, B>(
parsed: &mut Parsed,
mut s: &'b str,
items: I,
) -> Result<&'b str, ParseError>
) -> Result<&'b str, Error>
where
I: Iterator<Item = B>,
B: Borrow<Item<'a>>,
Expand Down Expand Up @@ -339,7 +339,7 @@ where

Item::Numeric(ref spec, ref _pad) => {
use super::Numeric::*;
type Setter = fn(&mut Parsed, i64) -> ParseResult<&mut Parsed>;
type Setter = fn(&mut Parsed, i64) -> Result<&mut Parsed, Error>;

let (width, signed, set): (usize, bool, Setter) = match *spec {
Year => (4, true, Parsed::set_year),
Expand Down Expand Up @@ -521,10 +521,10 @@ where
/// "2012-12-12T12:12:12Z".parse::<DateTime<FixedOffset>>()?;
/// "2012-12-12 12:12:12Z".parse::<DateTime<FixedOffset>>()?;
/// "2012- 12-12T12: 12:12Z".parse::<DateTime<FixedOffset>>()?;
/// # Ok::<(), chrono::ParseError>(())
/// # Ok::<(), chrono::Error>(())
/// ```
impl str::FromStr for DateTime<FixedOffset> {
type Err = ParseError;
type Err = Error;

fn from_str(s: &str) -> ParseResult<DateTime<FixedOffset>> {
let mut parsed = Parsed::default();
Expand Down Expand Up @@ -718,7 +718,7 @@ mod tests {
}

#[test]
fn test_parse_numeric() -> Result<(), ParseError> {
fn test_parse_numeric() -> Result<(), Error> {
use crate::format::Item::{Literal, Space};
use crate::format::Numeric::*;

Expand Down Expand Up @@ -845,7 +845,7 @@ mod tests {
}

#[test]
fn test_parse_fixed() -> Result<(), ParseError> {
fn test_parse_fixed() -> Result<(), Error> {
use crate::format::Fixed::*;
use crate::format::Item::{Literal, Space};

Expand Down Expand Up @@ -914,7 +914,7 @@ mod tests {
}

#[test]
fn test_parse_fixed_nanosecond() -> Result<(), ParseError> {
fn test_parse_fixed_nanosecond() -> Result<(), Error> {
use crate::format::Fixed::Nanosecond;
use crate::format::InternalInternal::*;
use crate::format::Item::Literal;
Expand Down Expand Up @@ -1015,7 +1015,7 @@ mod tests {
}

#[test]
fn test_parse_fixed_timezone_offset() -> Result<(), ParseError> {
fn test_parse_fixed_timezone_offset() -> Result<(), Error> {
use crate::format::Fixed::*;
use crate::format::InternalInternal::*;
use crate::format::Item::Literal;
Expand Down Expand Up @@ -1452,7 +1452,7 @@ mod tests {

#[test]
#[rustfmt::skip]
fn test_parse_practical_examples() -> Result<(), ParseError> {
fn test_parse_practical_examples() -> Result<(), Error> {
use crate::format::InternalInternal::*;
use crate::format::Item::{Literal, Space};
use crate::format::Numeric::*;
Expand Down Expand Up @@ -1832,6 +1832,6 @@ mod tests {
fn test_issue_1010() {
let dt = crate::NaiveDateTime::parse_from_str("\u{c}SUN\u{e}\u{3000}\0m@J\u{3000}\0\u{3000}\0m\u{c}!\u{c}\u{b}\u{c}\u{c}\u{c}\u{c}%A\u{c}\u{b}\0SU\u{c}\u{c}",
"\u{c}\u{c}%A\u{c}\u{b}\0SUN\u{c}\u{c}\u{c}SUNN\u{c}\u{c}\u{c}SUN\u{c}\u{c}!\u{c}\u{b}\u{c}\u{c}\u{c}\u{c}%A\u{c}\u{b}%a");
assert_eq!(dt, Err(ParseError(ParseErrorKind::Invalid)));
assert_eq!(dt, Err(Error::InvalidCharacter));
}
}