Opaque impl Trait return types unsoundly ignoring lifetime constraints

[steffahn]

trait StaticDefaultRef: 'static {
    fn default_ref() -> &'static Self;
}

impl StaticDefaultRef for str {
    fn default_ref() -> &'static str {
        ""
    }
}

fn into_impl(x: &str) -> &(impl ?Sized + AsRef<str> + StaticDefaultRef + '_) {
    x
}

fn extend_lifetime<'a>(x: &'a str) -> &'static str {
    let t = into_impl(x);
    helper(|_| t)
}

fn helper<T: ?Sized + AsRef<str> + StaticDefaultRef>(f: impl FnOnce(&T) -> &T) -> &'static str {
    f(T::default_ref()).as_ref()
}

fn main() {
    let r;
    {
        let x = String::from("Hello World?");
        r = extend_lifetime(&x);
    }
    println!("{}", r);
}

(playground)

�j��0V�

So IMO the biggest problem in the code above is that the opaque type returned by into_impl implements StaticDefaultRef even though its lifetime is '_, i.e. not 'static.

This particular code only compiles since 1.45, so this might be a regression.

@rustbot modify labels: T-compiler, A-impl-trait, A-lifetimes, A-typesystem
and someone please add “I-unsound 💥”.

[rustbot]

Error: Label T-compiler can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

[rustbot]

Error: Label T-compiler can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.

[steffahn]

So the code example compiles since nightly-2020-05-25

converted 2020-05-25 to 46e85b4328fe18492894093c1092dfe509df4370
looking for regression commit between 2020-05-24 and 2020-05-25
fetching (via remote github) commits from max(8970e8bcf6153d1ead2283f1a0ed7b192230eca6, 2020-05-22) to 46e85b4328fe18492894093c1092dfe509df4370
ending github query because we found starting sha: 8970e8bcf6153d1ead2283f1a0ed7b192230eca6
get_commits_between returning commits, len: 8
  commit[0] 2020-05-23UTC: Auto merge of #72504 - Dylan-DPC:rollup-6wi1ifz, r=Dylan-DPC
  commit[1] 2020-05-23UTC: Auto merge of #72474 - mati865:ci-fix, r=pietroalbini
  commit[2] 2020-05-24UTC: Auto merge of #72516 - Dylan-DPC:rollup-cc4w96z, r=Dylan-DPC
  commit[3] 2020-05-24UTC: Auto merge of #72362 - matthewjasper:remove-rescope, r=nikomatsakis
  commit[4] 2020-05-24UTC: Auto merge of #72524 - RalfJung:rollup-s9f1pcc, r=RalfJung
  commit[5] 2020-05-24UTC: Auto merge of #72529 - RalfJung:rollup-ydthv90, r=RalfJung
  commit[6] 2020-05-24UTC: Auto merge of #72531 - RalfJung:miri-upd, r=RalfJung
  commit[7] 2020-05-24UTC: Auto merge of #72539 - RalfJung:rollup-8yfidi8, r=RalfJung
ERROR: no commits between 8970e8bcf6153d1ead2283f1a0ed7b192230eca6 and 46e85b4328fe18492894093c1092dfe509df4370 within last 167 days

before that, you’ll get

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter in function call due to conflicting requirements
  --> src/main.rs:17:13
   |
17 |     let t = into_impl(x);
   |             ^^^^^^^^^^^^
   |
note: first, the lifetime cannot outlive the lifetime `'a` as defined on the function body at 16:20...
  --> src/main.rs:16:20
   |
16 | fn extend_lifetime<'a>(x: &'a str) -> &'static str {
   |                    ^^
note: ...so that reference does not outlive borrowed content
  --> src/main.rs:17:23
   |
17 |     let t = into_impl(x);
   |                       ^
note: but, the lifetime must be valid for the anonymous lifetime #2 defined on the body at 18:12...
  --> src/main.rs:18:12
   |
18 |     helper(|_| t)
   |            ^^^^^
note: ...so that the type `impl StaticDefaultRef+std::convert::AsRef<str>+?Sized` is not borrowed for too long
  --> src/main.rs:18:16
   |
18 |     helper(|_| t)
   |                ^

error: aborting due to previous error

[steffahn]

I guess that lifetime bounds aren’t the only problem. Here’s an unsound example that doesn’t need any : 'static bound on a trait.

use std::marker::PhantomData;
trait Foo<T: ?Sized> {
    fn fun(f: &dyn Fn(PhantomData<&Self>) -> &Self) -> &dyn Fn(PhantomData<&T>) -> &T;
}

impl<T: ?Sized> Foo<T> for T {
    fn fun(f: &dyn Fn(PhantomData<&Self>) -> &Self) -> &dyn Fn(PhantomData<&T>) -> &T {
        f
    }
}

fn extend_lifetime<'a, T: ?Sized>(x: &T) -> &'a T {
    Foo::fun(&|_| impl_foo(x))(PhantomData)
}

fn impl_foo<T: ?Sized>(x: &T) -> &(impl ?Sized + Foo<T> + '_) {
    x
}

fn main() {
    let r;
    {
        let x = String::from("Hello World?");
        r = extend_lifetime(&x);
    }
    println!("{}", r);
}

This new example works since 1.36. Weirdly only on godbolt it fails until 1.35. Locally this example compiles since 1.34.

In 1.33 it complains about some elided lifetime. Fixing that, i.e. using

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    Foo::fun(&|_| impl_foo(x))(PhantomData)
}

