From 691f58edecc4eaeb4f005d42e4e59755dc38fd08 Mon Sep 17 00:00:00 2001 From: "Andreas C. Osowski" Date: Wed, 1 Apr 2026 15:25:31 -0700 Subject: [PATCH] Fix Crubit issue where friend functions declared in a class and then redefined outside are not imported correctly. Previously, Crubit would try to import these functions via FriendDeclImporter as children of the class, and might also skip them when processing the outside definition due to canonicalization rules. This change: 1. Skips importing friend declarations in FriendDeclImporter if they are redeclared outside the class. 2. Modifies CanonicalizeDecl to prefer the outside definition as the canonical one if the first declaration was inside a record. This ensures it is imported as a top-level free function. This ensures that friend functions redefined outside are imported as normal top-level free functions. PiperOrigin-RevId: 893141473 --- rs_bindings_from_cc/importer.cc | 14 +++++++++ rs_bindings_from_cc/importers/friend.cc | 15 ++++++---- rs_bindings_from_cc/ir_from_cc_test.rs | 29 +++++++++++++------ .../test/golden/friend_functions_rs_api.rs | 12 ++++++++ .../golden/friend_functions_rs_api_impl.cc | 7 +++++ .../golden/unsafe_types_transitive_rs_api.rs | 16 +++++----- .../unsafe_types_transitive_rs_api_impl.cc | 12 ++++---- 7 files changed, 77 insertions(+), 28 deletions(-) diff --git a/rs_bindings_from_cc/importer.cc b/rs_bindings_from_cc/importer.cc index 370140de3..531e0cef7 100644 --- a/rs_bindings_from_cc/importer.cc +++ b/rs_bindings_from_cc/importer.cc @@ -476,6 +476,20 @@ const clang::Decl* Importer::CanonicalizeDecl(const clang::Decl* decl) const { CHECK(canonical != nullptr); return canonical; } + if (const auto* function_decl = llvm::dyn_cast(decl)) { + if (llvm::isa(function_decl)) { + return function_decl->getCanonicalDecl(); + } + const auto* canonical = function_decl->getCanonicalDecl(); + if (canonical->getLexicalDeclContext()->isRecord()) { + for (const auto* redecl : function_decl->redecls()) { + if (!redecl->getLexicalDeclContext()->isRecord()) { + return redecl; + } + } + } + return canonical; + } return decl->getCanonicalDecl(); } diff --git a/rs_bindings_from_cc/importers/friend.cc b/rs_bindings_from_cc/importers/friend.cc index 5f484e576..7e714efcd 100644 --- a/rs_bindings_from_cc/importers/friend.cc +++ b/rs_bindings_from_cc/importers/friend.cc @@ -45,12 +45,15 @@ std::optional FriendDeclImporter::Import( "DeclContext was unexpectedly not a CXXRecordDecl")); } - // If `!is_redeclared_outside_of_friend_decl`, then we need to emit a new item - // to support the following case from - // https://en.cppreference.com/w/cpp/language/adl: "ADL can find a friend - // function (typically, an overloaded operator) that is defined entirely - // within a class or class template, even if it was never declared at - // namespace level." + bool redeclared_outside = false; + for (const auto* redecl : named_decl->redecls()) { + if (redecl != named_decl && !redecl->getLexicalDeclContext()->isRecord()) { + redeclared_outside = true; + break; + } + } + if (redeclared_outside) return std::nullopt; + std::optional item = ictx_.ImportDecl(named_decl); if (!item.has_value()) return std::nullopt; if (std::holds_alternative(*item)) return std::nullopt; diff --git a/rs_bindings_from_cc/ir_from_cc_test.rs b/rs_bindings_from_cc/ir_from_cc_test.rs index 6b28f1842..8e520ee8a 100644 --- a/rs_bindings_from_cc/ir_from_cc_test.rs +++ b/rs_bindings_from_cc/ir_from_cc_test.rs @@ -3924,12 +3924,6 @@ fn test_function_redeclared_as_friend() { // The function should appear only once in IR items. (This is a bit redundant // with the assert below, but double-checks that `...` didn't miss a Func // item.) - let functions = ir - .functions() - .filter(|f| f.rs_name == UnqualifiedIdentifier::Identifier(ir_id("bar"))) - .collect_vec(); - assert_eq!(1, functions.len()); - let function_id = functions[0].id; // There should only be a single Func item. // @@ -3951,7 +3945,6 @@ fn test_function_redeclared_as_friend() { ItemId(...), ItemId(...), ItemId(...), - ItemId(#function_id), ] ... enclosing_item_id: None ... }), @@ -3965,14 +3958,32 @@ fn test_function_redeclared_as_friend() { cc_name: "bar", rs_name: "bar" ... enclosing_item_id: None ... - adl_enclosing_record: Some(ItemId(...)) ... + adl_enclosing_record: None ... }), ], - top_level_item_ids: map! { ... BazelLabel(#TESTING_TARGET): [ItemId(...)] ... } + top_level_item_ids: map! { ... BazelLabel(#TESTING_TARGET): [ItemId(...), ItemId(...)] ... } } ); } +#[gtest] +fn test_friend_not_definition_not_redeclared() { + let ir = ir_from_cc( + r#" + class SomeClass final { + friend void some_friend_func(); + }; + "#, + ) + .unwrap(); + + let functions = ir + .functions() + .filter(|f| f.rs_name == UnqualifiedIdentifier::Identifier(ir_id("some_friend_func"))) + .collect_vec(); + assert_eq!(1, functions.len()); +} + #[gtest] fn test_function_redeclared_in_separate_namespace_chunk() { let ir = ir_from_cc( diff --git a/rs_bindings_from_cc/test/golden/friend_functions_rs_api.rs b/rs_bindings_from_cc/test/golden/friend_functions_rs_api.rs index 1c07e7a84..c4c5c2f76 100644 --- a/rs_bindings_from_cc/test/golden/friend_functions_rs_api.rs +++ b/rs_bindings_from_cc/test/golden/friend_functions_rs_api.rs @@ -43,6 +43,15 @@ pub fn visible_val(mut __param_0: crate::SomeClass) { unsafe { crate::detail::__rust_thunk___Z11visible_val9SomeClass(&mut __param_0) } } +/// # Safety +/// +/// The caller must ensure that the following unsafe arguments are not misused by the function: +/// * `__param_0`: raw pointer +#[inline(always)] +pub unsafe fn multiple_declarations(__param_0: *const crate::SomeClass) -> ::ffi_11::c_int { + crate::detail::__rust_thunk___Z21multiple_declarationsRK9SomeClass(__param_0) +} + mod detail { #[allow(unused_imports)] use super::*; @@ -51,6 +60,9 @@ mod detail { pub(crate) unsafe fn __rust_thunk___Z11visible_val9SomeClass( __param_0: &mut crate::SomeClass, ); + pub(crate) unsafe fn __rust_thunk___Z21multiple_declarationsRK9SomeClass( + __param_0: *const crate::SomeClass, + ) -> ::ffi_11::c_int; } } diff --git a/rs_bindings_from_cc/test/golden/friend_functions_rs_api_impl.cc b/rs_bindings_from_cc/test/golden/friend_functions_rs_api_impl.cc index 887bf3640..f7ef8f40d 100644 --- a/rs_bindings_from_cc/test/golden/friend_functions_rs_api_impl.cc +++ b/rs_bindings_from_cc/test/golden/friend_functions_rs_api_impl.cc @@ -30,4 +30,11 @@ extern "C" void __rust_thunk___Z11visible_val9SomeClass( visible_val(std::move(*__param_0)); } +extern "C" int __rust_thunk___Z21multiple_declarationsRK9SomeClass( + class SomeClass const* __param_0) { + return multiple_declarations(*__param_0); +} + +static_assert((int (*)(class SomeClass const&)) & ::multiple_declarations); + #pragma clang diagnostic pop diff --git a/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api.rs b/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api.rs index c320b2bf8..13de68198 100644 --- a/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api.rs +++ b/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api.rs @@ -69,11 +69,6 @@ impl Default for PrivatePointer { } } -#[inline(always)] -pub fn DerefPrivatePointer(mut p: crate::PrivatePointer) -> ::ffi_11::c_int { - unsafe { crate::detail::__rust_thunk___Z19DerefPrivatePointer14PrivatePointer(&mut p) } -} - /// # Safety /// /// To call a function that accepts this type, you must uphold these requirements: @@ -151,6 +146,11 @@ pub unsafe fn DerefPublicPointer(mut p: crate::PublicPointer) -> ::ffi_11::c_int crate::detail::__rust_thunk___Z18DerefPublicPointer13PublicPointer(&mut p) } +#[inline(always)] +pub fn DerefPrivatePointer(mut p: crate::PrivatePointer) -> ::ffi_11::c_int { + unsafe { crate::detail::__rust_thunk___Z19DerefPrivatePointer14PrivatePointer(&mut p) } +} + /// # Safety /// /// The caller must ensure that the following unsafe arguments are not misused by the function: @@ -179,9 +179,6 @@ mod detail { pub(crate) unsafe fn __rust_thunk___ZN14PrivatePointerC1Ev( __this: *mut ::core::ffi::c_void, ); - pub(crate) unsafe fn __rust_thunk___Z19DerefPrivatePointer14PrivatePointer( - p: &mut crate::PrivatePointer, - ) -> ::ffi_11::c_int; pub(crate) unsafe fn __rust_thunk___ZN23TransitivePublicPointerC1Ev( __this: *mut ::core::ffi::c_void, ); @@ -193,6 +190,9 @@ mod detail { pub(crate) unsafe fn __rust_thunk___Z18DerefPublicPointer13PublicPointer( p: &mut crate::PublicPointer, ) -> ::ffi_11::c_int; + pub(crate) unsafe fn __rust_thunk___Z19DerefPrivatePointer14PrivatePointer( + p: &mut crate::PrivatePointer, + ) -> ::ffi_11::c_int; pub(crate) unsafe fn __rust_thunk___Z28DerefTransitivePublicPointer23TransitivePublicPointer( p: &mut crate::TransitivePublicPointer, ) -> ::ffi_11::c_int; diff --git a/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api_impl.cc b/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api_impl.cc index b56faac0f..1fb42c621 100644 --- a/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api_impl.cc +++ b/rs_bindings_from_cc/test/golden/unsafe_types_transitive_rs_api_impl.cc @@ -35,11 +35,6 @@ extern "C" void __rust_thunk___ZN14PrivatePointerC1Ev( crubit::construct_at(__this); } -extern "C" int __rust_thunk___Z19DerefPrivatePointer14PrivatePointer( - class PrivatePointer* p) { - return DerefPrivatePointer(std::move(*p)); -} - static_assert(CRUBIT_SIZEOF(struct TransitivePublicPointer) == 16); static_assert(alignof(struct TransitivePublicPointer) == 8); static_assert(CRUBIT_OFFSET_OF(pub, struct TransitivePublicPointer) == 0); @@ -68,6 +63,13 @@ extern "C" int __rust_thunk___Z18DerefPublicPointer13PublicPointer( static_assert((int (*)(struct PublicPointer)) & ::DerefPublicPointer); +extern "C" int __rust_thunk___Z19DerefPrivatePointer14PrivatePointer( + class PrivatePointer* p) { + return DerefPrivatePointer(std::move(*p)); +} + +static_assert((int (*)(class PrivatePointer)) & ::DerefPrivatePointer); + extern "C" int __rust_thunk___Z28DerefTransitivePublicPointer23TransitivePublicPointer( struct TransitivePublicPointer* p) {