Repository: BurntSushi/jiff (language: rust)

Issue:
Guarantee that `Timestamp::from_{nano,micro,milli,}second` _only_ errors on out-of-range conditions
It is currently not possible to infallibly convert timestamps that are obtained from either `std`, or `chrono` into `jiff::Timestamp`. `chrono` has a valid range of `i64::MIN..i64::MAX` nanoseconds, and `std` even `-u64::MAX..u64::MAX` nanoseconds, but `jiff`'s range is much more restricted. While timestamps beyond the year 10,000 are probably never needed, this introduces fallibility on the API, which could be avoided by saturating at `Timestamp::MIN` and `Timestamp::MAX`.

The current error values that `Timestamp` returns unfortunately don't let me do that in userland, as there is no information in the error (#8).


Discussion / hints:
You can definitely do it in user-land today, because the only documented way for, e.g., `Timestamp::from_second`, to fail is with an out-of-range error.

Perhaps there is a question of whether a new error condition could be added in a semver compatible release, but at least for routines like that, I'd be comfortable adding language like, "new error conditions for this routine are considered breaking, and won't be added in a semver compatible release."

I'd also be fine adding inspection APIs on `jiff::Error` like, `is_range_error` or something.

I'm unsure if I want to add explicit saturating routines for conversion. It seems like returning an error or treating it as a bug would be much more common. And adding saturating routines for `Timestamp` sets a precedent to do it for the rest of the datetime types too. That's a lot of additional APIs.
Currently I need to do
```rust
if x > Timestamp::MAX.as_nanosecond() {
    Timestamp::MAX
} else if x < Timestamp::MIN.as_nanosecond() {
    Timestamp::MIN
} else {
    Timestamp::from_nanosecond(x).unwrap()
}
```

