Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shouldn't lifetime elision apply to closure return type annotation? #56537

Closed
pnkfelix opened this issue Dec 5, 2018 · 10 comments
Closed

Shouldn't lifetime elision apply to closure return type annotation? #56537

pnkfelix opened this issue Dec 5, 2018 · 10 comments
Labels
A-closures Area: closures (`|args| { .. }`) A-lifetimes Area: lifetime related T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Dec 5, 2018

(Spawned off of #55526.)

Consider the following code (play):

fn foo(input: &str) -> &str {
    let id = |x: &str| -> &str { x };
    id(input)
}

The above does not compile today.

Error diagnostic:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
 --> src/main.rs:2:27
  |
2 |     let id = |x: &str| -> &str { x };
  |                           ^
  |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 2:14...
 --> src/main.rs:2:14
  |
2 |     let id = |x: &str| -> &str { x };
  |              ^^^^^^^^^^^^^^^^^^^^^^^
note: ...so that reference does not outlive borrowed content
 --> src/main.rs:2:34
  |
2 |     let id = |x: &str| -> &str { x };
  |                                  ^
note: but, the lifetime must be valid for the anonymous lifetime #1 defined on the function body at 1:1...
 --> src/main.rs:1:1
  |
1 | / fn foo(input: &str) -> &str {
2 | |     let id = |x: &str| -> &str { x };
3 | |     id(input)
4 | | }
  | |_^
note: ...so that reference does not outlive borrowed content
 --> src/main.rs:3:5
  |
3 |     id(input)
  |     ^^^^^^^^^

However, discussions with @nikomatsakis lead us to wonder why lifetime elision is not being applied to the type annotation attached to the closure. Since there is a single input argument type and a single return type, it seems reasonable to apply lifetime elision, yielding the type for <'r> (&'r str) -> &'r str.

I believe fixing this is actually pretty trivial, based on some initial experimentation with the code here:

// Everything else (only closures?) doesn't
// actually enjoy elision in return types.
_ => {
self.visit_ty(output);
return;
}

(The question will only be whether there are unintended consequences of the fix; I'm still looking into that.)

(This might require an RFC, not sure yet.)

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 5, 2018

Potentially related bugs (at least in terms of comparatively evaluating our specified guarantees and/or our currently implemented behavior):

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. A-lifetimes Area: lifetime related A-closures Area: closures (`|args| { .. }`) labels Dec 6, 2018
@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 6, 2018

Ugh. Here is some code, adapted from #23340, that my "fix" to this is going to "break":

fn print_first(list: Vec<String>) {
    let x: &str = list
        .first()
        .map(|s| -> &str { &s[..] })
        .unwrap_or("");
    println!("First element is {}", x);
}

That is, without the change described on this issue, the compiler accepts the above code.

But if I add the change so that lifetime elision is applied to the |s| -> &str, at least in a naive fashion, we get this error:

error[E0106]: missing lifetime specifier
 --> issue-22340-a.rs:6:21
  |
6 |         .map(|s| -> &str { &s[..] }) // works
  |                     ^ help: consider giving it an explicit bounded or 'static lifetime: `&'static`
  |
  = help: this function's return type contains a borrowed value with an elided lifetime, but the lifetime cannot be derived from the arguments

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 6, 2018

But if I add the change so that lifetime elision is applied to the |s| -> &str, at least in a naive fashion, we get this error:

the reason I wrote "at least in a naive fashion" is that the correct generalization of the lifetime elision rules to cover closure expressions is not trivially obvious. Why? Because those rules rely on definitions of lifetime positions in input/output, but the definitions given do not account for occurrences of the wildcard type _ (because that cannot occur in fn items nor impl headers).

And I might argue that it is the implicit occurrence of the wildcard type _ within |s| -> &str { ... } that is causing the case above to break when I added lifetime elision rules for |...| -> &T { ... }: because we do not (and cannot) syntactic identify any input lifetimes with the wildcard, it causes us to treat this as the case where there are no lifetime positions, and thus it is an error to elide the output lifetime on &str.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 6, 2018

But one way to generalize the rules in a manner that maintains backward compatibility might be to say: If there are any wildcard types _ on the input types of a closure, then we make no attempt to apply lifetime elision to the return type (and instead fallback on region inference).

This may yield surprising differences in behavior for end-users depending on which types annotations are supplied and which are not. But I think that is inevitable when you get into the weeds like this.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Dec 6, 2018

I left some thoughts on Zulip:

The TL;DR is that "annotations on closures are tricky". It feels like lifetime elision should apply (and I argued this before) -- but it's clear that can break code in light of how we handle the expected/provided split.

Right now, if there is an expected signature ES and a provided signature PS, we basically check that there exists some substitution Θ such that Θ[PS] = ES. This means that e.g. |x: &str, y: &str| would match an expected signature of for<'a> fn(&'a str, &'a str), even though the elision rules for a top-level fn would expand to for<'a, 'b> fn(&'a str, &'b str). Similar logic applies to elision.

I could imagine trying to "defer" the elision rules until we know if there is an expected type...but ugh.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 6, 2018

yet another option would be to sidestep trying to resolve this via elision, and instead focus attention on adding support for an expression form for <'r> CLOSURE so that we could do for <'r> |s: &'r _| -> &'r _ { ... }. (Which I think is issue #22340)

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 6, 2018

By the way...

I wrote:

But if I add the change so that lifetime elision is applied to the |s| -> &str, at least in a naive fashion, we get this error: ...

and @nikomatsakis subsequently referenced some comments on Zulip, which included this:

applying elision makes this break in some cases
e.g., |&str| -> &str

To make this quite concrete, I made the following example (play)

fn willy<'w>(p: &'w str) -> &'w str {
    let free_dumb = |x: &str| -> &str { p };
    let hello = format!("Hello");
    free_dumb(&hello)
}

fn main() {
    let world = format!("World");
    willy(&world);
}

In the above, the closure assigned to free_dumb has the signature |x: &str| -> &str, but it is relying on elision not applying. It doesn't want the type for <'r> fn(&'r str) -> &'r str; it wants the type for <'r> fn(&'r str) -> &'w str, where 'w is the lifetime on the input parameter.

And indeed, the compiler today accepts that code as written.

While my naive change to the compiler rejects it, with this error message from AST-borrowck:

error[E0312]: lifetime of reference outlives lifetime of borrowed content...
 --> free-willy.rs:2:41
  |
2 |     let free_dumb = |x: &str| -> &str { p };
  |                                         ^
  |
note: ...the reference is valid for the anonymous lifetime #1 defined on the body at 2:21...
 --> free-willy.rs:2:21
  |
2 |     let free_dumb = |x: &str| -> &str { p };
  |                     ^^^^^^^^^^^^^^^^^^^^^^^
note: ...but the borrowed content is only valid for the lifetime 'w as defined on the function body at 1:10
 --> free-willy.rs:1:10
  |
1 | fn willy<'w>(p: &'w str) -> &'w str {
  |          ^^

and this error message from NLL borrowck:

error: unsatisfied lifetime constraints
 --> free-willy.rs:2:41
  |
1 | fn willy<'w>(p: &'w str) -> &'w str {
  |          -- lifetime `'w` defined here
2 |     let free_dumb = |x: &str| -> &str { p };
  |                                         ^ returning this value requires that `'w` must outlive `'static`

error[E0597]: `hello` does not live long enough
 --> free-willy.rs:4:15
  |
2 |     let free_dumb = |x: &str| -> &str { p };
  |                                         - returning this value requires that `hello` is borrowed for `'static`
3 |     let hello = format!("Hello");
4 |     free_dumb(&hello)
  |               ^^^^^^ borrowed value does not live long enough
5 | }
  | - `hello` dropped here while still borrowed

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 7, 2018

