diagnostics: "one type is more general" for two identical types?

[matthiaskrgr]

While looking through some clippy lint warnings in rust-lang/rust-clippy#5939 I found this very confusing error message:

use std::io::{self};
use std::path::{Path, PathBuf};

pub fn main() {
    let path = &PathBuf::from("a");
    do_op(path, "remove file", std::fs::remove_file); // redundant closure
}

fn do_op<F>(path: &Path, _desc: &str, mut f: F)
where
    F: FnMut(&Path) -> io::Result<()>,
{
    match f(path) {
        Ok(()) => {}
        Err(ref _e) => {}
        Err(_e) => {}
    }
}

error[E0308]: mismatched types
 --> src/main.rs:6:5
  |
6 |     do_op(path, "remove file", std::fs::remove_file); // redundant closure
  |     ^^^^^ one type is more general than the other
  |
  = note: expected type `std::ops::FnOnce<(&std::path::Path,)>`
             found type `std::ops::FnOnce<(&std::path::Path,)>`

note that expected type and found type are identical which is not helpful 😅

This happens on

rustc 1.47.0-nightly (e15510ca3 2020-08-20)
binary: rustc
commit-hash: e15510ca33ea15c893b78710101c962b11459963
commit-date: 2020-08-20
host: x86_64-unknown-linux-gnu
release: 1.47.0-nightly
LLVM version: 10.0

and also on 1.46.0-beta.4.

On stable 1.45.2, the message looks different though:

error[E0631]: type mismatch in function arguments
  --> src/main.rs:6:32
   |
6  |     do_op(path, "remove file", std::fs::remove_file); // redundant closure
   |                                ^^^^^^^^^^^^^^^^^^^^
   |                                |
   |                                expected signature of `for<'r> fn(&'r std::path::Path) -> _`
   |                                found signature of `fn(_) -> _`
...
9  | fn do_op<F>(path: &Path, _desc: &str, mut f: F)
   |    ----- required by a bound in this
10 | where
11 |     F: FnMut(&Path) -> io::Result<()>,
   |        ------------------------------ required by this bound in `do_op`

error[E0271]: type mismatch resolving `for<'r> <fn(_) -> std::result::Result<(), std::io::Error> {std::fs::remove_file::<_>} as std::ops::FnOnce<(&'r std::path::Path,)>>::Output == std::result::Result<(), std::io::Error>`
  --> src/main.rs:6:5
   |
6  |     do_op(path, "remove file", std::fs::remove_file); // redundant closure
   |     ^^^^^ expected bound lifetime parameter, found concrete lifetime
...
9  | fn do_op<F>(path: &Path, _desc: &str, mut f: F)
   |    ----- required by a bound in this
10 | where
11 |     F: FnMut(&Path) -> io::Result<()>,
   |                        -------------- required by this bound in `do_op`

[matthiaskrgr]

Just for the record, the working code was
do_op(path, "remove file", |f| std::fs::remove_file(f));

[estebank]

I'm on the street right now. If someone can identify the nightly the output changed I can track the pr and hopefully see how we can fox it back tomorrow.

[Dylan-DPC-zz]

Marking this as P-high based on the wg-prioritization discussion

[matthiaskrgr]

I tried to get cargo-bisect-rustc to work and it seems the regressing merge is 1557fb0
(I hope I used the tool correctly... 😅 )

[nikomatsakis]

This is almost certainly due to #72493. The error seems legit, as discussed here. The error message is quite misleading, what's really happening is that the type of std::fs::remove_file doesn't implement for<'a> FnMut(&'a Path) but just FnMut(&'b Path) for some 'b. We have some specializd diagnostic paths to try and express this more clearly but they are not triggering here for reasons I'm not entirely sure of.

[pnkfelix]

discussed in T-compiler meeting. Assigning to @estebank to see if they can make progress on it over next week. Also nominating to ensure we circle back around to it next week if progress isn't made

[spastorino]

Is this one a stable regression now?, also is it the same as #74400 ?

[lqd]

yes that's on stable now

[lqd]

