Stream: project-error-handling

Topic: track_caller error crate


view this post on Zulip Ben Reeves (Sep 01 2021 at 20:17):

In response to https://github.com/rust-lang/rust/issues/87401, I'd like to start a conversation on error tracing.

Motivation

Libraries like anyhow provide easy access to backtraces when producing error values. However, there are a few major problems with using runtime backtrace collection:

  1. A backtrace only provides call stack information for the current thread. At my work, errors often pass from one thread to another on their way up to their final consumer.
  2. Runtime backtrace collection can be complicated and expensive. At my work, errors are common, so this performance overhead is meaningful.

Propagation Tracking

The solution is something I'm calling error propagation tracking, which differs from error backtracing. Rather than taking a point-in-time snapshot from the top of some thread's stack, it assembles a backtrace incrementally as the actual error object propagates through different parts of the code.

At my work, we do this in our C code by wrapping all return values in a special forward_error macro that appends the current __func__, __file__, and __line__ to a running stack of code locations.

I have experimented with implementing propagation tracking in Rust by writing a wrapper around std::result::Result, adding #[track_caller] to from_residual, and pushing a panic::Location onto a running stack each time an error value is propagated via ?.

Discussion

Here's what I want to do here:

I could start by cleaning up the experiment I wrote at work and putting that up on GitHub for folks to see how I approached it. Sound good?

See also this topic I started in #t-compiler regarding getting function names from panic::Location.

view this post on Zulip Jane Lusby (Sep 01 2021 at 22:02):

Sound good?

Sounds like a perfect first step

view this post on Zulip Ben Reeves (Sep 02 2021 at 20:00):

https://github.com/BGR360/propagate

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:24):

Ben Reeves said:

https://github.com/BGR360/propagate

sorry I didn't have chance to review this yesterday, taking a look at it now

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:24):

first thing that sticks out to me is the new_err method

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:24):

I really want to find ways to structure the usage so that this is never necessary

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:25):

my instinct is that we should try relying heavily on try { } blocks

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:26):

and then have the from_residual impl handle constructing the error stack

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:26):

and rely on people to just use Err(error)? when they need to return a new error rather than using Resuilt::new_err(error)

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:27):

Also I expect it will be better received if the recommended workflow isn't to import and override the std::result::Result type

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:27):

We've talked about this in libs API discussions before and our general recommendation is to not introduce new identifiers that overlap with identifiers in the std and core preludes

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:28):

because these can cause significant confusion when new devs encounter the source code and can be ambiguous in certain debugging situations

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:28):

having a prelude for the crate is fine, but it shouldn't bring in things as Result, probably better to stick to something like TrackedResult or TracedResult

view this post on Zulip Jane Lusby (Sep 03 2021 at 18:29):

ill try using this in one of my personal projects and see how I feel about the rest of the API and play with it a bit

view this post on Zulip Jane Lusby (Sep 03 2021 at 21:01):

also it looks like a lot of the doc tests are broken

view this post on Zulip Jane Lusby (Sep 03 2021 at 21:02):

curious why you set doctest = false

view this post on Zulip Jane Lusby (Sep 03 2021 at 22:44):

actually, after playing with this a bunch this might be a good candidate for another module in trial-and-error

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:05):

Jane Lusby said:

curious why you set doctest = false

was too lazy to make the doctests compile

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:06):

Jane Lusby said:

my instinct is that we should try relying heavily on try { } blocks

this is something I'll need to read up on, have never touched try

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:07):

Ben Reeves said:

Jane Lusby said:

curious why you set doctest = false

was too lazy to make the doctests compile

hehe, fair

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:08):

Jane Lusby said:

Also I expect it will be better received if the recommended workflow isn't to import and override the std::result::Result type

Good point. I brought in bias from my use case, which has no real need to ever touch std::result::Result again given the replacement exists. But setting that up would trivial for a user of the propagate crate; they just make their own prelude for their own code

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:09):

Jane Lusby said:

actually, after playing with this a bunch this might be a good candidate for another module in trial-and-error

What is that?

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:14):

Jane Lusby said:

my instinct is that we should try relying heavily on try { } blocks

Okay those are less involved than I thought. Can you paint for me a clearer picture of what you're thinking it would look like if we made use of try?

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:15):

Ben Reeves said:

Jane Lusby said:

actually, after playing with this a bunch this might be a good candidate for another module in trial-and-error

What is that?

It's a crate we're currently working on for experiments

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:16):

right now it has a boxerrror_replacement module and error_reporter module

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:23):

Ben Reeves said:

Can you paint for me a clearer picture of what you're thinking it would look like if we made use of try?

Nevermind, I didn't read close enough. Your idea is to require Err(error)? and then let from_residual convert it to the wrapped type. Would that not work without a try block?

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:27):

The error conversion works either way with try blocks but you always have to use question mark inside of it and you never do ok wrapping

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:28):

Which solves the forwarding concern that you have in the docs since with try blocks you have no choice but to forward correctly by using the question mark operator

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:28):

noiiiice. you do have to rely on users actually using try blocks though, yes?

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:28):

And it also makes it so you don't have to worry about shadowing Ok

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:29):

Yeah it's up to users to use it but I don't think it will take much encouragement if they already have to enable other nightly features just to use the crate

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:29):

And if all the examples use it and the APIs are much nicer to use with try blocks then I think people will just use try blocks

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:30):

Also getting more people testing try blocks and the new trait would be really good so we can iron those out and get stabilized faster

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:31):

Another thing to consider is the burden of duplicating all of the std::result::Result functionality. In theory, I think one could achieve the same behavior using std::result::Result if it had #[track_caller] on its from_residual:

#![feature(trait_specialization)]

/// Specializes the blanket `From<T> for T` implementation so we can capture all invocations of
/// `From::from` that `from_residual` makes.
impl<E, F: From<E>> From<ErrorStack<E>> for ErrorStack<F> {
    #[track_caller]
    fn from(mut e: ErrorStack<E>) -> Self {
        e.stack.push_caller();
        Self { error: From::from(e.error), stack: e.stack }
    }
}

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:31):

Yeah but we talked in the original issue about this and how that would cause some pretty serious perf issues across the ecosystem

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:32):

Oh wait you mean with specialization

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:33):

That might be a valid specialization but There's a decent chance it's not

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:33):

I don't know all the cases of when it's okay to specialize versus when it's not but I know usually when you specialize a generic impl with another generic impl it starts running into soundness issues

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:33):

Because lifetime dependencies can start sneaking in

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:34):

This is blocking a lot of other changes I want to make involving error handling

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:35):

Yes, specialization and #[track_caller] on from_residual.

I can understand how adding #[track_caller] to from_residual would cause code bloat, but I failed to understand where the perf hit would come from when it was brought up on GH.

Jane Lusby said:

I don't know all the cases of when it's okay to specialize versus when it's not but I know usually when you specialize a generic impl with another generic impl it starts running into soundness issues

To my naive mind, this use does seem to fall under "more specific" :shrug: . But yeah I believe you when you say it probably gets hairy.

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:37):

To my naive mind, this use does seem to fall under "more specific"

yea, the issue in particular pops up when you try to specialize std::fmt::Debug with std::fmt::Error, where error is clearly more specific than Debug but because the impls can depend on lifetimes the impls may not always apply

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:37):

https://smallcultfollowing.com/babysteps/blog/2018/02/09/maximally-minimal-specialization-always-applicable-impls/#the-soundness-problem

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:38):

but I failed to understand where the perf hit would come from when it was brought up on GH.

was the suggestion in the github issue something other than adding #[track_caller] to from_residual?

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:38):

I may have misunderstood if that's the case

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:39):

was the suggestion in the github issue something other than adding #[track_caller] to from_residual?

nope, you got it. I'm probably missing something obvious, but I don't know why #[track_caller] adds a perf hit

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:40):

is it a compiler perf hit? or a runtime perf hit?

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:40):

runtime perf

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:42):

I'm not sure exactly how it ends up causing the perf hit, I would expect a lot of the location data to optimize away, but @dtolnay indicated he thought it highly likely there would be a noticeable perf hit in our libs-api team meeting

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:43):

iirc track caller works by transparently adding an extra argument to the function before lowering it to llvm ir

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:43):

so I'm assuming it increases stack frame sizes

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:44):

also it adds a lot more static locations to the final binary, increasing the binary size

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:44):

and i would expect most of what goes unused to get optimized away but I'm guessing that doesn't happen always

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:45):

so I'm assuming it increases stack frame sizes

By no more than 8 bytes per frame I would think. The panic::Locations are statically allocated and Location::caller() gives a reference to one.

Anyway, sounds like it's at least worth getting clarity on and not totally ruling it out until we do.

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:46):

yea

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:46):

we're not certain it will cause perf problems so it's almost certainly worth testing

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:52):

I'm off work all next week so I'll have time to play around with this more. I will try switching over to using try blocks and see how that goes.

I also plan to generalize the ErrorStack so users can customize how they store their stack of locations. Roughly:

trait Stack: Default {
  type Item;
  type Iter: IntoIterator<Item = Self::Item>;
  fn push_location(&mut self, loc: &panic::Location);
  fn iter(&self) -> Self::Iter;
}

struct ErrorStack<E, S: Stack> {
  error: E,
  stack: S,
}

enum TracedResult<T, E, S: Stack  = DefaultStack> {
  Ok(T),
  Err(ErrorStack<E, S>),
}

This will be necessary for my use case at work to help us convert traces between Rust and C code.

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:55):

Oh cool

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:55):

and of course all names are stored in the bike shed :grinning_face_with_smiling_eyes:

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:56):

So this is becoming more similar to the air return trace proposal I've written in the past

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:56):

Error*

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:56):

link? haven't come across that yet

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:56):

And I'm wondering if instead ErrorStack should be the thing that gets swapped out and implements the trait

view this post on Zulip Ben Reeves (Sep 04 2021 at 00:57):

Jane Lusby said:

And I'm wondering if instead ErrorStack should be the thing that gets swapped out and implements the trait

hm, probably

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:57):

https://mobile.twitter.com/yaahc_/status/1253771822920634369?s=19

Proof Of Concept: Error Return Traces in rust aka lightweight backtraces https://twitter.com/yaahc_/status/1253771822920634369/photo/1

- Yaah (@yaahc_)

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:59):

The trait is like a subset of yours, where it only has the push_location method

view this post on Zulip Jane Lusby (Sep 04 2021 at 00:59):

And it goes on the error type itself

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:00):

And that error type could be a wrapper like ErrorStack that handles the storage for inner error types or some c ffi friendly equivalent or even stored directly in a vec within your error type

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:01):

The solution I've always imagined for this is a combination between this track trait and generic member access in order to get the locations back out of errors in the chain

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:02):

But I've never really liked the solution because I've always implicitly assumed that you would be duplicating vecs on multiple errors in a chain of sources and it would just be really inefficient to spread the storage across all the errors

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:04):

Tho it has the really cool property of letting you associate each return with the source in the chain that actually captured it

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:11):

Perhaps:

trait TracedError<E> {
  type StackEntry;
  fn wrap(error: E) -> Self;
  fn push_location(&mut self, loc: &Location);
  fn err(&self) -> &E;
  fn convert_err<F: From<E>>(self) -> TracedError<F>;
  /* some trait method(s) to access StackEntry's */
}

enum TracedResult<T, E, S: TracedError = SomeDefaultImpl> {
  Ok(T),
  Err(S<E>),
}

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:12):

I think most of these methods could be bounds on other traits where they're needed

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:13):

I'm definitely n00b at making good interfaces, please massage it to your will.

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:13):

So instead of convert error you have the from residual impl require E: Into<F> + TracedError

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:19):

edit: actually I don't think I can explain this without just ending up rewriting the error return trace stuff I'd written before

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:23):

let me see if I can dig up a copy of that

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:24):

yea i think that might be lost to an old PC

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:24):

dangit

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:29):

I think my plan ends up breaking when you try to write impl<E, F> From<E> for ErrorStack<F> where F: From<E>

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:29):

lets see if that's an illegal from impl

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:30):

yup

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:30):

error[E0119]: conflicting implementations of trait std::convert::From<ErrorStack<_>> for type ErrorStack<_>
--> src/main.rs:3:1
|
3 | impl<E, F> From<E> for ErrorStack<F> where F: From<E> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: conflicting implementation in crate core:

      - impl<T> From<T> for T;

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:30):

would that be any different with #[trait_specialization]?

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:31):

i think in theory but it requires lattice specialization

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:31):

and that isn't something on the near horizon

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:32):

I think this can work

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:36):

My experience wrestling with conflicting impls when making the prototype led me to a point where the only bounds I included were bounds for the inner error types. I think that's what caused the convert_inner function to be born.

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:37):

yea

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:37):

