Attempt to fix cyclic re-borrow issue

[Voultapher]

The fix for the issue described in #28 (reborrow_dependent_cyclic.rs) allows
compiling leak_dependent.rs which should not be possible. This is a first
attempt and incomplete as seen by the stubbed out impls for with_dependent and
with_dependent_mut.

DO NOT MERGE

fixes #28

[Voultapher]

@steffahn I'm a little worried about the change in leak_dependent.rs

test tests/invalid/leak_dependent.rs ... mismatch

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: lifetime may not live long enough
  --> $DIR/leak_dependent.rs:18:58
   |
18 |     let _leaked_ref = cell.with_dependent(|_, dependent| dependent);
   |                                            -           - ^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
   |                                            |           |
   |                                            |           return type of closure is &Cell<&'2 String>
   |                                            has type `&'1 String`
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0597]: `cell` does not live long enough
  --> $DIR/leak_dependent.rs:18:23
   |
18 |     let _leaked_ref = cell.with_dependent(|_, dependent| dependent);
   |                       ^^^^                               --------- returning this value requires that `cell` is borrowed for `'static`
   |                       |
   |                       borrowed value does not live long enough
19 | }
   | - `cell` dropped here while still borrowed
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

It asks for a static lifetime. Without having tried it, something where the owner is covariant over it's lifetime and the dependent is not_covariant, we might be able to coerce the cell via owner into a static lifetime which the leaked ref then abused as seen in #5.

What do you think?

[steffahn]

The new error mirrors what would happen with ouroboros

#[self_referencing]
struct NoCov2 {
    owner: String,

    #[not_covariant]
    #[borrows(owner)]
    dependent: NotCovariant<'this>,
}

fn main() {
    let cell = NoCov2::new("hi this is no good".into(), |owner| Cell::new(owner));
    let _leaked_ref = cell.with_dependent(|dependent| dependent);
}

    Checking small_pg v0.1.0 (/home/frank/Dokumente/playground/rust/small_pg)
error[E0597]: `cell` does not live long enough
  --> src/main.rs:29:23
   |
29 |     let _leaked_ref = cell.with_dependent(|dependent| dependent);
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^---------^
   |                       |                               |
   |                       |                               returning this value requires that `cell` is borrowed for `'static`
   |                       borrowed value does not live long enough
30 | }
   | - `cell` dropped here while still borrowed

For more information about this error, try `rustc --explain E0597`.
error: could not compile `small_pg` due to previous error

This is interesting behavior, but I think I understand what’s going on. If you were to get a &'static NoCov, then the closure in borrow_dependent gets a &'static Cell<&'a String>. But this type is only a valid type when 'a is 'static, too, so inside of the closure you’ll indeed get to assume you’ve gotten a &'static Cell<&'static String>.

I’m surprised that rustc seems to be able to come to that conclusion (i.e. that 'static would help here). Anyways, the error message itself doesn’t seem to be reason to worry. Btw, if the cell is leaked, you can indeed make the thing compile, e.g.

// in the ouroboros example
fn main() {
    let cell = NoCov2::new("hi this is no good".into(), |owner| Cell::new(owner));
    let cell = Box::leak(Box::new(cell));
    let _leaked_ref = cell.with_dependent(|dependent| dependent);
}

or

// in the self_cell example
fn main() {
    let cell = NoCov:new("hi this is no good".into(), |owner| Cell::new(owner));
    let cell = Box::leak(Box::new(cell));
    let _leaked_ref = cell.with_dependent(|_, dependent| dependent);
}

---

I have yet to look into #5 in order to give an answer to your question at the end.

[steffahn]

It asks for a static lifetime. Without having tried it, something where the owner is covariant over it's lifetime and the dependent is not_covariant, we might be able to coerce the cell via owner into a static lifetime which the leaked ref then abused as seen in #5.

What do you think?

I’m having a hard time connecting what you explain here with the issue described in #5. Could you perhaps try to explain your (potential) concern a bit more detailed? In particular you talk about an owner with a lifetime.

---

If the owner has a lifetime, the situation is

owner: Foo<'a>,
dependent: NotCovariant,

type NotCovariant<'q> = Cell<&'q Bar>;

then with_dependent provides &'_q Foo<'a> and &'outer_fn Cell<'_q Bar>, given an &'outer_fn Foo<'a>.

Here, initially we have 'a: 'outer_fn (in order for &'outer_fn Foo<'a> to be a valid type),
then the closure must handle a HRTB-quantified lifetime '_q such that 'a: '_q (due to &'_q Foo<'a>) and '_q: 'outer_fn (due to &'outer_fn Cell<'_q Bar>).

I.e. 'a is longer than 'outer_fn, and the '_q in the closure can be anything between 'a and 'outer_fn. In order for the reference to the Cell to break out of the closure, you’d need to pin down the '_q by making 'outer_fn and 'a be the same. Well, okay, with a covariant owner, creating a &'a Foo<'a> should actually be possible even without leaking the cell. In fact this compiles

use std::{cell::Cell, marker::PhantomData};

use self_cell::self_cell;

type NotCovariant<'a> = Cell<&'a String>;

struct Owner<'a>(String, PhantomData<&'a ()>);

