[ASTImporter] FriendDecl importing improvements

[szepet]

Summary

Set FriendObjectKind in case of decls.
RD->pushFriendDecl(FrD); statement removed from L2240 since it is done in function Create(..).
IsStructurallyEquivalent function(s) of FriendDecl and dependent decls (RecordDecl, FunctionDecl) added.
Test cases added,

[szepet]

ping

[spyffe]

Sorry for the late review, I saw your ping but the last week was busy. Anyway, thanks for the tests!

lib/AST/ASTImporter.cpp

2660

Since getFriendObjectKind() returns an enum, in my opinion it's better style to check explicitly whether it != FOK_None.

2663

From a close reading of setObjectOfFriendDecl(), it looks to me like we might not set the IDNS_Tag or IDNS_Ordinary bits in IdentifierNamespace since we're passing PerformFriendInjection = false and since getPreviousDecl() might return nullptr in ToContext.

This means that getFriendObjectKind() might return FOK_Undeclared whereas it returned FOK_Declared for the original. Does this matter?

lib/AST/ASTStructuralEquivalence.cpp

966

Would it be appropriate to emit a diagnostic here as well?

1228

What if D1->getFriendDecl() != nullptr but D2->getFriendType() != nullptr or vice versa?
I wouldn't want to return true in that case…

1238

For attributes on the function (e.g., __attribute__ ((always_inline))) are these relevant to structural equvalence? Or does really only the type matter?

1477

What about if FrD1->getFriendDecl() && FrD2->getFriendDecl()?

[szepet]

Updates based on comments.

[xazax.hun]

Could you add some context?

Generally LGTM!

One high level comment, I think we need to modify the importing of CXXRecordDecls to properly support friend declarations. When we are building the redeclarable chain of CXXRecordDecl declarations I suspect we also need to consider names in the IDNS_TagFriend namespace.

[martong]

lib/AST/ASTImporter.cpp

2260

This will assert unless the IdentifierNamespace is properly imported. See https://github.com/Ericsson/clang/pull/266/files#diff-7f5f26dbdb3322c6308b1018ff0dbe5aR6735

[xazax.hun]

Note that changes from https://reviews.llvm.org/D44100 might affect this.

[szepet]

Rebase + set the IDNS in a more general way.

[martong]

LGTM!

[a.sidorin]

Looks good to me as well. Thank you!

[szepet]

I have commited this patch (r330847) but unfortunately "refactored" the differential revision link from the commit message out. Added it as a dependent commit.

include/clang/AST/DeclBase.h

private:
   unsigned TopLevelDeclInObjCContainer : 1;
   
   /// \brief Whether statistic collection is enabled.
   static bool StatisticsEnabled;
 
 protected:
   friend class ASTDeclReader;
   friend class ASTDeclWriter;
+  friend class ASTImporter;
   friend class ASTReader;
   friend class CXXClassMemberWrapper;
   friend class LinkageComputer;
   template<typename decl_type> friend class Redeclarable;
 
   /// Access - Used by C++ decls for the access specifier.
   // NOTE: VC++ treats enums as signed, avoid using the AccessSpecifier enum
   unsigned Access : 2;

include/clang/Basic/DiagnosticASTKinds.td

def note_odr_objc_property_impl_kind : Note<
   "property %0 is implemented with %select{@synthesize|@dynamic}1 here">;
 def err_odr_objc_synthesize_ivar_inconsistent : Error<
   "property %0 is synthesized to different ivars in different translation "
   "units (%1 vs. %2)">;
 def note_odr_objc_synthesize_ivar_here : Note<
   "property is synthesized to ivar %0 here">;
 
 // Importing C++ ASTs
+def note_odr_friend : Note<"friend declared here">;
+def note_odr_missing_friend : Note<"no corresponding friend here">;
 def err_odr_different_num_template_parameters : Error<
   "template parameter lists have a different number of parameters (%0 vs %1)">;
 def note_odr_template_parameter_list : Note<
   "template parameter list also declared here">;
 def err_odr_different_template_parameter_kind : Error<
   "template parameter has different kinds in different translation units">;
 def note_odr_template_parameter_here : Note<
   "template parameter declared here">;