This still requires an `unwrap()`, which pulls in panic and formatting code, and to a reviewer it is not clear whether this ever panics. They would have to go read the docs to verify that this is panic-free (something the compiler probably won't be able to do), and trust that it will stay panic-free.

Compare with an API like this, where reading the code suffices:

```rust
match Timestamp::from_nanosecond(x) {
  Ok(t) => t
  Err(TimestampError::Overflow) => Timestamp::MAX,
  Err(TimestampError::Underflow) => Timestamp::MIN,
}
```

> It seems like returning an error or treating it as a bug would be much more common

`jiff` has made the decision that timestamps above 9999-12-31 are out of scope, but that doesn't make them a bug.
For context, I need this in https://github.com/frehberg/spa-rs/pull/12. I don't want this API to panic on timestamps beyond `jiff`s range, but I also don't want users having to unwrap every single result that this library returns.
> And adding saturating routines for Timestamp sets a precedent to do it for the rest of the datetime types too

Which other types are there? `Date`, `DateTime`, and `DateTime`? `Date` and `Time` should saturate the year value, all other overflows are invalid, and no other datetime library produces such values.
> jiff has made the decision that timestamps above 9999-12-31 are out of scope, but that doesn't make them a bug.

Can you please not put words in my mouth? I didn't say that and didn't imply it.

> Currently I need to do

That isn't how I'd write it. I'd write it like this:

```rust
use jiff::Timestamp;

fn main() -> anyhow::Result<()> {
    assert_eq!(from_nanosecond_saturating(0), Timestamp::UNIX_EPOCH);
    assert_eq!(
        from_nanosecond_saturating(-9999999999999999999999999999999999),
        Timestamp::MIN
    );
    assert_eq!(
        from_nanosecond_saturating(9999999999999999999999999999999999),
        Timestamp::MAX
    );

    Ok(())
}

fn from_nanosecond_saturating(nanos: i128) -> Timestamp {
    Timestamp::from_nanosecond(nanos).unwrap_or_else(|_| {
        if nanos < 0 {
            Timestamp::MIN
        } else {
            Timestamp::MAX
        }
    })
}
```

No `unwrap()` or panic branches.

> This still requires an unwrap(), which pulls in panic and formatting code, and to a reviewer it is not clear whether this ever panics.

There are tons of APIs in Jiff for which `unwrap()` is appropriate to use. It is not a goal of mine to offer infallible APIs for all such cases. I am okay doing it for _some_ cases where it is exceptionally well motivated, but I don't see a strong motivation here.

> > And adding saturating routines for Timestamp sets a precedent to do it for the rest of the datetime types too
> 
> Which other types are there? `Date`, `DateTime`, and `DateTime`? `Date` and `Time` should saturate the year value, all other overflows are invalid, and no other datetime library produces such values.

So what is the type signature and panicking behavior of a function that accepts a year, month and day? Does it panic for an invalid month? And thus have the same downsides as using `unwrap()` (bringing in the formatting machinery)? Or does it return a `Result` and force the caller to do an `unwrap()`?

Just saying "all other overflows are invalid" isn't sufficient specificity for this particular conversation given that you are trying to eliminate panicking branches and `unwrap()` calls and formatting machinery.
> Can you please not put words in my mouth? I didn't say that and didn't imply it.

`jiff` hasn't made the decision that timestamps above 9999-12-31 are out of scope? 

> So what is the type signature and panicking behavior of a function that accepts a year, month and day? Does it panic for an invalid month?

Yeah there's not much value in having saturating civil constructors, because they need to be fallible anyway. This doesn't.


> That isn't how I'd write it. I'd write it like this:

This still assumes that no other error will ever be added, but at least it avoids the panic. Thanks.
> `jiff` hasn't made the decision that timestamps above 9999-12-31 are out of scope?

I was referring to this part:

> but that doesn't make them a bug

...

> This still assumes that no other error will ever be added, but at least it avoids the panic. Thanks.

For now, I'll change this issue to tracking a doc improvement providing this as a semver guarantee.

I am not totally closed to adding saturating constructors in the future, but I'd like to see how far we can get with a lighter touch for now.
> There are tons of APIs in Jiff for which unwrap() is appropriate to use.

I think we have a fundamental mismatch of when `unwrap()` is appropriate to use. If I'm writing a library, I *never ever* want to panic. I return errors, and the downstream binary can then decide to unwrap. But returning `Result<jiff::Timestamp, jiff::Error>` as an alternative to `chrono::DateTime<Utc>` is not going to make people choose `jiff`.

I use `unwrap()` in libraries all the time. Just like std does. I've [thoroughly documented my views on `unwrap()`](https://burntsushi.net/unwrap/).

> I think we have a fundamental mismatch of when unwrap() is appropriate to use. If I'm writing a library, I never ever want to panic.

This is a non-sequitur. "use unwrap" and "never ever want to panic" are two distinct things. I agree with the latter. But the latter does not _necessarily_ follow from the former. An `unwrap()` introduces a panicking _branch_, not a panic. If you ever use `slice[i]` in a library, then you're just using syntactic sugar for `*slice.get(i).unwrap()`.
Ok. I don't think it's worth continuing this.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3d83d4ce..92700566 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,10 @@ Add `tz::{get,include}` macros for time zone support in core-only environments.
 Switch to `rearguard` tzdb data in `jiff-tzdb` and document it.
 * [#259](https://github.com/BurntSushi/jiff/pull/259):
 De-duplicate TZif data in `jiff-tzdb`.
+* [#273](https://github.com/BurntSushi/jiff/issues/273):
+Add "crate features" documentation to `jiff-sqlx` and `jiff-diesel`.
+* [#277](https://github.com/BurntSushi/jiff/issues/277):
+Document semver guarantee for error conditions on `Timestamp` constructors.
 
 Bug fixes:
 
diff --git a/crates/jiff-diesel/src/lib.rs b/crates/jiff-diesel/src/lib.rs
index 822ed009..8ba5b42f 100644
--- a/crates/jiff-diesel/src/lib.rs
+++ b/crates/jiff-diesel/src/lib.rs
@@ -4,6 +4,9 @@ This crate provides integration points for [Jiff](jiff) and [Diesel](diesel).
 Examples can be found in the
 [examples directory of the Jiff repository][examples].
 
+Note that to use this crate, you'll likely need to enable one of its
+[database backend features](#crate-features).
+
 # Organization
 
 This crates defines several types that wrap corresponding types in
@@ -58,6 +61,12 @@ admittedly, tough to stomach.
 In the future, it may be prudent for this crate to be upstreamed into Diesel
 itself.
 
+# Crate features
+
+* **mysql** - Enables `diesel/mysql_backend`.
+* **postgres** - Enables `diesel/postgres_backend`.
+* **sqlite** - Enables `disel/sqlite`.
+
 [examples]: https://github.com/BurntSushi/jiff/tree/master/examples
 [`chrono`]: https://docs.rs/chrono
 [`time`]: https://docs.rs/time
diff --git a/crates/jiff-sqlx/src/lib.rs b/crates/jiff-sqlx/src/lib.rs
index 443202a2..23978d23 100644
--- a/crates/jiff-sqlx/src/lib.rs
+++ b/crates/jiff-sqlx/src/lib.rs
@@ -4,6 +4,9 @@ This crate provides integration points for [Jiff](jiff) and [SQLx][sqlx].
 Examples can be found in the
 [examples directory of the Jiff repository][examples].
 
+Note that to use this crate, you'll likely need to enable one of its
+[database backend features](#crate-features).
+
 # Organization
 
 This crates defines several types that wrap corresponding types in Jiff. Each
@@ -55,6 +58,11 @@ admittedly, tough to stomach.
 In the future, it may be prudent for this crate to be upstreamed into SQLx
 itself.
 
+# Crate features
+
+* **postgres** - Enables the `sqlx-postgres` dependency.
+* **sqlite** - Enables the `sqlx-sqlite` dependency.
+
 [sqlx]: https://docs.rs/sqlx/0.8
 [examples]: https://github.com/BurntSushi/jiff/tree/master/examples
 [`chrono`]: https://docs.rs/chrono
diff --git a/src/timestamp.rs b/src/timestamp.rs
index 0a0fad73..ba31dc62 100644
--- a/src/timestamp.rs
+++ b/src/timestamp.rs
@@ -561,6 +561,10 @@ impl Timestamp {
     /// This returns an error if the given second corresponds to a timestamp
     /// outside of the [`Timestamp::MIN`] and [`Timestamp::MAX`] boundaries.
     ///
+    /// It is a semver guarantee that the only way for this to return an error
+    /// is if the given value is out of range. That is, when it is less than
+    /// `Timestamp::MIN` or greater than `Timestamp::MAX`.
+    ///
     /// # Example
     ///
     /// This example shows the instants in time 1 second immediately after and
@@ -580,6 +584,36 @@ impl Timestamp {
     ///
     /// # Ok::<(), Box<dyn std::error::Error>>(())
     /// ```
+    ///
+    /// # Example: saturating construction
+    ///
+    /// If you need a way to build a `Timestamp` value that saturates to
+    /// the minimum and maximum values supported by Jiff, then this is
+    /// guaranteed to work:
+    ///
+    /// ```
+    /// use jiff::Timestamp;
+    ///
+    /// fn from_second_saturating(seconds: i64) -> Timestamp {
+    ///     Timestamp::from_second(seconds).unwrap_or_else(|_| {
+    ///         if seconds < 0 {
+    ///             Timestamp::MIN
+    ///         } else {
+    ///             Timestamp::MAX
+    ///         }
+    ///     })
+    /// }
+    ///
+    /// assert_eq!(from_second_saturating(0), Timestamp::UNIX_EPOCH);
+    /// assert_eq!(
+    ///     from_second_saturating(-999999999999999999),
+    ///     Timestamp::MIN
+    /// );
+    /// assert_eq!(
+    ///     from_second_saturating(999999999999999999),
+    ///     Timestamp::MAX
+    /// );
+    /// ```
     #[inline]
     pub fn from_second(second: i64) -> Result<Timestamp, Error> {
         Timestamp::new(second, 0)
@@ -598,6 +632,10 @@ impl Timestamp {
     /// timestamp outside of the [`Timestamp::MIN`] and [`Timestamp::MAX`]
     /// boundaries.
     ///
+    /// It is a semver guarantee that the only way for this to return an error
+    /// is if the given value is out of range. That is, when it is less than
+    /// `Timestamp::MIN` or greater than `Timestamp::MAX`.
+    ///
     /// # Example
     ///
     /// This example shows the instants in time 1 millisecond immediately after
@@ -617,6 +655,36 @@ impl Timestamp {
     ///
     /// # Ok::<(), Box<dyn std::error::Error>>(())
     /// ```
+    ///
+    /// # Example: saturating construction
+    ///
+    /// If you need a way to build a `Timestamp` value that saturates to
+    /// the minimum and maximum values supported by Jiff, then this is
+    /// guaranteed to work:
+    ///
+    /// ```
+    /// use jiff::Timestamp;
+    ///
+    /// fn from_millisecond_saturating(millis: i64) -> Timestamp {
+    ///     Timestamp::from_millisecond(millis).unwrap_or_else(|_| {
+    ///         if millis < 0 {
+    ///             Timestamp::MIN
+    ///         } else {
+    ///             Timestamp::MAX
+    ///         }
+    ///     })
+    /// }
+    ///
+    /// assert_eq!(from_millisecond_saturating(0), Timestamp::UNIX_EPOCH);
+    /// assert_eq!(
+    ///     from_millisecond_saturating(-999999999999999999),
+    ///     Timestamp::MIN
+    /// );
+    /// assert_eq!(
+    ///     from_millisecond_saturating(999999999999999999),
+    ///     Timestamp::MAX
+    /// );
+    /// ```
     #[inline]
     pub fn from_millisecond(millisecond: i64) -> Result<Timestamp, Error> {
         let millisecond = UnixMilliseconds::try_new128(
@@ -639,6 +707,10 @@ impl Timestamp {
     /// timestamp outside of the [`Timestamp::MIN`] and [`Timestamp::MAX`]
     /// boundaries.
     ///
+    /// It is a semver guarantee that the only way for this to return an error
+    /// is if the given value is out of range. That is, when it is less than
+    /// `Timestamp::MIN` or greater than `Timestamp::MAX`.
+    ///
     /// # Example
     ///
     /// This example shows the instants in time 1 microsecond immediately after
@@ -658,6 +730,36 @@ impl Timestamp {
     ///
     /// # Ok::<(), Box<dyn std::error::Error>>(())
     /// ```
+    ///
+    /// # Example: saturating construction
+    ///
+    /// If you need a way to build a `Timestamp` value that saturates to
+    /// the minimum and maximum values supported by Jiff, then this is
+    /// guaranteed to work:
+    ///
+    /// ```
+    /// use jiff::Timestamp;
+    ///
+    /// fn from_microsecond_saturating(micros: i64) -> Timestamp {
+    ///     Timestamp::from_microsecond(micros).unwrap_or_else(|_| {
+    ///         if micros < 0 {
+    ///             Timestamp::MIN
+    ///         } else {
+    ///             Timestamp::MAX
+    ///         }
+    ///     })
+    /// }
+    ///
+    /// assert_eq!(from_microsecond_saturating(0), Timestamp::UNIX_EPOCH);
+    /// assert_eq!(
+    ///     from_microsecond_saturating(-999999999999999999),
+    ///     Timestamp::MIN
+    /// );
+    /// assert_eq!(
+    ///     from_microsecond_saturating(999999999999999999),
+    ///     Timestamp::MAX
+    /// );
+    /// ```
     #[inline]
     pub fn from_microsecond(microsecond: i64) -> Result<Timestamp, Error> {
         let microsecond = UnixMicroseconds::try_new128(
@@ -680,6 +782,10 @@ impl Timestamp {
     /// timestamp outside of the [`Timestamp::MIN`] and [`Timestamp::MAX`]
     /// boundaries.
     ///
+    /// It is a semver guarantee that the only way for this to return an error
+    /// is if the given value is out of range. That is, when it is less than
+    /// `Timestamp::MIN` or greater than `Timestamp::MAX`.
+    ///
     /// # Example
     ///
     /// This example shows the instants in time 1 nanosecond immediately after
@@ -699,6 +805,36 @@ impl Timestamp {
     ///
     /// # Ok::<(), Box<dyn std::error::Error>>(())
     /// ```
+    ///
+    /// # Example: saturating construction
+    ///
+    /// If you need a way to build a `Timestamp` value that saturates to
+    /// the minimum and maximum values supported by Jiff, then this is
+    /// guaranteed to work:
+    ///
+    /// ```
+    /// use jiff::Timestamp;
+    ///
+    /// fn from_nanosecond_saturating(nanos: i128) -> Timestamp {
+    ///     Timestamp::from_nanosecond(nanos).unwrap_or_else(|_| {
+    ///         if nanos < 0 {
+    ///             Timestamp::MIN
+    ///         } else {
+    ///             Timestamp::MAX
+    ///         }
+    ///     })
+    /// }
+    ///
+    /// assert_eq!(from_nanosecond_saturating(0), Timestamp::UNIX_EPOCH);
+    /// assert_eq!(
+    ///     from_nanosecond_saturating(-9999999999999999999999999999999999),
+    ///     Timestamp::MIN
+    /// );
+    /// assert_eq!(
+    ///     from_nanosecond_saturating(9999999999999999999999999999999999),
+    ///     Timestamp::MAX
+    /// );
+    /// ```
     #[inline]
     pub fn from_nanosecond(nanosecond: i128) -> Result<Timestamp, Error> {
         let nanosecond =
@@ -717,6 +853,10 @@ impl Timestamp {
     /// This returns an error if the given duration corresponds to a timestamp
     /// outside of the [`Timestamp::MIN`] and [`Timestamp::MAX`] boundaries.
     ///
+    /// It is a semver guarantee that the only way for this to return an error
+    /// is if the given value is out of range. That is, when it is less than
+    /// `Timestamp::MIN` or greater than `Timestamp::MAX`.
+    ///
     /// # Example
     ///
     /// How one might construct a `Timestamp` from a `SystemTime`:
@@ -761,6 +901,39 @@ impl Timestamp {
     ///
     /// # Ok::<(), Box<dyn std::error::Error>>(())
     /// ```
+    ///
+    /// # Example: saturating construction
+    ///
+    /// If you need a way to build a `Timestamp` value that saturates to
+    /// the minimum and maximum values supported by Jiff, then this is
+    /// guaranteed to work:
+    ///
+    /// ```
+    /// use jiff::{SignedDuration, Timestamp};
+    ///
+    /// fn from_duration_saturating(dur: SignedDuration) -> Timestamp {
+    ///     Timestamp::from_duration(dur).unwrap_or_else(|_| {
+    ///         if dur.is_negative() {
+    ///             Timestamp::MIN
+    ///         } else {
+    ///             Timestamp::MAX
+    ///         }
+    ///     })
+    /// }
+    ///
+    /// assert_eq!(
+    ///     from_duration_saturating(SignedDuration::ZERO),
+    ///     Timestamp::UNIX_EPOCH,
+    /// );
+    /// assert_eq!(
+    ///     from_duration_saturating(SignedDuration::from_secs(-999999999999)),
+    ///     Timestamp::MIN
+    /// );
+    /// assert_eq!(
+    ///     from_duration_saturating(SignedDuration::from_secs(999999999999)),
+    ///     Timestamp::MAX
+    /// );
+    /// ```
     #[inline]
     pub fn from_duration(
         duration: SignedDuration,


Test patch: