cpp_api_from_rust: NonNull<Ptr> -> pointer with crubit_nonnull in generated C++.

cc_bindings_from_rs/generate_bindings/BUILD

@@ -57,6 +57,7 @@ crubit_rust_test(
         "//cc_bindings_from_rs:run_compiler_test_support",
         "//cc_bindings_from_rs/generate_bindings/database",
         "//common:code_gen_utils",
+        "//common:crubit_feature",
         "//common:token_stream_matchers",
         "@crate_index//:proc-macro2",
         "@crate_index//:quote",  # v1

cc_bindings_from_rs/generate_bindings/format_type.rs

@@ -339,6 +339,37 @@ fn format_legacy_bridged_type_with_placeholders<'tcx>(
         start_idx = absolute_start + end + 1;
     }
 
+    // If the bridged wrapper type itself is annotated with `cpp_move_constructible=`,
+    // it means its C++ move constructor is unconditionally available (e.g. pointer/heap
+    // wrappers like `std::unique_ptr<T>` and `std::vector<T>` only transfer internal
+    // pointers upon move and never invoke `{T}`'s move constructor).
+    //
+    // NOTE: We also check for standard pointer/heap wrappers (`unique_ptr`, `shared_ptr`,
+    // `vector`) as a temporary fallback until the compiler rollout containing
+    // `cpp_move_constructible` reaches stable Crosstool, at which point `support/cc_std_impl`
+    // can be annotated directly without breaking the stable compiler on targets using `cc_std`.
+    //
+    // TODO(b/545883191): When `cpp_move_constructible` is in crosstool stable clean these up and
+    // annotate the types in `support/cc_std_impl` directly.
+    let is_unconditionally_cpp_movable = crubit_attr::get_attrs(tcx, adt.did())
+        .map(|attrs| attrs.cpp_move_constructible)
+        .unwrap_or(false)
+        || cpp_type_str.contains("unique_ptr")
+        || cpp_type_str.contains("shared_ptr")
+        || cpp_type_str.contains("vector")
+        // Unlike the three above, `NonNull<Ptr>` is *not* unconditionally movable: in C++ it is
+        // spelled as `Ptr` plus an attribute, so its movability is exactly `Ptr`'s. Exempting it
+        // is only sound because the check it suppresses would itself wrongly fail, as `Ptr` is
+        // always a `cc_std_impl` smart pointer that cannot carry `cpp_move_constructible` yet.
+        // Both halves of that go away together.
+        || cpp_type_str.contains("crubit_nonnull");
+    let is_passed_by_value = matches!(
+        location,
+        TypeLocation::FnReturn { is_constructor: false }
+            | TypeLocation::FnParam { .. }
+            | TypeLocation::NestedBridgeable
+    );
+
     for (param, subst) in generics.own_params.iter().zip(substs.iter()) {
         let ty::GenericArgKind::Type(ty) = subst.kind() else {
             continue;
@@ -348,29 +379,8 @@ fn format_legacy_bridged_type_with_placeholders<'tcx>(
         if !result_str.contains(&placeholder) {
             continue;
         }
-        // If the bridged wrapper type itself is annotated with `cpp_move_constructible=`,
-        // it means its C++ move constructor is unconditionally available (e.g. pointer/heap
-        // wrappers like `std::unique_ptr<T>` and `std::vector<T>` only transfer internal
-        // pointers upon move and never invoke `{T}`'s move constructor).
-        //
-        // NOTE: We also check for standard pointer/heap wrappers (`unique_ptr`, `shared_ptr`,
-        // `vector`) as a temporary fallback until the compiler rollout containing
-        // `cpp_move_constructible` reaches stable Crosstool, at which point `support/cc_std_impl`
-        // can be annotated directly without breaking the stable compiler on targets using `cc_std`.
-        let is_unconditionally_cpp_movable = crubit_attr::get_attrs(tcx, adt.did())
-            .map(|attrs| attrs.cpp_move_constructible)
-            .unwrap_or(false)
-            // TODO(b/545883191): When `cpp_move_constructible` is in crosstool stable clean these
-            // up and annotate the types in `support/cc_std_impl` directly.
-            || cpp_type_str.contains("unique_ptr")
-            || cpp_type_str.contains("shared_ptr")
-            || cpp_type_str.contains("vector");
-        if matches!(
-            location,
-            TypeLocation::FnReturn { is_constructor: false }
-                | TypeLocation::FnParam { .. }
-                | TypeLocation::NestedBridgeable
-        ) && !is_unconditionally_cpp_movable
+        if is_passed_by_value
+            && !is_unconditionally_cpp_movable
             && !db.is_cpp_move_constructible(ty)
         {
             bail!(
@@ -2052,6 +2062,19 @@ fn is_manually_annotated_bridged_adt<'tcx>(
     };
     let attrs = crubit_attr::get_attrs(db.tcx(), adt.did())
         .unwrap_or_else(|e| panic!("Invalid attrs for {ty}: {e}"));
+
+    // `NonNull<Ptr>` is spelled `Ptr crubit_nonnull`. Gate that on `nonnull_smart_pointers`, the
+    // same feature that lets `rs_bindings_from_cc` produce `NonNull` from an `_Nonnull`-annotated
+    // smart pointer, so a target opts into both directions of the bridge at once. Without the
+    // feature, treat `NonNull` as non-bridged, which is how it behaved before it was annotated.
+    if attrs.cpp_type.is_some_and(|cpp_type| cpp_type.as_str().contains("crubit_nonnull"))
+        && !db
+            .crate_features(db.source_crate_num())
+            .contains(crubit_feature::CrubitFeature::NonnullSmartPointers)
+    {
+        return Ok(None);
+    }
+
     let Some(bridging_attrs) = attrs.get_bridging_attrs()? else {
         return Ok(None);
     };

cc_bindings_from_rs/generate_bindings/format_type_test.rs

@@ -5,13 +5,16 @@
 #![feature(rustc_private)]
 
 use code_gen_utils::format_cc_includes;
-use database::TypeLocation;
+use database::{BindingsGenerator, TypeLocation};
 use proc_macro2::TokenStream;
 use query_compiler::liberate_and_deanonymize_late_bound_regions;
 use quote::quote;
 use run_compiler_test_support::{find_def_id_by_name, run_compiler_for_testing};
 use rustc_middle::ty::{Ty, TyCtxt};
-use test_helpers::{bindings_db_for_tests, bindings_db_for_tests_with_ignore_symbols_from_files};
+use test_helpers::{
+    bindings_db_for_tests, bindings_db_for_tests_with_features,
+    bindings_db_for_tests_with_ignore_symbols_from_files,
+};
 use token_stream_matchers::assert_cc_matches;
 
 fn test_ty<TestFn, Expectation>(
@@ -810,6 +813,10 @@ fn unique_ptr_preamble() -> TokenStream {
                 #[doc="CRUBIT_ANNOTATE: cpp_type = ::std::unique_ptr<{T}>"]
                 #[doc="CRUBIT_ANNOTATE: include_path = <memory>"]
                 pub struct virtual_unique_ptr<T: crate::operator::Delete>(pub *mut T);
+
+                #[doc="CRUBIT_ANNOTATE: cpp_type = {Ptr} crubit_nonnull"]
+                #[doc="CRUBIT_ANNOTATE: include_path = <crubit/support/annotations_internal.h>"]
+                pub struct NonNull<Ptr>(pub Ptr);
             }
         }
     }
@@ -884,6 +891,71 @@ fn test_format_ty_for_cc_unique_ptr_with_delete_fails() {
     );
 }
 
+/// The `NonNull` bridge is gated on `nonnull_smart_pointers`, which is not in the default test
+/// feature set.
+fn nonnull_bindings_db_for_tests(tcx: TyCtxt<'_>) -> BindingsGenerator<'_> {
+    bindings_db_for_tests_with_features(
+        tcx,
+        crubit_feature::CrubitFeature::Experimental
+            | crubit_feature::CrubitFeature::Supported
+            | crubit_feature::CrubitFeature::NonnullSmartPointers,
+        /* with_kythe_annotations= */ false,
+        None,
+    )
+}
+
+/// `NonNull<Ptr>` is spelled as `Ptr` plus an attribute, so the outer ADT is no longer
+/// `unique_ptr` and the `Delete` guard cannot match on it directly. The guard must still fire via
+/// the recursion that formats `{Ptr}`.
+#[test]
+fn test_format_ty_for_cc_nonnull_unique_ptr_with_delete_fails() {
+    test_ty(
+        TypeLocation::FnParam { is_self_param: false, elided_is_output: false },
+        &[(
+            "cc_std::std::NonNull<cc_std::std::unique_ptr<StructWithDelete>>",
+            "`cc_std::std::unique_ptr<StructWithDelete>` has no layout-compatible C++ type, \
+             but is used as a generic parameter\n  crubit.rs/errors/delete: \
+             `StructWithDelete` implements the `Delete` trait and cannot be used in a \
+             `unique_ptr`. Use `virtual_unique_ptr` instead.",
+        )],
+        unique_ptr_preamble(),
+        |desc, tcx, ty, expected_err| {
+            let db = nonnull_bindings_db_for_tests(tcx);
+            let anyhow_err = db
+                .format_ty_for_cc(
+                    ty,
+                    TypeLocation::FnParam { is_self_param: false, elided_is_output: false },
+                )
+                .expect_err(&format!("Expecting error for: {desc}"));
+            let actual_err = format!("{anyhow_err:#}");
+            assert_eq!(&actual_err, *expected_err, "{desc}");
+        },
+    );
+}
+
+#[test]
+fn test_format_ty_for_cc_nonnull_virtual_unique_ptr_with_delete_succeeds() {
+    test_ty(
+        TypeLocation::FnParam { is_self_param: false, elided_is_output: false },
+        &[(
+            "cc_std::std::NonNull<cc_std::std::virtual_unique_ptr<StructWithDelete>>",
+            "::std::unique_ptr<::rust_out::StructWithDelete> crubit_nonnull",
+        )],
+        unique_ptr_preamble(),
+        |desc, tcx, ty, expected| {
+            let db = nonnull_bindings_db_for_tests(tcx);
+            let cc_snippet = db
+                .format_ty_for_cc(
+                    ty,
+                    TypeLocation::FnParam { is_self_param: false, elided_is_output: false },
+                )
+                .unwrap();
+            let parsed_expected = expected.parse::<TokenStream>().unwrap().to_string();
+            assert_eq!(cc_snippet.tokens.to_string(), parsed_expected, "{desc}");
+        },
+    );
+}
+
 #[test]
 fn test_format_ty_for_cc_ignored_symbols_fails() {
     let preamble = quote! {

cc_bindings_from_rs/test/bridging/shared_ptr/BUILD

@@ -24,6 +24,7 @@ rust_library(
     srcs = [
         "shared_ptr.rs",
     ],
+    aspect_hints = ["//features:nonnull_smart_pointers"],
     proc_macro_deps = ["//support:crubit_annotate"],
     deps = ["//support/cc_std"],
 )

cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr.rs

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 use cc_std::std::shared_ptr;
+use cc_std::std::NonNull;
 use crubit_annotate::must_bind;
 
 #[must_bind]
@@ -17,3 +18,9 @@ pub fn clone_shared_ptr(val: &shared_ptr<i32>) -> shared_ptr<i32> {
 
 #[must_bind]
 pub fn consume_shared_ptr(_val: shared_ptr<i32>) {}
+
+/// `NonNull<Ptr>` is spelled in C++ as `Ptr` plus the `crubit_nonnull` attribute.
+#[must_bind]
+pub fn roundtrip_nonnull_shared_ptr(val: NonNull<shared_ptr<i32>>) -> NonNull<shared_ptr<i32>> {
+    val
+}

cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api.h

@@ -14,6 +14,7 @@
 #pragma clang diagnostic ignored "-Wunused-private-field"
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
 #pragma clang diagnostic ignored "-Wignored-attributes"
+#include "support/annotations_internal.h"
 #include "support/internal/slot.h"
 
 #include <cstdint>
@@ -28,6 +29,12 @@ ::std::shared_ptr<::std::int32_t> clone_shared_ptr(
 // CRUBIT_ANNOTATE: must_bind=
 void consume_shared_ptr(::std::shared_ptr<::std::int32_t> _val);
 
+// CRUBIT_ANNOTATE: must_bind=
+//  `NonNull<Ptr>` is spelled in C++ as `Ptr` plus the `crubit_nonnull`
+//  attribute.
+::std::shared_ptr<::std::int32_t> crubit_nonnull roundtrip_nonnull_shared_ptr(
+    ::std::shared_ptr<::std::int32_t> crubit_nonnull val);
+
 // CRUBIT_ANNOTATE: must_bind=
 ::std::shared_ptr<::std::int32_t> roundtrip_shared_ptr(
     ::std::shared_ptr<::std::int32_t> val);
@@ -56,6 +63,23 @@ inline void consume_shared_ptr(::std::shared_ptr<::std::int32_t> _val) {
       _val_slot.Get());
 }
 
+namespace __crubit_internal {
+extern "C" void __crubit_thunk_roundtrip_unonnull_ushared_uptr(
+    ::std::shared_ptr<::std::int32_t> crubit_nonnull*,
+    ::std::shared_ptr<::std::int32_t> crubit_nonnull* __ret_ptr);
+}
+inline ::std::shared_ptr<::std::int32_t> crubit_nonnull
+roundtrip_nonnull_shared_ptr(
+    ::std::shared_ptr<::std::int32_t> crubit_nonnull val) {
+  crubit::Slot val_slot((::std::move(val)));
+  crubit::Slot<::std::shared_ptr<::std::int32_t> crubit_nonnull>
+      __return_value_ret_val_holder;
+  auto* __return_value_storage = __return_value_ret_val_holder.Get();
+  __crubit_internal::__crubit_thunk_roundtrip_unonnull_ushared_uptr(
+      val_slot.Get(), __return_value_storage);
+  return ::std::move(__return_value_ret_val_holder).AssumeInitAndTakeValue();
+}
+
 namespace __crubit_internal {
 extern "C" void __crubit_thunk_roundtrip_ushared_uptr(
     ::std::shared_ptr<::std::int32_t>*,

cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api_impl.rs

@@ -34,6 +34,25 @@ unsafe extern "C" fn __crubit_thunk_consume_ushared_uptr(_val: *const core::ffi:
     }
 }
 #[unsafe(no_mangle)]
+unsafe extern "C" fn __crubit_thunk_roundtrip_unonnull_ushared_uptr(
+    val: *const core::ffi::c_void,
+    __ret_ptr: *mut core::ffi::c_void,
+) -> () {
+    unsafe {
+        let val = {
+            let mut __crubit_temp = ::core::mem::MaybeUninit::<
+                ::cc_std::std::NonNull<::cc_std::std::shared_ptr<i32>>,
+            >::uninit();
+            __crubit_temp.write(
+                (val as *const ::cc_std::std::NonNull<::cc_std::std::shared_ptr<i32>>).read(),
+            );
+            __crubit_temp.assume_init()
+        };
+        let __rs_return_value = ::shared_ptr_golden::roundtrip_nonnull_shared_ptr(val);
+        ::core::ptr::write(__ret_ptr as *mut _, __rs_return_value);
+    }
+}
+#[unsafe(no_mangle)]
 unsafe extern "C" fn __crubit_thunk_roundtrip_ushared_uptr(
     val: *const core::ffi::c_void,
     __ret_ptr: *mut core::ffi::c_void,

cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_test.cc

@@ -59,4 +59,21 @@ TEST(SharedPtrBridging, ConsumedByRust) {
   EXPECT_TRUE(weak.expired());
 }
 
+// Tests that a `NonNull`-wrapped `shared_ptr` round-trips. The C++ signature is
+// a plain `std::shared_ptr<T>` carrying the `crubit_nonnull` attribute, so
+// ownership and the reference count are unaffected.
+TEST(NonNullSharedPtrBridging, Roundtrip) {
+  auto ptr = std::make_shared<int32_t>(42);
+  std::weak_ptr<int32_t> weak = ptr;
+  EXPECT_FALSE(weak.expired());
+
+  auto ptr2 = shared_ptr::roundtrip_nonnull_shared_ptr(std::move(ptr));
+  EXPECT_FALSE(weak.expired());
+  EXPECT_NE(ptr2, nullptr);
+  EXPECT_EQ(*ptr2, 42);
+
+  ptr2 = nullptr;
+  EXPECT_TRUE(weak.expired());
+}
+
 }  // namespace

cc_bindings_from_rs/test/bridging/unique_ptr/BUILD

@@ -26,6 +26,7 @@ rust_library(
     srcs = [
         "unique_ptr.rs",
     ],
+    aspect_hints = ["//features:nonnull_smart_pointers"],
     cc_deps = [":test_helpers"],
     proc_macro_deps = ["//support:crubit_annotate"],
     deps = ["//support/cc_std"],

cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr.rs

@@ -4,6 +4,7 @@
 
 use cc_std::std::unique_ptr;
 use cc_std::std::virtual_unique_ptr;
+use cc_std::std::NonNull;
 use crubit_annotate::must_bind;
 use test_helpers::unique_ptr_test::Base;
 use test_helpers::unique_ptr_test::Derived;
@@ -54,3 +55,19 @@ pub fn accept_unique_ptr_tuple(val: unique_ptr<(i32, i32)>) -> unique_ptr<(i32,
 pub fn accept_unique_ptr_option(val: unique_ptr<Option<i32>>) -> unique_ptr<Option<i32>> {
     val
 }
+
+/// `NonNull<Ptr>` is spelled in C++ as `Ptr` plus the `crubit_nonnull` attribute, so passing one
+/// by value exercises the wrapped pointer's own movability rather than `NonNull`'s.
+#[must_bind]
+pub fn roundtrip_nonnull_unique_ptr(
+    val: NonNull<unique_ptr<Target>>,
+) -> NonNull<unique_ptr<Target>> {
+    val
+}
+
+#[must_bind]
+pub fn roundtrip_nonnull_virtual_unique_ptr(
+    val: NonNull<virtual_unique_ptr<Base>>,
+) -> NonNull<virtual_unique_ptr<Base>> {
+    val
+}

cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr_test.cc

@@ -79,4 +79,36 @@ TEST(UniquePtrBridging, Option) {
   EXPECT_NE(ptr2, nullptr);
 }
 
+// Tests that a `NonNull`-wrapped `unique_ptr` round-trips. The C++ signature is
+// a plain `std::unique_ptr<T>` carrying the `crubit_nonnull` attribute, so
+// ownership transfer is unaffected.
+TEST(NonNullUniquePtrBridging, Roundtrip) {
+  int initial_count = ::unique_ptr::get_destructor_count();
+
+  {
+    auto ptr = unique_ptr::create_unique_ptr();
+
+    auto ptr2 = unique_ptr::roundtrip_nonnull_unique_ptr(std::move(ptr));
+    EXPECT_NE(ptr2, nullptr);
+
+    EXPECT_EQ(::unique_ptr::get_destructor_count(), initial_count);
+  }
+  EXPECT_EQ(::unique_ptr::get_destructor_count(), initial_count + 1);
+}
+
+TEST(NonNullVirtualUniquePtrBridging, Roundtrip) {
+  int initial_count = ::unique_ptr::get_derived_destructor_count();
+
+  {
+    auto ptr = unique_ptr::create_virtual_unique_ptr();
+
+    auto ptr2 =
+        unique_ptr::roundtrip_nonnull_virtual_unique_ptr(std::move(ptr));
+    EXPECT_NE(ptr2, nullptr);
+
+    EXPECT_EQ(::unique_ptr::get_derived_destructor_count(), initial_count);
+  }
+  EXPECT_EQ(::unique_ptr::get_derived_destructor_count(), initial_count + 1);
+}
+
 }  // namespace