Try to tailor error messages for common scenarios

[nikomatsakis]

This is a vague bug but here is my intention:

1. Look at common bugs (returning a closure, etc) and see if we can improve the error messages for those
2. Try to improve the heuristics that suggest how to report a region inference failure, which frequently give nonsense results

[nikomatsakis]

triage: P-high (1.0)

[nikomatsakis]

I've been reading through this thread http://internals.rust-lang.org/t/confused-by-lifetime-error-messages-tell-me-about-it/358/ and trying to extract some specific examples I'd like to target. I will add them as comments here for now.

[nikomatsakis]

Outliving temporary lifetimes, particularly with the newer rules:

use std::collections::HashMap;
use std::io::BufRead;

fn main() {
    let map = (|| {
        let mut map = HashMap::new();
        map.insert("imo", "in my opinion");
        map
    })();

    for line in std::io::stdin().lock().lines().map(|l| l.unwrap()) {
        println!("{}", line.split(' ')
                 .map(|seg| if map.contains_key(seg) {
                     map[seg.as_slice()].to_string()
                 } else {
                     seg.to_string()
                 }).collect::<Vec<String>>().connect(" "));
    }
}

compiled yields an error (see below). Some thoughts:

1. I think part of the issue that the for loop expansion is yielding particularly poor explanations of the lifetimes involved.
2. A temporary value is being returned by std::io::stdin()
3. This temporary value is not valid long enough, but the lifetime mismatch here is somewhat subtle.

We do make a helpful suggestion, but we could make it more concrete perhaps, by indicating what specifically should be moved into a let, and indicating that earlier.

(In general, my impression is that indicating the precise scopes seems to (usually) not be that helpful, particularly in cases like these.)