if nothing else, I would like to encode the examples from the comments on this issue into tests that we put into our test suite. Because right now, if I add the naive change to apply lifetime elision rules to |s| -> &str (and |s: &str| -> &str, etc), our test suite passes, because it does not cover the tests that have been pointed out here.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 12, 2018

Okay, I have posted PR #56746 which encodes the examples from my comments here in a formal compile-pass test.

With that, I think I am convinced that we cannot immediately change our lifetime elision rules to apply to closure expressions.

@nikomatsakis has suggested that we might in the future add some other way to encode the signature of lambda expressons such that it would allow the user to indicate that lifetime elision should be applied.

But such an addition is not going to happen via the current |arg: ArgType| -> RetType { ... } syntax we have in place today, because it would not be backward compatible, as illustrated in the test in PR #56746.

Given that constraint (that we cannot just add this via the current syntax), I am going to close this issue.

pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 12, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 13, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
kennytm added a commit to kennytm/rust that referenced this issue Dec 13, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
kennytm added a commit to kennytm/rust that referenced this issue Dec 14, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 14, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 15, 2018
…osure-using-region-from-containing-fn, r=nikomatsakis

Add test of current behavior (infer free region within closure body)

This behavior was previously not encoded in our test suite.

it is pretty important that we test this behavior. In particular, in rust-lang#56537  I had proposed expanding the lifetime elision rules so that they would apply to some of the cases encoded in this test, which would cause them to start failing to compile successfully (because the lifetime attached to the return type would start being treated as connected to the lifetime on the input parameter to the lambda expression, which is explicitly *not* what the code wants in this particular case).

In other words, I am trying to ensure that anyone who tries such experiments with lifetime elision in the future quickly finds out why we don't support lifetime elision on lambda expressions (at least not in the naive manner described on rust-lang#56537).
@bstrie
Copy link
Contributor

bstrie commented Jul 20, 2021

I've filed #86921 to discuss the prospect of using a future edition to make this incompatible fix to the closure elision rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-lifetimes Area: lifetime related T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants