Allow hidden lifetimes in impl Trait, take 2

[alexreg]

This is an attempt at reworking #57870, and operates with the principle that lifetimes (with respect to which the opaque type may be invariant) can only be hidden if they outlive the opaque type. This was discussed informally with @nikomatsakis before, and seems to have sound semantics, though it would be good to consider that again before merging. Certainly, although more conservative than the previous attempt (which can produce UB), it loosens the presently-standing rules whilst still preventing bad situations such as #57870 (comment).

r? @nikomatsakis

CC @cramertj @matthewjasper

[matthewjasper]

This is a breaking change:

trait X<'x>: Sized {}

impl<U> X<'_> for U {}

fn hide<'a, 'b, T: 'static>(x: &'a mut &'b T) -> impl X<'b> + 'a {
    x
}

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:2089f16b:start=1553461422841257927,finish=1553461423787956486,duration=946698559
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:28:40]    Compiling rustc_data_structures v0.0.0 (/checkout/src/librustc_data_structures)
[00:28:42] error: lifetime may not live long enough
[00:28:42]    --> src/librustc_data_structures/graph/implementation/mod.rs:240:9
[00:28:42]     |
[00:28:42] 236 |     pub fn successor_nodes<'a>(
[00:28:42]     |                            -- lifetime `'a` defined here
[00:28:42] ...
[00:28:42] 240 |         self.outgoing_edges(source).targets()
[00:28:42]     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
[00:28:42] help: to allow this impl Trait to capture borrowed data with lifetime `'a`, add `'a` as a constraint
[00:28:42]     |
[00:28:42] 239 |     ) -> impl Iterator<Item = NodeIndex> + 'a + 'a {
[00:28:42] 
[00:28:42] error: lifetime may not live long enough
[00:28:42]    --> src/librustc_data_structures/graph/implementation/mod.rs:247:9
[00:28:42]     |
[00:28:42]     |
[00:28:42] 243 |     pub fn predecessor_nodes<'a>(
[00:28:42]     |                              -- lifetime `'a` defined here
[00:28:42] ...
[00:28:42] 247 |         self.incoming_edges(target).sources()
[00:28:42]     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
[00:28:42] help: to allow this impl Trait to capture borrowed data with lifetime `'a`, add `'a` as a constraint
[00:28:42]     |
[00:28:42] 246 |     ) -> impl Iterator<Item = NodeIndex> + 'a + 'a {
[00:28:42] 
[00:28:43] error: aborting due to 2 previous errors
[00:28:43] 
[00:28:43] error: Could not compile `rustc_data_structures`.
---
travis_time:end:000583c2:start=1553463184039864065,finish=1553463184044554438,duration=4690373
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:2841a47e
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:17084f4c
travis_time:start:17084f4c
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1e4a55b3
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[matthewjasper]

And this is still not sound:

use std::cell::RefCell;
use std::rc::Rc;

trait Swap: Sized {
    fn swap(self, other: Self);
}

impl<T> Swap for Rc<RefCell<T>> {
    fn swap(self, other: Self) {
        <RefCell<T>>::swap(&self, &other);
    }
}

fn hide<'a, 'b: 'a, T: 'static>(x: Rc<RefCell<&'b T>>) -> impl Swap + 'a {
    x
}

fn dangle() -> &'static [i32; 3] {
    let long = Rc::new(RefCell::new(&[4, 5, 6]));
    let x = [1, 2, 3];
    let short = Rc::new(RefCell::new(&x));
    hide(long.clone()).swap(hide(short));
    let res: &'static [i32; 3] = *long.borrow();
    res
}

fn main() {
    println!("{:?}", dangle()); // prints nonsense values
}

[Centril]

r? @matthewjasper

[alexreg]

r? @nikomatsakis He wanted to look at this closely and it makes sense for him to approve. @matthewjasper's reviews and suggestions are of course appreciate too!

[alexreg]

@matthewjasper What do you think the new rule should be? Because I think we were all thinking the one used here should work...

[alexreg]

@nikomatsakis @matthewjasper commented to me in chat, "I don't think that there's a way to remove the restriction on the impl trait producer without adding one to the consumer." -- and I'm starting to think he's right. In his example, the line let res: &'static [i32; 3] = *long.borrow(); means it can't be determined locally (to the function hide) that the return type needs to outlive (well, "outlive") 'static, and there's no way of expressing this without introducing a dummy trait like Captures<'a>, I believe. This is, I suppose, due to the semantics of impl Trait + 'a, which I probably don't fully understand... speaking of which, does anyone have an example of where simply appending + 'hidden_lifetime is not equivalent to + Captures<'hidden_lifetime> (i.e., the latter workers but the former doesn't)?

[bors]

☔ The latest upstream changes (presumably #59632) made this pull request unmergeable. Please resolve the merge conflicts.

[Centril]

Skipped on this week's T-Lang meeting since Niko wasn't here. Rescheduling for next week.

[nikomatsakis]

We discussed this on Zulip some time ago. I think that the summary from that conversation was "it will be a non-trivial effort to figure out how to do this work soundly". I am going to close this PR because I don't think we're particularly close to doing that work (nor likely to have the bandwidth to do it in the near term).

[alexreg]

@nikomatsakis @Centril Fair enough about the postponement, but should we at least create a tracking issue for this, and link to this PR, #57870, and #56047 as prior attempts, so anyone tackling this in the future (maybe one of us) has all the info in one place, and others can follow it too?

[Centril]

@alexreg That would be neat! If you can pack a good summary there it would be quite helpful.

[alexreg]

@Centril Is there a template for tracking issues?

[Centril]

@alexreg They are typically made after RFCs but that doesn't apply so much here. That link is the one I know of for tracking issues.