diff --git a/cc_bindings_from_rs/generate_bindings/BUILD b/cc_bindings_from_rs/generate_bindings/BUILD index 9f02adccc..33c1ff1fa 100644 --- a/cc_bindings_from_rs/generate_bindings/BUILD +++ b/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 diff --git a/cc_bindings_from_rs/generate_bindings/format_type.rs b/cc_bindings_from_rs/generate_bindings/format_type.rs index 4954c5f4b..02b11735a 100644 --- a/cc_bindings_from_rs/generate_bindings/format_type.rs +++ b/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` and `std::vector` 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` 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` and `std::vector` 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!( @@ -2041,6 +2051,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` 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); }; diff --git a/cc_bindings_from_rs/generate_bindings/format_type_test.rs b/cc_bindings_from_rs/generate_bindings/format_type_test.rs index 09c750fad..48c538125 100644 --- a/cc_bindings_from_rs/generate_bindings/format_type_test.rs +++ b/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( @@ -810,6 +813,10 @@ fn unique_ptr_preamble() -> TokenStream { #[doc="CRUBIT_ANNOTATE: cpp_type = ::std::unique_ptr<{T}>"] #[doc="CRUBIT_ANNOTATE: include_path = "] pub struct virtual_unique_ptr(pub *mut T); + + #[doc="CRUBIT_ANNOTATE: cpp_type = {Ptr} crubit_nonnull"] + #[doc="CRUBIT_ANNOTATE: include_path = "] + pub struct NonNull(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` 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` 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>", + "::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::().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! { diff --git a/cc_bindings_from_rs/test/bridging/shared_ptr/BUILD b/cc_bindings_from_rs/test/bridging/shared_ptr/BUILD index 5980ff875..e6c09ad7c 100644 --- a/cc_bindings_from_rs/test/bridging/shared_ptr/BUILD +++ b/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"], ) diff --git a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr.rs b/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr.rs index dd13850d8..761b63e08 100644 --- a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr.rs +++ b/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) -> shared_ptr { #[must_bind] pub fn consume_shared_ptr(_val: shared_ptr) {} + +/// `NonNull` is spelled in C++ as `Ptr` plus the `crubit_nonnull` attribute. +#[must_bind] +pub fn roundtrip_nonnull_shared_ptr(val: NonNull>) -> NonNull> { + val +} diff --git a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api.h b/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api.h index f9ddcc040..5ba14d405 100644 --- a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api.h +++ b/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 @@ -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` 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>*, diff --git a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api_impl.rs b/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api_impl.rs index c5f94b890..ab53a15fc 100644 --- a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_cc_api_impl.rs +++ b/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>, + >::uninit(); + __crubit_temp.write( + (val as *const ::cc_std::std::NonNull<::cc_std::std::shared_ptr>).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, diff --git a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_test.cc b/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_test.cc index f54fdd5ab..e0bcee6b6 100644 --- a/cc_bindings_from_rs/test/bridging/shared_ptr/shared_ptr_test.cc +++ b/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` carrying the `crubit_nonnull` attribute, so +// ownership and the reference count are unaffected. +TEST(NonNullSharedPtrBridging, Roundtrip) { + auto ptr = std::make_shared(42); + std::weak_ptr 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 diff --git a/cc_bindings_from_rs/test/bridging/unique_ptr/BUILD b/cc_bindings_from_rs/test/bridging/unique_ptr/BUILD index 6bad4ac4a..d0bba902c 100644 --- a/cc_bindings_from_rs/test/bridging/unique_ptr/BUILD +++ b/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"], diff --git a/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr.rs b/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr.rs index f0aa88a96..e1669c933 100644 --- a/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr.rs +++ b/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>) -> unique_ptr> { val } + +/// `NonNull` 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>, +) -> NonNull> { + val +} + +#[must_bind] +pub fn roundtrip_nonnull_virtual_unique_ptr( + val: NonNull>, +) -> NonNull> { + val +} diff --git a/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr_test.cc b/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr_test.cc index 84f1395e0..96503956d 100644 --- a/cc_bindings_from_rs/test/bridging/unique_ptr/unique_ptr_test.cc +++ b/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` 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 diff --git a/features/BUILD b/features/BUILD index 453b49fa3..4846075df 100644 --- a/features/BUILD +++ b/features/BUILD @@ -279,8 +279,9 @@ crubit_feature_hint( visibility = ["//visibility:public"], ) -# A feature set enabling `absl_nonnull` on `std::unique_ptr` and `std::shared_ptr` to generate -# `cc_std::std::NonNull`-wrapped Rust types. +# A feature set enabling the `cc_std::std::NonNull` bridge in both directions: `absl_nonnull` on +# `std::unique_ptr` and `std::shared_ptr` generates `NonNull`-wrapped Rust types, and a `NonNull` +# in a Rust API generates the wrapped C++ pointer type with the `crubit_nonnull` attribute. crubit_feature_hint( name = "nonnull_smart_pointers", compatible_with = ["//buildenv/target:non_prod"], diff --git a/support/cc_std_impl/non_null.rs b/support/cc_std_impl/non_null.rs index df68b9ec8..270a45bea 100644 --- a/support/cc_std_impl/non_null.rs +++ b/support/cc_std_impl/non_null.rs @@ -179,6 +179,10 @@ where /// assert_eq!(*non_null, 42); /// } /// ``` +#[crubit_annotate::cpp_layout_equivalent( + cpp_type = "{Ptr} crubit_nonnull", + include_path = "" +)] #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] #[repr(transparent)] pub struct NonNull {