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

Lifetime elision works incorrectly with explicit self types in methods #21400

Closed
netvl opened this issue Jan 19, 2015 · 5 comments
Closed

Lifetime elision works incorrectly with explicit self types in methods #21400

netvl opened this issue Jan 19, 2015 · 5 comments
Assignees
Labels
A-typesystem Area: The type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority
Milestone

Comments

@netvl
Copy link
Contributor

netvl commented Jan 19, 2015

Found in this SO question.

This code compiles:

struct Test;

impl Test {
    fn method1(&mut self, arg: &str) -> Result<usize, &str> {
        unreachable!()
    }

    fn method2(self: &mut Test, arg: &str) -> Result<usize, &str> {
        unreachable!()
    }

    fn test(self: &mut Test) -> Result<usize, &str> {
        let s = format!("abcde");
        let data = match self.method1(&*s) {
            Ok(r) => r,
            Err(e) => return Err(e)
        };
        unreachable!()
    }
}

fn main() {
}

But if you change self.method1(&*s) to self.method2(&*s) it will stop compiling:

test7.rs:14:41: 14:42 error: `s` does not live long enough
test7.rs:14         let data = match self.method2(&*s) {
                                                    ^
test7.rs:12:53: 19:6 note: reference must be valid for the anonymous lifetime #1 defined on the block at 12:52...
test7.rs:12     fn test(self: &mut Test) -> Result<usize, &str> {
test7.rs:13         let s = format!("abcde");
test7.rs:14         let data = match self.method2(&*s) {
test7.rs:15             Ok(r) => r,
test7.rs:16             Err(e) => return Err(e)
test7.rs:17         };
            ...
test7.rs:12:53: 19:6 note: ...but borrowed value is only valid for the block at 12:52
test7.rs:12     fn test(self: &mut Test) -> Result<usize, &str> {
test7.rs:13         let s = format!("abcde");
test7.rs:14         let data = match self.method2(&*s) {
test7.rs:15             Ok(r) => r,
test7.rs:16             Err(e) => return Err(e)
test7.rs:17         };
            ...

According to the lifetime elision rules this signature:

fn method2(self: &mut Test, arg: &str) -> Result<usize, &str>

should be equivalent to this one:

fn method2<'a, 'b>(self: &'a mut Test, arg: &'b str) -> Result<usize, &'a str>

However, it seems that currently it is equivalent to this one:

fn method2<'a>(self: &'a mut Test, arg: &'a str) -> Result<usize, &'a str>

which gives exactly the same error as the elided version.

Another funny thing is that if I remove match and write just

let data = self.method2(&*s);

It compiles again.

@kmcallister kmcallister added A-typesystem Area: The type system I-wrong labels Jan 19, 2015
@pnkfelix
Copy link
Member

I don't know if we interpret the lifetime elision rules the same way; I would have guessed that impl Test { fn m(self: &mut Test, ...) ... } is equivalent to impl Test { fn m(&mut self, ...) ... }, and when viewed that way, I'd say that explains the behavior you are seeing.

Nonetheless, nominating.

@pnkfelix
Copy link
Member

(my above explanation/interpretation was bogus)

@pnkfelix
Copy link
Member

1.0 polish, P-high.

@pnkfelix pnkfelix added this to the 1.0 milestone Feb 19, 2015
@pnkfelix pnkfelix added P-medium Medium priority and removed I-nominated labels Feb 19, 2015
@pnkfelix pnkfelix self-assigned this Apr 2, 2015
@pnkfelix
Copy link
Member

Hmm this no longer seems to reproduce.

@pnkfelix
Copy link
Member

seems like the remaining work here is to add a regression test.

@pnkfelix pnkfelix added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Apr 10, 2015
bors added a commit that referenced this issue Jan 28, 2017
Perform lifetime elision (more) syntactically, before type-checking.

The *initial* goal of this patch was to remove the (contextual) `&RegionScope` argument passed around `rustc_typeck::astconv` and allow converting arbitrary (syntactic) `hir::Ty` to (semantic) `Ty`.
I've tried to closely match the existing behavior while moving the logic to the earlier `resolve_lifetime` pass, and [the crater report](https://gist.github.com/eddyb/4ac5b8516f87c1bfa2de528ed2b7779a) suggests none of the changes broke real code, but I will try to list everything:

There are few cases in lifetime elision that could trip users up due to "hidden knowledge":
```rust
type StaticStr = &'static str; // hides 'static
trait WithLifetime<'a> {
    type Output; // can hide 'a
}

// This worked because the type of the first argument contains
// 'static, although StaticStr doesn't even have parameters.
fn foo(x: StaticStr) -> &str { x }

// This worked because the compiler resolved the argument type
// to <T as WithLifetime<'a>>::Output which has the hidden 'a.
fn bar<'a, T: WithLifetime<'a>>(_: T::Output) -> &str { "baz" }
```

In the two examples above, elision wasn't using lifetimes that were in the source, not even *needed* by paths in the source, but rather *happened* to be part of the semantic representation of the types.
To me, this suggests they should have never worked through elision (and they don't with this PR).

Next we have an actual rule with a strange result, that is, the return type here elides to `&'x str`:
```rust
impl<'a, 'b> Trait for Foo<'a, 'b> {
    fn method<'x, 'y>(self: &'x Foo<'a, 'b>, _: Bar<'y>) -> &str {
        &self.name
    }
}
```
All 3 of `'a`, `'b` and `'y` are being ignored, because the `&self` elision rule only cares that the first argument is "`self` by reference". Due implementation considerations (elision running before typeck), I've limited it in this PR to a reference to a primitive/`struct`/`enum`/`union`, but not other types, but I am doing another crater run to assess the impact of limiting it to literally `&self` and `self: &Self` (they're identical in HIR).

It's probably ideal to keep an "implicit `Self` for `self`" type around and *only* apply the rule to `&self` itself, but that would result in more bikeshed, and #21400 suggests some people expect otherwise.
Another decent option is treating `self: X, ... -> Y` like `X -> Y` (one unique lifetime in `X` used for `Y`).

The remaining changes have to do with "object lifetime defaults" (see RFCs [599](https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md) and [1156](https://github.com/rust-lang/rfcs/blob/master/text/1156-adjust-default-object-bounds.md)):
```rust
trait Trait {}
struct Ref2<'a, 'b, T: 'a+'b>(&'a T, &'b T);

// These apply specifically within a (fn) body,
// which allows type and lifetime inference:
fn main() {
    // Used to be &'a mut (Trait+'a) - where 'a is one
    // inference variable - &'a mut (Trait+'b) in this PR.
    let _: &mut Trait;

    // Used to be an ambiguity error, but in this PR it's
    // Ref2<'a, 'b, Trait+'c> (3 inference variables).
    let _: Ref2<Trait>;
}
```
What's happening here is that inference variables are created on the fly by typeck whenever a lifetime has no resolution attached to it - while it would be possible to alter the implementation to reuse inference variables based on decisions made early by `resolve_lifetime`, not doing that is more flexible and works better - it can compile all testcases from #38624 by not ending up with `&'static mut (Trait+'static)`.

The ambiguity specifically cannot be an early error, because this is only the "default" (typeck can still pick something better based on the definition of `Trait` and whether it has any lifetime bounds), and having an error at all doesn't help anyone, as we can perfectly infer an appropriate lifetime inside the `fn` body.

**TODO**: write tests for the user-visible changes.

cc @nikomatsakis @arielb1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. P-medium Medium priority
Projects
None yet
Development

No branches or pull requests

3 participants