Suggest explicit lifetime parameter #12238

ktt3ja · 2014-02-13T08:55:07Z

For the following code snippet:

struct Foo { bar: int }
fn foo1(x: &Foo) -> &int {
    &x.bar
}

This PR generates the following error message:

test.rs:2:1: 4:2 note: consider using an explicit lifetime parameter as shown: fn foo1<'a>(x: &'a Foo) -> &'a int
test.rs:2 fn foo1(x: &Foo) -> &int {
test.rs:3     &x.bar
test.rs:4 }
test.rs:3:5: 3:11 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:3     &x.bar
              ^~~~~~

Currently it does not support methods.

nikomatsakis · 2014-02-13T18:38:13Z

Exciting! I'll take a look asap

bstrie · 2014-02-14T14:11:42Z

Awesome. I think this will be very helpful. We should start collecting error messages that could benefit from simple suggestions like this... suggesting ref in pattern match arms to avoid cannot move out of foo errors comes to mind.

ktt3ja · 2014-02-15T01:03:30Z

This semester has turned out to be rather busy, so I won't be able to work on this for a week. Feel free to close this PR to clear up bors's queue.

@bstrie: I plan to look into the "cannot move out of dereference of {@, &} pointer" error once I'm done with this one (since I hate that error with a passion).

@nikomatsakis: I'd like some feedback on an approach I plan to take. When both sub_r and sup_r have named lifetimes, we have to decide which should replace which. For the following function,

fn foo<'a, 'b>(x: &'a Foo) -> (&'b int, &'a int) {
    (&x.bar, &x.bar)
}

it would be wrong to replace &'a Foo with &'b Foo. Instead, we should replace &'b int with &'a int. A heuristic we could use is to always give sub_r (return value) the lifetime name of sup_r (argument), but it's unclear to me if that always work.

