Replace asserts with Result<T, E>

[freakyzoidberg]

The library should be mindful that the input may not be valid for various reasons.

We generally avoid relying in assert/panics/abort in the library.

The Rust implementation should most likely favor Result<T, E> return instead of assert!

[notfilippo]

For now we've used unreachable! for invariants, which I reckon is a correct way of handling dev errors. The concrete use case we have for this is an InvalidParameter when creating an HllSketch and HllUnion.

[tisonkun]

assert! is used for checking invariants. When an assertion fails, it means an internal bug, and the program should crash instead of continuing in undefined behavior.

It's impossible to change it to Result<T, E>. Because it should never fail, if you know the bug, you will fix it rather than return Err(Bug).

I do understand some situations where we should avoid untrusted input can crash your program, like in deserialization. One should always use Err(SerdeError) rather than panic (here is a case apache/arrow-rs#8931 ¹), because they are known failure cases.

Footnotes

1. And in https://github.com/apache/datasketches-rust/pull/23/changes/243dc289b4e65d7b1df74c206570ec7fbc8242f7 before @leerho hints me we can filter out all inf/-inf/nan. Since the bad data can be constructed by users rather than the programmer, I'd rather return garbage on garbage input than crash. But fortunately, we can get rid of those cases, and I'll use unreachable there again. ↩

[tisonkun]

That is, code like NonZeroUsize::new(1).unwrap() should be acceptable. Nothing is expected to happen on the None variant because it won't happen.

Actually, when you're using Rust's stdlib, there are already many assert! inside (a[idx] would panic if index out of bound). Crates like nonempty-collections hide the assert/unwrap for you because it keeps the contracts internally. If they can write sound code with assertions to check invariants, we should be able to do that as well.

[tisonkun]

Take another example, we now have:

impl HllSketch {
    /// # Panics
    ///
    /// If lg_config_k is not in range [4, 21]
    pub fn new(lg_config_k: u8, hll_type: HllType) -> Self {
        assert!(
            lg_config_k > 4 && lg_config_k < 21,
            "lg_config_k must be in [4, 21]"
        );
    }
}

(N.B. @notfilippo This seems a bug that it should be lg_config_k >= 4 && lg_config_k <= 21)

Users will write:

// check lg_config_k before hand by any means; typically, it is a user specified constant.
// then ..
let hll = HllSketch::new(DEFAULT_HLL_K);

What if we change the assert to Result<Self, E>?

impl HllSketch {
    pub fn new(lg_config_k: u8, hll_type: HllType) -> Result<Self, SomeError> {
        if lg_config_k < 4 || lg_config_k > 21 { return SomeError; }
    }
}

Then users will write:

// check lg_config_k before hand by any means
let hll = HllSketch::new(DEFAULT_HLL_K).unwrap();

Note that we do check k on deserialization:

        // Verify lg_k range (4-21 are valid)
        if !(4..=21).contains(&lg_config_k) {
            return Err(SerdeError::InvalidParameter(format!(
                "lg_k must be in [4; 21], got {}",
                lg_config_k
            )));
        }

Some Rust method foo would have a try_foo variant to return SomeError here instead of letting the caller check the contract (they still will, but it is a type hint to read the error doc rather than you should know to read the panic doc). But in many cases, the contract can be checked.

[notfilippo]

(N.B. @notfilippo This seems a bug that it should be lg_config_k >= 4 && lg_config_k <= 21)

Just fixed in main, caught it with a test! 4f5f266

[notfilippo]

Would there be any reason for a user to dynamically set lg_config_k instead of setting it via a constant?

[tisonkun]

Would there be any reason for a user to dynamically set lg_config_k instead of setting it via a constant?

It's technically viable, so as datasketches-rust becomes more popular, someone will use it in this way. But then it's the user's responsibility to check the data they accept.

[tisonkun]

That said, I totally agree to use Result<T, E> for known fallible cases.

But I've experienced a Result everywhere policy applied once, even in cases where it shouldn't. In that story, the codebase ends up with unwrap everywhere and panic remains, even more subtle to debug.

One reason here is that Rust doesn't have a global ancient Exception/RuntimeException class. It doesn't have error interface as in Go. So converting ErrorTypeA to ErrorTypeB among code paths is very awkward. You can use anyhow but then you just know "something happen" rather than handling the Error when you can.

One might consider creating a large error enum to cover all possible fallible cases in the codebase, but that would result in a scary big mess. Actually, I'm feeling the current SerdeError a bit over-designed already. Because what users can do for every variant is know that the data is illegal. It can have only one variant with a (debug) cause message to be printed. (But let's keep it as is for now. It's still OK.)

However, it's possible to design an Error type for a Rust library and use it library-wise. Apache OpenDAL's Error does a good job of covering all common causes when accessing a storage. datasketches-rust may find such an Error, so most of the fallible methods can use that Error.

Besides, Rust has more tools to handle fallible cases. For example, in the TDigest PR, I return None if the TDigest is empty on rank/quantile/cdf/pmf/max_value/min_value, while in C++ and Java impl, they throw a std::runtime_error / SketchesStateException extends RuntimeException.

[freakyzoidberg]

Sorry, yeah I may have been too directive here.

What I think we want is guarantee that we cannot assert/panic due to corrupted/bad input

For logical errors that can happen independently of the input data - then yes - and we do have a few of those FWIW

[tisonkun]

Sorry, yeah I may have been too directive here.

What I think we want is guarantee that we cannot assert/panic due to corrupted/bad input

For logical errors that can happen independently of the input data - then yes - and we do have a few of those FWIW

Agree.

I think what we can do here, besides a consensus now, is to document it as a DESIGN.md/STYLE.md or even in AGENTS.md for discoverability :D

[tisonkun]

@freakyzoidberg we may close this issue as consensus achieved.

Or any other action you expect to do?

[freakyzoidberg]

sounds good. thanks