google / zerocopy

https://discord.gg/MAvWH2R6zk
Apache License 2.0
1.61k stars 105 forks source link

`Error` impls seem to require unneccessary bounds #1874

Open AaronKutch opened 1 month ago

AaronKutch commented 1 month ago

When trying to use the errors in certain ways, I get errors with certain error frameworks because simple things will not enable the Error impls such as

impl<Src, Dst> Error for SizeError<Src, Dst>
where
    Src: Deref
    Dst: KnownLayout+ ?Sized

Are these bounds really needed? It causes issues where I can't propogate errors in simple cases:

use std::error::Error;

use zerocopy::{FromBytes, Immutable, IntoBytes, KnownLayout, Unaligned, LE, U32};

#[derive(
    Debug, Clone, Copy, Hash, PartialEq, Eq, FromBytes, IntoBytes, KnownLayout, Immutable, Unaligned,
)]
#[repr(C)]
pub struct Test {
    pub test: U32<LE>,
}

fn impls_err(_err: impl Error) {}

fn main() {
    let mut buf = vec![0u8; 32];
    let buf = &mut buf;

    impls_err(Test::ref_from_bytes(&buf[..4]).unwrap_err().map_src(drop));
}

which gives the error

error[E0277]: the trait bound `(): Deref` is not satisfied
  --> src/main.rs:19:15
   |
19 |     impls_err(Test::ref_from_bytes(&buf[..4]).unwrap_err().map_src(drop));
   |     --------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Deref` is not implemented for `()`, which is required by `ConvertError<AlignmentError<(), Test>, SizeError<(), Test>, Infallible>: std::error::Error`
   |     |
   |     required by a bound introduced by this call
   |
   = help: the trait `std::error::Error` is implemented for `ConvertError<A, S, V>`
   = note: required for `AlignmentError<(), Test>` to implement `std::fmt::Display`
   = note: required for `ConvertError<AlignmentError<(), Test>, SizeError<(), Test>, Infallible>` to implement `std::error::Error`
note: required by a bound in `impls_err`
  --> src/main.rs:13:25
   |
13 | fn impls_err(_err: impl Error) {}
   |                         ^^^^^ required by this bound in `impls_err`

There are other impls that have the same problem

jswrenn commented 1 month ago

After you bubble up the error, are you reflecting on the structure of the error (e.g., downcasting it back to its concrete type and matching on it) or are you just pretty-printing it as a human readable message?

If it's the latter, then instead we recommend eagerly calling .to_string() on the error, and bubbling up that message instead. For example, if you were use eyre, you might do: .map_err(|error| Report::msg(error.to_string())).

AaronKutch commented 1 month ago

No there is nothing particularly special I am doing, various error frameworks take anything that implements Error. The new error types have a Src that can be very useful, but due to lifetime constraints it often cannot be returned. The documentation notes this and explicitly recommends .map_src(drop), but due to the strange bounds (the traits don't seem to actually be used unless I am missing something obvious?) it does not implement Error. I could just use .ok() and propogate a None instead, but it would be nice to have the new extra error information. I also definitely do not want to allocate strings in my use case. The definition in the source code is

impl<Src, Dst: ?Sized> Error for AlignmentError<Src, Dst>
where
    Src: Deref,
    Dst: KnownLayout,
{
}

but why is the Deref bound required in the first place? Maybe there is some alternate error format that can easily be converted to with some other function on the CastError type? Have a shorthand for .map_src(drop) or whatever it is?

jswrenn commented 1 month ago

For AlignmentError, we require Deref so we can print the source address and what power of 2 it's a multiple of.