What I plan to do is, first, collect all the SubSupConflict where sub_r and sup_r are free and have the same sup_r (unless there's the case where a return value has to have the same lifetime as two parameters). From those, I would create a replace set, which tentatively consists of NodeId of places I would have to replace, and extract their lifetime names into a same_lifetime set. Then I would walk through the parameters and the output to check for places where the lifetime name is in same_lifetime, and if it is, I add that NodeId into replace. Finally, I walk through replace to start rebuilding the function declaration.

So for the following function,

fn foo<'a, 'b>(x: &'a Foo) -> (&'b int, &'a int, &int) {
    (&x.bar, &x.bar, &x.bar)
}

we would get SubSupConflict(&'b int, &'a Foo) and SubSupConflict(&int, &'a Foo). If we pretend that the NodeId of the parameter and the three items in the return tuple are 1, 2, 3, 4, respectively, our replace set would be {1, 2, 4} and same_lifetime set would be {'a, 'b}. Then when we walk through the inputs and output and see that the 'b in &'b int is in same_lifetime, so 3 should also be added into replace. Finally, we generate a name, say 'c, and give them all the same name, which would give us: (x: &'c Foo) -> (&'c int, &'c int, &'c int).

eddyb · 2014-02-15T17:09:18Z

src/libsyntax/ast_util.rs

@@ -231,6 +232,12 @@ pub fn ident_to_pat(id: NodeId, s: Span, i: Ident) -> @Pat {
                span: s }
 }

+pub fn ident_to_dummy_lifetime(ident: Ident) -> Lifetime {
+    Lifetime { id: 0,


Shouldn't that use DUMMY_NODE_ID instead?

Hmm, turns out DUMMY_NODE_ID is -1. I actually just copied that code snippet from a pprust test, and assumed that DUMMY_NODE_ID is 0. :P

nikomatsakis · 2014-02-19T01:54:08Z

So this pull request looks pretty good! This is very close to what I had in mind. I am pleased we were able to do it purely in error reporting. I suspect that it would also be good to investigate and improve the code that selects what inputs to provide to error reporting as well, but that's a separate question.

@ktt3ja as far as your question concerning a more advanced example, I don't necessarily think we need to solve that particular problem, but I'll not that it's not necessarily wrong to suggest replacing 'a with 'b or vice versa. It seems like the key thing would be remove the unused lifetime parameter from the list. Nonetheless, I'm less concerned about cases where the user gave two lifetime names and they didn't match -- much more concerned about anonymous lifetime parameters.

I think the best thing to do would be to cleanup this patch a bit (for example, to find a good lifetime name, perhaps search through 'a ... 'z to find one that is unused?) and land it as is. If you have more time and want to go further, I suspect we can improve the algorithms in region inference that select the two regions to report errors about.

ktt3ja · 2014-02-21T03:46:25Z

@nikomatsakis: Ah oops. Somehow I missed the notification for your comment until now. Since I had already started working on handling named lifetimes and multiple lifetimes, I think I'll try finishing that up first.

ktt3ja · 2014-02-22T08:39:02Z

@nikomatsakis: I just finished putting my plan from two comments back into implementation. It turned out to be a lot longer than I expected. It's also a bit ugly because of all the clones. :(

The way I handle named lifetimes is rather naive at the moment, so I chose to not support methods for now. At least it can suggest something like this, though:

struct Foo<'x, 'y, 'z> { bar: &'y int, baz: int }
fn foo(x: &Foo) -> (&int, &int, &int) {
    (x.bar, &x.baz, &x.baz)
}

test.rs:24:13: 24:19 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:24     (x.bar, &x.baz, &x.baz)
                       ^~~~~~
test.rs:24:21: 24:27 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:24     (x.bar, &x.baz, &x.baz)
                               ^~~~~~
test.rs:24:5: 24:28 error: mismatched types: expected `(&int,&int,&int)` but found `(&int,&int,&int)` (lifetime mismatch)
test.rs:24     (x.bar, &x.baz, &x.baz)
               ^~~~~~~~~~~~~~~~~~~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'a, 'b, 'c, 'd>(x: &'d Foo<'b, 'a, 'c>) -> (&'a int, &'d int, &'d int)
error: aborting due to 3 previous errors

and for the same function, but with some lifetimes already defined:

fn foo<'a, 'b, 'c>(x: &Foo<'a, 'b, 'c>) -> (&int, &int, &int) {
    (x.bar, &x.baz, &x.baz)
}

test.rs:28:13: 28:19 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:28     (x.bar, &x.baz, &x.baz)
                       ^~~~~~
test.rs:28:21: 28:27 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:28     (x.bar, &x.baz, &x.baz)
                               ^~~~~~
test.rs:28:5: 28:28 error: mismatched types: expected `(&int,&int,&int)` but found `(&'b int,&int,&int)` (lifetime mismatch)
test.rs:28     (x.bar, &x.baz, &x.baz)
               ^~~~~~~~~~~~~~~~~~~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'d, 'e, 'a, 'c>(x: &'e Foo<'a, 'd, 'c>) -> (&'d int, &'e int, &'e int)

I don't know how to add test for non-span note, but along with the above here are the ones I have tried so far (with simpler Foo struct):

struct Foo<'x> { bar: int }
fn foo(x: &Foo) -> &int {
    &x.bar
}

test.rs:3:5: 3:11 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:3     &x.bar
              ^~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'a>(x: &'a Foo) -> &'a int
error: aborting due to previous error

fn foo<'a, 'b>(x: &'a Foo) -> &'b int {
    &x.bar
}

test.rs:7:5: 7:11 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:7     &x.bar
              ^~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'c>(x: &'c Foo) -> &'c int
error: aborting due to previous error

fn foo(x: &Foo) -> (&int, &int) {
    (&x.bar, &x.bar)
}

test.rs:11:6: 11:12 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:11     (&x.bar, &x.bar)
                ^~~~~~
test.rs:11:14: 11:20 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:11     (&x.bar, &x.bar)
                        ^~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'a>(x: &'a Foo) -> (&'a int, &'a int)
error: aborting due to 2 previous errors

fn foo<'a, 'b>(x: &'a Foo) -> (&'b int, &'a int, &int) {
    (&x.bar, &x.bar, &x.bar)
}

test.rs:15:6: 15:12 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:15     (&x.bar, &x.bar, &x.bar)
                ^~~~~~
test.rs:15:22: 15:28 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:15     (&x.bar, &x.bar, &x.bar)
                                ^~~~~~
note: consider using an explicit lifetime parameter as shown:
fn foo<'c>(x: &'c Foo) -> (&'c int, &'c int, &'c int)
error: aborting due to 2 previous errors

fn foo(x: &int) -> &int {
    x
}

test.rs:19:5: 19:6 error: cannot infer an appropriate lifetime due to conflicting requirements
test.rs:19     x
               ^
test.rs:19:5: 19:6 error: mismatched types: expected `&int` but found `&int` (lifetime mismatch)
test.rs:19     x
               ^
note: consider using an explicit lifetime parameter as shown:
fn foo<'a>(x: &'a int) -> &'a int
error: aborting due to 2 previous errors

