Lifetime inference related regression in latest nightly.

[mitchmindtree]

Travis failed to build one of conrod's examples using the latest nightly due to the following error.

examples/all_widgets.rs:330:24: 330:54 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements [E0495]
examples/all_widgets.rs:330             let elem = &mut app.bool_matrix[col][row];
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
examples/all_widgets.rs:313:5: 314:20 note: first, the lifetime cannot outlive the method call at 313:4...
examples/all_widgets.rs:313     WidgetMatrix::new(cols, rows)
examples/all_widgets.rs:314         .down(20.0)
examples/all_widgets.rs:313:5: 313:34 note: ...so that method receiver is valid for the method call
examples/all_widgets.rs:313     WidgetMatrix::new(cols, rows)
                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
examples/all_widgets.rs:330:24: 330:54 note: but, the lifetime must be valid for the expression at 330:23...
examples/all_widgets.rs:330             let elem = &mut app.bool_matrix[col][row];
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
examples/all_widgets.rs:330:24: 330:54 note: ...so that reference is valid at the time of borrow
examples/all_widgets.rs:330             let elem = &mut app.bool_matrix[col][row];
                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The area of the example code that fails is here.

It was able to build this same example without any problems using the rustc 1.5 stable.

[alexcrichton]

Minimized:

fn main() {                      
    let foo = &mut true;         
    (|| {                        
        let bar = &mut *foo;     
        (move |b: bool| *bar = b)
    });                          
}                                

cc @rust-lang/compiler

triage: I-nominated

[arielb1]

The code in the example would be broken if each_widget called the inner method twice with col=row=0 and set executed the closures in separate threads (making the necessary things Sync).

I can't get such a "live" example to compile, though.

[nikomatsakis]

This is probably related to the soundness fix that @pnkfelix landed, which required the return types of closures to outlive the call site.

[nikomatsakis]

#30341, more specifically.

[arielb1]

Could someone provide a self-contained environment to compile the all_widgets example so I can poke with it?

[nikomatsakis]

I've decided that both the original and minified code ought NOT to compile. Both examples, as @arielb1 pointed out, only ever compiled due to a bug. However, it's possible that if we improved closure kind inference, the minified snippet perhaps COULD compile. Let me elaborate.

The high-level reason that this code is being rejected is because the outer closure might be called more than once -- in the original code, it most certainly is -- and each call borrows data from the environment and returns it. Those references could well overlap with one another -- and in the minified example, they certainly would, since the data being re-borrowed is just the same value foo.

Let me give a more detailed look in terms of the minified example. First, let me define a few terms:

(|| let bar = &mut *foo; move |b| *bar = b) ;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ enclosing statement
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    outer closure
                         ^~~~~~~~~~~~~~~~~    inner closure

Intuitively, the inner closure (the one being returned), by capturing bar, requires a reference that lives as long as the enclosing statement. It must do so, because this closure is returned (and dropped) from there.

However, the outer closure does not move foo, it only borrows it. So this is an FnMut closure which can be called more than once. Therefore, it cannot lend out any references to foo that outlive its own closure body. So this code should not compile.

In terms of the closure desugaring, the outer closure contains no moves, and hence is an FnMut closure, so &mut *foo desugars to something like &mut *(*self).foo. The lifetime of the resulting borrow must therefore be less than the lifetimes of all the &mut borrows we must traverse along the way, which includes both self and foo here. And the lifetime of self is some anonymous lifetime not related to foo.

Therefore, to make this particular code snippet compile, we really need an FnOnce closure. We can get this by removing &mut *, which will cause the let bar = foo to be a move of foo and not a
reborrow: http://is.gd/gWSJZ3. We can also do this by providing a "hint" to the inferencer http://is.gd/7MaUbv in the form of a function that requires an FnOnce closures. (Note that it is NOT
enough to make this a move closure, since then it is still an FnMut closure, just one that takes ownership of foo rather than borrowing foo).

This is a bit of a corner case in the inference, actually. It is impossible to fix in general, but this particular example could be made to work, given a shift in strategy. Currently, we select whether a closure should be Fn, FnMut, or FnOnce based on two factors:

1. When creating the closure, does the immediate context give us a hint of what it must be? If so, just use that. This is what happens here http://is.gd/7MaUbv.
2. If not (which is the normal case), then look at what the closure does. We can only do this towards the very end of type-inference:
   • Does it move any upvars? If so, must be FnOnce.
   • Does it mutate any upvars? If so, must be FnMut.
   • Else, can be Fn.

But in this case, the strategy of #2 is insufficient: this is because while it does not move any upvars, it does loan an upvar out for a duration that is longer than the closure call itself. This also requires an FnOnce. But we don't know how long the loans will last at the time when we are deciding what kind of closure we need (and we can't know that) -- region inference has not run, and to run region inference, we have to know what kind of closures we have!

Still, it's plausible we could determine that the closure is only ever called once (or, in this case, not ever), and use THAT information to select FnOnce. I'm not sure what this would affect beyond this minified example, but I guess I can think of some cases. It certainly wouldn't affect the original code snippet.

[arielb1]

@nikomatsakis

The move-vs-reborrow case is a pretty annoying issue with the borrow checker in general (see @Gankro's struggles against it when implementing linked list iterators). I would prefer a more general solution.

[nikomatsakis]

@arielb1

The move-vs-reborrow case is a pretty annoying issue with the borrow checker in general (see @Gankro's struggles against it when implementing linked list iterators). I would prefer a more general solution.

I don't disagree, but the compiler is enforcing the current type system here correctly, afaict.

[pnkfelix]

current hypothesis is that this represents a bug fix.

Nonetheless, triaging as P-medium to investigate and confirm that hypothesis.

triage: P-medium

[nikomatsakis]

I can confirm that the minimized example successfully built before #30341 landed. Now attempting to confirm that it did NOT build afterwards.

[nikomatsakis]

OK, confirmed that it did not build afterwards. So this is a bug fix due to #30341. On that basis, I'm inclined to close the issue. Feel free to let me know if you disagree with that assessment!

UPDATE: correct from did build to did not build. :)

[mitchmindtree]

Sorry @nikomatsakis , did you mean to say that it did not build afterwards?

[nikomatsakis]

@mitchmindtree yes, corrected, thanks.

[mitchmindtree]

Ok great! Thanks a lot for the thorough explanation on the issue, it's given me enough understanding so that I should be able to easily adjust the original example.