lib/AST/ASTImporter.cpp

for (unsigned I = 0, N = FoundDecls.size(); I != N; ++I) {
               FoundWithoutBody = FoundFunction;
               break;
             }
             return Importer.Imported(D, FoundFunction);
           }
 
           // FIXME: Check for overloading more carefully, e.g., by boosting
           // Sema::IsOverload out to the AST library.
 
           // Function overloading is okay in C++.
           if (Importer.getToContext().getLangOpts().CPlusPlus)
             continue;
 
           // Complain about inconsistent function types.
           Importer.ToDiag(Loc, diag::err_odr_function_type_inconsistent)
             << Name << D->getType() << FoundFunction->getType();
           Importer.ToDiag(FoundFunction->getLocation(), 
if (D->getFriendDecl() && ImportedFriend->getFriendDecl()) {
             ImportedFriend->getFriendType()->getType(), true))
         return Importer.Imported(D, ImportedFriend);
     }
     ImportedFriend = ImportedFriend->getNextFriend();
   }
 
   // Not found. Create it.
   FriendDecl::FriendUnion ToFU;
-  if (NamedDecl *FriendD = D->getFriendDecl())
-    ToFU = cast_or_null<NamedDecl>(Importer.Import(FriendD));
-  else
+  if (NamedDecl *FriendD = D->getFriendDecl()) {
+    auto *ToFriendD = cast_or_null<NamedDecl>(Importer.Import(FriendD));
+    if (ToFriendD && FriendD->getFriendObjectKind() != Decl::FOK_None &&
+        !(FriendD->isInIdentifierNamespace(Decl::IDNS_NonMemberOperator)))
+      ToFriendD->setObjectOfFriendDecl(false);
+
+    ToFU = ToFriendD;
+  }  else // The friend is a type, not a decl.
     ToFU = Importer.Import(D->getFriendType());
   if (!ToFU)
     return nullptr;
 
   SmallVector<TemplateParameterList *, 1> ToTPLists(D->NumTPLists);
   TemplateParameterList **FromTPLists =
       D->getTrailingObjects<TemplateParameterList *>();
   for (unsigned I = 0; I < D->NumTPLists; I++) {
     TemplateParameterList *List = ImportTemplateParameterList(FromTPLists[I]);
     if (!List)
       return nullptr;
     ToTPLists[I] = List;
   }
 
   FriendDecl *FrD = FriendDecl::Create(Importer.getToContext(), DC,
                                        Importer.Import(D->getLocation()),
                                        ToFU, Importer.Import(D->getFriendLoc()),
                                        ToTPLists);
 
   Importer.Imported(D, FrD);
-  RD->pushFriendDecl(FrD);
 
   FrD->setAccess(D->getAccess());
   FrD->setLexicalDeclContext(LexicalDC);
   LexicalDC->addDeclInternal(FrD);
   return FrD;
 }
 
 Decl *ASTNodeImporter::VisitObjCIvarDecl(ObjCIvarDecl *D) {
for (SmallVectorImpl<TagDecl *>::iterator
           ToTag->setTypedefNameForAnonDecl(cast<TypedefNameDecl>(ToD));
           AnonTagsWithPendingTypedefs.erase(FromTag);
           break;
         }
       }
     }
   }
   
+  ToD->IdentifierNamespace = FromD->IdentifierNamespace;
   return ToD;
 }
 
 DeclContext *ASTImporter::ImportContext(DeclContext *FromDC) {
   if (!FromDC)
     return FromDC;
 
   DeclContext *ToDC = cast_or_null<DeclContext>(Import(cast<Decl>(FromDC)));

lib/AST/ASTStructuralEquivalence.cpp

if (CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(D2)) {
             Context.Diag2(Base2->getLocStart(), diag::note_odr_virtual_base)
                 << Base2->isVirtual() << Base2->getSourceRange();
             Context.Diag1(Base1->getLocStart(), diag::note_odr_base)
                 << Base1->isVirtual() << Base1->getSourceRange();
           }
           return false;
         }
       }
