Make LazyCell<T, F> and LazyLock<T, F> covariant in both T and F

[WaffleLapkin]

View all comments

See explanation for why it's valid that I wrote as a comment in LazyCell:

rust/library/core/src/cell/lazy.rs

Lines 55 to 91 in 5da2332

	// It is non-obvious why `LazyCell` can be covariant in both `T` and `F`
	// (and thus use `CovariantUnsafeCell`)...
	//
	// # `F`
	//
	// `F` is the easier to explain one; The state only ever transitions *out* of `Uninit(F)`
	// (to either `Poisoned` or `Init(T)`), and never into it. In other words `F` is only ever
	// read, not written.
	//
	// One could imagine `LazyCell` implemented as
	// ```
	// struct {
	// state: UnsafeCell<Uninit | Init(T) | Poisoned>,
	// f: ManuallyDrop<F>,
	// }
	// ```
	// which would make it "obviously" covariant in `F`.
	//
	// **NOTE**: the important invariant here is that we never allow writing `F` through
	// `&LazyCell`.
	//
	// # `T`
	//
	// Why `LazyCell` can be covariant in `T` is even more subtle. We do allow writing `T` through
	// a `&LazyCell`, which would normally force us to make `LazyCell` invariant in `T`. However,
	// the only value that we allow writing is one returned by `F`... and we don't allow changing
	// `F`. So even if a user gets a `&LazyCell<LessrestrictedVersionOfT>`, they cannot write the
	// `LessrestrictedVersionOfT` through it.
	//
	// **NOTE**: the important invariants here are that
	// 1. `T` can only be written through `&LazyCell<T, F>` by being returned from `F`
	// 2. `F` cannot be overwritten after `LazyCell` creation
	//
	// # Conclusion
	//
	// `LazyCell` can be covariant in both `T` and `F`... provided the non-local invariants which
	// are listed above.

r? libs-api

[ChayimFriedman2]

This two assumptions are both fragile. We might want to provide an API in the future to clear a LazyCell similar to OnceCell, and we might want to provide an API to set the value directly without the callback. Making this type covariant in T will prohibit both.

I think the safe thing to do is to make it covariant in F but leave it invariant in T.

View changes since the review

[WaffleLapkin]

What do you mean by "API [...] to clear a LazyCell similar to OnceCell"?

If you are talking about take, then it would not be a problem for these assumptions¹. For LazyCell you would need something like fn take(&mut self, new_initializer: F) -> Result<T, F>. Notably since it would have to take &mut self, it does not interact with variance².

Setting the value without callback would require LazyCell to be invariant in T. If we want to add such an API we should keep T invariant.

Footnotes

1. Maybe they are not very clearly stated... I wish we had a better language to talk about variance... ↩

2. &mut T is invariant in T and prohibits other references to the value, so you can't shorten the lifetime and give and unexpectedly dead reference to any observer. ↩

[ChayimFriedman2]

Alright, maybe take() isn't a problem, but fn set(&self, value: T) -> Result<(), T> is.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[clarfonthey]

r? libs

[Mark-Simulacrum]

I think this probably is better reviewed by types -- and in an ideal world, types(?) would provide us some way to assert the API shape guarantees the properties required, since I don't really trust us to get that right as the API evolves...

I think we'll also need libs FCP on this (@rustbot label: +needs-fcp) because this change is insta-stable AFAICT.

r? types

[rustbot]

Error: Parsing relabel command in comment failed: a label delta when parsing: +needs-fcp[!]) because

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[clarfonthey]

I don't think types necessarily has to review/sign off, but you're definitely right that it should have an FCP due to it being instantly stable.

Speaking of which, does semver-checks catch that?

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: e81aa4b (e81aa4b3c7bfbe1cec9795febc585aac0aba8aeb)
Base parent: feaadee (feaadeeaca7db0594da854e7c8c07495341c7439)

[clarfonthey]

@bors try jobs=test-x86_64-gnu-stdlib-semver-check