It seems to work fine on these functions. I'm still worried I may have made some wrong assumption along the way, though, since getting this to work was a little painful.

ktt3ja · 2014-02-22T08:48:28Z

src/librustc/middle/typeck/infer/error_reporting.rs

@@ -662,3 +1197,61 @@ impl Resolvable for @ty::TraitRef {
    }
 }

+struct LifeGiver {


I was very tempted to name this ForestSpirit since I was watching Princess Mononoke today. :p

nikomatsakis · 2014-02-24T10:58:33Z

The examples look nice. I think we may want to tweak the actual output, and I think we definitely want to give the output a span (ideally the function declaration). Probably we want the suggested fix to come first, before the details. I haven't read over the details yet, will try to do so today!

ktt3ja · 2014-02-25T22:49:59Z

@nikomatsakis: When you said it should be a span note, do you mean it should look like this:

test.rs:2:25: 4:2 note: consider using an explicit lifetime parameter as shown: fn foo<'a>(x: &'a Foo) -> &'a int
test.rs:2 fn foo(x: &Foo) -> &int {

or

test.rs:2:25: 4:2 note: consider using an explicit lifetime parameter as shown:
test.rs:2 fn foo<'a>(x: &'a Foo) -> &'a int

If it's the latter, I think I would have to do a custom fake_span_note since codemap only has the real AST, not the rebuilt one.

nikomatsakis · 2014-02-26T15:51:50Z

@ktt3ja I think I meant the former.

ktt3ja · 2014-02-27T01:21:44Z

@nikomatsakis: I changed the note into a span note and put it at the beginning. Unfortunately, the function declaration doesn't have its own span, so the note uses the span of the whole function.

test.rs:2:1: 4:2 note: consider using an explicit lifetime parameter as shown: fn foo1<'a>(x: &'a Foo) -> &'a int
test.rs:2 fn foo1(x: &Foo) -> &int {
test.rs:3     &x.bar
test.rs:4 }
test.rs:3:5: 3:11 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements
test.rs:3     &x.bar
              ^~~~~~

I also added some tests. The tests are fragile since changing the way lifetime names are generated will break them, but having some tests is still better than having none.

nikomatsakis · 2014-02-28T21:00:12Z

@ktt3ja sorry it took a while for me to read this. This is some excellent stuff. I made a bunch of suggestions, mostly pretty minor though, requests for comments and so forth. How much work do you think it is to handle methods? If it seems like a lot, let's open a follow up bug and land without that support.

The one thing I would like to do is to add a -Z flag that disables these suggestions (or perhaps not a -Z flag but just a normal flag?), and then get more aggressive by only reporting the suggestion and some line number that would be fixed. I want to get the message as simple as possible.

That said, maybe leave the code commented out or in some way disabled that iterates over and reports more details. I imagine we'll want to tweak this as we go.

ktt3ja · 2014-03-01T00:22:25Z

@nikomatsakis: I pushed a new version that addresses most of your comments. I'm unsure how hard it would be to support methods--I would need to run it on some test cases to get a better understanding, and unfortunately I can't work on it right now. I would prefer that this lands for now so that we get bug report for things I may have overlooked.

For your last two paragraphs, do you mean that we don't report the type/inference errors at all, and change the suggestion span note into an error? Do you want it to go with this PR?

huonw · 2014-03-01T06:54:58Z

