Clippy complains my error type is too big; what is the best solution?

[jbe]

I ran clippy on my crate sandkiste and got a lot of warnings regarding this type

pub struct MachineError {
    pub kind: MachineErrorKind,
    pub message: Option<String>,
    pub machine_backtrace: Option<String>,
    pub message_incl_chunk_name: bool,
    pub chunk_name: Option<String>,
    pub message_incl_pos: bool,
    pub line: Option<usize>,
    pub column: Option<usize>,
    pub position: Option<usize>,
}

(docs)

being too big.

warning: the `Err`-variant returned from this function is very large
   --> src/types.rs:287:28
    |
287 |     fn array_len(&self) -> Result<usize, MachineError>;
    |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `Err`-variant is at least 128 bytes
    |
    = help: try reducing the size of `errors::MachineError`, for example by boxing large elements or replacing it with `Box<errors::MachineError>`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
    = note: `#[warn(clippy::result_large_err)]` on by default

I guess I can't solve this without breaking semver compatibility (which isn't a huge problem for me, though, at this point).

But what's the best way to go? Change all functions to return a Result<T, Box<MachineError>> instead? Or should I modify the MachineError type somehow, e.g. by making an inner type, and then box that inner type? Which way is the idiomatic one? Or am I doing something fundamentally bad in regard to the error type design?

[sfackler]

Tracking useful contextual information in an error type is definitely a good way to go, you just want to keep the on-stack size of the error type low to avoid a bunch of memcpy overhead as it's propagated around.

I would do something like

pub struct MachineError(Box<Inner>);

struct Inner {
    kind: MachineErrorKind,
    // ...
}

impl MachineError {
    pub fn kind(&self) -> MachineErrorKind {
        self.0.kind
    }

    // ...
}

[tguichaoua]

With the new type pattern you can wrap a Box with your inner error type and use Deref to allow acces to the fields or you can implements getters if you don't want to expose the inner type.

pub struct MachineError(Box<ErrorImpl>);

Here's an example from serde_json

error.rs - source

[garagedragon]

I think a first question to ask that you seem to have skipped over is, do you care? Checking the playground, MachineError is exactly 128 bytes which is a trivial amount unless you're targeting embedded systems or extremely hot loops. Sure you're taking up that space even in the Ok case, but unless you are very constrained for space, you could simply turn up large-error-threshold and be done. Rust makes it easy to write extremely tight, space-efficient code, but that doesn't mean we have to.

Additionally, while you would have to break compatibility, it might help to tighten the possible values your type represents. For instance, I assume it's not possible for an error to have a reported column without a line number. Additionally, the message_incl_pos looks like it's already implied by whether or not the position values are Some, so it doesn't need to be stored separately. Refactoring the position out saves 16 bytes immediately.

[jbe]

@sfackler

I would do something like

pub struct MachineError(Box<Inner>);

That would require me to write getters and setters (or some sort of constructor functions or builder types), right? I tried to keep things as simple as possible by simply exposing the fields as pub.

@tguichaoua

With the new type pattern you can wrap a Box with your inner error type and use Deref to allow acces to the fields or you can implements getters if you don't want to expose the inner type.

pub struct MachineError(Box<ErrorImpl>);

Don't I need to make the inner type public if I want to use Deref?

use std::ops::Deref;

struct Inner {
}

pub struct Outer {
    inner: Inner,
}

impl Deref for Outer {
    type Target = Inner;
    fn deref(&self) -> &Inner {
        &self.inner
    }
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0446]: private type `Inner` in public interface
  --> src/lib.rs:11:5
   |