[rust-bors]

☀️ Try build successful (CI)
Build commit: 677ed5a (677ed5aac7ef5935d14c04cc17ab41998c789935)
Base parent: feaadee (feaadeeaca7db0594da854e7c8c07495341c7439)

[Mark-Simulacrum]

I don't think types necessarily has to review/sign off, but you're definitely right that it should have an FCP due to it being instantly stable.

I agree that types doesn’t necessarily have to sign off, but if types doesn’t then we need someone on libs comfortable reviewing this to do so (I am not that person). Types also seems best placed to answer how hard a lint / enforcement of the guarantees needed to make this guaranteed correct (rather than hoping humans catch bugs) in the compiler would be.

[clarfonthey]

FWIW I was mostly commenting since I figured this would need an FCP that probably it should just be libs, not types. In terms of review, I would not expect most people to be comfortable with reviewing variance changes since they're highly counterintuitive.

[steffahn]

I'm not sure if this is sound, let me take a look at this.

[steffahn]

I'm trying to build a counter-example, but I'm getting kinda stuck on rustc seemingly not considering CovariantUnsafeCell to be covariant in the first place? o.O

error: lifetime may not live long enough
  --> src/main.rs:35:87
   |
35 | fn test2<'a: 'b, 'b>(x: CovariantUnsafeCell<&'a ()>) -> CovariantUnsafeCell<&'b ()> { x }
   |          --      -- lifetime `'b` defined here                                        ^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
   |          |
   |          lifetime `'a` defined here
   |
   = help: consider adding the following bound: `'b: 'a`
   = note: requirement occurs because of the type `CovariantUnsafeCell<&()>`, which makes the generic argument `&()` invariant
   = note: the struct `CovariantUnsafeCell<T>` is invariant over the parameter `T`
   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance

[theemathas]

This is very technically a breaking change. See #153607

[steffahn]

Anyway.. I managed to simulate covariance with an unsafe wrapper to demonstrate potential soundness issues.

struct CovariantLazyCell<T, F> {
    data: *mut (),
    marker: PhantomData<fn() -> (T, F)>,
}

impl<T, F> CovariantLazyCell<T, F> {
    fn new(b: Box<LazyCell<T, F>>) -> Self {
        Self {
            data: Box::into_raw(b) as _,
            marker: PhantomData,
        }
    }

    fn as_ref(&self) -> &LazyCell<T, F> {
        unsafe {
            self.data
                .cast::<LazyCell<T, F>>()
                .cast_const()
                .as_ref_unchecked()
        }
    }
}

impl<T, F> Drop for CovariantLazyCell<T, F> {
    fn drop(&mut self) {
        let inner: *mut LazyCell<T, F> = self.data.cast();
        unsafe { drop(Box::from_raw(inner)) }
    }
}

In terms of soundness issue, here's what I could come up with: Once people can (eventually) manually implement FnOnce, this can be an issue:

#![feature(unboxed_closures)]
#![feature(fn_traits)]

use std::marker::PhantomData;

struct MyCustomFn<T>(PhantomData<T>);

