Setting/Getting value across closures

wuavcv · June 17, 2023, 9:52am

Playground - (mock code, with possible solution)
Playground - (more accurate mock code, with solution)

I was trying to refactor some code (VERSION 1) to get/set some values across some wasm_bindgen closures, without using static/Atomic and got stuck (VERSION 2), because:

In VERSION 2 (and many iterations of it):

The values I set are dropped immediately.
The cloned values are not visible from the other closures.
A captured variable in a closure may not live long enough. [0373]
Attempted to dereference a variable which cannot be dereferenced. [0614]
A variable used inside an inner function comes from a dynamic environment. [0434]
A variable was borrowed as mutable more than once. [0499]
This error occurs because you tried to mutably borrow a non-mutable variable. [0596]
Borrowed data escapes outside of closure. [0521]
A variable was used after its contents have been moved elsewhere. [0382]

And so on...
I gave up because the errors were becoming circular with each attempt.

Asking on IRC, and looking at posts like this:

Do I have to wrap everything in VERSION 2 with Rc, RefCell, Arc, Mutex, and the like? If so, I'll just stick with static/Atomic version because it seems simpler.

Also, reading Rust By Example: Capturing was helpful. But the example given is simpler than my situation.

How would I get VERSION 2 to work? Thanks in advance.

Edit 1: The closures in VERSION 1 use the move keyword because they are used for wasm_bindgen::closures. Compiler errors reference my wasm_bindgen::closure wrapper-function

hax10 · June 17, 2023, 11:14am

Does this do what you want?

Edit to add explanation:

closure_4 modifies touch.a
closure_5 modifies touch.b
closure_6 reads both touch.a and touch.b

Since these closures exist concurrently, there is a conflict here if you use a normal i32 type for touch.a and touch.b, because closure_6 will hold an immutable reference to both variables while mutable references exist in the other two closures. The compiler won't allow this.

By using an atomic type, we can sidestep this problem because it allows safe reads and writes behind an immutable reference, and the compiler is happy to let you create many of these concurrently.

// VERSION 2 (set, get, with struct)
struct TouchT {
    a: AtomicI32,
    b: AtomicI32,
}
let touch = TouchT {
    a: AtomicI32::new(0),
    b: AtomicI32::new(0),
};
// (mock) wasm_bindgen closures
let closure_4 = |n: i32| {
    touch.a.store(n, Ordering::Relaxed);
    println!("touch.a: {}", touch.a.load(Ordering::Relaxed));
};
let closure_5 = |n: i32| {
    touch.b.store(n, Ordering::Relaxed);
    println!("touch.b: {}", touch.b.load(Ordering::Relaxed));
};
let closure_6 = || {
    println!(
        "{} + {} = {}",
        touch.a.load(Ordering::Relaxed),
        touch.b.load(Ordering::Relaxed),
        touch.a.load(Ordering::Relaxed) + touch.b.load(Ordering::Relaxed)
    );
};
closure_4(N1);
closure_5(N2);
closure_6();

vague · June 17, 2023, 11:52am

github.com/rust-lang/rust

Moving structs with Copy fields into closures causes surprising behavior

opened 12:27AM - 09 May 23 UTC

alexblanck

C-bug

