feat: Support ManuallyDrop, MaybeUninit and Box as simplified types for C only

[Hywan]

Fixes #406.

This PR declares std::mem::{ManuallyDrop, MaybeUninit} and std::boxed::Box as simplified types, for C only.

I hope this is correct. I'm a long time user of cbindgen, but it's my first contribution.

[emilio]

So... some of this looks fair enough, but I'm not sure about the interaction with C++. Consider:

#[repr(C)]
struct HasDtors {
   x: i32,
}

impl Drop for HasDtors { ... }

Where HasDtors has a non-trivial destructor in C++. A struct with a ManuallyDrop<HasDtors> member will run destructors in C++...

I suspect the correct behavior here is putting the field inside a single-field union in C++, but I'm not sure how to better represent that internally.

[Hywan]

I'm not sure I'm following you.

If the destructor is implemented in Rust, it must be called by Rust, not by C++. The user must take care of that carefully. Thoughts?

[emilio]

Well, implementing the destructor in both rust and C++ is a common pattern (so that you can effectively use the type from both languages). I wrote a bit on how we do that in here fwiw.

[Hywan]

Can we just say that in case of ManuallyDrop, the user should avoid implementing a destructor the C++ side?

[Hywan]

@emilio I'm reading your article. Thanks for the great writing! I understand that Box<T> cannot be mapped easily to *const T in C. For the moment, cbindgen generates code like typedef struct Box_T Box_T; instead. Is that correct?

[emilio]

Can we just say that in case of ManuallyDrop, the user should avoid implementing a destructor the C++ side?

Yeah, documenting it is an option. One problem is that it's not only implementing a destructor for that type, but for any transitive types it uses as members as well... Basically you need to document that using ManuallyDrop<T> means that you need std::is_trivially_destructible_v<T> in C++. But it makes me a bit uncomfortable to bake in that assumption in the code without an opt-in...

Maybe we could do ManuallyDrop / MaybeUninit when the language is C only? I think that's totally uncontroversial. For C++ I'd be more comfortable with an opt-in which acknowledges that the user is aware of the risks.

@emilio I'm reading your article. Thanks for the great writing! I understand that Box cannot be mapped easily to *const T in C. For the moment, cbindgen generates code like typedef struct Box_T Box_T; instead. Is that correct?

Yeah, see the discussion in #451. I think you can technically exclude the box and define your own typedefs in C, but that's not great.

Doing that kind of simplification automatically in C mode is probably sensible (that PR was closed by the reporter, who was using C++ and for C++ you can choose the behavior you want quite trivially currently).

For C++ it could be done automatically in "top-level" context (arguments / return types), but not members (at least without an opt-in). Implementing that last bit is a bit more work I suspect, so if "magic in C, opt-in in C++" (or "C-only") makes sense to you, I think that should be pretty easy on top of your current patch :)

[Hywan]

Maybe we could do ManuallyDrop / MaybeUninit when the language is C only?

That was one my idea too. Is it possible to do that with the current cbindgen API? What happens when we want a C++ binding: Do we emit an error?

I think that should be pretty easy on top of your current patch :)

Yes, correct. Box has a similar process than Cell I believe.

[emilio]

Maybe we could do ManuallyDrop / MaybeUninit when the language is C only?

That was one my idea too. Is it possible to do that with the current cbindgen API? What happens when we want a C++ binding: Do we emit an error?

I think keeping the current behavior to that regard is better. In C++ you can get the same behavior with template <typename T> using MaybeUninit = T;.

So I think you need to plumb a config: &Config argument through simplify_standard_types (the library.rs caller has access to it), and just do something like:

"ManuallyDrop" | "MaybeUninit" if config.language == Language::C => Some(generic),

in simplified_type.

[Hywan]

I had to update the test “framework” to support the .skip_cpp suffix, following the example of the .skip_warning_as_error suffix. The code has been refactored a little bit, see 7571296 for more explanations and details.

ManuallyDrop and MaybeUninit are simplified only when C binding is targeted, see 8e42a71, where we inject &Config everywhere it is required. The immediate consequence of that is that we can drop the .cpp test artifact for those types, see ece9627.

Finally, Box<T> is simplified to *T for C only (excluding Swift!), see d925991.

[emilio]

This looks good, thanks! Instead of skip_cpp, you can just say language = "C" in the relevant toml file. Or even better, you can just make it work with C++.

I think the swift_name_macro check can also go... I'll do both of those before merging. Thanks!

[emilio]

I think the swift_name_macro check is not correct... Can you elaborate on why you added it?

[Hywan]

I wasn't sure it was correct. I had no idea what swift_name_macro is used for!

[emilio]

I merged this in 1e6234f...1fc4cb0. My two tweaks are 5d1217c and 0b9f90b.

Thanks for the patches, and for bearing with me! :)

[Hywan]

Thanks for the fast merge and reviews!