Closures should always implement the all applicable Fn* traits.

Opened by Steven Allen at 2019-04-13 19:32:57

If a closure is passed into a function call as temporary, it only implements the Fn* traits needed to satisfy the constraints specified by the function. However, if it is declared first and then moved into the function, it correctly implements all applicable Fn* traits.

fn strip<F: FnOnce()>(f: F) -> F { f }
fn fail<F: Fn()>(f: F) { f() }

// Works.
fn test_works() {
    let f = || {};
    let f = strip(f);
    fail(f);
}

// Doesn't
fn test_broken() {
    // f only implements FnOnce().
    let f = strip(|| {});
    fail(f);
}

fn main() {}

This bug has an observable effect with specialization too. Fn(X) is accepted as a more specific impl than FnMut(X), but it will not actually be used in practice. cc @aturon

Edited: I pasted the wrong code in my first attempt.

// This prints "default apply"
// rustc 1.9.0-nightly (c66d2380a 2016-03-15)
#![feature(specialization)]

pub fn apply<F>(f: F)
    where F: FnMut()
{
    <()>::apply(f);
}

trait Apply<F> {
    fn apply(f: F);
}

impl<F> Apply<F> for ()
    where F: FnMut()
{
    default fn apply(f: F) {
        println!("default apply")
    }
}

impl<F> Apply<F> for ()
    where F: Fn()
{
    fn apply(f: F) {
        println!("thread safe apply")
    }
}


fn main() {
    apply(|| {});
}

bluss at 2016-03-17 12:40:46

I don't know off-hand if this is a "mere compiler bug" or a deep language specification issue.

I'm going to throw it onto the compiler teams plate for the short term, trusting that the team can move it to lang if that's what makes sense.
Felix S Klock II at 2016-03-17 13:57:21
triage: I-nominated T-compiler
Felix S Klock II at 2016-03-17 13:57:41
So this was intentional, but not necessarily wise. It helped us to get closures up and going, and I left it in as a useful "shorcut" to the right answer for closure inference. It is also the only way to influence closure trait selection, should you want to do so (but that last part is bad; we should probably have some explicit syntax at some point).

So my feeling is we should try commenting the "expected type inference" out and see what breaks. That is probably fairly easy to do. Hopefully, things will "just work" -- but there may be corner cases that will start to fail, I'm not sure.
Niko Matsakis at 2016-03-17 20:23:51
triage: P-medium
Niko Matsakis at 2016-03-17 20:24:01
Could be P-low?
Brian Anderson at 2016-10-20 16:51:59
Triage: not aware of any changes here
Steve Klabnik at 2017-09-30 16:14:20
So my feeling is we should try commenting the "expected type inference" out and see what breaks. That is probably fairly easy to do.

This might make a good E-mentor or E-medium issue: as a first step, change the type inference and see what goes wrong and report back after that. I imagine src/librustc_typeck/check/closure.rs is the right place to start looking, but I haven't looked properly.
varkor at 2019-02-18 18:11:25

I've done a bit of experimenting with this, and it seems like just dropping the expected kind here works without issues for FnMut and Fn. It does mess up the ui tests though, as it changes the displayed error message.

FnOnce has some issues with closures like these where it incorrectly infers it to be a FnMut instead. Error in full:

error: captured variable cannot escape `FnMut` closure body            ] 21/36: core                                                                                    
  --> src/libcore/fmt/builders.rs:17:13
   |
12 |         Self::wrap_buf_test(fmt, move |buf| {
   |                                           - inferred to be a `FnMut` closure
...
17 |             slot.as_mut().unwrap()
   |             ^^^^^^^^^^^^^^^^^^^^^^ returns a reference to a captured variable which escapes the closure body
   |
   = note: `FnMut` closures only have access to their captured variables while they are executing...
   = note: ...therefore, they cannot allow references to captured variables to escape

It seems like the closure kind inference does not account for mutable references being returned from the closure, so it ends up inferring a less strict kind than necessary.

Mike Pedersen at 2019-04-04 20:23:01

Basically the problem is these kind of situations:
```
fn fn_once<'a, T: 'a, F: FnOnce() -> &'a mut T>(_: F) { }

fn main() {
    let mut slot = Box::new(1);
    fn_once(|| &mut slot);
}
```
Here it would infer a FnMut closure as the closure only borrows values by mutable reference. Unfortunately this clashes with the borrow checker, as a FnMut closure cannot return mutable borrows of it's captured variables.

In order to infer FnMut vs. FnOnce we would have to know if the closure returns a reference into itself. I am not very experienced with the design of the compiler, but from what I gather, this information is not available until borrowck, which itself relies on type checking to be done first. This might be difficult to solve.

I have found no evidence to suggest that Fn vs. FnMut would be difficult, though. Unless, maybe, if we really want to keep the old error messages.
Mike Pedersen at 2019-04-08 19:07:06
if the closure returns a reference into itself

I'm pretty sure this is impossible without GATs, so maybe you don't need to worry about it (yet?).
Jake Goulding at 2019-04-13 17:09:21
Actually, It is because it is impossible that we have to worry about it. The problem is that examples like the above would be inferred as FnMut, due to not using any variables by value, but then causing an error in the borrow checker.

I can see how that line is misleading though. A more precise version:

In order to infer FnMut vs. FnOnce we would have to know if the closure borrows itself if it were to be inferred as FnMut.
Mike Pedersen at 2019-04-13 19:32:57