[ASTImporter] Refactor IsStructurallyEquivalent's Decl overloads to be more consistent

[teemperor]

Summary

There are several ::IsStructurallyEquivalent overloads for Decl subclasses that are
used for comparing declarations. There is also one overload that takes just two Decl
pointers which ends up queuing the passed Decls to be later compared in
CheckKindSpecificEquivalence.

CheckKindSpecificEquivalence implements the dispatch logic for the different Decl
subclasses. It is supposed to hand over the queued Decls to the subclass-specific
::IsStructurallyEquivalent overload that will actually compare the Decl instance.
It also seems to implement a few pieces of actual node comparison logic inbetween the
dispatch code.

This implementation causes that the different overloads of ::IsStructurallyEquivalent do
different (and sometimes no) comparisons depending on which overload of ::IsStructurallyEquivalent
ends up being called.

For example, if I want to compare two FieldDecl instances, then I could either call the
::IsStructurallyEquivalent with Decl * or with FieldDecl * parameters. The overload
that takes FieldDecls is doing a correct comparison. However, the Decl * overload just
queues the Decl pair. CheckKindSpecificEquivalence has no dispatch logic for FieldDecl,
so it always returns true and never does any actual comparison.

On the other hand, if I try to compare two FunctionDecl instances the two possible overloads
of ::IsStructurallyEquivalent have the opposite behaviour: The overload that takes FunctionDecl
pointers isn't comparing the names of the FunctionDecls while the overload taking a plain Decl
ends up comparing the function names (as the comparison logic for that is implemented in
CheckKindSpecificEquivalence).

This patch tries to make this set of functions more consistent by making CheckKindSpecificEquivalence
a pure dispatch function without any subclass-specific comparison logic. Also the
dispatch logic is now autogenerated so it can no longer miss certain subclasses.

The comparison code from CheckKindSpecificEquivalence is moved to the respective ::IsStructurallyEquivalent overload
so that the comparison result no longer depends if one calls the Decl * overload or the overload for the
specific subclass. The only difference is now that the Decl * overload is queuing the parameter while
the subclass-specific overload is directly doing the comparison.

::IsStructurallyEquivalent is an implementation detail and I don't think the behaviour
causes any bugs in the current implementation (as carefully calling the right overload for the different classes
works around the issue), so the test for this change is that I added some new code for comparing MemberExpr.
The new comparison code always calls the dispatching overload and it previously failed as the dispatch didn't support
FieldDecls.

[teemperor]

• Removed an unrelated change.

[martong]

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

[teemperor]

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