/home/nmatsakis/tmp/lifetime-test-1.rs:11:17: 11:33 error: borrowed value does not live long enough
/home/nmatsakis/tmp/lifetime-test-1.rs:11     for line in std::io::stdin().lock().lines().map(|l| l.unwrap()) {
                                                          ^~~~~~~~~~~~~~~~
/home/nmatsakis/tmp/lifetime-test-1.rs:11:5: 19:2 note: reference must be valid for the destruction scope surrounding statement at 11:4...
/home/nmatsakis/tmp/lifetime-test-1.rs:11     for line in std::io::stdin().lock().lines().map(|l| l.unwrap()) {
/home/nmatsakis/tmp/lifetime-test-1.rs:12         println!("{}", line.split(' ')
/home/nmatsakis/tmp/lifetime-test-1.rs:13                  .map(|seg| if map.contains_key(seg) {
/home/nmatsakis/tmp/lifetime-test-1.rs:14                      map[seg.as_slice()].to_string()
/home/nmatsakis/tmp/lifetime-test-1.rs:15                  } else {
/home/nmatsakis/tmp/lifetime-test-1.rs:16                      seg.to_string()
                                          ...
/home/nmatsakis/tmp/lifetime-test-1.rs:11:5: 19:2 note: ...but borrowed value is only valid for the statement at 11:4
/home/nmatsakis/tmp/lifetime-test-1.rs:11     for line in std::io::stdin().lock().lines().map(|l| l.unwrap()) {
/home/nmatsakis/tmp/lifetime-test-1.rs:12         println!("{}", line.split(' ')
/home/nmatsakis/tmp/lifetime-test-1.rs:13                  .map(|seg| if map.contains_key(seg) {
/home/nmatsakis/tmp/lifetime-test-1.rs:14                      map[seg.as_slice()].to_string()
/home/nmatsakis/tmp/lifetime-test-1.rs:15                  } else {
/home/nmatsakis/tmp/lifetime-test-1.rs:16                      seg.to_string()
                                          ...
/home/nmatsakis/tmp/lifetime-test-1.rs:11:5: 19:2 help: consider using a `let` binding to increase its lifetime
/home/nmatsakis/tmp/lifetime-test-1.rs:11     for line in std::io::stdin().lock().lines().map(|l| l.unwrap()) {
/home/nmatsakis/tmp/lifetime-test-1.rs:12         println!("{}", line.split(' ')
/home/nmatsakis/tmp/lifetime-test-1.rs:13                  .map(|seg| if map.contains_key(seg) {
/home/nmatsakis/tmp/lifetime-test-1.rs:14                      map[seg.as_slice()].to_string()
/home/nmatsakis/tmp/lifetime-test-1.rs:15                  } else {
/home/nmatsakis/tmp/lifetime-test-1.rs:16                      seg.to_string()
                                          ...

[nikomatsakis]

see #23581

[nikomatsakis]

Suggesting move from closures would help with this example from @wycats:

<anon>:7:9: 7:41 error: `book1` does not live long enough
<anon>:7   spawn(|| println!("{}", &book1.title));
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: reference must be valid for the static lifetime...
<anon>:4:91: 8:2 note: ...but borrowed value is only valid for the block suffix following statement 0 at 4:90
<anon>:4   let mut book1 = Book { title: "Hello".to_string(), chapters: vec!["world".to_string()] };
<anon>:5   let mut book2 = Book { title: "Hello".to_string(), chapters: vec!["world".to_string()] };
<anon>:6 
<anon>:7   spawn(|| println!("{}", &book1.title));
<anon>:8 }

[nikomatsakis]

More complete example:

use std::thread::spawn;

fn main() {
    let mut books = vec![1,2,3];
    spawn(|| books.push(4));
}

[nikomatsakis]

Similar to that case is this one http://is.gd/ExwsE4

[wycats]

@nikomatsakis I also don't think that this message is very useful:

note: reference must be valid for the static lifetime...
<anon>:4:91: 8:2 note: ...but borrowed value is only valid for the block suffix following statement 0 at 4:90

If you already know roughly how things work, the basic message says what you need to know, and if you don't, it's totally obtuse.

[nikomatsakis]

PR for the escaping closure case: #24242

[nikomatsakis]

From Carl: https://gist.github.com/carllerche/d058aff63c9f9ae7a8a8

Things that would have helped:

• Report errors for callee first
• Not reporting duplicate Sized error for Option<Foo> and Foo

[nikomatsakis]

Another example of a dropck-related error: http://is.gd/KtjuPX

I'm not sure how best to handle these errors. The scenario is complicated. Perhaps suggesting an edit would help, though it may itself (maybe) trigger other errors.

[carllerche]

Another error that frustrated me a lot. Sometimes when I try to send a value to another thread, I will get something like:

*mut u8 doesn't implement Send

The problem is that I don't know where in the original struct this field is because it could be deeply nested in sub structs and span a number of crates.

A better error would probably be to error on the deepest type in the current crate and indicate which field is not Send (and possibly the path from there).

Hope this makes sense.

[carllerche]

A new rust user hit the following when using mio. Instead of doing mio::Io::new(...) they did mio::Io.new(...), this failed with the error message unresolved name 'mio::Io' implying that the type could not be found / loaded. There is most likely a better error message for this case.

Code:

extern crate mio;

fn main() {
    let _ = mio::Io.new(123);
}

Output:

src/main.rs:4:13: 4:20 error: unresolved name `mio::Io`
src/main.rs:4     let _ = mio::Io.new(123);
                          ^~~~~~~

[nikomatsakis]

On Thu, Apr 09, 2015 at 11:45:57PM -0700, Carl Lerche wrote:

Another error that frustrated me a lot. Sometimes when I try to send a value to another thread, I will get something like:

*mut u8 doesn't implement Send

The problem is that I don't know where in the original struct this field is because it could be deeply nested in sub structs and span a number of crates.

It's interesting because we try to give backtraces in such cases. I'm actually thinking of modifying the compiler to give more such
backtraces whenever we are processing a trait obligation like x: Foo because of something indirect, which would probably fix this case too
(presumably this is a case where somehow we cut off the stacktrace for some reason, I know it appears in other cases).

[nikomatsakis]

From #25885:

This example: http://is.gd/oDK41V yields an amazingly bad error message:

<anon>:18:23: 22:6 error: `r` does not live long enough
<anon>:18     r.listen(Box::new(|| {
<anon>:19         x = Some(22);
<anon>:20         y = Some(44);
<anon>:21         r.something();
<anon>:22     }));
<anon>:15:22: 23:2 note: reference must be valid for the block suffix following statement 1 at 15:21...
<anon>:15     let mut y = None;
<anon>:16     let mut r = Reactor::new();
<anon>:17     
<anon>:18     r.listen(Box::new(|| {
<anon>:19         x = Some(22);
<anon>:20         y = Some(44);
          ...
<anon>:16:32: 23:2 note: ...but borrowed value is only valid for the block suffix following statement 2 at 16:31
<anon>:16     let mut r = Reactor::new();
<anon>:17     
<anon>:18     r.listen(Box::new(|| {
<anon>:19         x = Some(22);
<anon>:20         y = Some(44);
<anon>:21         r.something();
          ...
error: aborting due to previous error
playpen: application terminated with error code 101

the problem here is that the variable r is being used mutably by both listen and the closure, as you can see from this fixed version: http://is.gd/Dz63j7

[tshepang]

@carllerche I recently hit that one, and it's easy to miss that one gots to use :: instead of . (coming from Python)

[tshepang]

I see the problem has been reported: #22692

[brson]

Closing old vague bug cc @nikomatsakis

[steveklabnik]

@jonathandturner you might want to look at this thread for inspiration