Bindings for Option and std::optional #1

kuecks · 2023-10-12T22:03:43Z

Transferring over from dtolnay#897

kuecks · 2023-10-18T17:11:36Z

include/cxx.h

+
+template <typename T>
+Option<T>::~Option() noexcept {
+  this->drop();


I'm not sure if I could get away with:

if (inner.empty != 0) { inner.value.~T() }

(or something like that at least) or really what the implications of that are

This bit is tricky. For Option<T>::Option(Option&& other), you're missing some calls to destructors (or assuming a previously constructed object. This works if the type T is trivially constructible, but is potentially UB if we ever relax that constraint.

inner.value = std::move(other.inner.value);

This assumes that value was previously constructed, which is not the case as value is a member of a union. Use new (&inner.value) T(std::move(other.inner.value)) instead.

other.inner.empty = 0;

This does not call the destructor of other.inner.value, which is potentially UB. Use reset/drop here.

If you introduce a construct private helper, you'll be able to use that across multiple places (e.g. if you plan to expose an assign member function. You might find folly::Optional as good inspiration here.

template <typename T> Option<T>::Option(Option&& other) noexcept { if (other.has_value()) { construct(std::move(other.inner.value)); other.reset(); } } template <typename T> Option<T>::~Option() noexcept { this->reset(); } template <typename T> Option<T>::reset() noexcept { this->drop(); } template <typename... Args> Option<T>::construct(Args&&... args) noexcept { new (&inner.value) T(std::forward<Args>(args)...); assert(innner.empty != 0); }

Option<T>::Option(Option&& other) is a constructor so this doesn't exist yet, but you are correct for (currently missing) operator=(Option&&). Thanks for the placement new, i definitely dont understand that but other bindings are doing something similar

kuecks · 2023-10-18T17:12:49Z

src/rust_option.rs

+#[cfg(feature = "alloc")]
+impl<T: Sized> OptionType for Box<T> {}
+
+impl<T> RustOption<T> {


Oops all of these are supposed to have T: OptionType

kuecks · 2023-10-18T17:15:00Z

tests/ui_build/option_not_sized.stderr

+ --> src/rust_option.rs
+  |
+  |                 assert!(core::mem::size_of::<Option<U>>() == core::mem::size_of::<usize>());
+  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: core::mem::size_of::<Option<U>>() == core::mem::size_of::<usize>()', $DIR/src/rust_option.rs:38:17


This one is annoying because the error message depends on the line of code in rust_option.rs. Ideally we could ignore the line number? Not sure if that is possible

kuecks · 2023-10-18T17:24:34Z

src/rust_option.rs

+
+// ABI compatible with C++ rust::Option<T> (not necessarily core::option::Option<T>).
+#[repr(C)]
+pub struct RustOption<T: OptionType> {


we need the bound on the struct itself (rather than just the impls) because we need it for the Drop impl

capickett

Looks great - the change request is mostly for UB in the C++ bindings, otherwise I have non-blocking questions.

Additionally, how should we handle documentation? I noticed that there's no doc page for Option in this PR. Should we maintain a page, similar to the rest of the bindings? https://cxx.rs/bindings.html

tests/ui/option_string.stderr

capickett · 2023-10-19T17:40:08Z

tests/ffi/tests.h

+rust::Option<const Shared*> c_return_rust_ref_option_shared();
+rust::Option<Shared*> c_return_rust_mut_option_shared();
+rust::Option<Shared*> c_return_rust_pin_mut_option_shared();
+rust::Option<const C*> c_return_rust_ref_option_opaque();
+rust::Option<C*> c_return_rust_pin_mut_option_opaque();
+rust::Option<const uint8_t*> c_return_rust_ref_option_native();
+rust::Option<uint8_t*> c_return_rust_mut_option_native();
+rust::Option<uint8_t*> c_return_rust_pin_mut_option_native();


I know that std::optional does not support optional<T&>. However, since we are providing our own type, we can support rust::Option<const Shared&> etc, which would provide a nice ergonomics benefit

I think that didn't work but I can try again

@kuecks to do this, you'll need a specialization for rust::Option<T&> that uses a raw pointer as the field in storage, but otherwise exposes T& APIs. I'm also okay if you'd like to punt on this feedback for later / separate PR / never.

Hrmm rather than that, maybe it would make more sense to introduce a new type, rust::OptionRef<T>. Potentially rust::Option<T> would also be renamed to rust::OptionBox<T> as well, or maybe I can restrict rust::Option<T> to only exist when T == Box

rust::OptionRef<T> seems reasonable as well. There's prior art for that in other codebases:

Chromium https://chromium.googlesource.com/chromium/src/base/+/refs/heads/main/types/optional_ref.h

TypeSafe https://github.com/foonathan/type_safe/blob/main/include/type_safe/optional_ref.hpp

Some more discussion here https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p1683r0.html

capickett · 2023-10-19T17:42:37Z

syntax/parse.rs

+                } else if ident == "Option" {
+                    if generic.args.len() == 1 {
+                        if let GenericArgument::Type(arg) = &generic.args[0] {
+                            let inner = parse_type(arg)?;
+                            return Ok(Type::RustOption(Box::new(Ty1 {
+                                name: ident,
+                                langle: generic.lt_token,
+                                inner,
+                                rangle: generic.gt_token,
+                            })));
+                        }
+                    } else {
+                        panic!("{}", generic.args.len());
+                    }


Panic here seems inappropriate, and inconsistent with the patten employed by the rest of the types.

Suggested change

} else if ident == "Option" {

if generic.args.len() == 1 {

if let GenericArgument::Type(arg) = &generic.args[0] {

let inner = parse_type(arg)?;

return Ok(Type::RustOption(Box::new(Ty1 {

name: ident,

langle: generic.lt_token,

inner,

rangle: generic.gt_token,

})));

}

} else {

panic!("{}", generic.args.len());

}

} else if ident == "Option" && generic.args.len() == 1 {

if let GenericArgument::Type(arg) = &generic.args[0] {

let inner = parse_type(arg)?;

return Ok(Type::RustOption(Box::new(Ty1 {

name: ident,

langle: generic.lt_token,

inner,

rangle: generic.gt_token,

})));

}

capickett · 2023-10-19T17:46:40Z

src/rust_option.rs

+mod private {
+    pub trait Sealed {}
+}
+pub trait OptionType: private::Sealed {}


nit-pick on the naming: Calling this OptionTarget would keep it consistent with UniquePtrTarget and SharedPtrTarget.

capickett · 2023-10-19T17:55:25Z

include/cxx.h

+  Option(Option&&) noexcept;
+  Option(T&&) noexcept;
+  ~Option() noexcept;
+  void drop() noexcept;


Looking at other types, like rust::String and rust::Vec, we usually expose "C++-like" APIs. I would call this reset() instead, to align with https://en.cppreference.com/w/cpp/utility/optional

capickett · 2023-10-19T18:06:47Z

include/cxx.h

+
+template <typename T>
+Option<T>::~Option() noexcept {
+  this->drop();


This bit is tricky. For Option<T>::Option(Option&& other), you're missing some calls to destructors (or assuming a previously constructed object. This works if the type T is trivially constructible, but is potentially UB if we ever relax that constraint.

inner.value = std::move(other.inner.value);

This assumes that value was previously constructed, which is not the case as value is a member of a union. Use new (&inner.value) T(std::move(other.inner.value)) instead.

other.inner.empty = 0;

This does not call the destructor of other.inner.value, which is potentially UB. Use reset/drop here.

If you introduce a construct private helper, you'll be able to use that across multiple places (e.g. if you plan to expose an assign member function. You might find folly::Optional as good inspiration here.

template <typename T> Option<T>::Option(Option&& other) noexcept { if (other.has_value()) { construct(std::move(other.inner.value)); other.reset(); } } template <typename T> Option<T>::~Option() noexcept { this->reset(); } template <typename T> Option<T>::reset() noexcept { this->drop(); } template <typename... Args> Option<T>::construct(Args&&... args) noexcept { new (&inner.value) T(std::forward<Args>(args)...); assert(innner.empty != 0); }

capickett · 2023-10-19T18:42:45Z

include/cxx.h

+
+template <typename T>
+const T *Option<T>::operator->() const {
+  return &inner.value;


Might be nice to add some debug check for has_value() where you assume value is set.

do you know how to do debug checks portably? I can't tell if I can rely on NDEBUG

An assert() would be good enough IMO.

kuecks · 2023-10-20T02:18:01Z

Looks great - the change request is mostly for UB in the C++ bindings, otherwise I have non-blocking questions.

Additionally, how should we handle documentation? I noticed that there's no doc page for Option in this PR. Should we maintain a page, similar to the rest of the bindings? https://cxx.rs/bindings.html

Yeah I was waiting for review on the implementation before I cluttered up the pr with documentation, but I will definitely be adding that in the next stage. Would definitely be nice if we can maintain a version of cxx.rs

kuecks · 2023-10-26T03:52:16Z

include/cxx.h

+  void reset();
+  void set(T&&) noexcept;
+private:
+  void* empty_;


In this update, I decided to never access the union directly from C++ and instead always access it in Rust. I then realizes that means I don't need the union at all on the cpp side? But maybe it's more clear if I keep it regardless

kuecks · 2024-01-17T21:33:01Z

There are 2 commits now, the first explicitly does not support Option<&Vec> and Option<&String> and the second adds support for those

kuecks · 2024-01-17T21:34:54Z

src/rust_option.rs

+
+    pub fn from_ref_option_vec_ref(v: Option<&Vec<T>>) -> Self {
+        let _: () = assert_option_safe::<&Vec<T>>();
+        Self::from_option_ref(unsafe { core::mem::transmute::<Option<&Vec<T>>, Option<&RustVec<T>>>(v) })


Changes like these are the scariest to me, but I think they are ok? I was looking at the RustVec implementation for guidance: https://github.com/dtolnay/cxx/blob/master/src/rust_vec.rs#L29

kuecks · 2024-01-24T17:58:31Z

gen/src/write.rs

+
+    writeln!(out, "template <>");
+    begin_function_definition(out);
+    writeln!(out, "void Option<{0}>::set({0}&& value) noexcept {{", inner);


This rvalue reference also seemed sketchy to me, I think it would probably be best to just do everything by value since we know we are just dealing with pointers on the C++ side anyway? Maybe this works though

kuecks · 2024-01-24T18:01:17Z

macro/src/expand.rs

+        OptionInner::RustBox(_) => quote! {
+            #[doc(hidden)]
+            #[export_name = #link_set]
+            unsafe extern "C" fn #local_set #impl_generics(this: *mut ::cxx::private::RustOption<#ty>, value: *mut ::core::mem::MaybeUninit<#ty>) {


Here is the actual implementation side of the spooky rvalue reference I mention above, it causes me to take in something line *mut MaybeUninit<&T> which feels weird. Likely I should just be taking in MaybeUninit<&T> (by passing the pointer by value)

capickett

Amazing work - my main thought was around how we might support passing Optional<i32> etc over FFI, where the primitive is held inside the option by value. But that is not blocking IMO, and we can start here.

capickett · 2024-02-14T17:07:08Z

include/cxx.h

+  Option(Option&&) noexcept;
+  Option(T&&) noexcept;


Do you want to support copy operations as well? If not, explicitly deleting them can provide a nicer compiler diagnostic:

// Option is not copyable Option(const Option&) = delete; Option(Option&&) noexcept; Option(T&&) noexcept;

Or if you do, I would add them here:

Option(const Option&) noexcept; Option(Option&&) noexcept; Option(const T&) noexcept; Option(T&&) noexcept;

capickett · 2024-02-14T17:07:24Z

include/cxx.h

+  T *operator->();
+  T &operator*();
+
+  Option<T>& operator=(Option&&) noexcept;


Same comment as earlier with copy constructor

capickett · 2024-02-14T17:27:30Z

src/cxx.cc

+  template <>                                                                  \
+  void Option<CXX_TYPE *>::set(CXX_TYPE *&& value) noexcept {                  \
+    return cxxbridge1$rust_option$##RUST_TYPE##$set(this, std::move(value));   \
+  }    \


I was a bit surprised to see Option<CXX_TYPE *> here. Do we support rust::Option<int32_t> for instance? Or would it have to instead be rust::Option<int32_t *>? It feels a bit odd to see the latter coming from other c++ codebases

No only pointers because we rely on the niche optimization :( I will try to support Option for builtin types like ints and bools in a follow up but I have no idea if it's possible so no promises

capickett · 2024-02-14T17:32:50Z

src/rust_option.rs

+        }
+    }
+
+    /// SAFETY: Pointer must have been constructed as a Box


I'd expand on these safety comments slightly. self must have been constructed as a Option<Box<T>>. Later on, the "Pointer must not be aliased and must have been constructed as an Option<&mut T>"

capickett · 2024-02-14T17:36:21Z

src/rust_option.rs

+{
+    pub fn from_option_ref(other: Option<&'a T>) -> Self {
+        let _: () = assert_option_safe::<&T>();
+        unsafe { core::mem::transmute::<Option<&'a T>, RustOption<&'a T>>(other) }


Earlier on your documented "ABI compatible with C++ rust::Option (not necessarily core::option::Option)." but here and throughout I see transmutations between RustOption and Option. Is the documentation wrong / imprecise?

This documentation is the same as for Vec which does the same kind of transmutes. It was explained to me as the layout is the same for our RustOption and std::option::Option, but std::option::Option doesn't necessarily have the same calling convention as RustOption. That is, it could be passed in via registers instead of on the stack which would break the C++ side and is part of the ABI contract

kuecks · 2024-02-26T19:29:19Z

Added docs to the book, see also the link to the option docs in the nav pane on the left

capickett · 2024-02-27T18:28:34Z

@kuecks can you add a note for the &Option<T> vs Option<&T> restriction? Otherwise LGTM!

kuecks changed the title ~~mvp~~ Bindings for Option and std::optional Oct 13, 2023

facebook-github-bot added the cla signed label Oct 18, 2023

kuecks commented Oct 18, 2023

View reviewed changes

capickett requested changes Oct 19, 2023

View reviewed changes

kuecks commented Oct 26, 2023

View reviewed changes

kuecks commented Jan 17, 2024

View reviewed changes

kuecks commented Jan 24, 2024

View reviewed changes

capickett approved these changes Feb 14, 2024

View reviewed changes

kuecks added 2 commits February 27, 2024 09:34

Add bindings for Rust std::option::Option

da8358f

Add support for Option<&String> and Option<&Vec>

9b92578

[Option] Add book documentation

db3ae28

kuecks merged commit fd03e02 into facebookexperimental:master Feb 28, 2024
16 checks passed

Bindings for Option and std::optional #1

Bindings for Option and std::optional #1

Conversation

kuecks commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capickett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capickett Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuecks commented Oct 20, 2023

Choose a reason for hiding this comment

kuecks commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capickett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuecks commented Feb 26, 2024

capickett commented Feb 27, 2024

capickett Oct 19, 2023 •

edited

Loading