Fix memory leaks caused by std::move on types with no move constructor

[jnbooth]

cxx uses the IsRelocatable struct to detect whether a C++ type is safe to move by memcpy. It permits only types that have trivial move constructors and no destructor. cxx-qt-lib and cxx-qt-lib-extras sometimes override that check, like so:

namespace rust {
template<>
struct IsRelocatable<MyType> : ::std::true_type
{};
}

This is a violation of cxx's assumptions, but it's valid if two conditions are met:

1. QTypeInfo<MyType>::isRelocatable must be true. This ensures memcpy is safe to use.
2. MyType must have a move constructor (i.e. MyType(MyType&& other)). This ensures that if the type is passed from Rust into C++ by value, C++ will take ownership of it, and vice versa.

The first condition can be checked at compile time with static_assert(QTypeInfo<MyType>::isRelocatable), which is exactly what cxx-qt-lib and cxx-qt-lib-extras do. However, as far as I know, there is no way to check the second condition at compile time. Due to this, cxx-qt-lib and cxx-qt-lib-extras declare IsRelocatable for several types that do not have move constructors: QFont, QMessageLogContext, QCommandLineOption, and QCommandLineParser. If any of these types are passed by value into C++, their reference counter will not decrement, which means they will never deallocate.

However, when those invalid IsRelocatable declarations are removed, another problem arises: it is no longer possible for Rust to bind to C++ functions that return those types, since those are also passing by value. That includes the FFI bindings we use for MyType::default(), MyType::clone(), and so on. The solution is to construct the types in-place using new (p) MyType(...), which is the C++ syntax for applying a constructor to the existing place in memory at pointer address p, rather than constructing a new value on the stack and returning it. (cxx makes heavy use of that syntax in generated code, which is how I learned about it.) So now common.h has a new constructor function:

// (CURRENT) Constructs a value on the stack and returns it.
template<typename T, typename... Args>
T
construct(Args... args)
{
  return T(std::forward<Args>(args)...);
}

// (ADDED) Constructs a value in-place at the specified pointer address.
template<typename T, typename... Args>
void
constructInPlace(T* uninit, Args... args)
{
  new (uninit) T(std::forward<Args>(args)...);
}

On the Rust side, those types now use the crate::util::new_in_place helper function to construct their values, which uses MaybeUninit under the hood.

pub(crate) unsafe fn new_in_place<T, F: FnOnce(*mut T)>(constructor: F) -> T
{
    let mut uninit = MaybeUninit::uninit();
    constructor(uninit.as_mut_ptr());
    unsafe { uninit.assume_init() }
}

So instead of this:

impl Clone for MyType {
    fn clone(&self) -> Self {
        ffi::mytype_clone(self)
    }
}

Corresponding implementations now do this:

impl Clone for MyType {
    fn clone(&self) -> Self {
        unsafe { new_in_place(|uninit| ffi::mytype_clone(uninit, self)) }
    }
}

It's a bit unwieldy, but I think it's the cleanest approach. The only other way to prevent memory leaks that I can think of is to wrap those types in UniquePtrs, but that would be a significant breaking change for those types and wouldn't really be an improvement anyway (in my opinion). This way, it's all kept internal behind the API.

EDIT: Also to note, this is how cxx handles it. When an initializer is defined in a bridge, this is the Rust side:

extern "C" {
void rust$cxxqtlib1$cxxbridge1$192$qfont_init_default(::QFont *return$) noexcept {
  ::QFont (*qfont_init_default$)() = ::rust::cxxqtlib1::construct;
  new (return$) ::QFont(qfont_init_default$());
}

And this is the C++ side:

    pub fn qfont_init_default() -> QFont {
        unsafe extern "C" {
            #[link_name = "rust$cxxqtlib1$cxxbridge1$192$qfont_init_default"]
            fn __qfont_init_default(__return: *mut QFont);

        }
        unsafe {
            let mut __return = ::cxx::core::mem::MaybeUninit::<QFont>::uninit();
            __qfont_init_default(__return.as_mut_ptr());
            __return.assume_init()
        }
    }

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (061eaad) to head (c58e778).

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1448   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           75        75           
  Lines        13455     13455           
=========================================
  Hits         13455     13455           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.