Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unwrap alternative? #161

Closed
Dirbaio opened this issue Sep 3, 2020 · 15 comments
Closed

unwrap alternative? #161

Dirbaio opened this issue Sep 3, 2020 · 15 comments
Labels
difficulty: easy Pretty easy to solve priority: low Low priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: enhancement Enhancement or feature request
Milestone

Comments

@Dirbaio
Copy link
Contributor

Dirbaio commented Sep 3, 2020

uwnrap() on a Result is quite convenient, but it's not usable with defmt since it requires the error type to be fmt::Debug, which is not useful to print it with defmt.

A solution could be add a trait defmt::Unwrap (or maybe Dewrap?) and implement it for Result, which does the same as unwrap() but printing the result with defmt.

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Sep 10, 2020

Here's a copypaste-ready implementation, in case someone finds it useful...

macro_rules! depanic {
    ($( $i:expr ),*) => {
        {
            defmt::error!($( $i ),*);
            panic!();
        }
    }
}

pub trait Dewrap<T> {
    /// dewrap = defmt unwrap
    fn dewrap(self) -> T;

    /// dexpect = defmt expect
    fn dexpect<M: defmt::Format>(self, msg: M) -> T;
}

impl<T> Dewrap<T> for Option<T> {
    fn dewrap(self) -> T {
        match self {
            Some(t) => t,
            None => depanic!("unwrap failed: enum is none"),
        }
    }

    fn dexpect<M: defmt::Format>(self, msg: M) -> T {
        match self {
            Some(t) => t,
            None => depanic!("unexpected None: {:?}", msg),
        }
    }
}

impl<T, E: defmt::Format> Dewrap<T> for Result<T, E> {
    fn dewrap(self) -> T {
        match self {
            Ok(t) => t,
            Err(e) => depanic!("unwrap failed: {:?}", e),
        }
    }

    fn dexpect<M: defmt::Format>(self, msg: M) -> T {
        match self {
            Ok(t) => t,
            Err(e) => depanic!("unexpected error: {:?}: {:?}", msg, e),
        }
    }
}

@kuon
Copy link

kuon commented Sep 15, 2020

I started working with this, it works, but it is a bit cumbersome to have both unwrap() and dewrap() in the code.

It would be really nicer to be able to do unwrap() everywhere and have it being handled properly behind the scene. But I don't know if this is possible.

@jonas-schievink
Copy link
Contributor

No, there is no way to override unwrap() behavior (besides providing a panic handler, but that will not be able to use defmt).

@kuon
Copy link

kuon commented Sep 15, 2020

Would it be possible to provide dewrap() for Debug trait? This would permit having to dewrap() everywhere. That's a minor issue, but it can help when dealing with data that mix defmt::Format and Debug.

@jonas-schievink
Copy link
Contributor

I think not without specialization, if we want it to work with both Debug and defmt::Format types.

@jonas-schievink jonas-schievink added difficulty: easy Pretty easy to solve priority: low Low priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: enhancement Enhancement or feature request labels Nov 10, 2020
@jonas-schievink jonas-schievink added this to the v0.1.1 milestone Nov 17, 2020
@japaric
Copy link
Member

japaric commented Nov 23, 2020

@Dirbaio now that defmt::panic! has landed in the main branch want to send a PR with the dewrap implementation you posted in #161 (comment) ?

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 23, 2020

I've seen the work on panic/assert, very nice!

The problem with that implementation of dewrap() & co is that when they fail, the printed location information points inside the dewrap() impl instead of the call site, so the experience is really bad: "Oops, a dewrap failed somewhere, but I won't tell you where!".

I've experimented with unwrap!() and expect!() macros here, but these still have problems:

  • Syntax is less nice. Regular unwrap reads left-to-right: foo.bar().unwrap(), whereas this reads weird: unwrap!(foo.bar()).
  • They still have bad location info because they're regular macro_rules!. That would be solved making them proc macros, but that makes rust-analyzer autocomplete break inside them :(

One upside is that would allow us to print the failed expression, like asserts: "Unwrap of foo.bar() failed".

AFAICT there's no way to make "postfix/method" syntax macros like foo.bar().dewrap!(), unfortunately.

So I'm not sure what to do :(

@harrysarson
Copy link

@Dirbaio will #[track_caller] help here?

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 23, 2020

I looked at it, the problem is it gives us the caller information at runtime, whereas we want it at compile time to put it in the defmt log metadata.

Yes, it's possible to use the info at runtime like defmt::panic!("Unwrap failed at {:str}:{:u32}", Location::caller().file(), Location::caller().line()), but that means the entire file path strings are stored in flash and transmitted over the wire, which is precisely what defmt tries to avoid.

If this overhead is acceptable to you, you can already achieve this today with panic-probe with feature print-defmt, and just using regular unwrap and expect. The goal is to have similar functionality without the runtime cost.

@jonas-schievink
Copy link
Contributor

DWARF has facilities for macro-expanded code, maybe we "just" need to fix this on the host side by making use of that?

@jonas-schievink
Copy link
Contributor

Ah, that would only help with the less nice macro_rules! version, so ignore me.

Don't we get a backtrace when the panic happens though? That should contain the frame that called .dewrap().

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 23, 2020

in my experience the unwind is not reliable enough with optimizations (and building without is not viable: slow and doesn't fit in flash). It does work for getting a rough location, but many times I've had to stick a few log calls before unwraps to figure things out.

IMO it's good to have guaranteed-accurate location info in unwraps, for UX and for consistency with logs/panic/assert.

I think the best solution is adding unwrap!(val) and expect!(val, format_args) proc macros. Rust-analyzer is already broken with the other macros anyway.

Maybe we can do a single macro for both instead: unwrap!(val[, format_args]?), thoughts? This is inconsistent with std unwrap/expect, but is consistent with assert!.

I'll work on this if we're OK with the design.

@japaric
Copy link
Member

japaric commented Nov 23, 2020

in my experience the unwind is not reliable enough with optimizations (and building without is not viable: slow and doesn't fit in flash). It does work for getting a rough location,

I would be interested in hearing more about where / when this happens. It may an upstream issue in addr2line but if backtraces are not accurate, that's something that should be fixed.

The problem with that implementation of dewrap() & co is that when they fail, the printed location information points inside the dewrap() impl instead of the call site

I was aware of this but thought it was not much of an issue because the real location is printed in the backtrace, but if that's not the case then perhaps we should consider using something else.

DWARF has facilities for macro-expanded code,

(you may be referring to the DWARF stuff used in the backtrace (addr2line); that one maps a PC (Program Counter) address to a location, or to a list of inlined function frames. Not applicable here because it would require dewrap obtaining the PC of the caller frame somehow)

I'll work on this if we're OK with the design.

we'll get back to you

@Dirbaio Dirbaio mentioned this issue Nov 24, 2020
@Lotterleben
Copy link
Contributor

with #273 merged, can we close this?

@Dirbaio
Copy link
Contributor Author

Dirbaio commented Nov 26, 2020

yup, this is now done 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy Pretty easy to solve priority: low Low priority for the Knurling team status: needs PR Issue just needs a Pull Request implementing the changes type: enhancement Enhancement or feature request
Projects
None yet
Development

No branches or pull requests

6 participants