But you may very well come up with something better that works, cuz my type-fu is still beginner

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:38):

yea I'm not sure I can, I'll keep thinking about it but so far I don't see a way

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:39):

hmmm

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:39):

maybe you can leave the From impls to the types being wrapped by ErrorStack

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:39):

and not try having a single generic impl to rule them all

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:39):

I think that might make it work

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:40):

where you have a Result<T, E> { Ok(T), Err(E) } and a type TracedResult<T, E> = Result<T, ErrorStack<E>>;

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:41):

where the type alias is something you'd provide for convenience or a pattern which you could copy to introduce alternative wrapper types

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:42):

Then the FromResidual impl on Result just requires F: From<E> + Traced and Traced just does push_location

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:42):

ooh

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:43):

wait fk no I think that ends up still not working

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:43):

you need to write impl<E> From<E> for ErrorStack<MyError>

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:43):

which is illegal I think

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:44):

not positive tho

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:44):

time to test

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:44):

oh yes I'm remembering now that this was a huge pain in my ass.

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:45):

I did initially want to use from_residual to both go from E to ErrorStack<E> and from ErrorStack<E> to ErrorStack<F>, but couldn't find a way to avoid conflicting impls

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:46):

it works so long as ErrorStack is hardcoded into the Result I think

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:46):

I tried some heinous things like defining an auto trait NotStackError that I put all over the place in trait bounds.

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:46):

lol

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:47):

but yea

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:47):

/// Starts a new stack when a [`std::result::Result`] is coerced to a [`Result`] using `?`.
impl<T, E, F: From<E>> FromResidual<std::result::Result<Infallible, E>> for Result<T, F> {
    #[inline]
    #[track_caller]
    fn from_residual(residual: std::result::Result<Infallible, E>) -> Self {
        match residual {
            std::result::Result::Ok(_) => unreachable!(),
            std::result::Result::Err(e) => Err(ErrorStack::new(From::from(e))),
        }
    }
}

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:47):

this compiles just fine

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:53):

oh but that's not what you meant

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:53):

you mean when you tried taking ErrorStack out of Result and into the FromResidual impl

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:53):

and yea that wouldn't work

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:55):

Ben Reeves said:

Perhaps:

trait TracedError<E> {
  type StackEntry;
  fn wrap(error: E) -> Self;
  fn push_location(&mut self, loc: &Location);
  fn err(&self) -> &E;
  fn convert_err<F: From<E>>(self) -> TracedError<F>;
  /* some trait method(s) to access StackEntry's */
}

enum TracedResult<T, E, S: TracedError = SomeDefaultImpl> {
  Ok(T),
  Err(S<E>),
}

does this compile?

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:55):

I feel like it shouldn't because it looks like S<E> is a bare trait object

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:56):

basically a HKT

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:56):

i thiink

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:56):

where you're trying to use the trait as a type constructor

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:57):

Would this work?

enum TracedResult<T, E, S: TracedError<E> = SomeDefaultImpl<E>> {
  Ok(T),
  Err(S),
}

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:57):

not sure

view this post on Zulip Jane Lusby (Sep 04 2021 at 01:57):

dubious

view this post on Zulip Ben Reeves (Sep 04 2021 at 01:59):

Going back a bit, did you arrive at thinking we can't get this to work?

enum Result<T, E> {
  Ok(T),
  Err(E),
}

type TracedResult<T, E> = Result<T, ErrorStack<E>>;

If that's the case, then idk how we'd get Err(error)? to work for wrapping.

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:00):

yea I don't see how we can make that work but I'm not sure

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:02):

in general im not sure how to make the ErrorStack type generic and get that same convenience as the current API that hard codes the ErrorStack type

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:02):

but i suspect there might be a clever way

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:02):

Ben Reeves said:

I'm off work all next week so I'll have time to play around with this more. I will try switching over to using try blocks and see how that goes.

I also plan to generalize the ErrorStack so users can customize how they store their stack of locations. Roughly:

trait Stack: Default {
  type Item;
  type Iter: IntoIterator<Item = Self::Item>;
  fn push_location(&mut self, loc: &panic::Location);
  fn iter(&self) -> Self::Iter;
}

struct ErrorStack<E, S: Stack> {
  error: E,
  stack: S,
}

enum TracedResult<T, E, S: Stack  = DefaultStack> {
  Ok(T),
  Err(ErrorStack<E, S>),
}

This will be necessary for my use case at work to help us convert traces between Rust and C code.

this may be the right path

view this post on Zulip Ben Reeves (Sep 04 2021 at 02:06):

I'm not sure how that approach makes the inherent challenge any easier. The inherent challenge being we need two from_residuals that probably conflict:

But I think we can solve that by introducing a new trait to serve the role of From for converting unwrapped to wrapped.

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:07):

I think those get disambiguated by one being FromResidual<propagate::Result> and the other being FromResidual<std::Result>

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:08):

and then we don't rely on ErrorStack::from

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:08):

we use ErrorStack::new

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:08):

then we don't have conflicting From impls

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:09):

this is the same approach used in trial-and-error

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:09):

https://github.com/yaahc/trial-and-error/blob/main/src/boxerror_replacement.rs

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:10):

https://github.com/yaahc/trial-and-error/blob/main/src/boxerror_replacement.rs#L121 this does the wrapping

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:10):

this one handles one that's already been wrapped

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:10):

https://github.com/yaahc/trial-and-error/blob/main/src/boxerror_replacement.rs#L133

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:11):

tho this doesn't need to keep doing From

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:11):

so it may actually not apply

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:11):

:grimacing:

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:11):

worth trying and seeing what the compiler says imo

view this post on Zulip Ben Reeves (Sep 04 2021 at 02:24):

What about this. The key here is not using From as a bound for any of the FromResidual impls.

trait WrapError<E> {
  fn wrap(e: E) -> Self;
}

trait ConvertError<E> {
  fn convert(e: E) -> Self;
}

impl<E, F: From<E>> WrapError<E> for ErrorStack<F> {
  /* ... */
}

impl<E, F: From<E>> ConvertError<ErrorStack<E>> for ErrorStack<F> {
  /* ... */
}

enum Result<T, E> {
  Ok(T),
  Err(E),
}

type TracedResult<T, E> = Result<T, ErrorStack<E>>;

/// Wraps error variant in an ErrorStack, starts new stack
impl<T, E, F: WrapError<E>> FromResidual<Result<!, E>> for Result<T, F> { /* ... */ }

