cxx icon indicating copy to clipboard operation
cxx copied to clipboard

Expose a nonconst reference wrapper binding

Open dtolnay opened this issue 4 years ago • 10 comments

This has very interesting implications for the SharedPtr and UniquePtr API. In Rust, it's not sound to expose a safe deref from &SharedPtr<T> to &mut T / Pin<&mut T> because the lifetime system is not expressive enough to rule out calling it twice, and calling it twice produces overlapping &mut references which is UB in Rust. However, in C++ it's not, so it's safe to expose &'a SharedPtr<T> to NonConstRef<'a, T> in Rust and have NonConstRef<T> become mutable T& on the C++ side, as long as there is no conversion NonConstRef<'a, T> to &'a mut T (&'a T is fine). This is a safe way to capture the semantics of T& std::shared_ptr<T>::operator*() const noexcept.

Needs design, but possibly we can tie this to https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper and piggy back on ReferenceWrapper<T>.

dtolnay avatar Dec 02 '20 20:12 dtolnay

Is using mutable keyword on C++ side a valid workaround for inability to call non-const methods behind SharedPtr?

vi avatar Jan 15 '21 06:01 vi

Yes, that is effectively equivalent to the semantics of Cell. You'd have to make sure to follow all the same restrictions as Cell, i.e. not expose references to the mutable field which outlive the method call and not implement Sync for the enclosing Rust type.

dtolnay avatar Jan 15 '21 07:01 dtolnay

Is there a reason to have this as a type rather than an attribute? I'm working with a library that uses a lot of cell-semantics nonconst referenes, and exposing them to Rust as &mut is very wrong due to the exclusivity guarantee. All I need is to be able to write something like

unsafe fn SetActorLocationAndRotation(
    #[nonconst] self: &AActor,
    NewLocation: FVector,
    // ...
) -> bool;

to drop the const-qualification. Anyone using these types is already well aware that they have Cell-semantic shared mutability, and adding any more ceremony to the reference would be needlessly verbose.

(EDIT: well, while #[nonconst] works for function arguments, it doesn't work for the return type, which is also important to be able to mark this way...)

CAD97 avatar May 09 '21 04:05 CAD97

Would using &Cell<T> or Pin<&Cell<T>> instead of Pin<&mut T> work? (Either explicitly written or generated by an attribute/macro.)

sollyucko avatar Aug 11 '21 14:08 sollyucko

@CAD97 #936 seems relevant. Ran into something similar myself and just ended up writing wrappers.

mattiekat avatar Oct 05 '21 23:10 mattiekat

I'll keep it in mind.

I'm mostly concerned about people accidentally stumbling into cases that use nonconst methods, potentially even in a non threadsafe manner (thus correctly nonconst per cxx's modern dialect of C++ it interfaces to), but which aren't compatible with Rust's exclusive ownership concept.

Especially with additional tooling such as autocxx, it's not that difficult to just use a Pin<&mut T> without realizing exactly what a restriction that puts onto the C++ side.

It's also not entirely decided what exactly exclusive ownership means w.r.t. FFI, as I understand it, especially when you have multiple layers of FFI boundaries passing &mut back and forth.

In these cases, it's much simpler to not assert language-level uniqueness on the Rust side.

It's fine for cxx to define the C++ dialect it works with as not including APIs with method receivers which can't be directly translated as &T, &mut T, or Pin<&mut T>. But the ease of getting it subtly but disastrously wrong scares me.

And at this point, I'm thinking, maybe it just should. There's no shortcut to triple checking your FFI invariants line up. cxx can automate a common subset of it, but you're still on the hook to make sure you match your FFI after adopting cxx's as well.

CAD97 avatar Oct 06 '21 00:10 CAD97

I've been playing around with this here (implementation here).

The ergonomics seem OK - no worse than Pin<&mut T> which it would be replacing. Unless I'm missing something.

More things to think about:

  • Can this indeed be a Rust peer to std::reference_wrapper<T>? Seems likely but I must admit I haven't thought that through in detail.
  • The whole point is that these references are non-exclusive, i.e. several may exist. Yet in normal usage we would prefer folks don't accidentally store or use several at the same time. We can encourage this by requiring any use of a NonConstRef<T> to actually require &mut NonConstRef<T>. If people really want to use several at the same time, then can always Clone the NonConstRef<T> - or of course get multiple such refs delivered to them from C++. So this is advisory at best but seems like it will avoid some footguns. In my sample code I have compromised - method calls require &mut but it was a bit awkward to require &mut when a function takes a NonConstRef as a parameter. More experimentation possibly required.
  • I wonder if this type should implement some trait CppRef which can later be implemented by various C++ smart pointer types too, e.g. to increment/decrement reference counts. Perhaps UniquePtr or SharedPtr themselves could implement such a trait. But, ideally, the receiver of void CppType::method() would be impl CppRef<CppType> and I don't think there's a way to do that. So this feels like a step too far.

