compiler fails to suggest `move` closure

[nikomatsakis]

This example gives a fairly opaque error message (playground):

pub struct VecMap<E, KeyFn> {
    data: Vec<E>,
    key_fn: KeyFn,
}

impl<E, K, KeyFn> VecMap<E, KeyFn>
where
    KeyFn: Fn(&E) -> K,
    K: Ord,
{
    fn iter<'k>(&'k self, key: &'k K) -> impl Iterator<Item = &'k E> {
        let (start, max) = (0, 0);
        self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
    }
}

We currently get:

error[E0597]: `self` does not live long enough
  --> src/lib.rs:13:57
   |
11 |     fn iter<'k>(&'k self, key: &'k K) -> impl Iterator<Item = &'k E> {
   |             --                           --------------------------- opaque type requires that `self` is borrowed for `'k`
   |             |
   |             lifetime `'k` defined here
12 |         let (start, max) = (0, 0);
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |                                                 ------  ^^^^ borrowed value does not live long enough
   |                                                 |
   |                                                 value captured here
14 |     }
   |     - `self` dropped here while still borrowed

But as of 1.34.0 we used to get:

error[E0373]: closure may outlive the current function, but it borrows `self`, which is owned by the current function
  --> /home/nmatsakis/tmp/foo.rs:13:49
   |
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |                                                 ^^^^^^  ---- `self` is borrowed here
   |                                                 |
   |                                                 may outlive borrowed value `self`
help: to force the closure to take ownership of `self` (and any other referenced variables), use the `move` keyword
   |
13 |         self.data[start..max].iter().take_while(move |elem| (self.key_fn)(elem) == *key)
   |                                                 ^^^^^^^^^^^

(There are also errors about key that are similar.)

[sinkuu]

Simpler reproduction: https://rust.godbolt.org/z/Ex3HIu

[david-mcgillicuddy-moixa]

It's interesting to compare to E0373:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=952f0e4a24e468e294e8051bfdd991ca
Which, at least in this case, have extremely similar error messages except that E0597 is missing the bit about move.

[ssanderson]

I ran into this issue today while working on a small project I'm using to learn Rust. I was quite stumped by the error I was seeing until some googling led me to this issue.

The code in my case looked like this:

Click to Expand

// Grid is a thin wrapper around a HashMap<(i64, i64), Cell> representing a 2D grid of values.
// Cell here is just an enum, not std::cell::Cell.
fn get_intersections<'a>(grid: &'a Grid<Cell>) -> impl Iterator<Item = &'a Coord> {
    grid.iter()
        // Filter to nonempty cells.
        .filter_map(|(coord, cell)| {
            if *cell != Cell::Empty {
                Some(coord)
            } else {
                None
            }
        })
        // Intersections are nonempty locations with at least 3 nonempty neighbors.
        .filter(|coord| {  // This needs to be a `move` closure to capture `grid`.
            let num_adjacent = grid
                .neighbors(coord)
                .filter(|&(_, cell)| cell != Cell::Empty)
                .count();
            num_adjacent >= 3
        })
}

and the error I get from rustc 1.40 is:

Click to Expand

error[E0597]: `grid` does not live long enough
   --> src/problem17.rs:214:32
    |
202 | fn get_intersections<'a>(grid: &'a Grid<Cell>) -> impl Iterator<Item = &'a Coord> {
    |                      --                           ------------------------------- opaque type requires that `grid` is borrowed for `'a`
    |                      |
    |                      lifetime `'a` defined here
...
213 |         .filter(|coord| {
    |                 ------- value captured here
214 |             let num_adjacent = grid
    |                                ^^^^ borrowed value does not live long enough
...
220 | }
    | - `grid` dropped here while still borrowed

As a relatively new rust user, I was confused because I thought this error message was telling me that my grid object wasn't going to outlive the returned iterator, so I wouldn't have thought to use move on the closure. After seeing the hint, however, it makes sense that I need to move the grid reference into the closure so that I can continue to refer to it after the function ends.

@nikomatsakis do you have a sense of how hard it might be to try to fix or improve this error? I'd be interested in taking a crack at it if you think it'd be a reasonable starting point.

[yuyoyuppe]

I was wondering recently if the current reference-capturing behavior of Fn is the most desired one, i.e. whether it should capture references by reference or by value. I've found this issue to be the most relevant one. If this was discussed before, please give me some pointers.

Consider this simple code:

fn f<'a>(v: &'a [i32]) -> impl Iterator<Item = i32> + 'a {
    v.iter().map(|e| if v.len() > 10 { e * 2 } else { e * 4 })
}