/// Converts ErrorStack to ErrorStack, pushes a location
impl<T, E, F: ConvertError<E>> FromResidual<Result<!, E>> for Result<T, F> { /* ... */ }

view this post on Zulip Ben Reeves (Sep 04 2021 at 02:27):

i'll try it

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:35):

I think i got it working

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:35):

let me push rq

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:35):

not sure

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:35):

i might have some messed up defaulted parameters on some trait impls

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:36):

tell me if this is remotely close to what you meant / need @Ben Reeves

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:37):

https://github.com/yaahc/propagate/commit/7f0b769814c6de320c160f3d44e13b63493a5d3d

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:37):

its extremely messy

view this post on Zulip Ben Reeves (Sep 04 2021 at 02:42):

oops i tried commenting thinking it was a PR and i could aggregate multiple into a review. we can just chat here

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:42):

there is a PR

view this post on Zulip Jane Lusby (Sep 04 2021 at 02:42):

https://github.com/BGR360/propagate/pull/1 includes that commit

view this post on Zulip Ben Reeves (Sep 04 2021 at 02:58):

ok yeah i think this is coming together for me

view this post on Zulip Ben Reeves (Sep 04 2021 at 03:04):

This looks pretty nice.

I was stuck on assuming that Err would be shadowed by propagate::Result::Err. Because of that I assumed we would need a third FromResidual impl to enable the Err(error)? wrapping. But with your approach it just uses the existing FromResidual<std::result::Result>.

I'm not a huge fan of seeing propagate::Ok(..). Is that fully unnecessary if the user always uses try blocks?

view this post on Zulip Jane Lusby (Sep 04 2021 at 03:06):

It still comes up in match statements if you don't bring it into scope

view this post on Zulip Jane Lusby (Sep 04 2021 at 03:06):

But everywhere else it would be gone if you use try blocks

view this post on Zulip Ben Reeves (Sep 04 2021 at 03:08):

That's probably a good thing actually. propagate::Err(e) => {}makes it more obvious that e is not the wrapped value

view this post on Zulip Ben Reeves (Sep 04 2021 at 03:09):

Though it's not really symmetric; propagate::Ok(t) => {} is in fact just T.

view this post on Zulip Sean Chen (he/him) (Sep 08 2021 at 16:03):

One thought I had while looking over the implementation is that it would nice if it wasn't necessary for users to have to do Ok(result?) in order to include the function in the stack trace. I have no idea if there's a more ergonomic work-around for this though :grimacing:

view this post on Zulip Ben Reeves (Sep 08 2021 at 22:28):

Sean Chen (he/him) said:

One thought I had while looking over the implementation is that it would nice if it wasn't necessary for users to have to do Ok(result?) in order to include the function in the stack trace. Either that or if the type system could catch when the user forgot to do this. I have no idea how or if these are possible though :grimacing:

I agree. The only way we see of doing this is for the user to use try blocks:

fn foo() -> propagate::Result<(), u32> {
  try {
    Err(42)?
  }
}
fn bar() -> propagate::Result<(), u32> {
  try {
    foo()?
  }
}

You don't need Ok because it's a try block, and the compiler will force you to do foo()? instead of it won't work with foo()

See the updated main branch for these two examples:
examples/usage.rs
examples/usage_no_try.rs

view this post on Zulip Ben Reeves (Sep 17 2021 at 07:24):

Ok all, after much churning and re-writing, I think I've landed on something that I like.

Please see the updated Propagate repo, now complete with rendered rustdocs, and let me know what you think of the current design of the crate.

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:00):

@Ben Reeves oh wow, the example in the readme looks beautiful

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:01):

o and the traced trait

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:01):

this looks great!

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:02):

o now I really wanna know if this could be made to act like eyre

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:02):

where the stack is also the formatter and general context

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:02):

so it could also do things like capture a backtrace+spantrace when you first convert it into a propagate::Result

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:02):

and you could do custom output

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:03):

hmmm

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:03):

actually this is kinda converging on eyre's old design

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:04):

where it was decided against because of how it's locally instantiated and conversion between context types gets confusing

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:04):

regardless

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:04):

this is fantastic work

view this post on Zulip Ben Reeves (Sep 17 2021 at 18:30):

Not sure I grok all of what you're saying here, but I'm pleased to have my work praised :grinning_face_with_smiling_eyes:

I would be interested in hearing more about "eyre's old design" that was decided against, and what exactly you mean by "locally instantiated" and "context types" and "spantrace"

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:31):

so eyre used to be defined as eyre::Report<H> where H was the EyreHandler

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:31):

and instead it got changed to have a Box<dyn EyreHandler> internally

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:32):

and a global hook for installing a function that constructs the EyreHandler that all Report's use to construct the correct user defined type and store it as a trait object

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:32):

the job of this trait object is two fold

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:32):

first, it stores context alongside your error types that you want to have available in all error reports

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:32):

things like a Backtrace

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:32):

so the default handler that eyre provides mimics the exact behavior of anyhow

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:33):

it has an Option<Backtrace> in the conetxt type and nothing else

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:33):

and when you construct one it checks if the &dyn Error its being associated with already captured a backtrace

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:33):

and if it doesn't it captures one then

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:33):

guaruanteeing a backtrace is available

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:33):

SpanTrace is tracing's Backtrace equivalent

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:34):

instead of capturing all the active stack frames it captures all the active tracing::Spans

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:34):

including runtime information

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:35):

https://docs.rs/color-eyre/0.5.11/color_eyre/ is crate that defines an alternative EyreHandler type that captures a backtrace and a spantrace and can also store suggestions and other custom sections

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:35):

so that kinda really expands on how you can use a "context type" which is more or less just a description of what that EyreHandler trait object does

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:35):

your ErrorStack type is quite similar

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:35):

in that it's storing additional context

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:35):

but its restricted in that it's only caring about one type of context, Location

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:36):

you could in theory create a custom Result type to wrap an eyre::Report and a custom context type and that same Traced trait and reimplement a lot of the stuff in propagate and have it instead downcast the trait object in the Report and then insert the Location every time you use ?

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:37):

actually I don't even think the traced trait would be necessary

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:37):

it would all be statically known types in that case

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:37):

though you cannot generalize it, because the trait object can only give access to the internals by downcasting to concrete known types

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:37):

so there are tradeoffs

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:38):

We could add the trace method to the EyreHandler trait

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:38):

and add a Result type the same as what you have

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:38):

alternatively we could add a more general generic member access style method

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:38):

for inserting arbitrary context

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:39):

it could be as simple as fn add_context(&mut self, context: Box<dyn Any>)

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:39):

which could then have the trace method built on top of it

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:40):

but that would still require a custom resuilt type to do the insertion and active downcasting to get the locations back out of those boxed type erased values

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:40):

so there's lots of tradeoffs in this space but your approach of moving the generic to the Result instead of as a parameter on the Error type is new

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:40):

so I'm really eager to see how it plays out

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:41):

but it would probably be a good idea to familiarize yourself with eyre as well since it is very similar conceptually

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:41):

and in the future we may want to combine the lessons learned in these two crates

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:42):

because I can imagine a version of eyre that has the same parameter on the Result type, and it has a default type that is a globally instantiated hook / type erased trait object, but where you can replace it with a custom type

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:43):

I'll have to do some remembering about the previous versions and the negatives that caused us to switch and do some thinking about if they do or don't apply with this newer design

view this post on Zulip Ben Reeves (Sep 17 2021 at 18:45):

Jane Lusby said:

so I'm really eager to see how it plays out

Plays out with whom? Not sure I have many wannabe users lined up at the door, aside from Qumulo of course.

Jane Lusby said:

but it would probably be a good idea to familiarize yourself with eyre as well since it is very similar conceptually

Agreed, will do as time allows.

view this post on Zulip Ben Reeves (Sep 17 2021 at 18:45):

I'll be convening with coworkers next week to see if the current incarnation of propagate would be a satisfactory approach. Some folks are really pining to have this be an optional feature of std::Result and worry that having to work with two different result types in a codebase will be too annoying.

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:48):

Plays out with whom?

Plays out for you at work mainly, though I think we could in theory attract more test users and feedback if we include it in trial-and-error

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:48):

Ben Reeves said:

I'll be convening with coworkers next week to see if the current incarnation of propagate would be a satisfactory approach. Some folks are really pining to have this be an optional feature of std::Result and worry that having to work with two different result types in a codebase will be too annoying.

that's a super legitimate worry, I think in theory we could argue that it would be better to have the fully featured if slightly slower Result in std and leave the performance optimized Result types for crates that need to avoid the extra codegen

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:49):

but obviously that's going to be a more long term solution and it's not guaranteed that it will be accepted

view this post on Zulip Ben Reeves (Sep 17 2021 at 18:57):

I'll try and study eyre this weekend, maybe gain some new inspiration. Quite a whirlwind of different approaches and tradeoffs, makes my head spin :flushed:

view this post on Zulip Jane Lusby (Sep 17 2021 at 18:57):

Ben Reeves said:

I'll try and study eyre this weekend, maybe gain some new inspiration. Quite a whirlwind of different approaches and tradeoffs, makes my head spin :flushed:

lmk if you have any questions :D

view this post on Zulip nagisa (Sep 17 2021 at 20:44):

Would be nice if your readme had examples of the output you can expect if some error occurred.

view this post on Zulip nagisa (Sep 17 2021 at 20:46):

From the description it sounds to me like the error trace will only include the locations of where in the source an error has occurred. I have personally avoided that kind of approach for I tend to find it hard to relate these back to what actually went wrong, even with source code at hand.

view this post on Zulip Ben Reeves (Sep 20 2021 at 02:29):

nagisa said:

From the description it sounds to me like the error trace will only include the locations of where in the source an error has occurred.

@nagisa It tracks the location where the error occurred and every location where the ? operator is applied to it on the way up the stack. I should find a way to make that more obvious in the readme

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:39):

Alright, I presented the Propagate crate to one of our more seasoned Rustaceans at work today, and I think I'm becoming convinced that this approach is not really a satisfying solution to the underlying issue.

There are some really notable issues with having two distinct Result types, even if you give them different names.

view this post on Zulip Jane Lusby (Sep 20 2021 at 21:40):

:eyes:

view this post on Zulip Jane Lusby (Sep 20 2021 at 21:41):

lol

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:46):

For one, reliable usage of the Propagate crate is sort of predicated on all std::result::Results being "leaf" nodes in the error chain. If you produce a std::result::Result in the middle of propagating a propagate::Result, you lose all the error trace information.

For example, say you want to implement a std::ops::TryFrom on a type, and the method for doing the conversion involves calling fallible application code that returns propagate::Result. If that fallible application code produces an error, you will forfeit any error trace information when you go to return the std::result::Result from try_from.

This issue could probably be solved by doing something similar to eyre, where propagate::Result::Err stores a trait object that implements the necessary methods. That way you can go back and forth between std::result::Result and propagate::Result and not lose any information. But this is not the only issue.

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:47):

There is of course the issue of user confusion ("why is there two result types? when do I use which?"), which is of debatable importance.

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:49):

Another point my colleague brought up is that, from a Rust community perspective, this approach is kind of problematic. He likened it to the discourse going around the community about async vs sync APIs. I'm not too familiar, but he says he hears a lot of people complaining that so many libraries these days need to define two sets of APIs: one for sync and one for async.

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:49):

If we release propagate, then if libraries want to make use of it, they have to find a way to satisfy users who want to use propagate and users who don't.

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:55):

Long story short, the new shiny golden egg in my crosshairs is trying to see what we can do to std::result::Result to make it enable this sort of tracing.

The very rough idea is to trait-specialize the FromResidual on Result to handle the case where the error type is traceable (i.e., wants to receive tracing information as it is propagated).

impl<T> FromResidual<Result<!, Box<dyn Traced>>> for Result<T, Box<dyn Traced>> {
  #[track_caller]
  fn from_residual(/* ... */) { /* ... */ }
}

view this post on Zulip Ben Reeves (Sep 20 2021 at 21:57):

Alternatively, rather than introducing a new Traced trait to the std library, it could be another method on the Error trait.

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:01):

I don't think adding a Traced trait would have any advantages over just adding #[track_caller] to the FromResidual impl for Result

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:02):

since you'd still have to hook into ? somehow for std::result::Result which would result in the same codegen I expect

view this post on Zulip Ben Reeves (Sep 20 2021 at 22:02):

Oh, another issue I forgot to mention is that, even if you ignore all the other issues, you still get no insight into the propagation of std::result::Result's. One scenario where that could be useful is parsing with FromStr. If parsing fails for some really deeply nested type, it would be nice to see more detail into where it failed.

Maybe not everybody would want Result tracing to be turned on by default in the std library, but that type of problem has been solved before with compile flags like panic={unwind,abort}. Granted it'll be a hard sell.

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:03):

