Voultapher · Voultapher · Oct 25, 2021 · Oct 11, 2021 · Oct 24, 2021 · Oct 25, 2021
diff --git a/src/lib.rs b/src/lib.rs
@@ -206,16 +206,16 @@ pub mod unsafe_self_cell;
 /// ```
 ///
 /// ```ignore
-/// fn with_dependent<Ret>(
-///     &self,
-///     func: impl for<'a> FnOnce(&'a $Owner, &'a $Dependent<'a>
+/// fn with_dependent<'outer_fn, Ret>(
+///     &'outer_fn self,
+///     func: impl for<'a> FnOnce(&'a $Owner, &'outer_fn $Dependent<'a>
 /// ) -> Ret) -> Ret
 /// ```
 ///
 /// ```ignore
-/// fn with_dependent_mut<Ret>(
-///     &mut self,
-///     func: impl for<'a> FnOnce(&'a $Owner, &'a mut $Dependent<'a>) -> Ret
+/// fn with_dependent_mut<'outer_fn, Ret>(
+///     &'outer_fn mut self,
+///     func: impl for<'a> FnOnce(&'a $Owner, &'outer_fn mut $Dependent<'a>) -> Ret
 /// ) -> Ret
 /// ```
 ///
@@ -311,19 +311,17 @@ macro_rules! self_cell {
 ) => {
     #[repr(transparent)]
     $(#[$StructMeta])*
-    $Vis struct $StructName $(<$OwnerLifetime>)* {
+    $Vis struct $StructName $(<$OwnerLifetime>)? {
         unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell<
             $StructName$(<$OwnerLifetime>)?,
             $Owner,
             $Dependent<'static>
         >,
 
-        // marker to ensure that contravariant owners don't imply covariance
-        // over the dependent. See issue #18
-        owner_marker: core::marker::PhantomData<$(&$OwnerLifetime)* ()>,
+        $(owner_marker: $crate::_covariant_owner_marker!($Covariance, $OwnerLifetime) ,)?
     }
 
-    impl $(<$OwnerLifetime>)* $StructName $(<$OwnerLifetime>)* {
+    impl $(<$OwnerLifetime>)? $StructName $(<$OwnerLifetime>)? {
         $Vis fn new(
             owner: $Owner,
             dependent_builder: impl for<'_q> FnOnce(&'_q $Owner) -> $Dependent<'_q>
@@ -343,7 +341,7 @@ macro_rules! self_cell {
                 // bad<'_q>(outside_ref: &'_q String) -> impl for<'x> FnOnce(&'x
                 // Owner) -> Dependent<'x>`.
 
-                type JoinedCell<'_q $(, $OwnerLifetime)*> =
+                type JoinedCell<'_q $(, $OwnerLifetime)?> =
                     $crate::unsafe_self_cell::JoinedCell<$Owner, $Dependent<'_q>>;
 
                 let layout = $crate::alloc::alloc::Layout::new::<JoinedCell>();
@@ -373,7 +371,7 @@ macro_rules! self_cell {
                     unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new(
                         joined_void_ptr,
                     ),
-                    owner_marker: core::marker::PhantomData,
+                    $(owner_marker: $crate::_covariant_owner_marker_ctor!($OwnerLifetime) ,)?
                 }
             }
         }
@@ -388,7 +386,7 @@ macro_rules! self_cell {
             unsafe {
                 // See fn new for more explanation.
 
-                type JoinedCell<'_q $(, $OwnerLifetime)*> =
+                type JoinedCell<'_q $(, $OwnerLifetime)?> =
                     $crate::unsafe_self_cell::JoinedCell<$Owner, $Dependent<'_q>>;
 
                 let layout = $crate::alloc::alloc::Layout::new::<JoinedCell>();
@@ -419,7 +417,7 @@ macro_rules! self_cell {
                             unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new(
                                 joined_void_ptr,
                             ),
-                            owner_marker: core::marker::PhantomData,
+                            $(owner_marker: $crate::_covariant_owner_marker_ctor!($OwnerLifetime) ,)?
                         })
                     }
                     Err(err) => Err(err)
@@ -437,7 +435,7 @@ macro_rules! self_cell {
             unsafe {
                 // See fn new for more explanation.
 
-                type JoinedCell<'_q $(, $OwnerLifetime)*> =
+                type JoinedCell<'_q $(, $OwnerLifetime)?> =
                     $crate::unsafe_self_cell::JoinedCell<$Owner, $Dependent<'_q>>;
 
                 let layout = $crate::alloc::alloc::Layout::new::<JoinedCell>();
@@ -468,7 +466,7 @@ macro_rules! self_cell {
                             unsafe_self_cell: $crate::unsafe_self_cell::UnsafeSelfCell::new(
                                 joined_void_ptr,
                             ),
-                            owner_marker: core::marker::PhantomData,
+                            $(owner_marker: $crate::_covariant_owner_marker_ctor!($OwnerLifetime) ,)?
                         })
                     }
                     Err(err) => {
@@ -489,7 +487,10 @@ macro_rules! self_cell {
             unsafe { self.unsafe_self_cell.borrow_owner::<$Dependent<'_q>>() }
         }
 
-        $Vis fn with_dependent<Ret>(&self, func: impl for<'_q> FnOnce(&'_q $Owner, &'_q $Dependent<'_q>) -> Ret) -> Ret {
+        $Vis fn with_dependent<'outer_fn, Ret>(
+            &'outer_fn self,
+            func: impl for<'_q> FnOnce(&'_q $Owner, &'outer_fn $Dependent<'_q>
+        ) -> Ret) -> Ret {
             unsafe {
                 func(
                     self.unsafe_self_cell.borrow_owner::<$Dependent>(),
@@ -498,7 +499,10 @@ macro_rules! self_cell {
             }
         }
 
-        $Vis fn with_dependent_mut<Ret>(&mut self, func: impl for<'_q> FnOnce(&'_q $Owner, &'_q mut $Dependent<'_q>) -> Ret) -> Ret {
+        $Vis fn with_dependent_mut<'outer_fn, Ret>(
+            &'outer_fn mut self,
+            func: impl for<'_q> FnOnce(&'_q $Owner, &'outer_fn mut $Dependent<'_q>) -> Ret
+        ) -> Ret {
             let (owner, dependent) = unsafe {
                     self.unsafe_self_cell.borrow_mut()
             };
@@ -525,7 +529,7 @@ macro_rules! self_cell {
         }
     }
 
-    impl $(<$OwnerLifetime>)* Drop for $StructName $(<$OwnerLifetime>)* {
+    impl $(<$OwnerLifetime>)? Drop for $StructName $(<$OwnerLifetime>)? {
         fn drop(&mut self) {
             unsafe {
                 self.unsafe_self_cell.drop_joined::<$Dependent>();
@@ -565,6 +569,35 @@ macro_rules! _covariant_access {
     };
 }
 
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _covariant_owner_marker {
+    (covariant, $OwnerLifetime:lifetime) => {
+        // Ensure that contravariant owners don't imply covariance
+        // over the dependent. See issue https://github.com/Voultapher/self_cell/issues/18
+        core::marker::PhantomData<&$OwnerLifetime ()>
+    };
+    (not_covariant, $OwnerLifetime:lifetime) => {
+        // See the discussion in https://github.com/Voultapher/self_cell/pull/29
+        //
+        // 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 ()>
+    };
+    ($x:ident, $OwnerLifetime:lifetime) => {
+        compile_error!("This macro only accepts `covariant` or `not_covariant`");
+    };
+}
+
+#[doc(hidden)]
+#[macro_export]
+macro_rules! _covariant_owner_marker_ctor {
+    ($OwnerLifetime:lifetime) => {
+        // Helper to optionally expand into PhantomData for construction.
+        core::marker::PhantomData
+    };
+}
+
 #[doc(hidden)]
 #[macro_export]
 macro_rules! _impl_automatic_derive {

diff --git a/src/unsafe_self_cell.rs b/src/unsafe_self_cell.rs
@@ -50,6 +50,8 @@ impl<ContainedIn, Owner, DependentStatic> UnsafeSelfCell<ContainedIn, Owner, Dep
         }
     }
 
+    // Calling any of these *unsafe* functions with the wrong Dependent type is UB.
+
     pub unsafe fn borrow_owner<'a, Dependent>(&'a self) -> &'a Owner {
         let joined_ptr =
             transmute::<NonNull<u8>, NonNull<JoinedCell<Owner, Dependent>>>(self.joined_void_ptr);

diff --git a/tests/invalid/covariant_owner_non_covariant_dependent.rs b/tests/invalid/covariant_owner_non_covariant_dependent.rs
@@ -0,0 +1,23 @@
+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()));
+}
diff --git a/tests/invalid/covariant_owner_non_covariant_dependent.stderr b/tests/invalid/covariant_owner_non_covariant_dependent.stderr
@@ -0,0 +1,28 @@
+error[E0597]: `s` does not live long enough
+  --> $DIR/covariant_owner_non_covariant_dependent.rs:19:16
+   |
+18 |     foo.with_dependent(|_, d| {
+   |                        ------ value captured here
+19 |         d.set(&s);
+   |                ^ borrowed value does not live long enough
+...
+23 | }
+   | -
+   | |
+   | `s` dropped here while still borrowed
+   | borrow might be used here, when `foo` is dropped and runs the `Drop` code for type `Foo`
+   |
+   = note: values in a scope are dropped in the opposite order they are defined
+
+error[E0505]: cannot move out of `s` because it is borrowed
+  --> $DIR/covariant_owner_non_covariant_dependent.rs:21:10
+   |
+18 |     foo.with_dependent(|_, d| {
+   |                        ------ borrow of `s` occurs here
+19 |         d.set(&s);
+   |                - borrow occurs due to use in closure
+20 |     });
+21 |     drop(s);
+   |          ^ move out of `s` occurs here
+22 |     foo.with_dependent(|_, d| println!("{}", d.get()));
+   |     --- borrow later used here
diff --git a/tests/invalid/escape_dependent.rs b/tests/invalid/escape_dependent.rs
@@ -0,0 +1,28 @@
+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(""));
+    });
+}
diff --git a/tests/invalid/escape_dependent.stderr b/tests/invalid/escape_dependent.stderr
@@ -0,0 +1,24 @@
+error[E0597]: `cell` does not live long enough
+  --> $DIR/escape_dependent.rs:22:22
+   |
+22 |     let leaked_ref = cell.with_dependent(|_, dependent| dependent);
+   |                      ^^^^ borrowed value does not live long enough
+...
+28 | }
+   | -
+   | |
+   | `cell` dropped here while still borrowed
+   | borrow might be used here, when `cell` is dropped and runs the `Drop` code for type `NoCov`
+
+error[E0716]: temporary value dropped while borrowed
+  --> $DIR/escape_dependent.rs:23:21
+   |
+23 |     leaked_ref.set(&String::from("temporary"));
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^ - temporary value is freed at the end of this statement
+   |                     |
+   |                     creates a temporary which is freed while still in use
+...
+28 | }
+   | - borrow might be used here, when `cell` is dropped and runs the `Drop` code for type `NoCov`
+   |
+   = note: consider using a `let` binding to create a longer lived value
diff --git a/tests/invalid/leak_dependent.stderr b/tests/invalid/leak_dependent.stderr
@@ -1,8 +1,9 @@
-error: lifetime may not live long enough
-  --> $DIR/leak_dependent.rs:18:58
+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 `'1` must outlive `'2`
-   |                                            |           |
-   |                                            |           return type of closure is &Cell<&'2 String>
-   |                                            has type `&'1 String`
+   |                       ^^^^                               --------- returning this value requires that `cell` is borrowed for `'static`
+   |                       |
+   |                       borrowed value does not live long enough
+19 | }
+   | - `cell` dropped here while still borrowed
diff --git a/tests/invalid/reborrow_dependent_cyclic.rs b/tests/invalid/reborrow_dependent_cyclic.rs
@@ -0,0 +1,34 @@
+use std::cell::RefCell;
+
+use self_cell::self_cell;
+
+struct Bar<'a>(RefCell<(Option<&'a Bar<'a>>, String)>);
+
+self_cell! {
+    struct Foo {
+        owner: (),
+
+        #[not_covariant]
+        dependent: Bar,
+    }
+}
+
+fn main() {
+    let mut x = Foo::new((), |_| Bar(RefCell::new((None, "Hello".to_owned()))));
+
+    x.with_dependent(|_, dep| {
+        dep.0.borrow_mut().0 = Some(dep);
+    });
+
+    x.with_dependent_mut(|_, dep| {
+        let r1 = dep.0.get_mut();
+        let string_ref_1 = &mut r1.1;
+        let mut r2 = r1.0.unwrap().0.borrow_mut();
+        let string_ref_2 = &mut r2.1;
+
+        let s = &string_ref_1[..];
+        string_ref_2.clear();
+        string_ref_2.shrink_to_fit();
+        println!("{}", s); // prints garbage
+    });
+}
diff --git a/tests/invalid/reborrow_dependent_cyclic.stderr b/tests/invalid/reborrow_dependent_cyclic.stderr
@@ -0,0 +1,33 @@
+error[E0597]: `x` does not live long enough
+  --> $DIR/reborrow_dependent_cyclic.rs:19:5
+   |
+19 |       x.with_dependent(|_, dep| {
+   |       ^ borrowed value does not live long enough
+   |  _____|
+   | |
+20 | |         dep.0.borrow_mut().0 = Some(dep);
+21 | |     });
+   | |______- argument requires that `x` is borrowed for `'static`
+...
+34 |   }
+   |   - `x` dropped here while still borrowed
+
+error[E0502]: cannot borrow `x` as mutable because it is also borrowed as immutable
+  --> $DIR/reborrow_dependent_cyclic.rs:23:5
+   |
+19 |       x.with_dependent(|_, dep| {
+   |       - immutable borrow occurs here
+   |  _____|
+   | |
+20 | |         dep.0.borrow_mut().0 = Some(dep);
+21 | |     });
+   | |______- argument requires that `x` is borrowed for `'static`
+22 |
+23 | /     x.with_dependent_mut(|_, dep| {
+24 | |         let r1 = dep.0.get_mut();
+25 | |         let string_ref_1 = &mut r1.1;
+26 | |         let mut r2 = r1.0.unwrap().0.borrow_mut();
+...  |
+32 | |         println!("{}", s); // prints garbage
+33 | |     });
+   | |______^ mutable borrow occurs here
diff --git a/tests/self_cell.rs b/tests/self_cell.rs
@@ -63,7 +63,7 @@ impl PackedAst {
         self.ast_cell.borrow_owner()
     }
 
-    fn with_ast(&self, func: impl for<'a> FnOnce(&'a String, &'a Ast<'a>)) {
+    fn with_ast<'o>(&'o self, func: impl for<'a> FnOnce(&'a String, &'o Ast<'a>)) {
         self.ast_cell.with_dependent(func)
     }