+
+      // Check the friends for consistency.
+      CXXRecordDecl::friend_iterator Friend2 = D2CXX->friend_begin(),
+              Friend2End = D2CXX->friend_end();
+      for (CXXRecordDecl::friend_iterator Friend1 = D1CXX->friend_begin(),
+                   Friend1End = D1CXX->friend_end();
+           Friend1 != Friend1End; ++Friend1, ++Friend2) {
+        if (Friend2 == Friend2End) {
+          if (Context.Complain) {
+            Context.Diag2(D2->getLocation(),
+                          diag::warn_odr_tag_type_inconsistent)
+                    << Context.ToCtx.getTypeDeclType(D2CXX);
+            Context.Diag1((*Friend1)->getFriendLoc(), diag::note_odr_friend);
+            Context.Diag2(D2->getLocation(), diag::note_odr_missing_friend);
+          }
+          return false;
+        }
+
+        if (!IsStructurallyEquivalent(Context, *Friend1, *Friend2)) {
+          if (Context.Complain) {
+            Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+              << Context.ToCtx.getTypeDeclType(D2CXX);
+            Context.Diag1((*Friend1)->getFriendLoc(), diag::note_odr_friend);
+            Context.Diag2((*Friend2)->getFriendLoc(), diag::note_odr_friend);
+          }
+          return false;
+        }
+      }
+
+      if (Friend2 != Friend2End) {
+        if (Context.Complain) {
+          Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
+                  << Context.ToCtx.getTypeDeclType(D2);
+          Context.Diag2((*Friend2)->getFriendLoc(), diag::note_odr_friend);
+          Context.Diag1(D1->getLocation(), diag::note_odr_missing_friend);
+        }
+        return false;
+      }
     } else if (D1CXX->getNumBases() > 0) {
       if (Context.Complain) {
         Context.Diag2(D2->getLocation(), diag::warn_odr_tag_type_inconsistent)
             << Context.ToCtx.getTypeDeclType(D2);
         const CXXBaseSpecifier *Base1 = D1CXX->bases_begin();
         Context.Diag1(Base1->getLocStart(), diag::note_odr_base)
             << Base1->getType() << Base1->getSourceRange();
         Context.Diag2(D2->getLocation(), diag::note_odr_missing_base);
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
   // Check template parameters.
   if (!IsTemplateDeclCommonStructurallyEquivalent(Context, D1, D2))
     return false;
 
   // Check the templated declaration.
   return Context.IsStructurallyEquivalent(D1->getTemplatedDecl()->getType(),
                                           D2->getTemplatedDecl()->getType());
 }
 
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     FriendDecl *D1, FriendDecl *D2) {
+  if ((D1->getFriendType() && D2->getFriendDecl()) || 
+      (D1->getFriendDecl() && D2->getFriendType())) {
+      return false; 
+  }
+  if (D1->getFriendType() && D2->getFriendType())
+    return IsStructurallyEquivalent(Context,
+                                    D1->getFriendType()->getType(),
+                                    D2->getFriendType()->getType());
+  if (D1->getFriendDecl() && D2->getFriendDecl())
+    return IsStructurallyEquivalent(Context, D1->getFriendDecl(),
+                                    D2->getFriendDecl());
+  return false;
+}
+
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     FunctionDecl *D1, FunctionDecl *D2) {
+  // FIXME: Consider checking for function attributes as well.
+  if (!IsStructurallyEquivalent(Context, D1->getType(), D2->getType()))
+    return false;
+
+  return true;
+}
+
 /// Determine structural equivalence of two declarations.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      Decl *D1, Decl *D2) {
   // FIXME: Check for known structural equivalences via a callback of some sort.
 
   // Check whether we already know that these two declarations are not
   // structurally equivalent.
   if (Context.NonEquivalentDecls.count(
if (RecordDecl *Record1 = dyn_cast<RecordDecl>(D1)) {
       if (TemplateTemplateParmDecl *TTP2 =
               dyn_cast<TemplateTemplateParmDecl>(D2)) {
         if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
           Equivalent = false;
       } else {
         // Kind mismatch.
         Equivalent = false;
       }
+    } else if (FunctionDecl *FD1 = dyn_cast<FunctionDecl>(D1)) {
+      if (FunctionDecl *FD2 = dyn_cast<FunctionDecl>(D2)) {
+        if (!::IsStructurallyEquivalent(FD1->getIdentifier(),
+                                        FD2->getIdentifier()))
+          Equivalent = false;
+        if (!::IsStructurallyEquivalent(*this, FD1, FD2))
+          Equivalent = false;
+      } else {
+        // Kind mismatch.
+        Equivalent = false;
+      }
+    } else if (FriendDecl *FrD1 = dyn_cast<FriendDecl>(D1)) {
+      if (FriendDecl *FrD2 = dyn_cast<FriendDecl>(D2)) {
+          if (!::IsStructurallyEquivalent(*this, FrD1, FrD2))
+            Equivalent = false;
+      } else {
+        // Kind mismatch.
+        Equivalent = false;
+      }
     }
 
     if (!Equivalent) {
       // Note that these two declarations are not equivalent (and we already
       // know about it).
       NonEquivalentDecls.insert(
           std::make_pair(D1->getCanonicalDecl(), D2->getCanonicalDecl()));
       return true;
     }
     // FIXME: Check other declaration kinds!
   }
 
   return false;
 }
 } // namespace clang

