Error handling of distributions::Uniform::new

[dhardy]

Currently, Uniform::new will panic on invalid parameters:

• low >= high (or > for new_inclusive)
• (float, debug only) non-finite low or high
• (float) non-finite scale = high - low

Since #581 / #770, all other distributions with fallible constructor return a Result instead of panic (I think).

For: consistency and utility (error checking in derived types)

Against: this is another significant breaking change, when we hope to be getting close to stable (though now is certainly better than later)

Alternative: try_new, try_new_inclusive constructors - but this is inconsistent and results in far too many methods.

Also related: Rng::gen_range uses this. This should probably still panic on failure (note also Rng::fill vs Rng::try_fill; Rng::gen_bool and Rng::gen_ratio which may panic).

I suggest this error type:

enum Error {
    /// Low > high, or equal in case of exclusive range
    EmptyRange,
    /// Input or range (high - low) is non-finite. Not relevant to integer types. In release mode only the range is checked.
    NonFinite,
}

Link: #1165

[dhardy]

Follow up change: WeightedIndex::new and assign_weights (whatever it gets called) should be updated to account for Uniform::new returning a Result.

[kazcw]

I agree that this should be done now or never, and IMO it's better to have a breaking change now than have this be an inconsistency in the API forever.

---

If the error type exposes the distinction between EmptyRange and NonFinite, we should assume people will rely on that distinction, and commit to a policy about which error code will be returned when both have occurred. Preferably, this policy should be the same between debug and release builds, without imposing a performance burden that we currently avoid by making finer distinctions in debug_asserts only. I think designing and implementing such a policy would be possible, but not trivial.

I propose avoiding that hullabaloo by returning an opaque error type. The type's debug output can include details about the exact error encountered, which would be as informative to a debugging developer as the current assert approach; if it's ever decided in the future that users actually do need to distinguish these cases programmatically, an accessor could be added in a backward-compatible manner in the style of io::ErrorKind.

[SWW13]

When using WeightedIndex Uniform::new: range overflow is a rather surprising panic with no direct mention in the documentation. The panic is painful to debug compared to a Result, so there should be a panic free way to use all random distributions.

[vks]

@kazcw Unfortunately, we have to make the error type public if we want to allow other crates to add support for their types to Uniform by implementing UniformSampler.

I'm not sure how often that is used in practice, but removing this option would be a regression.

[kazcw]

The solution does not need to preclude other crates from implementing UniformSampler.

The plan is to make UniformSampler::new fallible, right? Presumably something like this:

pub trait UniformSampler {
    // The possible errors vary for different types, and we cannot anticipate the classes of errors
    // for all implementors of the trait, so this is an associated type.
    type Error: Display;
    fn new<B1, B2>(low: B1, high: B2) -> Result<Self, Self::Error>;
    // ...
}

impl<X: SampleUniform> Uniform<X> {
    fn new<B1, B2>(low: B1, high: B2) -> Result<Self, X::Error> { ... }
    // ...
}

pub struct InvalidFloatRange(FloatRangeError);
enum FloatRangeError {
    Empty,
    Infinite,
}
impl Display for FloatRangeError {
    // ...
}
// In the macro defining floating point samplers:
impl UniformSampler for UniformFloat<$Ty> {
    type Error = FloatRangeError;
    // ...
}

pub struct EmptyRange;
// In the macro defining integer samplers:
impl UniformSampler for UniformInt<$Ty> {
    type Error = EmptyRange;
    // ...
}

So, the question is whether, in a case like InvalidFloatRange where more than one reason for failure is possible, we:

1. Hide the specific reasons for the provided implementations' errors (as above), and only use that information to produce messages for debugging purposes.
2. Programmatically expose the reason for failure.

My point earlier was, if we go with (2), we should have a consistent and well-documented precedence of errors when multiple failures are possible; but i can't picture much need for programmatic inspection of the error here, and (1) would be easier.

[dhardy]

I agree with @kazcw that we likely don't need to programmatically expose error details.

But, in this case, why have an associated Error type?

We could do the following, or store Box<dyn Error> cause. (Though about the only thing these cfgs save is allowing the cause strings/types to be optimised out of release binaries.)

pub struct Error {
    #[cfg(debug_assertions)]
    cause: &'static str,
}
impl Error {
    pub fn new(cause: &'static str) -> Self {
        #[cfg(not(debug_assertions))] {
            let _ = cause;
        }
        Error {
            #[cfg(debug_assertions)]
            cause,
        }
    }
}
impl std::fmt::Display for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        #[cfg(debug_assertions)] {
            write!(f, "invalid range: {}", self.cause)
        }
        #[cfg(not(debug_assertions))] {
            write!(f, "invalid range")
        }
    }
}