I noticed some counterintuitive behavior related to partial moves of Copy struct… fields (ex: u64 fields) into closures. ## Minimal Reproducible Example ```rust #[derive(Debug)] struct Statistics { calls: u64, successes: u64, } fn main() { let mut statistics = Statistics { calls: 0, successes: 0, }; let mut generator = move || { statistics.calls += 1; }; generator(); statistics.successes += 1; println!("{statistics:?}"); } ``` This code compiles and prints ``` Statistics { calls: 0, successes: 1 } ``` [Playground Link](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c4180fa1795ad2b77b0c0aa28aeaf37a) ## What I Expected I would have expected a `borrow of partially moved value: 'statistics'` error at the `println!`, since the closure takes ownership of the `statistics.calls` field and then the field is later read by the print call. The actual behavior, where the updates to `statistics.calls` have no observable effect, looks just like a silent copy of the whole struct. This surprised me. When I wrote the code, I expected either a compiler error or an output of `Statistics { calls: 1, successes: 1 }`. ## Investigation I experimented a bit with the types of the fields within the struct, and I do get the compiler error I expect if `Statistics.calls` is a non-Copy type like `Vec`. I didn't know about partial moves when I wrote the initial code. I expected the whole struct to move into the closure. It's still surprising to me that partial moves can create this type of "silent copy" behavior depending on the type of the struct's fields. I did find a similar issue titled [Move closure copies :Copy variables silently](https://github.com/rust-lang/rust/issues/63220), but the code that I wrote is not covered by the "unused variable" warning added in response to that issue. ## Solution I'm new to Rust so I'm not sure this is a bug, but it's still confusing behavior. Perhaps, like the other issue I linked, there is room for a lint or warning here. Something that warned me that the closure's modification of `statistics.calls` had no effect would be helpful, for example.

The pattern is simplified as follows: Rust Playground

// only compiles after 2021 edition
fn main() {
    let mut a = A { a: 123 };
    let mut f = move || {
        a.a = 456; // disjointly capture the field: i.e. let mut val = a.a; val = 456
        // (note the field type is Copy, so this is a copy & move 
        // but the original field is still there with value unmodified)
        dbg!(a.a); // i.e dbg!(val);  => 456
    };
    dbg!(&a); // 123
    f();
    dbg!(&a); // 123
}

#[derive(Debug)]
struct A {
    a: i32,
}

If the captured field is not Copy, the code won't comple (since the value is moved into the closure, and it can never be used again). Rust Playground

Update: I think the behavior here should be clearly documented

Disjoint capture in closures - The Rust Edition Guide should clearly describe the case on fields that are Copy
Closure types - The Rust Reference should be updated too because
- every time I read the capture mode in closures, the rule on move is actually one sentence, which seems concise, but it's really hard for me to understand at once:
If the move keyword is used, then all captures are by move or, for Copy types, by copy, regardless of whether a borrow would work.
- disjoint capture isn't mentioned at all (for a long time)

wuavcv · June 17, 2023, 2:44pm

That is an interesting page. I'm still absorbing it. Thanks for that link!

I also updated your playground to make sure that it wasn't just a syntax issue holding me up.

wuavcv · June 17, 2023, 2:48pm

This is kind of what I was looking for. On first glance it seems like an in-between: Atomic still used, but the values are now in a struct. I will try out this method in the actual code and see if it compiles. Thank you!

Edit 1: Back to E0373 in my actual code.

error[E0373]: closure may outlive the current function, but it borrows `touch.start_x`, which is owned by the current function

Edit 2: Updated playground reference.

vague · June 17, 2023, 3:01pm

The keypoint here is:

for Copy fields and move capture mode, the field value is copied into the closure, i.e.:
- the field value is essentially not moved and not modified
- the copied value is modified in the colsure
- the field access expressions in the closure, i.e. a.a and a.b, are considered to be new values in the smaller scope instead of the values from the struct

so the result is dbg!(a.a + a.b); // dbg!(val1 + val2); => remains 0, not 3.

Update: I forget to explain why the version1 works:

though you're using move mode on capture, static A: AtomicI32 = AtomicI32::new(0); is not Copy, so only the &AtomicI32 is moved into the closure, so the values are modified.

vague · June 17, 2023, 3:35pm

Based on your code, some cases to support my explanation

the field type is AtomicI32 which is not Copy, and the capture mode is move, then the ownership problem occurs: Rust Playground

so consider the non-move capture mode this time:

the field type is AtomicI32, and the capture mode is default (not by move in most cases), then the borrow rule should stand: Rust Playground (this works as you require)
the field type is non-Copy Wrapper(i32), and the capture mode is not by move: Rust Playground (this works as you require)

The Wrapper(i32) case answers your question

