rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.98k stars 12.68k forks source link

#[track_caller] on closures? #74042

Closed anp closed 2 years ago

anp commented 4 years ago

One problem I have is that it's not possible to put #[track_caller] on closures, this is really useful when thread_local is involved because then there's a lot of logic in closures that you might want to propagate ie

thread_local! {static DONE: bool = false;}

#[track_caller]
fn assert_done() {
    DONE.with(
        #[track_caller]
        |b| assert!(b),
    );
}

fn main() {
    assert_done();
}

Originally posted by @elichai in https://github.com/rust-lang/rust/issues/47809#issuecomment-653777647

anp commented 4 years ago

cc @eddyb I cant' recall right now whether this restriction is a requirement of the implementation we chose or whether we just left it as-designed in the RFC. Can you?

nikomatsakis commented 4 years ago

If there is no technical reason not to support it, I would think we should. I imagine one challenge is that calls to closures are being dispatched through the Fn trait, but I imagine we resolve that because we know the callee at monomorphization time, unless it's a virtual call in which case we use a shim?

eddyb commented 4 years ago

I would expect this to "just work", if the attribute syntax parses.

eddyb commented 4 years ago

Although... the example is a bit suspect, it doesn't specify what the expected behavior is, but the call of the closure in the with method is what you'll get as the assert! failure location, if #[track_caller] works correctly on closures.

If you want to propagate all the way to main, that's hard. You want to defer errors until you can panic outside the closure, so that you're not inside the with call.

We could also try to make a way to override the "caller location" of a call (and pass in an outer one), but the ergonomics of that are worse for this example (where assert!(DONE.with(|b| b)) "just works").

elichai commented 4 years ago

Although... the example is a bit suspect, it doesn't specify what the expected behavior is, but the call of the closure in the with method is what you'll get as the assert! failure location, if #[track_caller] works correctly on closures.

Ha! That's interesting, I didn't think about the fact that I'm not actually the one calling the closure, so my #[track_caller] on assert_done doesn't do anything because there's no #[track_caller] on the with method and whatever its internals are.

nikomatsakis commented 4 years ago

I had assumed that, in the example, the with function would also have #[track_caller]. I could see that becoming a common pattern, but yeah, it does rely on that.

anp commented 4 years ago

I've opened https://github.com/rust-lang/rust/pull/74492 with a bare minimum change and test included. It's currently failing because MIR argument counts aren't happy. I'll dig in at some point soon, if anyone has suggestions I'm all ears.

shepmaster commented 3 years ago

We could also try to make a way to override the "caller location" of a call (and pass in an outer one)

Is there some way of doing that? I'm trying to do something in the same vein as the original post, but using Result::map_err. That function doesn't have #[track_caller] (and I don't think it should), but for this specific usage, I'd like it to.

A made-up example:

use core::panic::Location;

#[track_caller]
fn demo(r: Result<u8, String>) -> Result<u8, String> {
    let location = Location::caller();
    r.map_err(|e| {
        Location::pretend_we_are_at(location);
        another_function();
        e
    })
}

#[track_caller]
fn another_location() {}

Another route would be if I could get access to the inner implementation of another_function... IIRC there was a function that accepted a hidden argument of the location. Then perhaps an attribute like:

r.map_err(|e| {
    #[delegate_caller_location(location)]
    another_function();
    e
})
dtolnay commented 2 years ago

#[track_caller] on closures (and generators) got implemented in https://github.com/rust-lang/rust/pull/87064, unstable for now. I am closing this issue in favor of https://github.com/rust-lang/rust/issues/87417 which is the tracking issue for the feature(closure_track_caller) feature.