Drop impl on Shared types is not called if dropped on C++ side

[sbrocket]

The C++ types generated for shared types are currently POD types, with only the trivial destructor. This means that if a shared type defined in a bridge has the Drop trait implemented, that Drop impl is only called if an instance of the shared type is dropped within Rust code, not within C++. Since there is nothing else that prevents implementing Drop for a shared type, this behavior is surprising and can lead to resource leaks or other unexpected behavior.

I believe we can fix this pretty easily by generating a destructor for shared types that calls std::ptr::drop_in_place, similar to the Box wrapper. This will make the generated shared types non-POD types, but they will still be standard layout and thus should be fine for our FFI usage. (Also, it probably wouldn't hurt to generate some std::is_standard_layout static asserts on the C++ side?)

I'm about to put up a PR that has a currently failing test which would pass once this issue is fixed. I can contribute a PR to fix this if the plan above sounds fine.

[sbrocket]

One issue with above is that it seems like this could lead to double drops if the shared struct contains a Box or another struct that impls Drop. Not quite sure how to avoid that.

It also seems like that could be a current issue, maybe something that's already been accounted for? How do we avoid double drop if you Box a shared struct that contains another Box?

[sbrocket]

Perhaps moving the object into a rust::ManuallyDrop in the destructor before passing it along to drop_in_place would do the job? I can't say I've ever written a destructor with std::move(*this) before so I'm not sure if that's problematic for reasons I haven't encountered before.

Actually, perhaps there isn't a way to make this work as expected at all, because moved-from variables are still destructed on the C++ side whereas they aren't on the Rust side. Consider a hypothetical CXX shared type that contains a single c_int field representing an "owned" file descriptor, for instance, with a Drop impl that closes the fd. FdType fd1{open("foo.txt")}; FdType fd2 = std::move(fd1); would result in the Drop impl being called twice with the proposal above, not once.

Any thoughts on how to make this work? It'd be nice to be able to create types on the FFI interface that 1) don't require heap allocation (Box/unique_ptr) and 2) can guarantee that owned resources are closed or cleaned up appropriately. But perhaps that's not possible and we should figure out how to block implementing Drop instead, to remove the footgun.

[dtolnay]

C++ doesn't have any equivalent of Drop, so the current behavior is intentional. C++ destructors are a different thing; emulating Drop using destructors would require storing a drop flag inside the data structure, which is incompatible with having a matching layout on both sides.

It'd be nice to be able to create types on the FFI interface that 1) don't require heap allocation (Box/unique_ptr) and 2) can guarantee that owned resources are closed or cleaned up appropriately.

ExternType is the way to do this -- along the lines of https://cxx.rs/extern-c++.html#integrating-with-bindgen-generated-or-handwritten-unsafe-bindings. You can handwrite a destructor for the drop logic on the C++ side, using a drop flag or some other sentinel value for the moved-from state.

[sbrocket]

C++ doesn't have any equivalent of Drop, so the current behavior is intentional. C++ destructors are a different thing; emulating Drop using destructors would require storing a drop flag inside the data structure, which is incompatible with having a matching layout on both sides.

It'd be nice to be able to create types on the FFI interface that 1) don't require heap allocation (Box/unique_ptr) and 2) can guarantee that owned resources are closed or cleaned up appropriately.

ExternType is the way to do this -- along the lines of https://cxx.rs/extern-c++.html#integrating-with-bindgen-generated-or-handwritten-unsafe-bindings. You can handwrite a destructor for the drop logic on the C++ side, using a drop flag or some other sentinel value for the moved-from state.

I meant a shared type that could meet those requirements, but sure I can make do with some manual ExternType bindings, I hadn’t thought of that.

We should prevent users from implementing Drop on shared types then IMO, to keep others from falling in the same trap. Perhaps if we just generated a static_assertions::assert_not_impl_any!(T: Drop) or equivalent for each shared type on the Rust side?