in multithread context, it's true for Arc/Mutex since the synchronization is a cost that must be paid (but your code snippet doesn't apply here)
what you give so far is a case related to the capture mode in closure, so you don't have to use types you mentioned above

wuavcv · June 17, 2023, 4:08pm

I'm back on E0373 in the actual code...

error[E0373]: closure may outlive the current function, but it borrows `touch.start_x`, which is owned by the current function
   --> src/swipe.rs:32:24
    |
32  |     let r_closure_ts = |e: TouchEvent| {
    |                        ^^^^^^^^^^^^^^^ may outlive borrowed value `touch.start_x`
...
48  |         touch.start_x.store(page_x, Ordering::Relaxed);
    |         ------------- `touch.start_x` is borrowed here
    |
note: function requires argument type to outlive `'static`
   --> src/swipe.rs:100:25
    |
100 |     let wb_closure_ts = r_closure_ts.to_wb_closure_fnmut();
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `touch.start_x` (and any other referenced variables), use the `move` keyword
    |
32  |     let r_closure_ts = move |e: TouchEvent| {

e.TouchEvent corresponds to the N1, N2 static items in the playground code.
The help suggestion:

help: to force the closure to take ownership of touch.start_x (and any other referenced variables), use the move keyword

...will break the suggested solution because the variables will be moved (and lost).

error[E0382]: use of moved value: `touch.start_x`
  --> src/swipe.rs:76:24
   |
32 |     let r_closure_ts = move |e: TouchEvent| {
   |                        -------------------- value moved into closure here
...
48 |         touch.start_x.store(page_x, Ordering::Relaxed);
   |         ------------- variable moved due to use in closure
...
76 |     let r_closure_te = move || {
   |                        ^^^^^^^ value used here after move
...
81 |         let start_x = touch.start_x.load(Ordering::Relaxed);
   |                       ------------- use occurs due to use in closure
   |
   = note: move occurs because `touch.start_x` has type `AtomicI32`, which does not implement the `Copy` trait

to_wb_closure_fnmut is a wrapper function (IN: rust closure, OUT: wasm_bindgen closure) that uses static. I did not add it to the playground code, but on second thought, maybe I should, because it's part of the roadblock. (edit: probably won't work because use wasm_bindgen::closure::Closure is not available in the playground).

pub trait ToWBClosureFnMut<Dyn: ?Sized> {
    fn to_wb_closure_fnmut(self) -> Closure<Dyn>;
}

impl<F> ToWBClosureFnMut<dyn FnMut()> for F
where
    F: FnMut() + 'static,
{
    fn to_wb_closure_fnmut(self) -> Closure<dyn FnMut()> {
        let bx_closure = Box::new(self) as Box<dyn FnMut()>;
        Closure::wrap(bx_closure)
    }
}

impl<F, A, R> ToWBClosureFnMut<dyn FnMut(A) -> R> for F
where
    F: FnMut(A) -> R + 'static,
    A: wasm_bindgen::convert::FromWasmAbi + 'static,
    R: wasm_bindgen::convert::IntoWasmAbi + 'static,
{
    fn to_wb_closure_fnmut(self) -> Closure<dyn FnMut(A) -> R> {
        let bx_closure = Box::new(self) as Box<dyn FnMut(A) -> R>;
        Closure::wrap(bx_closure)
    }
}

hax10 · June 18, 2023, 3:46am

Can you describe what you're trying to do with these events? Maybe there's some way to rethink the solution in a way the compiler will accept.

wuavcv · June 18, 2023, 5:10am

Well, I actually made some progress today.

GOAL: Try to get playground VERSION 3 working with actual wasm_bindgen code.
For that to happen, the 4 closures in the swipe module CANNOT have the move keyword.

Step 1. (swipe closures 1-3)
let closure_r_ts = |e: TouchEvent| { /.../ }
let closure_r_tm = |e: TouchEvent| { /.../ }
let closure_r_tc = || {};

Step 1. The move keyword was able to be removed from 3 of the 4 closures without issue - even with Atomic references

Step 2. (click closure, separate module)
error[E0501]: cannot borrow `*slides` as immutable because previous closure requires unique access
   --> src/lib.rs:241:14
    |
231 |     let closure_r = |e: Event| {
    |                     ---------- closure construction occurs here
...
234 |         slides.get_next(event);
    |         ------ first borrow occurs due to use of `*slides` in closure
...
237 |     let closure_wb = closure_r.to_closure_wb_fnmut();
    |                      ------------------------------- argument requires that `*slides` is borrowed for `'static`
...
241 |     let el = slides.get_el(id);
    |              ^^^^^^^^^^^^^^^^^ second borrow occurs here

Step 2. Cleared this error by reordering the borrows.
I put the borrow without the closure FIRST, then the borrow with the closure AFTER.
Compiler let both borrows through.

Step 3. (click closure, separate module)
error[E0521]: borrowed data escapes outside of function
   --> src/lib.rs:245:22
    |
229 | fn add_click(slides: &mut SlidesT) {
    |              ------  - let's call the lifetime of this reference `'1`
    |              |
    |              `slides` is a reference that is only valid in the function body
...
245 |     let closure_wb = closure_r.to_closure_wb_fnmut();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                      |
    |                      `slides` escapes the function body here
    |                      argument requires that `'1` must outlive `'static`

Step 3. Cleared this error by using the clone + move method.
slides was cloned outside closure.
Cloned value was used inside closure.
Force ownership of cloned value by using move keyword.
(OK to use move keyword here because there aren't any Atomics.)

Step 4. (swipe closure #4)
error[E0501]: cannot borrow `*slides` as immutable because previous closure requires unique access
  --> src/swipe.rs:85:17
   |
61 |     let closure_r_te = || {
   |                        -- closure construction occurs here
...
75 |                 slides.get_next(event); // left swipe [1]<[2]
   |                 ------ first borrow occurs due to use of `*slides` in closure
...
85 |     let el_id = slides.get_el(id);
   |                 ^^^^^^^^^^^^^^^^^ second borrow occurs here
...
95 |     let closure_wb_te = closure_r_te.to_closure_wb_fnmut();
   |                         ---------------------------------- argument requires that `*slides` is borrowed for `'static`
(Same as click closure)

Step 4. Cleared this error by reordering the borrows.
I put the borrow without the closure FIRST, then the borrow with the closure (closure +4) AFTER.
Compiler let both borrows through.

Step 5. (swipe closure)
error[E0521]: borrowed data escapes outside of function
  --> src/swipe.rs:99:25
   |
7  | pub fn add_swipe(slides: &mut SlidesT) {
   |                  ------  - let's call the lifetime of this reference `'1`
   |                  |
   |                  `slides` is a reference that is only valid in the function body
...
99 |     let closure_wb_te = closure_r_te.to_closure_wb_fnmut();
   |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |                         |
   |                         `slides` escapes the function body here
   |                         argument requires that `'1` must outlive `'static`
(Same as click closure)

Step 5. Cleared this error by splitting the closure into two parts:

A closure that uses the clone + move method (see above)
A closure that contains the Atomics (no move keyword).
The closure with the moved slides value then calls the closure with the Atomics.

So now every closure containing Atomics do not have the move keyword. So can I now do VERSION 3 with the struct/Atomics?

Not yet: The borrowed touch is the newly added struct. This is where I currently am.

error[E0373]: closure may outlive the current function, but it borrows `touch.start_x`, which is owned by the current function
   --> src/swipe.rs:32:24
    |
32  |     let closure_r_ts = |e: TouchEvent| {
    |                        ^^^^^^^^^^^^^^^ may outlive borrowed value `touch.start_x`
...
52  |         touch.start_x.store(page_x, Ordering::Relaxed);
    |         ------------- `touch.start_x` is borrowed here
    |
note: function requires argument type to outlive `'static`
   --> src/swipe.rs:119:25
    |
119 |     let closure_wb_ts = closure_r_ts.to_closure_wb_fnmut();
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `touch.start_x` (and any other referenced variables), use the `move` keyword
    |
32  |     let closure_r_ts = move |e: TouchEvent| {

hax10 · June 18, 2023, 10:50am

Does this code example give any inspiration?

https://rustwasm.github.io/wasm-bindgen/examples/paint.html

wuavcv · June 18, 2023, 2:13pm

Not really. I was trying to refactor the static+atomic references into struct+atomic. The example you linked makes no use of structs.

I feel like I'm close. The wasm_closures use Box which triggers static lifetimes. Which is why I think it's necessary to use the move and clone() so much in wasm code.

E.g.

Closure::wrap(Box::new(<rust closure>) as Box<dyn FnMut()>)

error[E0716]: temporary value dropped while borrowed
   --> src/swipe.rs:26:18
    |
26  |           move_y: &AtomicI32::new(0),
    |                    ^^^^^^^^^^^^^^^^^ creates a temporary value which is freed while still in use
...
38  |       let closure_wb_ts = Closure::wrap(Box::new(
    |  _______________________________________-
39  | |             |e: TouchEvent| {
40  | |             let tgt_touch = e.target_touches().get(0);
41  | |             let tgt_touch = match tgt_touch {
...   |
61  | |         }
62  | |     ) as Box<dyn FnMut(TouchEvent)>);
    | |_____- cast requires that borrow lasts for `'static`
...
161 |   }
    |   - temporary value is freed at the end of this statement

error: lifetime may not live long enough
   --> src/swipe.rs:104:39
    |
8   |   pub fn add_swipe(slides: &mut SlidesT) {
    |                            - let's call the lifetime of this reference `'1`
...
104 |       let closure_wb_te = Closure::wrap(Box::new(
    |  _______________________________________^
105 | |         |e: TouchEvent| {
106 | |             e.prevent_default();
107 | |             let event = "swipe";
...   |
132 | |         }
133 | |     ) as Box<dyn FnMut(TouchEvent)>);
    | |_____^ cast requires that `'1` must outlive `'static`

I was trying to find a way NOT to use move and clone(), just so I can put some [shared] integers in a struct, but it just might not be a reasonable/feasible ask.

wuavcv · June 18, 2023, 5:27pm

Update: I have a playground implementation that better simulates what is actually happening in the wasm code:

playground

Pretty proud of myself of being able to reproduce the same kind of errors without access to the wasm_bindgen::closure::Closure.

The problematic function is struct_atomic - trying to get that to work without moving or cloning. Again, it may not be possible at all using a struct.

quinedot · June 18, 2023, 6:07pm

The barrier isn't "struct or not", it's "static or local variable". You can't borrow a local variable for 'static, and that's why you need an Arc or such instead of borrowing in the case of variables.

(You can put your struct in a static instead of putting the individual fields in statics.)

wuavcv · June 18, 2023, 7:13pm

I agree. Which is why I'm thinking the method I already have (declaring static/Atomic for each variable) is what I'll stay with, out of simplicity.

I tried creating a struct with static elements but it's not sticking yet...

    // struct/atomic
    struct TouchT {
        a: &'static i32,
        b: &'static i32,
    }
    let mut touch = TouchT {
        a: &0,
        b: &0,
    };

quinedot · June 18, 2023, 7:38pm

I meant like so:

    static TOUCH: TouchT = TouchT {
        a: AtomicI32::new(0),
        b: AtomicI32::new(0),
    };

wuavcv · June 18, 2023, 8:32pm

That's absolutely what I was looking for (as far as using atomics with a struct)!
It's way better that where I was heading:

    // struct/atomic
    struct TouchT {
        a: &'static i32,
        b: &'static i32,
    }
    static A: &'static i32 = &0;
    static B: &'static i32 = &0;
    let mut touch = TouchT {
        a: A,
        b: B,
    };

I tried your struct on the actual wasm code I have and it worked fine. Not one issue.
Marking this thread solved. Thanks everyone for the help and the links!

system · September 16, 2023, 8:33pm

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.