Prevent implementing both ArcCastPtr and BoxCastPtr on the same type

[jsha]

In https://github.com/rustls/rustls-ffi/pull/341#issuecomment-1745711553 we ran into a problem: both ArcCastPtr and BoxCastPtr were implemented for rustls_client_cert_verifier. This turned out fine because only the Box-related casts were used, but it could have turned out badly if, for instance, a pointer was created as a Box and later (incorrectly) cast to an Arc.

We should see if we can use the Rust type system to make it impossible to implement both of these for the same type.

Also, right now we have helper traits CastPtr / ConstConstPtr that are trait bounds for ArcCastPtr and BoxCastPtr. So for each C-facing type, we need to do two trait impls. We could probably simplify things by removing the trait bound, and instead adding a blanket impl of CastConstPtr for all ArcCastPtr and all BoxCastPtr; and a blanket impl of CastPtr for all BoxCastPtr. That way we would only need to impl one of ArcCastPtr / BoxCastPtr for any given Rust type:

today:

impl CastPtr for rustls_root_cert_store_builder {
    type RustType = Option<RootCertStoreBuilder>;
}

impl BoxCastPtr for rustls_root_cert_store_builder {}

future?

impl BoxCastPtr for rustls_root_cert_store_builder {
    type RustType = Option<RootCertStoreBuilder>;
}

[cpu]

We could probably simplify things by removing the trait bound, and instead adding a blanket impl of CastConstPtr for all ArcCastPtr and all BoxCastPtr;

I think this bumps into trait coherence issues. If we have:

impl<T, R> CastConstPtr for T
where
    T: ArcCastPtr<RustType = R>,
{
    type RustType = R;
}

impl<T, R> CastConstPtr for T
where
    T: BoxCastPtr<RustType = R>,
{
    type RustType = R;
}

Then the compiler rejects this as a conflicting implementation of CastConstPtr. I think because of the potential for a type that implements both BoxCastPtr and ArcCastPtr (precisely what we're trying to avoid) :thinking:

[cpu]

I spent a few hours banging my head against this and I think I made a bit of progress towards the goal of making ArcCastPtr and BoxCastPtr mutually exclusive w/ compiler verification (but haven't gotten there 100%).

To try and minimize my own confusion I've been implementing this separate from rustls-ffi so no guarantees this plan of attack will survive first encounter with the real codebase. Here's a playground with the approach I've been fiddling with, largely inspired by this StackOverflow post.

The major problem I'm still grappling with is how to implement BoxCastPtr::to_mut_ptr and ArcCastPtr::to_const_ptr. In both cases I'm running into a compile error about trying to cast a thin pointer to a fat pointer :thinking:

[jsha]

For the casting thin pointer to a fat pointer, I think you're running into the problem I described here: https://github.com/rustls/rustls-ffi/issues/350

I think there are two reasons you're running into that: one is the + ?Sized bound on the associated type; the other is thedyn CastPtrFromArc bound in trait ArcCastPtr<T>: CastPtr<CastType = dyn CastPtrFromArc<RustType = T>> {, which I assume implicitly introduces a ?Sized bound. That's because dyn means trait object, and trait objects are dynamically-sized types (DSTs), and DSTs require fat pointers.

I suspect the "double indirect" approach we reached in the other PR (Box<Arc<dyn Foo>>) may be the best we can do since C doesn't support a notion of fat pointers.

My recommendation for this issue is to separate the issue of representing trait objects (dyn Foo) from the issue of making ArcCastPtr / BoxCastPtr impossible to implement for the same type.

[cpu]

Another attempt: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=57e07a98c747a5310501c75c0de442ba (Type names haven't been given significant thought)

I think this might be more workable :crossed_fingers: Actually integrating it into the codebase will probably be the real test. In particular I haven't looked at the various macros from src/lib.rs to make sure they'll be able to carry over to this approach.

[jsha]

Nice. I think you're exactly on the right track. I tried rewriting it with free functions, because I think the impl ... dyn Castable thing winds up getting confusing (trait objects again!).

Here's my attempt:

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

As part of it I wound up adding free_box for consistency, and renaming free to free_arc. Similarly I made to_mut_ptr and set_mut_ptr into to_boxed_mut_ptr and set_boxed_mut_ptr.

[cpu]

Nice. I think you're exactly on the right track

Simplifying to the one Castable trait with two associated types was the main thing I think made this a lot easier for me to reason about :+1:

I tried rewriting it with free functions, because I think the impl ... dyn Castable thing winds up getting confusing (trait objects again!).

Ahhh yes. That's much simpler to work with. Thank you.

As part of it I wound up adding free_box for consistency, and renaming free to free_arc. Similarly I made to_mut_ptr and set_mut_ptr into to_boxed_mut_ptr and set_boxed_mut_ptr.

Good ideas, thanks again.

I rolled all of those changes into a PR that replaces the existing CastPtr, CastConstPtr, BoxCastPtr, and ArcCastPtr traits with the new formulation: https://github.com/rustls/rustls-ffi/pull/353

I also did my best to do a pass through all of the (crate internal) doc strings to try and make explicit some of the things I've learned with your help while working through some of this. PTAL!