Feedback on the new `closure_attr` utility/macro crate

code review
1

I’ve started giving feedback on this crate here

but that’s an off-topic discussion there. So we’re moving it into this new topic instead :slight_smile:

2

Thanks!

Great idea. It should be a straight-forward addition.

3

Actually… not just the body. It’s fairly niche, but closures could even be lurking in absurd places like types (e.g. on the some closure arguments). E.g. cases like

#[closure_attr::with_closure]
fn f() {
    let x: [i32; {
        fn f() {
            let n = 42;
            let closure = #[closure(clone n)]
            move || {
                let _x = n;
            };
        };
        0
    }] = [];
}

So you probably just want to use the syn::visit_mut::visit_expr_mut on the whole expression unconditionally. (I don’t know if there’s any advantages or disadvantages to doing this call before or after handling the closure case. Doing it before would however help with allowing all later short-circuiting (missing attribute case, and not-a-closure case) to be a simple return;).

Also excuse the off-topic discussions; maybe I should have opened an issue :slight_smile:

Another thing I could imagine would be nicer would be if the annotated captures were enforced to actually happen. The way this can work is via let _ = &name; lines[1] inside if the closure body. (Or more realistically for a macro, inside of a new block that replaces the closure body, and has the original closure body in return position.) This would make code like

fn send<T: Send>(_: T) {}

#[derive(Clone)]
struct SendPointer(*const ());
unsafe impl Send for SendPointer {}

#[closure_attr::with_closure]
fn f() {
    let p = SendPointer(std::ptr::null());
    send(#[closure(clone p)] move || {
        // let _ = &p; // uncomment to make this compile
        // without this, our new edition2021 closure capturing
        // only captures p.0 by move, not the whole of p
        p.0;
    });
}

compile, and overall it seems less surprising to me if the promised explicit captures were forced to actually happen (e.g. also even if the variable is completely unused in the closure body). One downside would be that unused-variable warnings would disappear.

  1. inspired by the compiler suggestions from our edition 2018 -> 2021 migration lints ↩︎

4

Do you have powers to split the thread in 2?

I'm willing to make this trade if it solves errors that wouldn't be in the original. e.g. removing the attribute produces the same error:

fn f() {
    let p = SendPointer(std::ptr::null());
    send(move || {
        // let _ = &p; // uncomment to make this compile
        // without this, our new edition2021 closure capturing
        // only captures p.0 by move, not the whole of p
        p.0;
    });
}

The compiler placed the error from your example at a bad spot; I need to do some span work.

5

Yeah, already working on it :slight_smile:

Edit: Here we are :tada:

1 Like
6

Oh, I didn’t even notice the span pointing to the #[closure_attr::with_closure].

My argument was that your syntax reads like an explicit “I want to capture these variables in these ways” lists, but then what’s actually happening is that the actual capture still depends on the usage in the move closure. With that context, even supporting a move x mode in your attribute list could be useful.

On the other hand, it’s probably a relatively rare issue, and on the other hand unused variable warnings are probably useful. If only there was a way to do what let _ = &p; does, without it counting as a usage of the variable.

7

Maybe an option?

force clone x? whole clone x?

whole fits nicely with your example. force fits nicely with making move always move.

8

Ah, and of course, the variable is also used by moving it into the closure. Nonetheless turns out, there is a way

loop {
    break;
    let _ = &p; // uncomment to make this compile
}

This makes unused-variable stay (slightly different error message, containing a not entirely helpful “did you mean to capture by reference instead?” hint, but good enough IMO!)

fn send<T: Send + FnMut()>(_: T) {}

#[derive(Clone)]
struct SendPointer(*const ());
unsafe impl Send for SendPointer {}

#[closure_attr::with_closure]
fn f() {
    let p = SendPointer(std::ptr::null());
    send(#[closure(clone p)] move || {
        #[allow(unreachable_code)]
        loop {
            break;
            let _ = &p; // makes this code compile even without the below commented out
        }
        {
            // p.0; // commented out this to make `p` unused
        }
    });
}

warning: unused variable: `p`
  --> src/main.rs:18:22
   |
18 |             let _ = &p; // uncomment to make this compile
   |                      ^
   |
   = help: did you mean to capture by reference instead?
   = note: `#[warn(unused_variables)]` on by default
9

I agree. Good enough.

10

The split between rcweak and arcweak bothers me. I'm thinking of replacing them with weak and make it use a new trait.

1 Like
11

New version published with all the changes above. I also added move and move mut since it now forces full capture.

2 Likes
12

Hmm… the behavior of weak seems suboptimal. For something like an Option-returning function, the choice of Default::default() i.e. None might be reasonable, but I feel like for other cases, it can be surprising.

Of course, I haven’t practically used this feature, so I’m not fully aware of what the intended/common use-case even looks like – for me a closure returning () just silently doing nothing, or a function returning an i32 just silently doing nothing and returning a garbage-value (0) seems bad.

I don’t have a good idea on what the best alternative behavior would be. Maybe the default could be to just support any Result<T, E> return type with E: From<AccessWeakInClosureError>, for some custom error type provided by the crate (that may contain helpful extra information such as the source location and the name of the variable that failed to upgrade); and then there could be some syntax for opting into panicking behavior, and maybe some syntax for providing a completely custom way of handling the error.

Unrelated but also about weak (even for the current behavior involving Default, but also easily adapted to alternative choices of fallback behavior), is there any reason against a more straightforward desugaring such as the following?

{
    let x = ::closure_attr::Downgrade::downgrade(&x);
    move || {
        let Some(x) = ::closure_attr::Upgrade::upgrade(&x) else {
            return ::…path-to…::Default::default();
        };
        CLOSURE_BODY
    }        
}

instead of the current

{
    let x = ::closure_attr::Downgrade::downgrade(&x);
    move || {
        (|| {
            let x = ::closure_attr::Upgrade::upgrade(&x)?;
            Some((|| CLOSURE_BODY)())
        })()
            .unwrap_or_default()
    }
}
13

There's not. ? seemed more convenient until it ballooned.

The most common return type for my use case is (). Weak pointers dieing will happen during teardown, with occasional queued up events still firing which no longer matter (think mouse_move on a child window before it has received notification that its parent has started destruction). Default::default was an easy generalization, but I now see where it would cause major issues for other use cases.

14

I switched to:

  • weak <ident>: Downgrade an Rc, Arc, or anything else which implements [Downgrade]. Captures the downgraded pointer. This helps break up reference loops.
  • fail(<expr>) <ident>: Like weak, but upgrades the weak pointer before executing the closure body. If the upgrade fails, it skips executing the body and returns the expression.
  • panic <ident>: Like weak, but upgrades the weak pointer before executing the closure body. If the upgrade fails, it panics with message "Closure failed to upgrade weak pointer".

Thanks for the feedback you've given me so far! I believe it improves the crate's quality.

15

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.