Fix Crubit issue where friend functions declared in a class and then redefined outside are not imported correctly.

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<clang::FunctionDecl>(decl)) {
+    if (llvm::isa<clang::CXXMethodDecl>(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();
 }
 

rs_bindings_from_cc/importers/friend.cc

@@ -45,12 +45,15 @@ std::optional<IR::Item> 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<IR::Item> item = ictx_.ImportDecl(named_decl);
   if (!item.has_value()) return std::nullopt;
   if (std::holds_alternative<UnsupportedItem>(*item)) return std::nullopt;

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(

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;
     }
 }
 

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

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;

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) {