3  | struct Inner {
   | ------------ `Inner` declared as private
...
11 |     type Target = Inner;
   |     ^^^^^^^^^^^ can't leak private type

warning: private type `Inner` in public interface (error E0446)
  --> src/lib.rs:12:5
   |
12 |     fn deref(&self) -> &Inner {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #34537 <https://github.com/rust-lang/rust/issues/34537>
   = note: `#[warn(private_in_public)]` on by default

For more information about this error, try `rustc --explain E0446`.
warning: `playground` (lib) generated 1 warning
error: could not compile `playground` due to previous error; 1 warning emitted

@tguichaoua

Here's an example from serde_json

error.rs - source

In that example the inner type is private, and I don't see Deref being used there.

@garagedragon

I think a first question to ask that you seem to have skipped over is, do you care? Checking the playground, MachineError is exactly 128 bytes which is a trivial amount unless you're targeting embedded systems or extremely hot loops.

Well, I target running Lua code, and Lua is a pretty lightweight language. I would like to avoid overhead where it isn't too painful. I could also imagine scenarios where short Lua snippets are executed a lot of times, so I guess that's the "hot loop" scenario you're talking about. But I take that 128 bytes is not that bad yet.

@garagedragon

Additionally, while you would have to break compatibility, it might help to tighten the possible values your type represents. For instance, I assume it's not possible for an error to have a reported column without a line number.

But I have scenarios where I might only have a line number reported and no column.

@garagedragon

Additionally, the message_incl_pos looks like it's already implied by whether or not the position values are Some, so it doesn't need to be stored separately.

Not quite, because depending on the implementor, a message might include the position but I might still want to include the position as additional data. However, when rendering the error, I don't want to include the information twice. So that field really isn't redundant.

[tguichaoua]

@jbe

Don't I need to make the inner type public if I want to use Deref?

@jbe

In that example the inner type is private, and I don't see Deref being used there.

Either you make the inner type pub and you can use Deref or either you keep it private but you have to implements getters on the outer type.

[cuviper]

Remember that the size of a Result (or any enum) is at least as big as the largest variant, and you have to copy/move that whole size every time. So even small Ok values will have to copy a lot if the Err is large.

[Hyeonu]

@garagedragon

MachineError is exactly 128 bytes which is a trivial amount

It may not be trivial because unlike memory, registers are still precious even on decent machines.

Compiler Explorer - Rust (rustc 1.68.0)

You can see Result<usize, usize> can be returned completely by register while Result<usize, u128> is passed via stack. Does it really matter? It depends. The stack space used here is almost always on L1 cache. And any problem related with ABI wiped out when the function get inlined. But usually there's no significant reason not to box the error case.

[kpreid]

@tguichaoua

With the new type pattern you can wrap a Box with your inner error type and use Deref

You don't even need a newtype. Box<MachineError> will implement Error, so Result<T, Box<MachineError>> is entirely usable. Yes, it exposes the box, but that is no more disruptive than any other choice, presuming that the inner struct is to remain visible for pattern matching.

[jbe]

I feel like exposing the Box may be easiest? And also idiomatic if I "just" have a bunch of pub fields in my error?

[garagedragon]

@jbe

Well, I target running Lua code , and Lua is a pretty lightweight language. I would like to avoid overhead where it isn't too painful. I could also imagine scenarios where short Lua snippets are executed a lot of times, so I guess that's the "hot loop" scenario you're talking about. But I take that 128 bytes is not that bad yet.

That's fair enough, I just think it's important to ask the question even if in this case the answer is yes.

@Hyeonu

It may not be trivial because unlike memory, registers are still precious even on decent machines.

While you're not wrong, I think that's just another aspect of whether to care about the performance difference. Spilling to L1 vs registers is a minor difference in the same way that 128 bytes is a minor amount of memory - not none, but small enough that IMO there should be special consideration about why we're going out of the way to avoid it.

@Hyeonu

But usually there's no significant reason not to box the error case.

You can't (currently) pattern-match through boxes, so I'd disagree "box it as soon as clippy complains" is a good strategy without further consideration of what you're gaining for that loss of ergonomics, but as I said, fair enough if the answer to that is, "yes we do want to save the extra ns"

@jbe

I feel like exposing the Box may be easiest? And also idiomatic if I "just" have a bunch of pub fields in my error?

It's worth keeping in mind that exposing fields does constrain you if/when you need to maintain compatibility with existing clients, e.g. if you'd had getters rather than pub fields, you could do things like refactor fields to consolidate the Option discriminants without affecting clients at all. Since you've already done that though, you might as well continue, it's not unidiomatic for a PDO.

[H2CO3]

@jbe

I could also imagine scenarios where short Lua snippets are executed a lot of times, so I guess that's the "hot loop" scenario you're talking about.

I would be extremely surprised if memcpying 128 bytes wouldn't be dominated into oblivion by executing any sort of code in an interpreted (non-JIT-ed) language. Running the Lua code will likely incur several heap allocations and a ton of branching in the first place, so copying even a few kBs or dozens of kBs should not be comparable to the time required for that.

Edit: I performed a quick and dirty benchmark. I ran a trivial Lua chunk that simply returns an integer, and prepared two versions of the code running it: one with the construction of a big error value, and one without. I ran the chunk in both ways 100'000 times in release mode, and repeated the test 100 times, then timed the inner hot loop using std::time::{Instant, Duration}.

My timings were the following, in µs:

statistic	big error present	no big error
mean	13846.1	13736.6
std	712.1	471.8

for which a paired-sample one-sided T-test gives a P-value of 0.06731436706906734.

The code of the test can be found here.

[jbe]

@garagedragon

It's worth keeping in mind that exposing fields does constrain you if/when you need to maintain compatibility with existing clients, e.g. if you'd had getters rather than pub fields, you could do things like refactor fields to consolidate the Option discriminants without affecting clients at all. Since you've already done that though, you might as well continue, it's not unidiomatic for a PDO.

The API will be used both by providers and consumers of the error type, so I guess if I want to achieve that, I need a fully fledged builder pattern rather than "just" getters. But maybe I should go that way.

---

@H2CO3

My timings were the following, in µs:

statistic	big error present	no big error
mean	13846.1	13736.6
std	712.1	471.8

for which a paired-sample one-sided T-test gives a P-value of 0.06731436706906734.

Thanks for testing. I think the p-value isn't relevant that much (it will show more significance if you let the test run longer). I think the effect size as well as the absolute average overhead in relation to typical program execution is more relevant. But I do see that this is small indeed. So maybe the issue is not worth much thinking about.

Anyway, I'd also like to know what's generally the best approach for this problem (i.e. too big error types). Refactoring the type, or exposing Box? I guess exposing the Box, in theory, isn't the worst. Maybe it becomes unergonomic, but it also allows you to unbox if you want or use Box's methods on it.

[H2CO3]

@jbe

Thanks for testing. I think the p-value isn't relevant that much (it will show more significance if you let the test run longer). I think the effect size as well as the absolute average overhead in relation to typical program execution is more relevant.

Both of those assertions are correct. I was not necessarily expecting people to be familiar with effect sizes, though (P-values are more commonly recognized).

If I compute a Cohen's D from the pooled variance, I get about 0.128…0.181 (depending on whether or not I apply Bessel's correction), which feels small-ish in this case.

[garagedragon]

@jbe

The API will be used both by providers and consumers of the error type, so I guess if I want to achieve that, I need a fully fledged builder pattern rather than "just" getters. But maybe I should go that way.

FWIW, there's a crate for automatically producing that out of the struct definition

[emy]

Unconventional, but you could also consider using a C-style out parameter:

pub struct MyError {
    pub stuff: [u64; 16]
}

pub fn array_len<'a>(arr: &[u8], errinfo: &'a mut Option<MyError>)
    -> Result<usize, &'a mut MyError>
{
    let ret = arr.len();
    if ret >= 20 { // whatever the error condition is
        *errinfo = Some(MyError{ stuff: [42; 16] });
        Err(errinfo.as_mut().unwrap())
    }else{
        Ok(ret)        
    }
}

