Decimal::try_new is const fn, but not usefully so in certain cases

[SohumB]

Decimal::try_new is intended to be used in const contexts; but there's a glitch. The function declares itself as returning Result<Decimal, Error>, where Error is the general error type for the crate. However, some of those enums involve Strings, and any attempt to call try_new and match on it in a const context results in:

error[E0277]: can't drop `std::vec::Vec<u8>` in const contexts    
...
note: the trait `Destruct` is implemented for `std::vec::Vec<u8>`, but that implementation is not `const`                                                                                                            

(This precise error is due to me turning on a truckload of other const-related features, but the basic thrust of the error remains.)

This isn't actually a real issue, in that try_new doesn't actually construct any strings — the only error case it returns is ScaleExceedsMaximumPrecision which absolutely could be dropped in const contexts. The bugfix here would, however, require bumping semver, as at a minimum it would involve changing try_new's type.

I'm willing to submit a PR for this, but there's at least two distinct ways to break semver here, both with good arguments for it, so I didn't want to do so without opening a discussion first.

[c410-f3r]

Where the Vec<u8> came from? try_new does not perform any kind of heap allocation.

use rust_decimal::Decimal;

const N: rust_decimal::Result<Decimal> = {
    Decimal::try_new(i64::MAX, u32::MAX)
};

pub fn main() {
    dbg!(N.unwrap_err());
}

There are Strings in the Error enum because that is what was rooted and spread across the code way before its introduction. Besides, any other type of constant error is more related to the current limitations of the compiler rather than the method itself.

use rust_decimal::Decimal;

const _: () = {
    // Nope. Destructor is not allowed
    match Decimal::try_new(i64::MAX, u32::MAX) {
        Ok(elem) => Ok(elem),
        Err(err) => Err(err),
    };

    // Nope. No const unwrap Result
    Decimal::try_new(i64::MAX, u32::MAX).unwrap_err();

    // Nope. No const unwrap Option
    Decimal::try_new(i64::MAX, u32::MAX).ok().unwrap();
};

pub fn main() {
}

[SohumB]

Strings are Vec<u8>s under the hood. The compiler doesn't know try_new doesn't perform heap allocation, it merely is checking based on the return type of rust_decimal::Error. If try_new accurately reported the subset of rust_decimal::Error it uses, this error would not happen; otherwise, the compiler must needs assume that dropping rust_decimal::Error might require dropping a String and thus a Vec.

And yes, on current stable none of those work for larger reasons. But under const_option and const_result_drop, .ok().unwrap() would work if not for the String in the error type, and under const_precise_live_drops, the match statement would work too. This suggests that this issue will only become more relevant as those features are stabilised. Here's a quick demo.

#![feature(const_result_drop, const_option)]

enum ErrorWithString {
    Str(String),
    Num(i8)
}

enum ErrorWithoutString {
    Num(i8)
}

const fn pos(num: i8) -> Result<u8, ErrorWithoutString> {
    if num < 0 {
        return Err(ErrorWithoutString::Num(num))
    } else {
        return Ok(num.abs_diff(0))
    }
}

const fn pos2(num: i8) -> Result<u8, ErrorWithString> {
    if num < 0 {
        return Err(ErrorWithString::Num(num))
    } else {
        return Ok(num.abs_diff(0))
    }
}


pub const A: u8 = pos(5).ok().unwrap();
pub const B: u8 = pos2(5).ok().unwrap();

My recommendation would be to keep rust_decimal::Error as it is, and create a rust_decimal::ConstError type, that can be dropped in const contexts; perhaps with a From<ConstError> for Error impl. This has the minimal effect on most consumers of the library.

The other potential solution would be to modify rust_decimal::Error to replace ScaleExceedsMaximumPrecision with ConstError. While this would be technically nicer, in that under the previous suggestion ScaleExceedsMaximumPrecision is duplicated in both types, it would also have a larger impact on existing consumers.

[c410-f3r]

Oh, now I see what you meant. Vec<u8> wasn't showing up because of the nightly flags.

Beware of const_precise_live_drops and its drop order -> rust-lang/rust#73255 (comment).

The general feeling within Rustc development is that more concrete constant machinery (CTFE) will take some years to flourish.

My recommendation would be to keep rust_decimal::Error as it is, and create a rust_decimal::ConstError type, that can be dropped in const contexts; perhaps with a From for Error impl. This has the minimal effect on most consumers of the library.

The other potential solution would be to modify rust_decimal::Error to replace ScaleExceedsMaximumPrecision with ConstError. While this would be technically nicer, in that under the previous suggestion ScaleExceedsMaximumPrecision is duplicated in both types, it would also have a larger impact on existing consumers.

These are nice solutions and both have their own trade-offs. Not sure if Cow<'static, str> would help (probably not).

Well, ultimately, it is all up to @paupino :)

[paupino]

This all makes sense and is something we should be thinking towards. Honestly, ergonomic error handling was a bit of an afterthought when this library was first started in 2015 (i.e. it used to return String errors directly hence why we still have Error::ErrorString).

I'll have to have a bit of think through the options here since there may be a bit of a trade off between future optimizations and backwards compatibility. I'm not a big fan of #528 namely because it exposes some of the internal library visibility however am open to start designing the error library to what it should look like - even if it is feature flagged for now. Since it is a nightly only feature right now - perhaps that's reasonable?

[SohumB]

I see. If this would require a semver break anyway, maybe it would be worthwhile to just restructure the Error type to completely remove the String cases, while also making sure that it's const-droppable? I'd be happy to submit a PR for that.

As for #528 — I understand, and I also would very much prefer that impl const was stable. Would it be acceptable if I limited that PR to just finding the const-safe rewrites of the underlying functions, even though we can't support impl Const yet? (For my particular usecase I guess I can just copy/paste the const functions for now.)

[c410-f3r]

I see. If this would require a semver break anyway, maybe it would be worthwhile to just restructure the Error type to completely remove the String cases, while also making sure that it's const-droppable? I'd be happy to submit a PR for that.

Not sure if all variants can be converted but it is probably worth a try. If it is OK to @paupino , I can help you in this task.

[paupino]

Sorry: a very delayed reply!

Ultimately, I'd like to start development towards v2 with a focus towards how the library could become simpler/lighter/more efficient/ergonomic by default. If it means starting work towards major version changes then I'm certainly open to that. Depending on the size/complexity of the changes then either a branch or a feature flag could work... but that's really open to experimentation. A while back I started a version/2 branch for experimentation however I wonder if it's worthwhile recreating that and properly planning what a good v2 api could look like? i.e. Starting from Error.

Anyway, certainly open to any assistance one may be willing to offer!

[c410-f3r]

In regards to configurable precision and/or configurable size, https://gitlab.com/tspiteri/fixed is stating that a new release is blocked by generic_const_exprs but generic_const_exprs won't be stabilized in the near future. Makes me wonder what a 2.0 release would look like with the current compiler limitations.

[pymongo]

Any update for this issue? I want to define a const Decimal like const ONE_MILLION: Decimal = Decimal::try_new(1000000,0).unwrap()

but this crate is #![forbid(unsafe_code)] can't provide a unsafe const new function like decimal_rs crate's from_parts_unchecked

https://docs.rs/decimal-rs/latest/decimal_rs/struct.Decimal.html#method.from_parts_unchecked

[Tony-Samuels]

If you just want to unwrap it immediately, maybe use the dec! macro?