SUPERCILEX / io-adapters

Adapters to convert between different writable APIs.
https://crates.io/crates/io-adapters
Apache License 2.0
21 stars 1 forks source link

Improved error handling #4

Open Hedgehogo opened 1 week ago

Hedgehogo commented 1 week ago

Add an fmt to io adapter that accepts an object whose type implements FnMut(std::io::Error) -> std::fmt::Error as one of the private fields. This object will be used to convert the error.

pub struct FmtToIo<W, F> {
    inner: W,
    convert_error: F,
}

impl<W, F> fmt::Write for FmtToIo<W, F>
where
    W: io::Write,
    F: FnMut(io::Error) -> fmt::Error 
{
    fn write_str(&mut self, s: &str) -> fmt::Result {
        self.inner.write_all(s.as_bytes()).map_err(&mut self.convert_error)
    }
}

This will force the user to handle any error and not forget about it, also it will allow to do some actions on every error that comes up and for example collect them into a list. This is useful if I passed an adapter to some library function.

It might be worth making such an API directly for FmtToIo, but that would break the API.

P.S. I will make a pull request if the proposal as a whole is approved.

SUPERCILEX commented 1 week ago

So the original design idea was that you'd match on the format error and then read the error field to get details. Any reason that doesn't work? Ideally I'd like to see an example of what you're trying to do and how it would work with the old and proposed approach.

As for this proposal, if we do go through with it I'd like to see an inspect_err: io::Error -> () function on FmtToIo which defaults to doing nothing. We could possibly also return a result so you can consume the error but I don't know how you'd use that.

SUPERCILEX commented 1 week ago

Actually I think we could have both approaches with yet another wrapper:

struct FmtToIoErrWrapper {
  conv: FmtToIo,
  inspect_err: FnMut(io::Error)
}

Then we implement fmt::Write where we match on the error, read the saved source error from FmtToIo and call the function on it. And honestly not sure if it makes sense to put that in this lib but might be convenient.

Hedgehogo commented 1 week ago

So the original design idea was that you'd match on the format error and then read the error field to get details. Any reason that doesn't work? Ideally I'd like to see an example of what you're trying to do and how it would work with the old and proposed approach.

I have some function that takes impl fmt::Write. It can call it an unlimited number of times.

fn foo(writer: impl fmt::Write) -> fmt::Result {
    write!(&mut writer, "Hello")?;
    write!(&mut writer, "World")
}

It can be a trait method (in my case it is) or a function of a connected library. Passing the adapter there, in both cases I will get only the last error, without the possibility to handle the rest.

In addition, the original approach does not induce the user of the library to handle the error in any way, and the user can simply forget about it, which will not cause an error, and it would be desirable that such moments are monitored more strictly.

SUPERCILEX commented 1 week ago

What do you mean by "last error"? In your example there's one and only one error as the ? returns early. I could see a function that ignores errors losing the intermediate ones but then you're ignoring errors anyway so I'm not sure why you would want to check the source of the ignored errors.

I do agree that it's unfortunate you're led to ignore the underlying error by default. My beef with the closure idea is that it isn't all that useful IMO—if I'm handling errors, what I probably want to do is catch the fake fmt error and then return the true error, but you can't return from the outer function from within a closure so your only option with the closure is logging it seems. Maybe we can provide some kind of fmt error to up error conversion. The next problem is that it's perfectly possible to get both a formatting error and an io error from the same callsite so you need to handle both which means the return type needs to be an enum. So maybe a macro which does the match for you but expects your error type to support both errors would be more helpful? I just don't know. What were you planning on doing with the error in the closure?