type A = for<'a> fn(&'a str);
type B = fn(&'static str);

impl FnOnce<()> for MyCustomFn<A> {
    type Output = A;
    extern "rust-call" fn call_once(self, _: ()) -> Self::Output {
        println!("A");
        |_| {}
    }
}

static STRING: Mutex<&'static str> = Mutex::new("");

impl FnOnce<()> for MyCustomFn<B> {
    type Output = B;
    extern "rust-call" fn call_once(self, _: ()) -> Self::Output {
        println!("B");
        |s| {
            *STRING.lock().unwrap() = s;
        }
    }
}

use std::cell::LazyCell;
use std::sync::Mutex;

fn main() {
    let r_a: &CovariantLazyCell<A, MyCustomFn<A>> =
        &CovariantLazyCell::new(Box::new(LazyCell::new(MyCustomFn::<A>(PhantomData))));
    let r_b: &CovariantLazyCell<B, MyCustomFn<B>> = r_a;
    let f_b: &B = LazyCell::force(r_b.as_ref());
    let f_a: &A = LazyCell::force(r_a.as_ref());

    let string = String::from("Hello, World");
    f_a(&string);

    let s: &'static str = *STRING.lock().unwrap();

    println!("{s}");

    drop(string);

    println!("{s}");
}

(playground)

The problem is that coercing F to a subtype may switch out the FnOnce implementation to one that only produces a value that's legal as a subtype of T (which would have been coerced simultaneously).

Reading the result from the original LazyCell thus illegally coerces this value of a subtype of T back up into type T.

---

On the other hand, at least:

AFAICT, the argument for why making LazyCell<T, F> covariant (only) in F should be sound seems quite convincing.

[theemathas]

People have just found out that CovariantUnsafeCell has a bug.

It's not covariant.

https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Are.20variance.20changes.20non-breaking.3F/with/625487849

[asquared31415]

Filed #163051 to make CovariantUnsafeCell properly covariant

[steffahn]

Here is the same soundness issue demonstration as above, but without the CovariantLazyCell workaround, running “successfully” on a rustc that merges this PR into #163051:

#![feature(unboxed_closures)]
#![feature(fn_traits)]

use std::marker::PhantomData;

struct MyCustomFn<T>(PhantomData<T>);

type A = for<'a> fn(&'a str);
type B = fn(&'static str);

impl FnOnce<()> for MyCustomFn<A> {
    type Output = A;
    extern "rust-call" fn call_once(self, _: ()) -> Self::Output {
        println!("A");
        |_| {}
    }
}

static STRING: Mutex<&'static str> = Mutex::new("");

impl FnOnce<()> for MyCustomFn<B> {
    type Output = B;
    extern "rust-call" fn call_once(self, _: ()) -> Self::Output {
        println!("B");
        |s| {
            *STRING.lock().unwrap() = s;
        }
    }
}

use std::cell::LazyCell;
use std::sync::Mutex;

fn main() {
    let r_a: &LazyCell<A, MyCustomFn<A>> = &LazyCell::new(MyCustomFn::<A>(PhantomData));
    let r_b: &LazyCell<B, MyCustomFn<B>> = r_a; // <- coercion using covariance happens here
    let f_b: &B = LazyCell::force(r_b);
    let f_a: &A = LazyCell::force(r_a);

    let string = String::from("Hello, World");
    f_a(&string);

    let s: &'static str = *STRING.lock().unwrap();

    println!("{s}");

    drop(string);

    println!("{s}");
}

$ cargo +stage1 run
   Compiling play-cell v0.1.0 (/home/frank/playground/play_cell)
warning: conflicting implementations of trait `FnOnce()` for type `MyCustomFn<for<'a> fn(&'a str)>`
  --> src/main.rs:21:1
   |
11 | impl FnOnce<()> for MyCustomFn<A> {
   | --------------------------------- first implementation here
...
21 | impl FnOnce<()> for MyCustomFn<B> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `MyCustomFn<for<'a> fn(&'a str)>`
   |
   = note: this behavior recently changed as a result of a bug fix; see rust-lang/rust#56105 for details
   = warning: the behavior may change in a future release
   = note: for more information, see issue #56105 <https://github.com/rust-lang/rust/issues/56105>
   = note: `#[warn(coherence_leak_check)]` (part of `#[warn(future_incompatible)]`) on by default

warning: unused variable: `f_b`
  --> src/main.rs:37:9
   |
37 |     let f_b: &B = LazyCell::force(r_b);
   |         ^^^ help: if this is intentional, prefix it with an underscore: `_f_b`
   |
   = note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default

warning: `play-cell` (bin "play-cell") generated 2 warnings (run `cargo fix --bin "play-cell" -p play-cell` to apply 1 suggestion)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.18s
     Running `target/debug/play-cell`
B
Hello, World
S�С�j^