[workerd-cxx] Add kj::Maybe type to kj-rs #42

LordGoatius · 2025-07-03T21:28:36Z

No description provided.

harrishancock · 2025-07-18T09:51:05Z

kj-rs/tests/test-maybe.c++

+// Static var to return non-dangling pointer without heap allocating
+int64_t var = 14;
+
+// Returns a heap pointer


Nit: s/heap pointer/pointer-to-static/

harrishancock · 2025-07-18T09:58:52Z

kj-rs/maybe.rs

+    fn is_niche(value: &MaybeUninit<Self>) -> bool;
+}
+
+// In `kj`, null pointers are considered niche, and `kj::none` if null


It doesn't look like there's actually any specialization of Maybe<T*>. In fact, when I add this to kj/common-test.c++, it passes:

Maybe<int*> m = static_cast<int*>(nullptr); KJ_EXPECT(m != kj::none); auto* p = KJ_ASSERT_NONNULL(m); KJ_EXPECT(p == nullptr);

(And yes, this is the opposite semantics of Maybe<Own<T>>, /sigh.)

FWIW, grepping for Maybe<[^>]+\*> in our codebase yields no results, so it looks like we don't actually use Maybe<T*> anywhere. Which makes sense, because we should probably be using Maybe<T&>, or a raw pointer, in such cases.

I wonder if we should just static_assert in our generated code that no one is using Maybe<T*>, with an error message suggesting the author use Maybe<T&>? That would avoid having to deal with this case.

A static assert is reasonable, and I can remove the Rust treatment of T* as special.

We can also change the check_type function to give an error here, and recommend Maybe<T&>

harrishancock · 2025-07-18T11:05:17Z

kj-rs/own.rs

    }

+    /// This type allows me to expose the same API that [`NonNull`] does while maintaining the invariant


I feel like this sentence might have been cut off.

I think this was meant for the newtype I made to replace NonNull in the Own definition, it must've found its way there when I was shuffling things around for that.

harrishancock · 2025-07-18T11:08:45Z

kj-rs/own.rs


 assert_eq_size!(repr::Own<()>, [*const (); 2]);
 assert_eq_align!(repr::Own<()>, *const ());

+/// When we want to use an `Own`, we want the guarantee of being not null only
+/// in direct `Own<T>`, not Maybe<Own<T>>, and using a [`NonNull`] in `Own`
+/// but allowing Nulls for Niche Value Optimization is undefined behavior.


This makes me curious, do you know how Option<Box<T>> handles this? I would guess it encounters the same issues.

Option<Box<T>> being an enum means the Rust compiler knows it can optimize, but since Maybe is a struct we have to manually control this behavior. This makes me wonder if it's actually possible to use an enum... I'm going to do some testing on that side of things. I'm still inclined to say no, since using an enum may result in Rust optimizing things when we don't want it to. The current solution maintains absolute control over layout.

harrishancock · 2025-07-18T11:19:11Z

kj-rs/maybe.rs

+
+impl_maybe_item_for_has_niche!(crate::Own<T>, &T, &mut T, *const T, *mut T);
+impl_maybe_item_for_primitive!(
+    u8, u16, u32, u64, u128, i8, i16, i32, i64, i128, usize, isize, bool


f32, f64, and char, too?

I guess () wouldn't be representable in C++ (Maybe<void>).

harrishancock · 2025-07-18T11:25:35Z

gen/src/write.rs

@@ -1669,6 +1676,17 @@ fn write_kj_own(out: &mut OutFile, key: NamedImplKey) {
    );
 }

+// cxx boilerplate function for possible drop trait
+// TODO: Add asserts and function generation


I mention elsewhere that perhaps an assertion could be that no one is actually using Maybe<T*>.

harrishancock · 2025-07-18T11:27:29Z

macro/src/expand.rs

@@ -647,7 +650,7 @@ fn expand_cxx_function_shim(efn: &ExternFn, types: &Types) -> TokenStream {
    let call = if indirect_return {
        let ret = expand_extern_type(efn.ret.as_ref().unwrap(), types, true);
        setup.extend(quote_spanned! {span=>
-            let mut __return = ::cxx::core::mem::MaybeUninit::<#ret>::uninit();
+            let mut __return = ::cxx::core::mem::MaybeUninit::<#ret>::zeroed();


This was related to the non-deterministic test failure you mentioned, right?

I think it'd be really good to understand this better, since it looks like this affects much more than just the kj::Maybe integration.

Yeah, this is the change I made that fixed the non-deterministic test failure. It's probably possible to scope this change to only happen if the return type is Maybe, if necessary. I would like to understand the cause of the bug better as well, because this could be unnecessary, but it's the only thing that fixed it, or even got close.

It's possible this was from the incorrect Maybe pointer code too. I'm testing that right now.

harrishancock · 2025-07-18T11:31:58Z

syntax/check.rs

+        }
+        Type::Ptr(_ptr) => {
+            return;
+            // TODO: ("Reminder to check pointer type later")


Reminder to make sure these TODOs are addressed. Should we be calling check_type_ptr() and check_type_ref()? I see those two functions do exist.

I think until we have a good reason to not follow cxx's rules around pointer/reference types using those functions is probably best, yeah.

harrishancock · 2025-07-18T11:38:21Z

kj-rs/maybe.rs

+            if self.is_none() {
+                write!(f, "kj::None")
+            } else {
+                write!(f, "kj::Some({:?})", unsafe { self.some.assume_init_ref() })


Nit for discussion: Perhaps these should be lowercased, like kj::none (the value instead of its type kj::None) and kj::some() (the function instead of kj::Some, which doesn't actually exist)?

I tried to have it more accurately represent Rust naming conventions here, with capitalized variants, as if it was a Rust enum. That's ideally the way we want to work with Maybe, but it does raise questions about when to use what conventions, where, and how. Keeping it consistent with C++ is probably worth it, since that's where we're migrating from, but should that change in the future?

harrishancock · 2025-07-18T11:43:46Z

kj-rs/tests/test_maybe.rs

+    fn test_some_ptr() {
+        let maybe = ffi::return_maybe_ptr_some();
+        assert!(maybe.is_some());
+        assert!(!maybe.is_none());


Can we also assert that the pointed-to-value is indeed 14? I have a suspicion it won't be, due to my observation that there's no actual Maybe<T*> specialization.

Yeah, that's definitely a good idea.

LordGoatius force-pushed the jostler/2025-06-30-kj-maybe branch 6 times, most recently from e674fb2 to 2944364 Compare July 14, 2025 21:23

LordGoatius marked this pull request as ready for review July 16, 2025 17:42

LordGoatius requested review from mikea and harrishancock July 16, 2025 17:42

harrishancock approved these changes Jul 18, 2025

View reviewed changes

[workerd-cxx] add kj::Maybe to kj-rs and workerd-cxx

3990b31

LordGoatius force-pushed the jostler/2025-06-30-kj-maybe branch from 2944364 to 3990b31 Compare July 18, 2025 19:49

		}

		/// This type allows me to expose the same API that [`NonNull`] does while maintaining the invariant

[workerd-cxx] Add kj::Maybe type to kj-rs #42

Are you sure you want to change the base?

[workerd-cxx] Add kj::Maybe type to kj-rs #42

Uh oh!

Conversation

LordGoatius commented Jul 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!