(FWIW, putting the suggested updates inline in an note: ... message makes those lines really long; but... this is a fundamental problem with our diagnostics, e.g. #3533 (and #10492, kinda), and so doesn't have to be addressed here.)

nikomatsakis · 2014-03-01T12:05:15Z

@ktt3ja I'm thinking now let's just land the PR with the error messages as they are (which seems like a strict improvement over what we have) and we can tweak as we go. I'll review one more time.

nikomatsakis · 2014-03-10T10:26:20Z

r+ but for dead methods

ktt3ja · 2014-03-10T19:09:56Z

@nikomatsakis: dead methods have been removed

alexcrichton · 2014-03-10T19:50:59Z

You may be interested in the travis failure.

nikomatsakis · 2014-03-10T21:37:28Z

@ktt3ja looks like you have to replace uses of ~[T] with the new Vec<T>

ktt3ja · 2014-03-11T05:01:11Z

I have replaced all the ~[T] with Vec<T>. Since I use the shift method that isn't available in vec_ng, I copied the one from vec over.

nikomatsakis · 2014-03-11T13:55:25Z

OK. It seems like we'll want to have all the mutation methods available on vec (and only vec, I would think).

nikomatsakis · 2014-03-12T16:55:10Z

@ktt3ja I think this needs a rebase

Some types of error are caused by missing lifetime parameter on function or method declaration. In such cases, this commit generates some suggestion about what the function declaration could be. This does not support method declaration yet.

ktt3ja · 2014-03-12T22:51:10Z

@nikomatsakis: rebase done

For the following code snippet: ```rust struct Foo { bar: int } fn foo1(x: &Foo) -> &int { &x.bar } ``` This PR generates the following error message: ```rust test.rs:2:1: 4:2 note: consider using an explicit lifetime parameter as shown: fn foo1<'a>(x: &'a Foo) -> &'a int test.rs:2 fn foo1(x: &Foo) -> &int { test.rs:3 &x.bar test.rs:4 } test.rs:3:5: 3:11 error: cannot infer an appropriate lifetime for borrow expression due to conflicting requirements test.rs:3 &x.bar ^~~~~~ ``` Currently it does not support methods.

feat: Change VSCode extension publisher to `rust-lang`

eddyb reviewed Feb 15, 2014
View reviewed changes

ktt3ja reviewed Feb 22, 2014
View reviewed changes

alexcrichton mentioned this pull request Mar 10, 2014

Lifetime error messages could be improved. #9716

Closed

ktt3ja added 2 commits March 12, 2014 15:31

Add shift and remove methods for Vec

ed2b3a2

bors merged commit 9faa2a5 into rust-lang:master Mar 13, 2014

ktt3ja deleted the lifetime-error-msg branch March 13, 2014 19:15

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022

Auto merge of rust-lang#12238 - Veykril:publisher, r=Veykril

da96963

feat: Change VSCode extension publisher to `rust-lang`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest explicit lifetime parameter #12238

Suggest explicit lifetime parameter #12238

ktt3ja commented Feb 13, 2014

nikomatsakis commented Feb 13, 2014

bstrie commented Feb 14, 2014

ktt3ja commented Feb 15, 2014

eddyb Feb 15, 2014

ktt3ja Feb 21, 2014

nikomatsakis commented Feb 19, 2014

ktt3ja commented Feb 21, 2014

ktt3ja commented Feb 22, 2014

ktt3ja Feb 22, 2014

nikomatsakis commented Feb 24, 2014

ktt3ja commented Feb 25, 2014

nikomatsakis commented Feb 26, 2014

ktt3ja commented Feb 27, 2014

nikomatsakis commented Feb 28, 2014

ktt3ja commented Mar 1, 2014

huonw commented Mar 1, 2014

nikomatsakis commented Mar 1, 2014

nikomatsakis commented Mar 10, 2014

ktt3ja commented Mar 10, 2014

alexcrichton commented Mar 10, 2014

nikomatsakis commented Mar 10, 2014

ktt3ja commented Mar 11, 2014

nikomatsakis commented Mar 11, 2014

nikomatsakis commented Mar 12, 2014

ktt3ja commented Mar 12, 2014

Suggest explicit lifetime parameter #12238

Suggest explicit lifetime parameter #12238

Conversation

ktt3ja commented Feb 13, 2014

nikomatsakis commented Feb 13, 2014

bstrie commented Feb 14, 2014

ktt3ja commented Feb 15, 2014

eddyb Feb 15, 2014

Choose a reason for hiding this comment

ktt3ja Feb 21, 2014

Choose a reason for hiding this comment

nikomatsakis commented Feb 19, 2014

ktt3ja commented Feb 21, 2014

ktt3ja commented Feb 22, 2014

ktt3ja Feb 22, 2014

Choose a reason for hiding this comment

nikomatsakis commented Feb 24, 2014

ktt3ja commented Feb 25, 2014

nikomatsakis commented Feb 26, 2014

ktt3ja commented Feb 27, 2014

nikomatsakis commented Feb 28, 2014

ktt3ja commented Mar 1, 2014

huonw commented Mar 1, 2014

nikomatsakis commented Mar 1, 2014

nikomatsakis commented Mar 10, 2014

ktt3ja commented Mar 10, 2014

alexcrichton commented Mar 10, 2014

nikomatsakis commented Mar 10, 2014

ktt3ja commented Mar 11, 2014

nikomatsakis commented Mar 11, 2014

nikomatsakis commented Mar 12, 2014

ktt3ja commented Mar 12, 2014