test/ASTMerge/class/Inputs/class1.cpp

struct C {
   C(const C&);
   C &operator=(C&);
   ~C();
 };
 
 enum E {
   b = 1
 };
+
+//Friend import tests
+void f();
+int g(int a);
+struct X;
+struct Y;
+
+struct F1 {
+public:
+  int x;
+  friend struct X;
+  friend int g(int);
+  friend void f();
+};
+
+struct F2 {
+public:
+  int x;
+  friend struct X;
+  friend void f();
+};
+
+struct F3 {
+public:
+  int x;
+  friend int g(int);
+  friend void f();
+};

test/ASTMerge/class/Inputs/class2.cpp

 struct A {
   public:
     int x;
 };
 
 struct B : A {
   int y;
   int foo();
 };
 
 enum E {
   a = 0,
   b = 1
 };
+
+//Friend import tests
+void f();
+int g(int a);
+struct X;
+struct Y;
+
+struct F1 {
+public:
+  int x;
+  friend struct X;
+  friend int g(int);
+  friend void f();
+};
+
+struct F2 {
+public:
+  int x;
+  friend struct X;
+};
+
+struct F3 {
+public:
+  int x;
+  friend void f();
+};

test/ASTMerge/class/test.cpp

 // RUN: %clang_cc1 -emit-pch -o %t.1.ast %S/Inputs/class1.cpp
 // RUN: %clang_cc1 -emit-pch -o %t.2.ast %S/Inputs/class2.cpp
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 | FileCheck %s
 // RUN: %clang_cc1 -ast-merge %t.1.ast -ast-merge %t.2.ast -fsyntax-only %s 2>&1 -Wno-odr -Werror
 
 // CHECK: class1.cpp:6:8: warning: type 'B' has incompatible definitions in different translation units
 // CHECK: class1.cpp:7:9: note: field 'y' has type 'float' here
 // CHECK: class2.cpp:7:7: note: field 'y' has type 'int' here
 
 // FIXME: we should also complain about mismatched types on the method
 
 // CHECK: class1.cpp:18:6: warning: type 'E' has incompatible definitions in different translation units
 // CHECK: class1.cpp:19:3: note: enumerator 'b' with value 1 here
 // CHECK: class2.cpp:12:3: note: enumerator 'a' with value 0 here
+
+// CHECK: class1.cpp:36:8: warning: type 'F2' has incompatible definitions in different translation units
+// CHECK: class1.cpp:39:3: note: friend declared here
+// CHECK: class2.cpp:30:8: note: no corresponding friend here
+
+// CHECK: class1.cpp:43:8: warning: type 'F3' has incompatible definitions in different translation units
+// CHECK: class1.cpp:46:3: note: friend declared here
+// CHECK: class2.cpp:36:8: note: no corresponding friend here
+
+// CHECK: 4 warnings generated.