Mutating self inside an iter.map(...)?

help
1

I am trying to build a method on self that builds a set of reverse lookup structures based on the information in a passed in structures. I used methods on the parameter to get iterators to get everything that needs to be looked up. (I did have this as a free standing methods. I thought it would be better as a structure method, and hoped that would fix this problem. But it didn't. The compiler is complaining about moves into the closures:

    pub fn mk_indices(&mut self, rolo: &RolodexBase) {
        self.short_label_list = Vec::new();
        self.long_label_list = Vec::new();
        self.label_to_cat = HashMap::new();

        rolo.short_cat_iter().into_iter().map(|cstr| {
            rolo.cat_label_iter(cstr.clone()).into_iter().map(move |label| {
                self.short_label_list.push(label.clone());
                self.label_to_cat.insert(label.clone(), cstr.clone());
            })
        });
        rolo.long_cat_iter().into_iter().map(|cstr| {
            rolo.cat_label_iter(cstr.clone()).into_iter().map(move |label| {
                self.long_label_list.push(label.clone());
                self.label_to_cat.insert(label.clone(), cstr.clone());
            })
        });
    }

And the compiler errors are:

error[E0507]: cannot move out of `self`, a captured variable in an `FnMut` closure
   --> src\rolo_window\person_gui.rs:81:63
    |
 69 |     pub fn mk_indices(&mut self, rolo: &RolodexBase) {
    |                       ---------
    |                       |
    |                       captured outer variable
    |                       move occurs because `self` has type `&mut PeopleGuiState`, which does not implement the `Copy` trait
...
 80 |         rolo.long_cat_iter().into_iter().map(|cstr| {
    |                                              ------ captured by this `FnMut` closure
 81 |             rolo.cat_label_iter(cstr.clone()).into_iter().map(move |label| {
    |                                                               ^^^^^^^^^^^^ `self` is moved here
 82 |                 self.long_label_list.push(label.clone());
    |                 -------------------- variable moved due to use in closure
    |
help: `Fn` and `FnMut` closures require captured values to be able to be consumed multiple times, but `FnOnce` closures may consume them only once
   --> C:\Users\jmh\.rustup\toolchains\stable-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\iter\traits\iterator.rs:776:12
    |
776 |         F: FnMut(Self::Item) -> B,
    |            ^^^^^^^^^^^^^^^^^^^^^^

error[E0382]: use of moved value: `self`
  --> src\rolo_window\person_gui.rs:80:46
   |
69 |     pub fn mk_indices(&mut self, rolo: &RolodexBase) {
   |                       --------- move occurs because `self` has type `&mut PeopleGuiState`, which does not implement the `Copy` trait
...
74 |         rolo.short_cat_iter().into_iter().map(|cstr| {
   |                                               ------ value moved into closure here
75 |             rolo.cat_label_iter(cstr.clone()).into_iter().map(move |label| {
76 |                 self.short_label_list.push(label.clone());
   |                 --------------------- variable moved due to use in closure
...
80 |         rolo.long_cat_iter().into_iter().map(|cstr| {
   |                                              ^^^^^^ value used here after move
81 |             rolo.cat_label_iter(cstr.clone()).into_iter().map(move |label| {
82 |                 self.long_label_list.push(label.clone());
   |                 -------------------- use occurs due to use in closure

The point is to mutate self. Copying it to use in the iterators won't do.
I tried using local variables instead of the self fields, and it gave me similar errors with proposed resolutions that would not leave the constructed information after the iterations.
I presume I ma missing something obvious. But staring at it for a few hours didn't reveal anything. Sorry.
Thanks,
Joel

2

You'll need to create temporary variables outside of the closure to prevent self from being captured.

3

The root cause here is that you are misusing Iterator::map(). Iterator::map() produces another iterator and lazily executes the provided function as the produced iterator is iterated. But what you're trying to do is take an action on each element immediately, so you should be using for_each() or a plain for loop. (Your code, if it had compiled, would not have done anything, because all of the the mapped iterators were discarded without getting iterated.)

I recommend the plain for loop because it means you do not need any closures at all. (Using closures reduces how much flexibility the borrow checker can offer you.)

pub fn mk_indices(&mut self, rolo: &RolodexBase) {
    self.short_label_list = Vec::new();
    self.long_label_list = Vec::new();
    self.label_to_cat = HashMap::new();

    for cstr in rolo.short_cat_iter() {
        for label in rolo.cat_label_iter(cstr.clone()) {
            self.short_label_list.push(label.clone());
            self.label_to_cat.insert(label.clone(), cstr.clone());
        }
    }
    for cstr in rolo.long_cat_iter() {
        for label in rolo.cat_label_iter(cstr.clone()) {
            self.long_label_list.push(label.clone());
            self.label_to_cat.insert(label.clone(), cstr.clone());
        }
    }
}

The immediate reason for your borrowing errors is that the move from the closures is inappropriate here. move makes each mentioned value moved into the closure — in particular, the mutable reference that is self — but what you need is to allow self to be reborrowed twice to do your two things. As a general rule, move is most helpful when you are returning a closure, and least helpful when all the use of the closure will happen immediately — and your code was incorrectly trying to return closures inside of map().

1 Like
4

Given that I need to end up changing the fields in self, can you elaborate on what temporary variables I should use, and how? I could clearly make copes of the parts of self locally, but then the .push() and .insert() methods wouldn't be changing what is in self. The whole point is to be able to use the constructed fields in self later for lookups.
Yours,
Joel

5

Ahh. I am happy to use for loops instead. I think I see why that would solve the problem.
And indeed it did work. I guess I had over-reacted to the frequent advice to use iterators instead of for loops. So it goes.
Thank you,
Joel

6

It can't work in the OP nested scenario where it is attempting to create an iterator where every item holds a &mut Self capture.

1 Like
7

Ah, right! Had not seen that he had nested iterators.

1 Like