self_cell!(
    struct NoCov<'a> {
        owner: Owner<'a>,

        #[not_covariant]
        dependent: NotCovariant,
    }
);

fn main() {
    let cell = NoCov::new(Owner("hi this is no good".into(), PhantomData), |owner| {
        Cell::new(&owner.0)
    });
    let _leaked_ref = cell.with_dependent(|_, dependent| dependent);
}

let’s see if that’s problematic ……… yes it is 🙈

use std::{cell::Cell, marker::PhantomData};

use self_cell::self_cell;

type NotCovariant<'a> = Cell<&'a str>;

struct Owner<'a>(String, PhantomData<&'a ()>);

self_cell!(
    struct NoCov<'a> {
        owner: Owner<'a>,

        #[not_covariant]
        dependent: NotCovariant,
    }
);

fn main() {
    let cell = NoCov::new(Owner("hi this is no good".into(), PhantomData), |owner| {
        Cell::new(&owner.0)
    });
    let leaked_ref = cell.with_dependent(|_, dependent| dependent);
    leaked_ref.set(&String::from("temporary"));

    cell.with_dependent(|_, dep| {
        println!("{}", dep.replace(""));
    });
}

goddammit.

[steffahn]

It’s a bit too late today already, I think I’ll do further comparisons to ouroboros (and evaluations on how to fix this problem) tomorrow.

[Voultapher]

I've not yet found the time to look at your examples in detail. A rough idea:

pub fn with_dependent<'outer_fn, Ret>(
    &'outer_borrow self,
    func: impl for<'_q, 'outer_fn: '_q> FnOnce(&'_q (), &'_q Bar<'_q>) -> Ret
) -> Ret

Somehow apply both constraints the '_q as before that stops leaking inner refs and the fn lifetime that stops #28

[steffahn]

Right, I should probably continue to write something here :-D

Anything that's providing any &'a Bar<'a> is unsound, i.e. both lifetimes being the same, is unsound, since you'll be able to get first access to an &'a Bar<'a> (in with_dependent), then access to a &'b Bar<'b> (in with_dependent_mut) to the same object.

---

W.r.t. what ouroboros does: They simply rule out a covariant self-referential struct where the dependent is not covariant, i.e. something like

#[self_referencing]
struct NoCov<'a> {
    owner: Owner<'a>,

    #[not_covariant]
    #[borrows(owner)]
    dependent: NotCovariant<'this>,
}

would make NoCov<'a> invariant in 'a.

They do this by using 'a instead of 'static internally for storing the dependent field, i.e. while something like

#[self_referencing]
struct Foo {
    owner: Owner,

    #[not_covariant]
    #[borrows(owner)]
    dependent: NotCovariant<'this>,
}

translates to

// actually inside of a module, so the fields are really inaccessible
struct Foo {
    owner: AliasableBox<Owner>,
    dependent: NotCovariant<'static>,
}

, something like

#[self_referencing]
struct NoCov<'a> {
    owner: Owner<'a>,

    #[not_covariant]
    #[borrows(owner)]
    dependent: NotCovariant<'this>,
}

translates to

// actually inside of a module, so the fields are really inaccessible
struct NoCov<'a> {
    owner: AliasableBox<Owner<'a>>,

    dependent: NotCovariant<'a>,
}

[steffahn]

Look, I just noticed the same kind of problem as described above can happen on the current version of self_cell as well. Making the reference escape the call to with_dependent isn’t actually the main problem

use std::{cell::Cell, marker::PhantomData};

use self_cell::self_cell;

self_cell! {
    struct Foo<'a> {
        owner: PhantomData<&'a ()>,
        #[not_covariant]
        dependent: Dependent,
    }
}

type Dependent<'q> = Cell<&'q str>;

fn main() {
    let foo = Foo::new(PhantomData, |_| Cell::new(""));
    let s: String = "Hello World".into();
    foo.with_dependent(|_, d| {
        d.set(&s);
    });
    drop(s);
    foo.with_dependent(|_, d| println!("{}", d.get()));
}

the problem here is the owner part of with_dependent. It being &'q PhantomData<&'a ()> here means that 'q is artificially constrained to be shorter than 'a. This is not really a problem if the self_cell struct Foo<'a> isn't covariant, because then the lifetime 'a would be longer than how long the Foo<'a> could ever exist. However when, like above, Foo<'a> is covariant, you can constrain 'q to be much shorter, short enough to be able to coerce a &'b str into &'q str which was obtained from a String that gets dropped before the Foo does.

Really, the most logical/straightforward might be just to make with_dependend hand out &'outer_fn Owner and &'outer_fn Dependent<'q> instead of – what this PR does – just &'q Owner and &'outer_fn Dependent<'q>. However this change would disallow re-building the dependent from the borrowed owner in a call to with_dependent_mut in a way that's similar to how the closure passed to ::new(…) builds the dependent in the first place.

