[modules] Handle friend function that was a definition but became only a declaration during AST deserialization (#132214)

Fix for regression #130917, changes in #111992 were too broad. This change reduces scope of previous fix. Added `ExternalASTSource::wasThisDeclarationADefinition` to detect cases when FunctionDecl lost body due to declaration merges.

Author: dmpolukhin

clang/include/clang/AST/ExternalASTSource.h

@@ -191,6 +191,10 @@ class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
 
   virtual ExtKind hasExternalDefinitions(const Decl *D);
 
+  /// True if this function declaration was a definition before in its own
+  /// module.
+  virtual bool wasThisDeclarationADefinition(const FunctionDecl *FD);
+
   /// Finds all declarations lexically contained within the given
   /// DeclContext, after applying an optional filter predicate.
   ///

clang/include/clang/Sema/MultiplexExternalSemaSource.h

@@ -92,6 +92,8 @@ class MultiplexExternalSemaSource : public ExternalSemaSource {
 
   ExtKind hasExternalDefinitions(const Decl *D) override;
 
+  bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;
+
   /// Find all declarations with the given name in the
   /// given context.
   bool FindExternalVisibleDeclsByName(const DeclContext *DC,

clang/include/clang/Serialization/ASTReader.h

@@ -1392,6 +1392,10 @@ class ASTReader
 
   llvm::DenseMap<const Decl *, bool> DefinitionSource;
 
+  /// Friend functions that were defined but might have had their bodies
+  /// removed.
+  llvm::DenseSet<const FunctionDecl *> ThisDeclarationWasADefinitionSet;
+
   bool shouldDisableValidationForFile(const serialization::ModuleFile &M) const;
 
   /// Reads a statement from the specified cursor.
@@ -2374,6 +2378,8 @@ class ASTReader
 
   ExtKind hasExternalDefinitions(const Decl *D) override;
 
+  bool wasThisDeclarationADefinition(const FunctionDecl *FD) override;
+
   /// Retrieve a selector from the given module with its local ID
   /// number.
   Selector getLocalSelector(ModuleFile &M, unsigned LocalID);

clang/lib/AST/ExternalASTSource.cpp

@@ -38,6 +38,10 @@ ExternalASTSource::hasExternalDefinitions(const Decl *D) {
   return EK_ReplyHazy;
 }
 
+bool ExternalASTSource::wasThisDeclarationADefinition(const FunctionDecl *FD) {
+  return false;
+}
+
 void ExternalASTSource::FindFileRegionDecls(FileID File, unsigned Offset,
                                             unsigned Length,
                                             SmallVectorImpl<Decl *> &Decls) {}

clang/lib/Sema/MultiplexExternalSemaSource.cpp

@@ -107,6 +107,14 @@ MultiplexExternalSemaSource::hasExternalDefinitions(const Decl *D) {
   return EK_ReplyHazy;
 }
 
+bool MultiplexExternalSemaSource::wasThisDeclarationADefinition(
+    const FunctionDecl *FD) {
+  for (const auto &S : Sources)
+    if (S->wasThisDeclarationADefinition(FD))
+      return true;
+  return false;
+}
+
 bool MultiplexExternalSemaSource::FindExternalVisibleDeclsByName(
     const DeclContext *DC, DeclarationName Name,
     const DeclContext *OriginalDC) {

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -2604,11 +2604,13 @@ Decl *TemplateDeclInstantiator::VisitFunctionDecl(
   // Friend function defined withing class template may stop being function
   // definition during AST merges from different modules, in this case decl
   // with function body should be used for instantiation.
-  if (isFriend) {
-    const FunctionDecl *Defn = nullptr;
-    if (D->hasBody(Defn)) {
-      D = const_cast<FunctionDecl *>(Defn);
-      FunctionTemplate = Defn->getDescribedFunctionTemplate();
+  if (ExternalASTSource *Source = SemaRef.Context.getExternalSource()) {
+    if (isFriend && Source->wasThisDeclarationADefinition(D)) {
+      const FunctionDecl *Defn = nullptr;
+      if (D->hasBody(Defn)) {
+        D = const_cast<FunctionDecl *>(Defn);
+        FunctionTemplate = Defn->getDescribedFunctionTemplate();
+      }
     }
   }
 

clang/lib/Serialization/ASTReader.cpp

@@ -9661,6 +9661,10 @@ ExternalASTSource::ExtKind ASTReader::hasExternalDefinitions(const Decl *FD) {
   return I->second ? EK_Never : EK_Always;
 }
 
+bool ASTReader::wasThisDeclarationADefinition(const FunctionDecl *FD) {
+  return ThisDeclarationWasADefinitionSet.contains(FD);
+}
+
 Selector ASTReader::getLocalSelector(ModuleFile &M, unsigned LocalID) {
   return DecodeSelector(getGlobalSelectorID(M, LocalID));
 }

clang/lib/Serialization/ASTReaderDecl.cpp

@@ -523,6 +523,9 @@ void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
   }
   // Store the offset of the body so we can lazily load it later.
   Reader.PendingBodies[FD] = GetCurrentCursorOffset();
+  // For now remember ThisDeclarationWasADefinition only for friend functions.
+  if (FD->getFriendObjectKind())
+    Reader.ThisDeclarationWasADefinitionSet.insert(FD);
 }
 
 void ASTDeclReader::Visit(Decl *D) {

clang/test/SemaCXX/friend-default-parameters-modules.cpp

@@ -0,0 +1,39 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -std=c++20 -fmodule-map-file=modules.map -xc++ -emit-module -fmodule-name=foo modules.map -o foo.pcm
+// RUN: %clang_cc1 -std=c++20 -fmodule-map-file=modules.map -O1 -emit-obj main.cc -verify -fmodule-file=foo.pcm
+
+//--- modules.map
+module "foo" {
+  export *
+  module "foo.h" {
+    export *
+    header "foo.h"
+  }
+}
+
+//--- foo.h
+#pragma once
+
+template <int>
+void Create(const void* = nullptr);
+
+template <int>
+struct ObjImpl {
+  template <int>
+  friend void ::Create(const void*);
+};
+
+template <int I>
+void Create(const void*) {
+  (void) ObjImpl<I>{};
+}
+
+//--- main.cc
+// expected-no-diagnostics
+#include "foo.h"
+
+int main() {
+  Create<42>();
+}

clang/test/SemaCXX/friend-default-parameters.cpp

@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -std=c++20 -verify -emit-llvm-only %s
+
+template <int>
+void Create(const void* = nullptr);
+
+template <int>
+struct ObjImpl {
+  template <int>
+  friend void ::Create(const void*);
+};
+
+template <int I>
+void Create(const void*) {
+  (void) ObjImpl<I>{};
+}
+
+int main() {
+  Create<42>();
+}
+
+// expected-no-diagnostics