5 ms·

I don't like having to wrap my function with a macro, with all disadvantages (e.g. worse IDE support), just to get error location. Especially given we can do th

I don't like having to wrap my function with a macro, with all disadvantages (e.g. worse IDE support), just to get error location. Especially given we can do this without macros at all: use std::error::Error as StdError; use std::panic::Location; use std::fmt; #[derive(Debug)] pub struct MyError { pub error: Box<dyn StdError + Send + Sync>, pub location: &'static Location<'static>, } impl<E: StdError + Send + Sync + 'static> From<E> for MyError { #[track_caller] #[inline] fn from(error: E) -> Self { Self { error: Box::new(error), location: Location::caller(), } } } impl fmt::Display for MyError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}, {}", self.error, self.location) } } We can also optimize this more (e.g. a simple optimization to be two words instead of three: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=2a46e8a71103b4a2f432defc320dc06a https://play.rust-lang.org/?version=stable&mode=debug&editio.... Also allows opting `Send` and `Sync` out or using non-`'static` lifetimes), but this works as a baseline. The only disadvantage is that if called in a function with `#[track_caller]` this will report the location of the caller... But this is something I can live with.

insanitybit 3y ago
I think the disadvantage you mention is actually an advantage. I would probably want this to propagate alongside other `track_caller`'s since anyone using `track_caller` is "in the know" about where that error should be appearing to come from. TBH I had assumed this crate was using the `#[track_caller]` attribute. The main downside I see is that if I manually construct the error type I'll miss that place, unless the constructor of that error also tracks the caller.
- JoelJacobson 3y ago
  Author of `wherr` here. > I think the disadvantage you mention is actually an advantage. I would probably want this to propagate alongside other `track_caller`'s since anyone using `track_caller` is "in the know" about where that error should be appearing to come from. I've now read about `#[track_caller]`, and that's my conclusion too. For those who haven't heard about `#[track_caller]` like myself, I can recommend this page: https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#track_caller https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#track... > TBH I had assumed this crate was using the `#[track_caller]` attribute. TBH, having read about the `#[track_caller]` attribute, I'm starting to think the crate should use it. If you and/or @afdbcreid would be interested in working with me on a new implementation of the `wherr` crate that uses the `#[track_caller]` approach instead, please contact me at joel at compiler dot org. Hope to hear from you! > The main downside I see is that if I manually construct the error type I'll miss that place, unless the constructor of that error also tracks the caller. You mean, we would need to make sure the error type is always created via a new() constructor method, and that the new() method also has the `#[track_caller]` attribute? Something like this? impl MyError { #[track_caller] pub fn new<E: StdError + Send + Sync + 'static>(error: E) -> Self { Self { error: Box::new(error), location: Location::caller(), } } }
  - insanitybit 3y ago
    > You mean, we would need to make sure the error type is always created via a new() constructor method, and that the new() method also has the `#[track_caller]` attribute? Something like this? Exactly. Although I think because `T: Impl Into<T>` it may not be necessary.
  - afdbcreid 3y ago
    You're free to use it and/or create a crate for it. If you do, I would go with the second version (the one I linked in the playground), as it is more optimized and allows for not `Send + Sync` and lifetimes other than `'static`. It can be optimized further using unsafe code to use one word (like `anyhow`) does, but I don't think it is necessary. The code I posted is minimal: I would also add `From` impls for not `Send + Sync`, and maybe other features of popular error crates: the two most important that come to mind is downcasting and more importantly, adding a context to an error before propagating it. Downcasting requires unsafe code though, I think. I actually have another nice new idea about contexts, so I may actually publish my own crate... If you don't do that before me. If you do, maybe I'll contribute :)
    - JoelJacobson 3y ago
      I've added a branch for this: https://github.com/joelonsql/wherr/tree/track_caller https://github.com/joelonsql/wherr/tree/track_caller `src/lib.rs` is your code copy/pasted, with the addition of `impl<'a, E: StdError + 'a> From<E> for Box<dyn MyError + 'a>` like suggested (and also just `Send`). I've also added a `fn inner_error<'a>(&'a self) -> &'a (dyn StdError + 'a);`, but not sure this is the proper way. I can't wrap my head around how to match on the inner error though, see `examples/matching_inner.rs`: match my_error_wrapped().unwrap_err() { _ => todo!(), } Can you figure it out?
JoelJacobson 3y ago
Author of `wherr` here again. Your approach is indeed elegant, and leveraging `#[track_caller]` is quite idiomatic. I also see the value in opting out of macros when possible. However, I'm pondering the error matching aspect. Using your `MyErrorInner` design, one might want to downcast and match on specific error types: if let Some(specific_err) = err.downcast_ref::<SpecificErrorType>() { match specific_err { SpecificErrorType::Variant => { /* ... */ }, // ... other matches } } But this raises a question: Given that `MyErrorInner` holds a concrete `E` (which is, at runtime, a specific error type) and the public API exposes a boxed trait object, how would we effectively match against inner error types without resorting to static lifetimes? In your design, while `location` is static, the concrete error type `E` is hidden away. Exposing it, especially without requiring static lifetimes, could be a challenge. Your thoughts? Always a pleasure to dive into these Rust intricacies. Thanks for taking the time to contribute to this discussion!
JoelJacobson 3y ago
Author of `wherr` here. Thanks for sharing a really nice alternative solution to the problem, I like it! It seems to be a bit faster: running 2 tests test tests::benchmark_location_caller ... bench: 15 ns/iter (+/- 0) test tests::benchmark_wherr ... bench: 18 ns/iter (+/- 1) I also like that it's more explicit; the actual error type is then spelled out, instead of being magically wrapped by the macro, that's nice! Sounds like a better approach overall, but I'm curious to understand more about the disadvantage you mention, the problem with using `#[track_caller]`, that then masquerades the actually error line number. For my own code, I can of course control that and just ensure I don't make use of `#[track_caller]` anywhere in my code. But if other crates would be making use of `#[track_caller]`, that would prevent getting the correct error line. Can anything be said about how common it is to use `#[track_caller]` in crates? If very uncommon, then I can live with it. But it would be annoying if it works sometimes and other times not.
loeg 3y ago
Can you elaborate on the usage of your error type? Is the implicit conversion via return type and ? operator sufficient? E.g., fn add(s1: &str, s2: &str) -> Result<i64, MyError> { ... let i1 = i64::from_str_radix(s1, radix)?; ... } Thanks!
- Zezima 3y ago
  Not the poster. What exactly are you asking about? The usage of the error type the OP wrote is to get the line number of any error without using a macro on top of every single function in our crate. The person who wrote `MyError` accomplished it by defining their error type as "something that can be constructed from any other error" (`impl From<E: StdErr> for MyError`) and additionally heap-allocated the original error with `Box::new(error)`. So the usage would be to write one single error type in a crate which can "capture" or be constructed from any error type `E` and also prints out the line number of that error via `impl Display for MyError`.
  - loeg 3y ago
    I'm looking for a usage example for the MyError type.
    - Zezima 3y ago
      The code you wrote would be a valid use for `MyError`
      - loeg 3y ago
        Thanks!
- SpaceNugget 3y ago
  Not the person who posted the snippet. Yea, that should work. It tracks the position of the line that calls from() on the other error to convert it into MyError, which is what the ? operator will expand into.

insanitybit 3y ago

I think the disadvantage you mention is actually an advantage. I would probably want this to propagate alongside other `track_caller`'s since anyone using `track_caller` is "in the know" about where that error should be appearing to come from. TBH I had assumed this crate was using the `#[track_caller]` attribute. The main downside I see is that if I manually construct the error type I'll miss that place, unless the constructor of that error also tracks the caller.

JoelJacobson 3y ago
Author of `wherr` here. > I think the disadvantage you mention is actually an advantage. I would probably want this to propagate alongside other `track_caller`'s since anyone using `track_caller` is "in the know" about where that error should be appearing to come from. I've now read about `#[track_caller]`, and that's my conclusion too. For those who haven't heard about `#[track_caller]` like myself, I can recommend this page: https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#track_caller https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#track... > TBH I had assumed this crate was using the `#[track_caller]` attribute. TBH, having read about the `#[track_caller]` attribute, I'm starting to think the crate should use it. If you and/or @afdbcreid would be interested in working with me on a new implementation of the `wherr` crate that uses the `#[track_caller]` approach instead, please contact me at joel at compiler dot org. Hope to hear from you! > The main downside I see is that if I manually construct the error type I'll miss that place, unless the constructor of that error also tracks the caller. You mean, we would need to make sure the error type is always created via a new() constructor method, and that the new() method also has the `#[track_caller]` attribute? Something like this? impl MyError { #[track_caller] pub fn new<E: StdError + Send + Sync + 'static>(error: E) -> Self { Self { error: Box::new(error), location: Location::caller(), } } }
- insanitybit 3y ago
  > You mean, we would need to make sure the error type is always created via a new() constructor method, and that the new() method also has the `#[track_caller]` attribute? Something like this? Exactly. Although I think because `T: Impl Into<T>` it may not be necessary.
- afdbcreid 3y ago
  You're free to use it and/or create a crate for it. If you do, I would go with the second version (the one I linked in the playground), as it is more optimized and allows for not `Send + Sync` and lifetimes other than `'static`. It can be optimized further using unsafe code to use one word (like `anyhow`) does, but I don't think it is necessary. The code I posted is minimal: I would also add `From` impls for not `Send + Sync`, and maybe other features of popular error crates: the two most important that come to mind is downcasting and more importantly, adding a context to an error before propagating it. Downcasting requires unsafe code though, I think. I actually have another nice new idea about contexts, so I may actually publish my own crate... If you don't do that before me. If you do, maybe I'll contribute :)
  - JoelJacobson 3y ago
    I've added a branch for this: https://github.com/joelonsql/wherr/tree/track_caller https://github.com/joelonsql/wherr/tree/track_caller `src/lib.rs` is your code copy/pasted, with the addition of `impl<'a, E: StdError + 'a> From<E> for Box<dyn MyError + 'a>` like suggested (and also just `Send`). I've also added a `fn inner_error<'a>(&'a self) -> &'a (dyn StdError + 'a);`, but not sure this is the proper way. I can't wrap my head around how to match on the inner error though, see `examples/matching_inner.rs`: match my_error_wrapped().unwrap_err() { _ => todo!(), } Can you figure it out?

JoelJacobson 3y ago

Author of `wherr` here again. Your approach is indeed elegant, and leveraging `#[track_caller]` is quite idiomatic. I also see the value in opting out of macros when possible. However, I'm pondering the error matching aspect. Using your `MyErrorInner` design, one might want to downcast and match on specific error types: if let Some(specific_err) = err.downcast_ref::<SpecificErrorType>() { match specific_err { SpecificErrorType::Variant => { /* ... */ }, // ... other matches } } But this raises a question: Given that `MyErrorInner` holds a concrete `E` (which is, at runtime, a specific error type) and the public API exposes a boxed trait object, how would we effectively match against inner error types without resorting to static lifetimes? In your design, while `location` is static, the concrete error type `E` is hidden away. Exposing it, especially without requiring static lifetimes, could be a challenge. Your thoughts? Always a pleasure to dive into these Rust intricacies. Thanks for taking the time to contribute to this discussion!

JoelJacobson 3y ago

Author of `wherr` here. Thanks for sharing a really nice alternative solution to the problem, I like it! It seems to be a bit faster: running 2 tests test tests::benchmark_location_caller ... bench: 15 ns/iter (+/- 0) test tests::benchmark_wherr ... bench: 18 ns/iter (+/- 1) I also like that it's more explicit; the actual error type is then spelled out, instead of being magically wrapped by the macro, that's nice! Sounds like a better approach overall, but I'm curious to understand more about the disadvantage you mention, the problem with using `#[track_caller]`, that then masquerades the actually error line number. For my own code, I can of course control that and just ensure I don't make use of `#[track_caller]` anywhere in my code. But if other crates would be making use of `#[track_caller]`, that would prevent getting the correct error line. Can anything be said about how common it is to use `#[track_caller]` in crates? If very uncommon, then I can live with it. But it would be annoying if it works sometimes and other times not.

loeg 3y ago

Can you elaborate on the usage of your error type? Is the implicit conversion via return type and ? operator sufficient? E.g., fn add(s1: &str, s2: &str) -> Result<i64, MyError> { ... let i1 = i64::from_str_radix(s1, radix)?; ... } Thanks!

Zezima 3y ago
Not the poster. What exactly are you asking about? The usage of the error type the OP wrote is to get the line number of any error without using a macro on top of every single function in our crate. The person who wrote `MyError` accomplished it by defining their error type as "something that can be constructed from any other error" (`impl From<E: StdErr> for MyError`) and additionally heap-allocated the original error with `Box::new(error)`. So the usage would be to write one single error type in a crate which can "capture" or be constructed from any error type `E` and also prints out the line number of that error via `impl Display for MyError`.
- loeg 3y ago
  I'm looking for a usage example for the MyError type.
  - Zezima 3y ago
    The code you wrote would be a valid use for `MyError`
    - loeg 3y ago
      Thanks!
SpaceNugget 3y ago
Not the person who posted the snippet. Yea, that should work. It tracks the position of the line that calls from() on the other error to convert it into MyError, which is what the ? operator will expand into.