My guess is the best bet is arguing that the default std::result::Result should support track_caller and that more optimized Result types should be left to 3rd party libraries

view this post on Zulip Ben Reeves (Sep 20 2021 at 22:03):

Jane Lusby said:

I don't think adding a Traced trait would have any advantages over just adding #[track_caller] to the FromResidual impl for Result

I'm not seeing how you can implement propagation tracing by only modifying the existing FromResidual for Result. You'd have nowhere to store the trace.

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:04):

ah you're right, it has to be actively done in every from_residual call

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:05):

which would need to be in the std::Result from_residual impl

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:05):

brain fart

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:09):

@Ben Reeves regarding the loss of trace information when round tripping through a std::result::Result, you could do something similar to what I do here in trial-and-error

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:09):

https://github.com/yaahc/trial-and-error/blob/main/src/boxerror_replacement.rs#L141

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:09):

https://github.com/yaahc/trial-and-error/blob/main/src/boxerror_replacement.rs#L68-L73

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:10):

where when it goes from propagate::Result -> std::result::Result it bundles the trace with the error and type erases it, and then you can try to check for that when going backwards

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:10):

not sure if that will apply well here though

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:10):

this one assumes everything is type erased

view this post on Zulip Ben Reeves (Sep 20 2021 at 22:14):

I don't think we have a super strict need for error types to be non-type-erased, so I think that would be a fine limitation

view this post on Zulip Ben Reeves (Sep 20 2021 at 22:14):

If I'm correct in understanding that "type erased" means "hidden behind a trait object"

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:14):

yup

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:17):

but yea, the Traced trait solution seems identical to https://paper.dropbox.com/doc/Collaborative-Summary-3-Fact-Finding-Pre-RFCs-around-Error-Handling-Reporting--BSzjky_QLvb5ReDhXm4PN2OAAg-dnShKo1SsHtdF4Yheeoco

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:17):

tho that's old and based on the old Try trait

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:18):

either way though I think it will probably end up depending on generic member acecss

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:18):

which also solves the leaf error issue

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:18):

and if you're not familiar with generic member access: https://github.com/rust-lang/rfcs/pull/2895

view this post on Zulip Ben Reeves (Sep 20 2021 at 22:23):

You're always a wealth of knowledge, thank you :praise:

view this post on Zulip Jane Lusby (Sep 20 2021 at 22:23):

^_^

view this post on Zulip Ben Reeves (Sep 21 2021 at 02:25):

Jane Lusby said:

Ben Reeves regarding the loss of trace information when round tripping through a std::result::Result, you could do something similar to what I do here in trial-and-error

How does this impl even work? I thought you couldn't have an implementation where both the trait and implementee are from foreign crates.

view this post on Zulip simulacrum (Sep 21 2021 at 02:26):

DynResult is local, and (I believe) only the first "uncovered" type from left to right needs to be local to allow it, or something like that? These rules were somewhat recently changed, let me find the RFC

view this post on Zulip simulacrum (Sep 21 2021 at 02:27):

https://rust-lang.github.io/rfcs/2451-re-rebalancing-coherence.html

view this post on Zulip Ben Reeves (Sep 21 2021 at 02:34):

Thanks!

view this post on Zulip Ben Reeves (Sep 21 2021 at 04:35):

Ben Reeves said:

nagisa said:

From the description it sounds to me like the error trace will only include the locations of where in the source an error has occurred.

nagisa It tracks the location where the error occurred and every location where the ? operator is applied to it on the way up the stack. I should find a way to make that more obvious in the readme

Readme now has a much more motivating example: https://github.com/BGR360/propagate/commit/141d3d6262de1b76d4cc0cfd1d1dfeeadf320402?short_path=b335630#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

view this post on Zulip Ben Reeves (Sep 21 2021 at 05:37):

Jane Lusby said:

but yea, the Traced trait solution seems identical to https://paper.dropbox.com/doc/Collaborative-Summary-3-Fact-Finding-Pre-RFCs-around-Error-Handling-Reporting--BSzjky_QLvb5ReDhXm4PN2OAAg-dnShKo1SsHtdF4Yheeoco

Gave this a read, wish I would have found this sooner. I didn't know about Zig's error return traces, but they are exactly what I want!!

Maybe I'll start using "return tracing" instead of "propagation tracing" as a more "agreed upon" term for this.

A lot of the comments on that document do seem to suggest that people were in favor of adding support for this... :eyes: Guess it's just a matter of waiting for supporting features?

either way though I think it will probably end up depending on generic member acecss

Could you explain why?

which also solves the leaf error issue

Could you explain how?

view this post on Zulip Jane Lusby (Sep 21 2021 at 08:44):

In that if you're using a traced trait or method on error types you need a way to extract that location data back out of those error types later

view this post on Zulip Jane Lusby (Sep 21 2021 at 08:45):

Generic member access is the mechanism that lets you extract information such as locations as a slice or as an iterator or whatever through dyn error trait objects

view this post on Zulip Jane Lusby (Sep 21 2021 at 08:46):

And this helps a leaf errors because you can have this information be extracted from source multiple errors deep in a chain and it doesn't really matter what interfaces it gets returned through or how it gets rewrapped

view this post on Zulip Jane Lusby (Sep 21 2021 at 08:46):

The downside of this is that you end up running into duplicated storage of the same vecs of locations theoretically if you're storing them in each error in a chain of errors

view this post on Zulip Jane Lusby (Sep 21 2021 at 08:48):

But for example even with the propagate crate kind of as it is right now if you had generic member access you could type erase the error and stack of locations when roundtripping through a std result and then when reporting your locations you could use generic number access to get back the older stack and recombine them in the final report

view this post on Zulip Ben Reeves (Sep 29 2021 at 23:07):

Just as a little update, I hit a breakthrough and was able to prototype a trait-specialized std::Result using the sound(er) #[feature(min_specialization)] rather than the unsound #[feature(specialization)]: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=9bc783874eeecdc3c6f68625cace9bcb

The breakthrough is the usage of a marker trait configured with #[rustc_specialization_trait] that users would implement on all of their Traced error types, so the compiler can enforce the "always applicable" rule on the user side of things.

Very exciting, this might be the direction Qumulo takes for its error handling, since many are opposed to working with two different result types.

view this post on Zulip Ben Reeves (Sep 29 2021 at 23:10):