and to answer the question: both are indeed duplicates of each other, they are higher-ranked subtype errors involving placeholder regions from the same universe. NLL's nice region error reporting code doesn't kick in because it handles TraitRefs while these are PolyTraitRefs.

[estebank]

I can get the following output:

error: implementation of `std::ops::FnOnce` is not general enough
   --> fil3.rs:6:5
    |
6   |       do_op(path, "remove file", std::fs::remove_file); // redundant closure
    |       ^^^^^ doesn't satisfy where-clause
    |
   ::: /Users/ekuber/workspace/rust/library/core/src/ops/function.rs:219:1
    |
219 | / pub trait FnOnce<Args> {
220 | |     /// The returned type after the call operator is used.
221 | |     #[lang = "fn_once_output"]
222 | |     #[stable(feature = "fn_once_output", since = "1.12.0")]
...   |
227 | |     extern "rust-call" fn call_once(self, args: Args) -> Self::Output;
228 | | }
    | |_- trait `std::ops::FnOnce` defined here
    |
note: due to this where-clause on `do_op`...
   --> fil3.rs:11:24
    |
11  |     F: FnMut(&Path) -> io::Result<()>,
    |                        ^^^^^^^^^^^^^^
    = note: ...`std::ops::FnOnce<(&'1 std::path::Path,)>` would have to be implemented for the type `fn(&std::path::Path) -> std::result::Result<(), std::io::Error> {std::fs::remove_file::<&std::path::Path>}`, for any lifetime `'1`...
    = note: ...but `std::ops::FnOnce<(&'2 std::path::Path,)>` is actually implemented for the type `fn(&'2 std::path::Path) -> std::result::Result<(), std::io::Error> {std::fs::remove_file::<&'2 std::path::Path>}`, for some specific lifetime `'2`

But I feel it is not yet ideal.

[lqd]

Is there enough information at this point of type-checking to output either previous errors (or both ? the E0631 seems more user-friendly, but these spans look more like the previous E0271), so that even if the message doesn't improve it at least doesn't regress anymore ?

Maybe having the previous signatures would be clearer ?

expected signature of `for<'r> fn(&'r std::path::Path) -> _`
   |                                found signature of `fn(_) -> _`

I wonder how we should explain these kinds of errors, maybe linking to closures/Fn* documentation, suggesting wrapping in a closure (but that doesn't always work). Maybe those suggestions are also too tailored to these 2 specific issues but higher-ranked subtyping errors could be arbitrarily complicated.

[spastorino]

Adding T-compiler label so this can be picked up by our automated prioritization tool and added to the following T-compiler weekly meeting agenda.

[spastorino]

As noted this is a stable regression already, adjusting labels accordingly.

[spastorino]

This was discussed on today's compiler weekly meeting

[solson]

I hope this is the right place for this — it seems like all relevant issues are getting closed as duplicates of this one. For reference, here is more minimal reproduction of the problem, from @khyperia:

fn thing(x: impl FnOnce(&u32)) {}

fn main() {
    let f = |_| ();
    thing(f);
}

Playground link

It can be worked around like this:

fn thing(x: impl FnOnce(&u32)) {}

fn main() {
    let f = |_: &_| ();
    thing(f);
}

[estebank]

Current output:

error: implementation of `FnOnce` is not general enough
 --> src/main.rs:6:5
  |
6 |     do_op(path, "remove file", std::fs::remove_file); // redundant closure
  |     ^^^^^ implementation of `FnOnce` is not general enough
  |
  = note: `fn(&'2 Path) -> Result<(), std::io::Error> {remove_file::<&'2 Path>}` must implement `FnOnce<(&'1 Path,)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 Path,)>`, for some specific lifetime `'2`

error: implementation of `FnOnce` is not general enough
 --> src/main.rs:5:5
  |
5 |     thing(f);
  |     ^^^^^ implementation of `FnOnce` is not general enough
  |
  = note: closure with signature `fn(&'2 u32)` must implement `FnOnce<(&'1 u32,)>`, for any lifetime `'1`...
  = note: ...but it actually implements `FnOnce<(&'2 u32,)>`, for some specific lifetime `'2`

I think this might be a duplicate of #71723

[jackh726]

Closing as a duplicate of #71723