The alternative would be the ouroboros approach. I don't even know if they did that intentionally, but it seems to prevent the unsoundness successfully: Make a self_cell struct not covariant if the dependent is not covariant, by incorporating some form of Dependent<'a> in the implementation of self_cell! { Foo<'a> { … } }.

Actually, thinking about the first approach again… it might be enough to make with_dependent feature a &'outer_fn Owner, while with_dependent_mut can probably keep the &'q Owner. This would still allow re-building of the dependent: Dependent<'q> starting from the &'q Owner reference by means of using with_dependent_mut.

[Voultapher]

@steffahn sorry for taking so long, I've been busy. Marking the result struct as invariant over the owner lifetime if the dependent is not covariant seems like the more prudent approach to me. I would appreciate it if you could take a look at fcc2dc6

I still unsure about &'a mut owners, there might be some nasty surprise left there but I don't see how. And I wonder if UnsafeCell<&$OwnerLifetime ()> was the right choice, maybe the implied !Sync is too strict, or maybe that is yet another bug lurking in the previous versions.

[steffahn]

“it’s” → “its” 😉

Regarding the UnsafeCell, the best way of getting only the (in-)variance constraints and nothing else like !Sync is by using fn(T) -> T. A similar thing also generally applies for generic arguments, e.g. it can make sense to use PhantomData<fn() -> T> instead of PhantomData<T>; in this case (i.e. in the covariant case above) it doesn’t matter because &'lt () doesn’t come with any restrictions on auto-traits (Send/Sync/UnwindSafe/Unpin/etc…) anyways so PhantomData<&'lt ()> is okay. So

Suggested change

	// If the dependent is non_covariant, mark the owner as invariant over it's
	// lifetime. Otherwise unsound use is possible.
	core::marker::PhantomData<core::cell::UnsafeCell<&$OwnerLifetime ()>>
	// If the dependent is non_covariant, mark the owner as invariant over its
	// lifetime. Otherwise unsound use is possible.
	core::marker::PhantomData<fn(&$OwnerLifetime ()) -> &$OwnerLifetime ()>

[Voultapher]

Maybe I'm missing something, I don't understand how that would imply invariance, here https://doc.rust-lang.org/nomicon/subtyping.html it only talks about contravariant and covariant for function pointers. Also wouldn't this have the same problem as #18 ?

[steffahn]

If fn() -> T is covariant in T, but not contravariant; and fn(T) -> () is contravariant, but not covariant; then fn(T) -> T is invariant (neither covariant nor contravariant).

If you preferer it, fn() -> UnsafeCell<T> would be an option, too, that solves the !Sync problem of UnsaveCell<T> as well. I guess, Mutex<T> would also work in this case; since T == &'lt () implements Send, that doesn't introduce any extra bounds on auto traits either.

[steffahn]

AFAICT, you’re using (even before this PR) a lot of $(…)* or $(…),* for the $OwnerLifetime, even though that’s coming from a $(<$OwnerLifetime:lifetime>)?. It would IMO improve clarity of these were to be changed to use $(…)? consistently on the use-site, too :-)

[steffahn]

I still unsure about &'a mut owners, there might be some nasty surprise left there but I don't see how.

Well, one can hardly be certain 100% that there’s no unknown unsoundness hiding, but it seems good to me now; I’ve taken a look through the commit and added two comments for review above, the rest looks fine.

[Voultapher]

I'm starting to wonder if this whole owner with lifetime feature is worth it, if your dependent depends on the owners lifetime you should just reuse that lifetime eg. https://godbolt.org/z/6P9oshd91. It only seems useful for cases where the owner contains some unrelated lifetime and you want to depend on parts unrelated to that lifetime https://godbolt.org/z/vdKzbzbz9.

[Voultapher]

I'm starting to wonder if this whole owner with lifetime feature is worth it, if your dependent depends on the owners lifetime you should just reuse that lifetime eg. https://godbolt.org/z/6P9oshd91. It only seems useful for cases where the owner contains some unrelated lifetime and you want to depend on parts unrelated to that lifetime https://godbolt.org/z/vdKzbzbz9.

In which case one could restructure the code to separate out the lifetime independent part https://godbolt.org/z/8Y895bj5s

So really is there good reason to keep it? @steffahn what do you think?

[steffahn]

I suppose for an owner like e.g. a cell::Ref<'a> it can be useful.

#![allow(unused)]

use std::{cell::{self, RefCell}, ops::{self, Not}};

type Dep<'a> = &'a str;

self_cell::self_cell!(
    struct SliceRef<'a> {
        owner: cell::Ref<'a, String>,
        #[covariant]
        dependent: Dep,
    }
);
impl<'a> ops::Deref for SliceRef<'a> {
    type Target = str;

    fn deref(&self) -> &Self::Target {
        self.borrow_dependent()
    }
} 

// nontrivial example
fn all_but_first_char(x: &RefCell<String>) -> Option<SliceRef<'_>> {
    let x = x.borrow();
    x.is_empty()
        .not()
        .then(|| SliceRef::new(x, |s| &s[s[0..].chars().next().unwrap().len_utf8()..]))
}