Oh and the other key thing is that this approach doesn't require type erasure using boxes or what-have-you. Purely static dispatch with specialized treatment of Traced error types is achieved.

view this post on Zulip Jane Lusby (Sep 29 2021 at 23:28):

ooo

view this post on Zulip Jane Lusby (Sep 29 2021 at 23:28):

that looks fancy!

view this post on Zulip Jane Lusby (Sep 29 2021 at 23:31):

So I'm guessing the next step is to implement a PR adding Traced to std unstably using the specialization marker and then we get the compiler team to review it to make sure it's all sound

view this post on Zulip Ben Reeves (Sep 30 2021 at 00:01):

Yeah, I'll try to get it working on a fork.

Looking again, I think the marker trait is redundant. It's used in std to make existing traits specializable without breaking things.

Since Traced is a wholly new trait I think we can annotate it directly with #[rustc_specialization_trait].

view this post on Zulip Sean Chen (he/him) (Oct 02 2021 at 17:29):

That's a really neat and elegant solution :smile:

view this post on Zulip Ben Reeves (Oct 05 2021 at 04:44):

Well that was pretty quick to code up. I think I spent more time setting up my rustc repo and rust analyzer than I did actually writing the code and tests.

view this post on Zulip Ben Reeves (Oct 05 2021 at 04:44):

Draft PR imminent

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:46):

Lol

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:46):

Hype

view this post on Zulip Ben Reeves (Oct 05 2021 at 04:50):

my poor 2015 macbook is NOT having a fun time running rust-analyzer on this repo

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:53):

Yuuuup

view this post on Zulip Ben Reeves (Oct 05 2021 at 04:53):

anyway: https://github.com/rust-lang/rust/pull/89547

it's got working tests

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:53):

I tried streaming rustc dev once

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:54):

Not a good idea to do obs and rust analyzer on the same machine it turns out

view this post on Zulip Jane Lusby (Oct 05 2021 at 04:56):

Very cool

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:01):

I really wish I understood specialization in rust better

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:02):

I feel confused wondering where you need to add default

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:02):

surprised you can just put it on the impl you're specializing, rather than needing to opt in on the trait definition

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:09):

default is just saying "this particular impl of this trait method is allowed to be specialized by a more specific impl of this trait method, but this is the default impl to fall back to"

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:09):

or something like that

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:16):

The important detail of doing it the way I've done it is that, due to the presence of #[rustc_attr_specialization_trait], the Traced trait will be limited in the types it can be implemented on. This is in order to obey the rules of min_specialization. I guess because people like sound languages. Whatever :rolling_eyes:

Anyway we'd have to decently document that. There are already some traits in core / std that use that attribute, so maybe we can graft some words from those docs to help explain the restrictions.

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:20):

The too-long;didn't-read-niko's-blog-posts-a-dozen-times version of it is that you can't have an impl Traced with additional trait bounds:

// Okay.
impl Traced for MyScalarType {...}

// Okay (I think?).
impl<T> Traced for MyGenericType<T> {...}

// Not okay.
impl<T: Bound> Traced for SomeOtherType<T> {...}

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:21):

And any impl Traced has to be completely generic WRT lifetimes, i.e., no 'static anywhere.

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:29):

Sounds about right

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:30):

I just had no idea that they'd already added support for always applicable impls already via this attribute

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:30):

i didn't either. when i figured that out i was like :bangbang: :bulb: :bulb: :bulb: :bangbang:

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:31):

And yea I understand what default means, just not when you're allowed to apply it. Like, I would have initially worried that the "can be specialized" status of a trait has to somehow tie all the way back to the definition

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:31):

Like it has to be tied to the root of the definition tree essentially

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:31):

That was my intuition initially too

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:31):

But that clearly isn't the case

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:31):

TIL

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:31):

Very happy that's not a restriction

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:32):

This knowledge will be very useful going forward :smiling_devil:

view this post on Zulip Ben Reeves (Oct 05 2021 at 05:32):

project-specialize-everything

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:32):

Just unwrap and friends

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:33):

Tho honestly the thing I want to specialize there would be sad face if that's how we have to fix it

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:33):

Because then you have to opt into the good unwrap support

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:33):

Because we can't apply that specialization to Error itself

view this post on Zulip Jane Lusby (Oct 05 2021 at 05:34):

Either way, I'm sure it will be useful

view this post on Zulip Charles Ellis O'Riley Jr. (Oct 11 2021 at 18:56):

:octopus:

view this post on Zulip scottmcm (Oct 16 2021 at 03:07):

I'm super-late to this thread, but I thought this was a really interesting note: https://github.com/BGR360/propagate/blob/2300fab6bd8670019e8e6c2eabd26558f029a76b/src/result.rs#L108

Makes me glad we finally agreed on wrapping the final value in try blocks :smile:

view this post on Zulip Jane Lusby [she/her] (Oct 16 2021 at 03:47):

:big_smile:

view this post on Zulip Ben Reeves (Oct 29 2021 at 18:17):

It's been a while since I've updated. At work, I have applied the specializing core::result::Result patch to our Rust fork. All of our third- and first-party crates compile perfectly.

I am working on the design for a TracedError wrapper that we will use to wrap all of our existing error enums to make them Traced. Using auto_traits and negative_impls, I was able to create blanket From impls that cover all of the necessary ?-conversion cases without colliding with the blanket impls in std. This approach could pretty easily turn into a "user-land" crate that folks can use on top of the standard library patch to get free tracing for their existing error types.

Long story short, the future for this approach is very bright, at least for us at Qumulo.

At the moment, my time is being eaten up as I try to get things in motion for our code base. There's a lot to do since I have to also interoperate with our C errors. Once that has reached a good place, though, I think I might want to rekindle the conversation about making a for realsies RFC.

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:21):

Oh be careful with negative impls

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:21):

Their interaction with specialization is currently broken I think

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:22):

Let me find the thread in wg-traits

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:23):

https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/negative.20impls.20in.20coherence

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:24):

regardless though, I do think it would be a good idea to open a PR to add the Traced trait work you did from your Rust fork to the rust-lang/rust repo on nightly

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:25):

particularly since we can get the people working on negative-impls to review it and make sure it's all good

view this post on Zulip Ben Reeves (Oct 29 2021 at 18:49):

Good looking out, I'll take a better look at that thread at some point.

Implementing the necessary Froms could be done using a proc macro instead, but using auto_traits and negative_impls was just easier. But yeah if there's an issue, then there's other ways to achieve the behavior.

