Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code broke with crossbeam-utils 0.8.10. Worked in 0.8.9. Not a ZST. #854

Closed
rickwebiii opened this issue Jun 24, 2022 · 2 comments
Closed

Comments

@rickwebiii
Copy link

rickwebiii commented Jun 24, 2022

I discovered that my code was failing to build after running cargo update. The root cause is due to MR 834 and I think there's a teaching moment for me to understand why this change broke my code. Here is a minimal repro of code that compiles (don't run it, it will panic due to bounds checks) using AtomicCell:

use crossbeam_utils::atomic::AtomicCell;
use std::borrow::Cow;

#[derive(Clone)]
struct Data {
    // Don't be a ZST
    val: u8
}

fn fn_2<F>(f: F)
where 
    F: Fn(i64) -> Result<(), ()> + Sync + Send {
}

fn get_data<'a>(data: &'a [AtomicCell<Cow<Data>>], i: usize) -> &'a Data {
    unsafe { data[i].as_ptr().as_ref().unwrap() }
}

fn main() {
    let data: Vec<AtomicCell<Cow<Data>>> = vec![];

    fn_2(|i| {
        data[0].store(Cow::Owned(Data { val: 0 }));

        let a = get_data(&data, 0);

        data[1].store(Cow::Borrowed(a));

        Ok(())
    });

    let _ = &data[0];
}

If you use crossbeam-utils = "=0.8.9" in your Cargo.toml, this code compiles (with warnings). If you update the version to =0.8.10, you get this error:

error[E0597]: `data` does not live long enough
  --> src/main.rs:24:9
   |
23 |     fn_2(|i| {
   |          --- value captured here
24 |         data[0].store(Cow::Owned(Data { val: 0 }));
   |         ^^^^ borrowed value does not live long enough
...
34 | }
   | -
   | |
   | `data` dropped here while still borrowed
   | borrow might be used here, when `data` is dropped and runs the `Drop` code for type `Vec`

The code is kinda sketchy, but the idea is some entries in the Vec are values while other entries are references to other values in the same Vec. While this approach is likely a bad idea (you can get dangling references when data is dropping) and I've since switched from Cow to Arc, I do want to know why compilation broke.

Reading the MR, there was discussion on whether to do a minor or major semver bump and that people using ZSTs would be broken. But the above repro doesn't use a ZST. I'm curious why this code breaks without using a zero sized type and whether this is actually a bug in the Rust compiler.

@taiki-e
Copy link
Member

taiki-e commented Jun 24, 2022

Sorry for the breakage! This is due to the addition of the Drop implementation to the AtomicCell. (See nomicon's this page for details)

We were aware of the possibility of breakage due to the Drop implementation, but I considered this a minor change based on the fact that the RFC1105 does not consider this to be a major change:

Adding Drop implementation is also a breaking change, but RFC1105 treats only the "fundamental" trait implementation as a major change and all others as minor changes (Drop is not fundamental trait).


Reading the MR, there was discussion on whether to do a minor or major semver bump and that people using ZSTs would be broken.

The main discussion in #834 was about DSTs (Dynamically Sized Types), not ZSTs (Zero Sized Types).

@taiki-e taiki-e pinned this issue Jun 24, 2022
@rickwebiii
Copy link
Author

The drop check makes total sense, and the example in the Nomicon is eerily similar to what I was doing. You all did everything by the book, so c'est la vie. We'll adopt a different strategy on our end. The understanding is all I really wanted out of this.

Thanks!

@taiki-e taiki-e unpinned this issue Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants