A simple fix for a common case of lifetime miselission

[matklad]

Hi! A while ago a proposed to create a lint for a case where lifetime elision might be wrong, but the idea was rejected. Today the issue crossed my mind again, and I still think that the lint is worthwhile, so I’d like to discuss it a bit more

So, consider this Rust code:

#[derive(Copy, Clone)]
struct Holder<'a> {
    data: &'a String
}

impl<'a> Holder<'a> {
    fn data(&self) -> &String {
        self.data
    }
}

It compiles cleanly, without any errors or warnings. If you try to use it, you may even think that it works correctly. The following code compiles fine, after all:

fn f1(h: Holder) -> bool {
    h.data().contains("Hello")
}

However, problems come if you try to use two holders simultaneously:

fn f2(h1: Holder, h2: Holder) -> & str {
    if true {
        &h1.data()
    } else {
        &h2.data()
    }
}

error[E0106]: missing lifetime specifier
  --> src/main.rs:17:34
   |
17 | fn f2(h1: Holder, h2: Holder) -> & str {
   |                                  ^ expected lifetime parameter
   |
   = help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from `h1` or `h2`

You may try to add a single lifetime to both holders and the return type:

fn f2<'h>(h1: Holder<'h>, h2: Holder<'h>) -> &'h str {
    if true {
        &h1.data()
    } else {
        &h2.data()
    }
}

And it gives you a bunch of lifetime errors:

error[E0597]: `h1` does not live long enough
  --> src/main.rs:19:10
   |
19 |         &h1.data()
   |          ^^ does not live long enough
...
23 | }
   | - borrowed value only lives until here
   |
note: borrowed value must be valid for the lifetime 'h as defined on the function body at 17:1...
  --> src/main.rs:17:1
   |
17 | / fn f2<'h>(h1: Holder<'h>, h2: Holder<'h>) -> &'h str {
18 | |     if true {
19 | |         &h1.data()
20 | |     } else {
21 | |         &h2.data()
22 | |     }
23 | | }
   | |_^

error[E0597]: `h2` does not live long enough
  --> src/main.rs:21:10
   |
21 |         &h2.data()
   |          ^^ does not live long enough
22 |     }
23 | }
   | - borrowed value only lives until here
   |
note: borrowed value must be valid for the lifetime 'h as defined on the function body at 17:1...
  --> src/main.rs:17:1
   |
17 | / fn f2<'h>(h1: Holder<'h>, h2: Holder<'h>) -> &'h str {
18 | |     if true {
19 | |         &h1.data()
20 | |     } else {
21 | |         &h2.data()
22 | |     }
23 | | }
   | |_^

Crucially, all these error messages are wrong, because the actual problem is not inside f2, it’s the signature of get_data.

If we specify lifetimes manually, we’ll get

impl<'a> Holder<'a> {
    fn data<'b>(&'b self) -> &'b String
}

while the correct signature is

impl<'a> Holder<'a> {
    fn data<'b>(&'b self) -> &'a String
}

Here, lifetime elision is wrong, because it prefers lifetime of Self, even though it is smaller than the lifetime of data.

I think this is an important problem to fix because

1. It happens in practice pretty often (personal experience).
2. Error manifests itself some time after you’ve written the code: most simple functions work with smaller lifetime just fine.
3. The error message is completely misleading, it points to the wrong bit of code.
4. It’s not me who made the error, it’s the compiler who chose the wrong lifetime.

And I think this can be easily fixed with a lint, without any changes to the actual elision rules:

If the lifetime in the return position is elided and if the actual (inferred) lifetime is strictly greater than the elided one, produce a warning saying:

Potentially wrong lifetime elision: the signature without elision is

fn data<'b>(&'b self) -> &'b String

but the signature 

fn data<'b>(&'b self) -> &'a String

also works and more general. Provide explicit lifetimes to silence this warning.

The reported issue is https://github.com/rust-lang/rust/issues/42287.

Do you think that this problem is important to fix? What are other possible solutions? What are the drawbacks of the lint?

[matklad]

cc @Mark_Simulacrum

[leodasvacas]

@matklad

A while ago a proposed to create a lint for a case where lifetime elision might be wrong, but the idea was rejected.

Rejected? Where? Why?

[matklad]

https://github.com/rust-lang/rust/issues/42287

However now that I think more about it, I start to understand that it might not be so easy to detect if the return lifetime could be larger than the one elided: borrowchecker uses return types during inference, so it will infer exactly specified lifetime.

[nikomatsakis]

I’m not sure yet what I think, but I will say that I think that the current lifetime elision rules need some tinkering in general, and I suppose this might be another case of that (though it’s rather different from the concerns that e.g. the in-band lifetimes RFC addressed).

The case that I was most concerned with, which the in-band lifetimes RFC addressed, is when the lifetimes in the return value are “hidden” (e.g., -> Holder). The in-band lifetimes RFC addresses this by encouraging you to write -> Holder<'_>, making the lifetime evident.

The way you proposed the lint, it was based on inference results, which is fine and maybe a good idea (though I’d have to think about how best to implement; seems like it could be done though). However, another way to think of it though is more mechanical – for example, it has been proposed in the past that we should disable return type elision in the case where the self type contains multiple lifetime parameters (e.g., this applies to your example, the self type is &'a Holder<'b>).

Given that the in-band lifetimes RFC makes it more light-weight to add annotations, this might be tolerable. In other words, in the world I am anticipating, your code would more idiomatically look like:

impl Holder<'data> {
//          ^^^^^ the proposed pattern is to discourage single-letter
//          names outside of individual functions. Hence you would
//          use the more meaningful name here, `'data`.

    fn data(&self) -> &'data String { self.holder }
}

The idea would be that if you wrote &String (i.e., made use of elision) we would warn you here. If, in fact, you wanted to tie it to self and not the lifetime parameter, then you would have to do fn data(&'a self) -> &'a String (single-letter names are fine within a single function).

Another thing to consider is around traits, where the lifetime may not be yours to choose:

trait GetData {
    fn data(&self) -> &String; // fine
}

impl GetData for Holder<'_> {
    fn data(&self) -> &String { // warn?
    }
}

It might make sense to warn here too, just to help make it clear to the user that – because of the trait – which lifetime they are getting, but it might be annoying.

cc @aturon