Rc uses visibly behavior-changing specialization: is that okay?

Specifically, Rc and Arc are specialized to check pointer equality before calling Eq::eq when the contained T is Eq. This is correct when Eq is implemented correctly, but because Eq is a safe trait, it doesn't have to be implemented correctly.

Obviously, this can't be used (directly) for unsoundness, so it's not an unsound specialization. However, I seem to recall picking up a semi-formal rule that we aren't supposed to be using specialization to achieve results that strictly require specialization while it's still unstable, and this is a case where you can stably observe that stdlib is doing specialization.

This is documented ("If T also implements Eq (implying reflexivity of equality), two Arcs that point to the same allocation are always equal"), but it still feels kind of odd to me.

[playground]

12 Likes

I think the restriction is only where compilation requires specialization.

Because the main "implement clone by copying for Copy types" specialization is also observable in its results.

2 Likes

Interesting, where do we do this?

E.g. for Vec

struct LoudClone<'a> { r: &'a () }
impl Clone for LoudClone<'_> {
    fn clone(&self) -> Self {
        println!("cloning!");
        Self { r: self.r }
    }
}
// comment out this next line to change behavior
impl Copy for LoudClone<'static> {}

fn silence(x: Vec<LoudClone<'_>>) {
    x.clone();
}

fn main() {
    let local: () = ();
    let r = &local;
    silence(vec![LoudClone { r }, LoudClone { r }, LoudClone { r }]);
    // prints nothing!
}
3 Likes

All over the place.

use std::fmt::Debug;

struct NoisyClone<T>(T);

impl<T: Clone + Debug> Clone for NoisyClone<T> {
    fn clone(&self) -> Self {
        println!("before {:?}", self.0);
        let x = NoisyClone(self.0.clone());
        println!("after {:?}", x.0);
        x
    }
}

// Try removing this and see the difference
impl <T: Copy + Debug> Copy for NoisyClone<T> {}

fn main() {
    let x = [NoisyClone(1), NoisyClone(2)];
    let _ = x.clone();
    
    let mut y = [NoisyClone(0), NoisyClone(0)];
    y.clone_from_slice(&x);
    
    let v = vec![NoisyClone(3), NoisyClone(4)];
    let _ = v.clone();
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6e9d66c47bb8e8a1c9379904146e0f70

3 Likes

Incidentally, something I learned recently: both the Copy and Eq specializations are technically unsound – in the sense that types can be copied even if they're not Copy, or assumed reflexive even if they're not Eq. However, this only happens when a type is conditionally Copy or Eq depending on lifetimes, which is a pattern that doesn't really occur, so the practical consequences are limited.

11 Likes

I just realized that things like the array’s Clone implementation specializing on Copy can be used to specialize on Copy in third-party crates (in stable rust), too, e.g. with a helper function like this

pub fn is_copy<T>() -> bool {
    #[derive(Copy)]
    struct S<'a, T>(&'a Cell<bool>, PhantomData<T>);
    impl<T> Clone for S<'_, T> {
        fn clone(&self) -> Self {
            self.0.set(false);
            Self(self.0, self.1)
        }
    }
    let is_copy = Cell::new(true);
    let _ = [S(&is_copy, PhantomData::<T>)].clone();
    is_copy.get()
}

Rust Playground


Similarly for Eq:

pub fn is_eq<T>() -> bool {
    #[derive(Eq)]
    struct S<'a, T>(&'a Cell<bool>, PhantomData<T>);
    impl<T> PartialEq for S<'_, T> {
        fn eq(&self, _other: &Self) -> bool {
            self.0.set(false);
            true
        }
    }
    let is_eq = Cell::new(true);
    let r = Rc::new(S(&is_eq, PhantomData::<T>));
    let _ = r == r;
    is_eq.get()
}

Rust Playground

(yes, the allocation for the Rc is properly optimized away here)

5 Likes

Wow, cool! I think there should be a clippy lint for not deriving Clone when T: Copy

Cool trick! I'd pondered using it to make a clone_or_copy method that used Copy where possible, but hadn't gone the extra mile to getting specialization from it.

I think you can simplify it a bit to avoid the Cell:

fn is_copy<T>() -> bool {
    #[derive(Copy)]
    struct Magic<T> { is_copy: bool, _none: Option<T> }
    impl<T> Clone for Magic<T> {
        fn clone(&self) -> Self { Magic { is_copy: false, _none: None } }
    }
    [Magic::<T> { is_copy: true, _none: None }].clone()[0].is_copy
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=60f58326f64c33fd1e6d40f9d851577a


Actually, I might even use this in core. Because core won't let you specialize on Copy without already having the Clone bound, but this would let me do that as a runtime check!

4 Likes

Is there a reason you used Option<T> instead of PhantomData<T>? The optimizer will magic all of it away anyway, but why introduce that space at all?

It's a bit more complex than that, because you can have a case where derive generates the wrong bounds

But yes, linting that Clone is implemented as a copy when it can be is a good thing.

Wouldn't Copy also have the same problem?

I think you mean example like this:

struct Foo<T: Iterator> {
    item: T::Item,
}

Needs to use T::Item: Copy + Clone instead of T: Copy + Clone.

Probably not a good one. I just avoid PhantomData if I don't need to think about it -- and hey, no use that way :upside_down_face:

FWIW, your version crashes on the playground with is_copy::<[bool; 1_000_000_000]>() -- SIGKILL, maybe OOM? -- but that works with PhantomData. Not that this is a practical type, but if you do add anything like this to core, take care. :slight_smile:

2 Likes

Yeah, absolutely. The way I write code for core is very different from how I write it for random IRLO demos :upside_down_face:

3 Likes

It looks like you can do this with any trait actually. Unfortunately, it seems you can only check if the type implements such trait, but you may not use it (because Copy and Eq have no methods).

struct IsDebug<'a, T> {
    is_debug: &'a Cell<bool>,
    _marker: PhantomData<T>,
}

impl<T> Clone for IsDebug<'_, T> {
    fn clone(&self) -> Self {
        self.is_debug.set(false);
        IsDebug {
            is_debug: self.is_debug,
            _marker: PhantomData,
        }
    }
}
impl<T: Debug> Copy for IsDebug<'_, T> {}

fn is_it_though<T>() -> bool {
    let is_debug = Cell::new(true);
    let _ = [IsDebug::<T> {
        is_debug: &is_debug,
        _marker: PhantomData,
    }]
    .clone();
    is_debug.get()
}
struct Foo; 
println!("i32: {}", is_it_though::<i32>()); 
println!("Foo: {}", is_it_though::<Foo>());

prints:

i32: true
Foo: false

If Copy and Eq weren't mere marker traits they could leak specialization features in stable.

4 Likes

Oh, wow.. good observation!

That's not unfortunate, that's actually very fortunate because using any methods in specialized implementations still runs into lots of unsoundness in the current state of the specialization feature.

1 Like

Fortunate or unfortunate, it's not a coincidence. In the past, certain uses of specialization in the standard library have had to be modified – either removed, or cut down to an approved list of types – precisely because they could call methods on the trait being specialized on, and therefore were unsound.

1 Like