Could it really just be as simple as a PR? I would have thought changes like this would have to go through some sort of RFC before being put into nightly

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:59):

Yea, for nightly features we only do an RFC if a team member feels it's necessary

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 18:59):

And even then having the pr open first doesn't hurt anything

view this post on Zulip Jane Lusby [she/her] (Oct 29 2021 at 19:00):

https://github.com/rust-lang/governance/blob/master/teams/libs/subteam-api.md#making-changes-to-the-standard-libraries

view this post on Zulip Ben Reeves (Nov 08 2021 at 07:48):

I got the TracedError wrapper working at work. Encountered no issue with negative_impls messing with specialization. I converted a small portion of our code to start using it, and the conversion was painless but monotonous. Most changes ended up being in test code (had to add a lot of .unwrap_err().into_inner()).

I've started replicating the design into a new crate. Taking inspiration from eyre, I've dubbed this crate tres: https://github.com/BGR360/tres. The meat of it is all there, with pretty good documentation.

I think tres could become a generally useful crate. It would very easily pair with thiserror, and hopefully with eyre too, but I have yet to think that through.

For the time being, it comes with a companion crate, tres-result, which provides the specialized Result type that core currently lacks. At some point in the future, users of tres would probably be able to seamlessly switch from tres_result::{Result, Traced} to core::result::{Result, Traced}.

I also want to make it easy for library developers to add a feature flag to their libraries to let their users choose whether they want to work with traced or untraced errors.

tres could also be used no-std, which is neat.

view this post on Zulip Ben Reeves (Nov 08 2021 at 07:49):

Some remaining work to do for tres, in no particular order:

view this post on Zulip Jane Lusby [she/her] (Nov 08 2021 at 13:06):

That sounds amazing, can't wait to take a look at it

view this post on Zulip Jane Lusby [she/her] (Nov 08 2021 at 13:07):

(currently on vacation)

view this post on Zulip Sean Chen (he/him) (Nov 08 2021 at 14:38):

@Ben Reeves would you like help with any of these tasks? :smile:

view this post on Zulip Ben Reeves (Nov 08 2021 at 18:13):

@Sean Chen (he/him) I think I'll be good for now! But I will keep that offer in my back pocket :)

view this post on Zulip Ben Reeves (Nov 08 2021 at 20:46):

Ben Reeves said:

the conversion was painless

Actually, something to note here. It's true that, once I ironed out the kinks with the auto trait approach, converting the code was pretty painless. However, I ran into a pretty mind-bending compiler error that took me an afternoon to figure out. Once I fixed the issue with my auto traits, converting the code was easy. HOWEVER, anybody writing NEW code can also run into this same unhelpful diagnostic if they accidentally forget a From impl somewhere, and that's pretty bad for usability.

I filed this issue and started this topic after spending some time digging into the root cause of the unhelpful diagnostic. It hasn't gotten any attention so far (granted, it hasn't even existed for a full weekday yet). But if anybody here has some knowledge of trait selection and/or specialization, or knows how to get the attention of somebody who does, then that'd be super helpful.

view this post on Zulip Joshua Nelson (Nov 08 2021 at 20:59):

trait selection is IMO one of the most complicated parts of the compiler, and there's unfortunately not a ton of experts on it ... maybe Matthew Jasper or nikomatsakis would have suggestions

view this post on Zulip Ben Reeves (Nov 08 2021 at 21:20):

To see more concretely how this affects tres, consider this code that compiles correctly if I uncomment the From impl:

use tres::{Err, Ok, Result, TracedError};

struct Error(pub String);

/* OOPS I FORGOT TO WRITE THIS FROM IMPL
impl<T: ToString> From<T> for Error {
    fn from(error: T) -> Self {
        Self(error.to_string())
    }
}
*/

fn foo() -> Result<(), TracedError<Error>> {
    Ok(bar()?)
}

fn bar() -> Result<(), TracedError<&'static str>> {
    Err(TracedError::new("Oops!"))
}

fn main() {
    let _ = foo();
}

As is, the compiler produces the following diagnostic, which hopefully depicts just how "bad for usability" this is:

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> examples/bad_diagnostic.rs:14:13
    |
13  | / fn foo() -> Result<(), TracedError<Error>> {
14  | |     Ok(bar()?)
    | |             ^ cannot use the `?` operator in a function that returns `tres::Result<(), TracedError<Error, Locations>>`
15  | | }
    | |_- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `FromResidual<tres::Result<Infallible, TracedError<&str, Locations>>>` is not implemented for `tres::Result<(), TracedError<Error, Locations>>`
note: required by `from_residual`
   --> /Users/Ben/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/try_trait.rs:339:5
    |
339 |     fn from_residual(residual: R) -> Self;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For reference, if I use core::result::Result instead, I get a more helpful message:

error[E0277]: `?` couldn't convert the error to `TracedError<Error, Locations>`
   --> examples/bad_diagnostic.rs:14:13
    |
13  | fn foo() -> Result<(), TracedError<Error>> {
    |             ------------------------------ expected `TracedError<Error, Locations>` because of this
14  |     Ok(bar()?)
    |             ^ the trait `From<TracedError<&str, Locations>>` is not implemented for `TracedError<Error, Locations>`
    |
    = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
    = help: the following implementations were found:
              <TracedError<F, T> as From<E>>
              <TracedError<F, T> as From<TracedError<E, T>>>
    = note: required because of the requirements on the impl of `FromResidual<std::result::Result<Infallible, TracedError<&str, Locations>>>` for `std::result::Result<(), TracedError<Error, Locations>>`
note: required by `from_residual`
   --> /Users/Ben/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/ops/try_trait.rs:339:5
    |
339 |     fn from_residual(residual: R) -> Self;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That message could also be improved, but seems workable, as it mentions the core issue (missing a From somewhere).

view this post on Zulip Ben Reeves (Nov 08 2021 at 21:26):

Joshua Nelson said:

trait selection is IMO one of the most complicated parts of the compiler, and there's unfortunately not a ton of experts on it ... maybe Matthew Jasper or nikomatsakis would have suggestions

@Joshua Nelson thank you for the pointer. That at least validates why I felt so overwhelmed trying to understand it :joy:.

On a scale from "Please Do" to "Niko Hates His Life", how hesitant should I feel about pinging those parties regarding this issue?

view this post on Zulip Joshua Nelson (Nov 08 2021 at 21:30):

"probably fine to ping, but could be several days before you get a response"


Last updated: Jan 26 2022 at 14:46 UTC