Stashing a reference to it in the result gives it similar ergonomics to normal:

pub fn use_array_len() -> usize {
    let foo = b"hello!";
    let mut errinfo = None;
    match array_len(&foo[..], &mut errinfo) {
        Ok(len) => len,
        Err(errinfo) => {
            let stuff = errinfo.stuff;
            todo!();
        }
    }
}

[jbe]

I noticed that I currently use constructions like this in (the only) downstream crate:

MachineError {
    message: Some("lua_Integer too small to store array index".to_string()),
    ..Default::default()
}

(source)

If MachineError was #[non_exhaustive] (which might be a good idea for forward compatibility), then I could not use Default::default() anymore. So maybe I need builder methods anyway. I don't like that this is so verbose in Rust (see link). But I guess it's the way it is for now.

But if I end up with builder methods, do I still expose the fact that everything is Boxed, e.g. by making an #[non_exhaustive] struct MachineErrorUnboxed?

#[non_exhaustive]
pub struct MyErrorUnboxed {
    pub field1: T1,
    pub field2: T2,
    …
}

pub type MyError = Box<MyErrorUnboxed>;

Or should I rather hide details?

struct MyErrorImpl {
    field1: T1,
    field2: T2,
    …
}

pub struct MyError(Box<MyErrorImpl>);

What's more idiomatic in your opinion?

[kornel]

You can make the type itself a bit smaller by using smaller field types.

• String → Box<str>. This saves one usize for redundant capacity.
• Option<usize> → Option<NonZeroUsize> or even Option<NonZeroU32> if you don't work with data larger than 4GB. Non-zero avoids large padding for Option's boolean.

[parasyte]

@jbe

I don't like that this is so verbose in Rust (see link). But I guess it's the way it is for now.

Arguably it should probably be more verbose for the rest of time if that verbosity aids in reabality and preservation of intent. It turns out that playing code golf just leads to write-only applications.

@jbe

What's more idiomatic in your opinion?

The newtype will be harder to use in other modules unless you add boilerplate like accessor methods for the inner fields (or even impl Deref ). On the other hand, the type alias means that pattern matching won't work as naturally as a struct reference.

Per usual, there are tradeoffs.