A related question: You see any problem with doing the same generated Traversal* code we do for the Stmts also for the Decls? This way we could simplify some common checks (e.g., NameDecl's name comparison could cover all subclasses that want it). It seems that for example the Records can't just reuse the name comparison because of the getTypedefNameForAnonDecl() trick, so it might not be as simple as the Stmt code.

[martong]

In D87619#2273462, @teemperor wrote:

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

A related question: You see any problem with doing the same generated Traversal* code we do for the Stmts also for the Decls? This way we could simplify some common checks (e.g., NameDecl's name comparison could cover all subclasses that want it). It seems that for example the Records can't just reuse the name comparison because of the getTypedefNameForAnonDecl() trick, so it might not be as simple as the Stmt code.

No, I don't see any problem with that, could be another step forward. About the difficulties with getTypedefNameForAnonDecl(), perhaps we could have an overload for TagDecls that dispatches further (?).

[martong]

In D87619#2273450, @martong wrote:

Thank you! This is a great simplification of the code and it is much cleaner this way, nice work!
I queued a CTU build, just to verify nothing will be break:
#1​71​9 at http://codechecker-buildbot.eastus.cloudapp.azure.com:8080/job/ctu_pipeline_clang-master-monorepo/

The build passed.

[a_sidorin]

Hello Raphael! Nice patch, thank you for this refactoring!

clang/lib/AST/ASTStructuralEquivalence.cpp

1867

What happens if D2 is an overloaded operator and D1 is not? It looks like this block doesn't handle this case.

[teemperor]

clang/lib/AST/ASTStructuralEquivalence.cpp

1867

Just to clarify: This code is just moved from the dispatch code below and not added by this patch.

But I actually noticed the same when moving the code here and it's on my list of TODO list :)

[a_sidorin]

clang/lib/AST/ASTStructuralEquivalence.cpp

1867

Yes, this is a perfect example of how code simplification makes it easier to spot bugs.

clang/lib/AST/ASTStructuralEquivalence.cpp

 #include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
+#include "clang/AST/DeclOpenMP.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprConcepts.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/StmtObjC.h"
 #include "clang/AST/StmtOpenMP.h"
bool IsStmtEquivalent(const ImplicitCastExpr *CastE1,
     return IsStructurallyEquivalent(Context, CastE1->getType(),
                                     CastE2->getType());
   }
 
   bool IsStmtEquivalent(const IntegerLiteral *E1, const IntegerLiteral *E2) {
     return E1->getValue() == E2->getValue();
   }
 
+  bool IsStmtEquivalent(const MemberExpr *E1, const MemberExpr *E2) {
+    return IsStructurallyEquivalent(Context, E1->getFoundDecl(),
+                                    E2->getFoundDecl());
+  }
+
   bool IsStmtEquivalent(const ObjCStringLiteral *E1,
                         const ObjCStringLiteral *E2) {
     // Just wraps a StringLiteral child.
     return true;
   }
 
   bool IsStmtEquivalent(const Stmt *S1, const Stmt *S2) { return true; }
 
if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
     return false;
 
   return true;
 }
 
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      RecordDecl *D1, RecordDecl *D2) {
+
+  // Check for equivalent structure names.
+  IdentifierInfo *Name1 = D1->getIdentifier();
+  if (!Name1 && D1->getTypedefNameForAnonDecl())
+    Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
+  IdentifierInfo *Name2 = D2->getIdentifier();
+  if (!Name2 && D2->getTypedefNameForAnonDecl())
+    Name2 = D2->getTypedefNameForAnonDecl()->getIdentifier();
+  if (!IsStructurallyEquivalent(Name1, Name2))
+    return false;
+
   if (D1->isUnion() != D2->isUnion()) {
     if (Context.Complain) {
       Context.Diag2(D2->getLocation(), Context.getApplicableDiagnostic(
                                            diag::err_odr_tag_type_inconsistent))
           << Context.ToCtx.getTypeDeclType(D2);
       Context.Diag1(D1->getLocation(), diag::note_odr_tag_kind_here)
           << D1->getDeclName() << (unsigned)D1->getTagKind();
     }
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
 
   return true;
 }
 
 /// Determine structural equivalence of two enums.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      EnumDecl *D1, EnumDecl *D2) {
 
+  // Check for equivalent enum names.
+  IdentifierInfo *Name1 = D1->getIdentifier();
+  if (!Name1 && D1->getTypedefNameForAnonDecl())
+    Name1 = D1->getTypedefNameForAnonDecl()->getIdentifier();
+  IdentifierInfo *Name2 = D2->getIdentifier();
+  if (!Name2 && D2->getTypedefNameForAnonDecl())
+    Name2 = D2->getTypedefNameForAnonDecl()->getIdentifier();
+  if (!IsStructurallyEquivalent(Name1, Name2))
+    return false;
+
   // Compare the definitions of these two enums. If either or both are
   // incomplete (i.e. forward declared), we assume that they are equivalent.
   D1 = D1->getDefinition();
   D2 = D2->getDefinition();
   if (!D1 || !D2)
     return true;
 
   EnumDecl::enumerator_iterator EC2 = D2->enumerator_begin(),
return IsStructurallyEquivalent(Context,
                                     D2->getFriendType()->getType());
   if (D1->getFriendDecl() && D2->getFriendDecl())
     return IsStructurallyEquivalent(Context, D1->getFriendDecl(),
                                     D2->getFriendDecl());
   return false;
 }
 
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
+                                     TypedefNameDecl *D1, TypedefNameDecl *D2) {
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+    return false;
+
+  return IsStructurallyEquivalent(Context, D1->getUnderlyingType(),
+                                  D2->getUnderlyingType());
+}
+
+static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      FunctionDecl *D1, FunctionDecl *D2) {
+  if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
+    return false;
+
+  if (D1->isOverloadedOperator()) {
+    if (!D2->isOverloadedOperator())
+      return false;
+    if (D1->getOverloadedOperator() != D2->getOverloadedOperator())
+      return false;
+  }
+
   // 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.
bool StructuralEquivalenceContext::CheckCommonEquivalence(Decl *D1, Decl *D2) {
 
   // FIXME: Move check for identifier names into this function.
 
   return true;
 }
 
 bool StructuralEquivalenceContext::CheckKindSpecificEquivalence(
     Decl *D1, Decl *D2) {
-  // FIXME: Switch on all declaration kinds. For now, we're just going to
-  // check the obvious ones.
-  if (auto *Record1 = dyn_cast<RecordDecl>(D1)) {
-    if (auto *Record2 = dyn_cast<RecordDecl>(D2)) {
-      // Check for equivalent structure names.
-      IdentifierInfo *Name1 = Record1->getIdentifier();
-      if (!Name1 && Record1->getTypedefNameForAnonDecl())
-        Name1 = Record1->getTypedefNameForAnonDecl()->getIdentifier();
-      IdentifierInfo *Name2 = Record2->getIdentifier();
-      if (!Name2 && Record2->getTypedefNameForAnonDecl())
-        Name2 = Record2->getTypedefNameForAnonDecl()->getIdentifier();
-      if (!::IsStructurallyEquivalent(Name1, Name2) ||
-          !::IsStructurallyEquivalent(*this, Record1, Record2))
-        return false;
-    } else {
-      // Record/non-record mismatch.
-      return false;
-    }
-  } else if (auto *Enum1 = dyn_cast<EnumDecl>(D1)) {
-    if (auto *Enum2 = dyn_cast<EnumDecl>(D2)) {
-      // Check for equivalent enum names.
-      IdentifierInfo *Name1 = Enum1->getIdentifier();
-      if (!Name1 && Enum1->getTypedefNameForAnonDecl())
-        Name1 = Enum1->getTypedefNameForAnonDecl()->getIdentifier();
-      IdentifierInfo *Name2 = Enum2->getIdentifier();
-      if (!Name2 && Enum2->getTypedefNameForAnonDecl())
-        Name2 = Enum2->getTypedefNameForAnonDecl()->getIdentifier();
-      if (!::IsStructurallyEquivalent(Name1, Name2) ||
-          !::IsStructurallyEquivalent(*this, Enum1, Enum2))
-        return false;
-    } else {
-      // Enum/non-enum mismatch
-      return false;
-    }
-  } else if (const auto *Typedef1 = dyn_cast<TypedefNameDecl>(D1)) {
-    if (const auto *Typedef2 = dyn_cast<TypedefNameDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(Typedef1->getIdentifier(),
-                                      Typedef2->getIdentifier()) ||
-          !::IsStructurallyEquivalent(*this, Typedef1->getUnderlyingType(),
-                                      Typedef2->getUnderlyingType()))
-        return false;
-    } else {
-      // Typedef/non-typedef mismatch.
-      return false;
-    }
-  } else if (auto *ClassTemplate1 = dyn_cast<ClassTemplateDecl>(D1)) {
-    if (auto *ClassTemplate2 = dyn_cast<ClassTemplateDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, ClassTemplate1,
-                                      ClassTemplate2))
-        return false;
-    } else {
-      // Class template/non-class-template mismatch.
-      return false;
-    }
-  } else if (auto *FunctionTemplate1 = dyn_cast<FunctionTemplateDecl>(D1)) {
-    if (auto *FunctionTemplate2 = dyn_cast<FunctionTemplateDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, FunctionTemplate1,
-                                      FunctionTemplate2))
-        return false;
-    } else {
-      // Class template/non-class-template mismatch.
-      return false;
-    }
-  } else if (auto *ConceptDecl1 = dyn_cast<ConceptDecl>(D1)) {
-    if (auto *ConceptDecl2 = dyn_cast<ConceptDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, ConceptDecl1, ConceptDecl2))
-        return false;
-    } else {
-      // Concept/non-concept mismatch.
-      return false;
-    }
-  } else if (auto *TTP1 = dyn_cast<TemplateTypeParmDecl>(D1)) {
-    if (auto *TTP2 = dyn_cast<TemplateTypeParmDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
-        return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  } else if (auto *NTTP1 = dyn_cast<NonTypeTemplateParmDecl>(D1)) {
-    if (auto *NTTP2 = dyn_cast<NonTypeTemplateParmDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, NTTP1, NTTP2))
-        return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  } else if (auto *TTP1 = dyn_cast<TemplateTemplateParmDecl>(D1)) {
-    if (auto *TTP2 = dyn_cast<TemplateTemplateParmDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, TTP1, TTP2))
-        return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  } else if (auto *MD1 = dyn_cast<CXXMethodDecl>(D1)) {
-    if (auto *MD2 = dyn_cast<CXXMethodDecl>(D2)) {
-      if (!::IsStructurallyEquivalent(*this, MD1, MD2))
-        return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  } else if (FunctionDecl *FD1 = dyn_cast<FunctionDecl>(D1)) {
-    if (FunctionDecl *FD2 = dyn_cast<FunctionDecl>(D2)) {
-      if (FD1->isOverloadedOperator()) {
-        if (!FD2->isOverloadedOperator())
-          return false;
-        if (FD1->getOverloadedOperator() != FD2->getOverloadedOperator())
-          return false;
-      }
-      if (!::IsStructurallyEquivalent(FD1->getIdentifier(),
-                                      FD2->getIdentifier()))
-        return false;
-      if (!::IsStructurallyEquivalent(*this, FD1, FD2))
-        return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  } else if (FriendDecl *FrD1 = dyn_cast<FriendDecl>(D1)) {
-    if (FriendDecl *FrD2 = dyn_cast<FriendDecl>(D2)) {
-        if (!::IsStructurallyEquivalent(*this, FrD1, FrD2))
-          return false;
-    } else {
-      // Kind mismatch.
-      return false;
-    }
-  }
+
+  if (D1->getKind() != D2->getKind())
 
+  // Cast the Decls to their actual subclass so that the right overload of
+  // IsStructurallyEquivalent is called.
+  switch (D1->getKind()) {
+#define ABSTRACT_DECL(DECL)
+#define DECL(DERIVED, BASE)                                                    \
+  case Decl::Kind::DERIVED:                                                    \
+    return ::IsStructurallyEquivalent(*this, static_cast<DERIVED##Decl *>(D1), \
+                                      static_cast<DERIVED##Decl *>(D2));
+#include "clang/AST/DeclNodes.inc"
+  }
   return true;
 }
 
 bool StructuralEquivalenceContext::Finish() {
   while (!DeclsToCheck.empty()) {
     // Check the next declaration.
     std::pair<Decl *, Decl *> P = DeclsToCheck.front();
     DeclsToCheck.pop();

clang/unittests/AST/StructuralEquivalenceTest.cpp

TEST_F(StructuralEquivalenceStmtTest, IntegerLiteralDifferentValue) {
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceStmtTest, IntegerLiteralDifferentTypes) {
   auto t = makeWrappedStmts("1", "1L", Lang_CXX03, integerLiteral());
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceStmtTest, MemberExpr) {
+  std::string ClassSrc = "struct C { int a; int b; };";
+  auto t = makeStmts(ClassSrc + "int wrapper() { C c; return c.a; }",
+                     ClassSrc + "int wrapper() { C c; return c.a; }",
+                     Lang_CXX03, memberExpr());
+  EXPECT_TRUE(testStructuralMatch(t));
+}
+
+TEST_F(StructuralEquivalenceStmtTest, MemberExprDifferentMember) {
+  std::string ClassSrc = "struct C { int a; int b; };";
+  auto t = makeStmts(ClassSrc + "int wrapper() { C c; return c.a; }",
+                     ClassSrc + "int wrapper() { C c; return c.b; }",
+                     Lang_CXX03, memberExpr());
+  EXPECT_FALSE(testStructuralMatch(t));
+}
+
 TEST_F(StructuralEquivalenceStmtTest, ObjCStringLiteral) {
   auto t =
       makeWrappedStmts("@\"a\"", "@\"a\"", Lang_OBJCXX, fallbackExprMatcher());
   EXPECT_TRUE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceStmtTest, ObjCStringLiteralDifferentContent) {
   auto t =