fn main() {
    let v = vec![1, 2, 3];
    for e in f(&v) {
        dbg!(e);
    }
}

It won't compile because we're essentially capturing the pointer to v reference which resides on the stack, so we must qualify our lambda with move to solve this.

However, I'd argue that the majority of users would expect that code to work, since it's very uncommon, I believe, to intentionally borrow a local reference.

Even if we change reference-capturing behavior of Fn, it would be possible to have the equivalent of current behavior by explicitly declaring a local variable:

fn f<'a>(v: &'a [i32]) -> impl Iterator<Item = i32> + 'a {
    let r = &v;
    v.iter().map(|e| if r.len() > 10 { e * 2 } else { e * 4 })
}

So, I think current Fn behavior could be improved. Of course, I may have missed some other side-effects by introducing this change.

[nikomatsakis]

@yuyoyuppe this can't be lightly changed in a backwards compatible fashion. We have to decide the mode of capture before we learn about the error that results from this code. Furthermore, there are cases where the choice to capture "in place" or "by move" results in user-visible distinctions in terms of what drops and what doesn't, so it's best for the rules there to be relatively predictable.

[m-ou-se]

Even simpler test case:

fn main() {
    let _f = {
        let i = 1;
        || i
    };
}

error[E0597]: `i` does not live long enough
 --> src/main.rs:4:12
  |
2 |     let _f = {
  |         -- borrow later stored here
3 |         let i = 1;
4 |         || i
  |         -- ^ borrowed value does not live long enough
  |         |
  |         value captured here
5 |     };
  |     - `i` dropped here while still borrowed

This one did also not have a suggestion for move in 1.34, unlike the first example in this issue.

[estebank]

Triage: the original report is fixed

error[E0373]: closure may outlive the current function, but it borrows `self`, which is owned by the current function
  --> src/lib.rs:13:49
   |
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |                                                 ^^^^^^ ------------- `self` is borrowed here
   |                                                 |
   |                                                 may outlive borrowed value `self`
   |
note: closure is returned here
  --> src/lib.rs:13:9
   |
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `self` (and any other referenced variables), use the `move` keyword
   |
13 |         self.data[start..max].iter().take_while(move |elem| (self.key_fn)(elem) == *key)
   |                                                 ++++

error[E0373]: closure may outlive the current function, but it borrows `key`, which is owned by the current function
  --> src/lib.rs:13:49
   |
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |                                                 ^^^^^^                        ---- `key` is borrowed here
   |                                                 |
   |                                                 may outlive borrowed value `key`
   |
note: closure is returned here
  --> src/lib.rs:13:9
   |
13 |         self.data[start..max].iter().take_while(|elem| (self.key_fn)(elem) == *key)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `key` (and any other referenced variables), use the `move` keyword
   |
13 |         self.data[start..max].iter().take_while(move |elem| (self.key_fn)(elem) == *key)
   |                                                 ++++

as well as the report in one of the comments

error[E0373]: closure may outlive the current function, but it borrows `v`, which is owned by the current function
 --> src/main.rs:4:18
  |
4 |     v.iter().map(|e| if v.len() > 10 { e * 2 } else { e * 4 })
  |                  ^^^    - `v` is borrowed here
  |                  |
  |                  may outlive borrowed value `v`
  |
note: closure is returned here
 --> src/main.rs:4:5
  |
4 |     v.iter().map(|e| if v.len() > 10 { e * 2 } else { e * 4 })
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `v` (and any other referenced variables), use the `move` keyword
  |
4 |     v.iter().map(move |e| if v.len() > 10 { e * 2 } else { e * 4 })
  |                  ++++

but the last repro isn't

error[E0597]: `i` does not live long enough
 --> src/main.rs:4:12
  |
2 |     let _f = {
  |         -- borrow later stored here
3 |         let i = 1;
4 |         || i
  |         -- ^ borrowed value does not live long enough
  |         |
  |         value captured here
5 |     };
  |     - `i` dropped here while still borrowed

We should also try to group together the errors in the first case, for multiple invalid captures in the same closure and emit a single error.

Edit: Looking at it, the final repro is covered by #58497, so I'm going to close this one.