produces this warning

warning: unsatisfied lifetime constraints
  --> src/main.rs:14:19
   |
13 | fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
   |                    -- lifetime `'a` defined here
14 |     Foo::fun(&|_| impl_foo(x))(PhantomData)
   |                   ^^^^^^^^^^^ returning this value requires that `'a` must outlive `'static`
   |
   = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
   = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future

    Finished dev [unoptimized + debuginfo] target(s) in 0.83s

Weirdly, from 1.34 onwards this warning did just disappear even though the code contains UB. I do know nothing about the details behind this warning though.

Edit: That’s interesting… changing it to

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    let y = impl_foo(x);
    Foo::fun(&|_| y)(PhantomData)
}

moves the regression back up to 1.45/nightly-2020-05-25.

[steffahn]

So, the code above but with

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    Foo::fun(&|_| impl_foo(x))(PhantomData)
}

stops producing the warning about “potential undefined behavior” at nightly-2019-02-15

converted 2019-02-15 to f47ec2ad5b6887b3d400aee49e2294bd27733d18
looking for regression commit between 2019-02-14 and 2019-02-15
fetching (via remote github) commits from max(e54494727855cd14229f5d456591ed2a2f027c46, 2019-02-12) to f47ec2ad5b6887b3d400aee49e2294bd27733d18
ending github query because we found starting sha: e54494727855cd14229f5d456591ed2a2f027c46
get_commits_between returning commits, len: 4
  commit[0] 2019-02-13UTC: Auto merge of #56951 - oli-obk:auto_toolstate_issue, r=kennytm
  commit[1] 2019-02-13UTC: Auto merge of #58432 - Centril:rollup, r=Centril
  commit[2] 2019-02-14UTC: Auto merge of #58446 - Centril:rollup, r=Centril
  commit[3] 2019-02-14UTC: Auto merge of #58455 - Centril:rollup, r=Centril
ERROR: no commits between e54494727855cd14229f5d456591ed2a2f027c46 and 47ec2ad5b6887b3d400aee49e2294bd27733d18 within last 167 days

looking through the description of these PRs, the point of regression (where the warning disappeared) might be #58347

---

The warning first appeared in nightly-2018-12-05 (at least that’s what my bisection tells me) – before that the code compiles without warning. Weirdly enough it is already present on 1.30.0 which came out earlier. I haven’t tested anything pre-Rust-2018 yet.

---

Edit: Ah, the choice of edition explains the different godbolt behavior, its errors are probably due to NNL not being available on edition 2015 or something like that.

[steffahn]

I’m more and more getting the feeling that this issue might be about how closures interact with opaque impl 𝑇𝑟𝑎𝑖𝑡 return types. The way the closure captures a value of impl 𝑇𝑟𝑎𝑖𝑡 type seems to be hard to recreate without using a closure, AFAICT. Let’s add @rustbot modify labels: A-closures.

[apiraino]

Assigning priority as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@rustbot label -I-prioritize +P-high

[steffahn]

To be clear: on further thought, I don’t think that the impl StaticDefaultRef + '_ return type in the original example is problematic. The actual problem is that the compiler does infer an implied bound 'a: 'b from a type &'b impl StaticDefaultRef + 'a in a closure body (where impl … refers to some concrete opaque return type of a function).

It all becomes a bit more explicit and visible when we use min_type_alias_impl_trait. With the ability to name the types, we don’t need any closures anymore:

#![feature(min_type_alias_impl_trait)]

type WithLifetime<'a> = impl Equals<SelfType = ()>;
fn _defining_use<'a>() -> WithLifetime<'a> {}

trait Convert<'a> {
    type Witness;
    fn convert<'b, T: ?Sized>(_proof: &'b Self::Witness, x: &'a T) -> &'b T;
}

impl<'a> Convert<'a> for () {
    type Witness = WithLifetime<'a>;

    fn convert<'b, T: ?Sized>(_proof: &'b WithLifetime<'a>, x: &'a T) -> &'b T {
        // compiler thinks it gets to assume 'a: 'b here because of the `&'b WithLifetime<'a>` argument
        x
    }
}

fn extend_lifetime<'a, 'b, T: ?Sized>(x: &'a T) -> &'b T {
    WithLifetime::<'a>::convert_helper::<(), T>(&(), x)
}

trait Equals {
    type SelfType;
    fn convert_helper<'a, 'b, W: Convert<'a, Witness = Self>, T: ?Sized>(
        proof: &'b Self::SelfType,
        x: &'a T,
    ) -> &'b T;
}

impl<S> Equals for S {
    type SelfType = Self;
    fn convert_helper<'a, 'b, W: Convert<'a, Witness = Self>, T: ?Sized>(
        proof: &'b Self,
        x: &'a T,
    ) -> &'b T {
        W::convert(proof, x)
    }
}

fn main() {
    let r;
    {
        let x = String::from("Hello World?");
        r = extend_lifetime(&x);
    }
    println!("{}", r);
}

(playground)

The important thing that has to change IMO is that a &'b WithLifetime<'a> type must no longer provide evidence for a 'a: 'b relation the way it currently does in the definition of <() as Convert>::convert above. The only implied bound known inside of the function body should be WithLifetime<'a>: 'b and there’s not much that can be deduced from that. Pretty similar to how associated types currently work in this regard. And of course we still know that 'a: 'b does imply WithLifetime<'a>: 'b just not necessarily the other way around.