adetaylor avatar Apr 08 '22 04:04 adetaylor

In Rust, it's not sound to expose a safe deref from &SharedPtr<T> to &mut T / Pin<&mut T> because the lifetime system is not expressive enough to rule out calling it twice

Would building RefCell semantics into cxx::SharedPtr to enable dynamically checked borrow rules work? The C++ side would manually have to uphold the invariant to not save incompatible references while Rust code is running, but on the Rust side we could ensure safety I think. Could cxx::SharedPtr add an extra word to track borrow state and define these instead of pub fn as_ref(&self)?

pub fn borrow(&self) -> Ref<'_, T>
pub fn borrow_mut(&self) -> RefMut<'_, Pin<T>>
pub fn try_borrow(&self) -> Option<Ref<'_, T>>
pub fn try_borrow_mut(&self) -> Option<RefMut<'_, Pin<T>>>

I don't think the original pub fn as_ref(&self) -> Option<&T> can remain unfortunately, since the dynamically checked mutable borrows would not statically prevent calling as_ref(). This approach adds runtime overhead and adds memory writes where there used to only be reads. However, I think it would make the container more usable overall. I'm curious what folks think of this approach.

dcsommer avatar Sep 26 '22 16:09 dcsommer

I don't think the original pub fn as_ref(&self) -> Option<&T> can remain unfortunately, since the dynamically checked mutable borrows would not statically prevent calling as_ref(). This approach adds runtime overhead and adds memory writes where there used to only be reads. However, I think it would make the container more usable overall. I'm curious what folks think of this approach.

Even worse than just adding writes, it adds atomic writes. And unlike RefCell, it's split across languages where compilers will have more trouble eliminating them. I can see value in having it as an option, but I don't think it's worth using it everywhere.

bsilver8192 avatar Sep 26 '22 18:09 bsilver8192

I also had the case where I wanted to make functionality available that was implemented with non-const C++ member functions, but which only performed read-actions. So, the natural representations in the Rust API I created needed to have &self parameters. Thus, no mutable access to Pin<&mut T> struct fields.

I created this Rust equivalent to C++'s const_cast:

pub(crate) unsafe fn cxx_const_cast<T: UniquePtrTarget>(value: &T) -> Pin<&mut T> {
    #![inline]
    //! Presents an immutable reference as a mutable one for the purpose of calling a CXX bridge
    //! function (casts the constness away). The mutable reference must not actually be mutated!
    //! (Otherwise, bring mutability into the Rust code.)
    //!
    //! This is meant as a last resort to avoid having to write a C++ wrapper function every
    //! time some API function isn't declared as `const` on the C++ side, even though it should
    //! be. In that wrapper, the same thing would be done with a C++ `const_cast<...>(...)`
    //! anyway.

    #[allow(clippy::cast_ref_to_mut)]
    Pin::new_unchecked(&mut *(value as *const T as *mut T))
}

It worked well. Later, I discovered this warning, however, which I don't fully understand, but which seems to be very relevant:

The assumption "If you run this... you get a segfault" suggests that the compiler is actually bound to compile the code to something meaningful, but incorrect given the values provided at runtime. In fact, very_bad_function may not be compiled to anything meaningful at all, as the optimizer may make assumptions about reachability based on the non-aliasing of &mut T. Breaking aliasing rules is not an extra reason "in addition to" the problem of writing to read-only memory -- it's the only reason why this code has undefined behavior...

https://stackoverflow.com/a/54242058/10749231

Perhaps, this could be solved by preventing such compiler optimizations?


I now have no need for the function anymore. I had a point where more and more lifetimes came into play with the Pin<&mut T>s I had in my structs, which brought be me into Pin hell. I viewed handling these lifetimes to be a very unnecessary hassle and shifted to using *mut T where T: UniquePtrTarget and a technique I call just-in-time pinning where I transform *mut T into either &T or Pin<&mut T> as the CXX bridge requires it.

Enyium avatar Jan 24 '23 20:01 Enyium