[ASTImporter] Fix import of unnamed structs

[martong]

Summary

D48773 simplified ASTImporter nicely, but it introduced a new error: Unnamed
structs are not imported correctly, if they appear in a recursive context.
This patch provides a fix for structural equivalency.

[a_sidorin]

Hi Gabor,
The change is OK but I have some questions regarding tests.

unittests/AST/ASTImporterTest.cpp

2495 ↗

(On Diff #155386)

There is already a similar lambda definitions for getRecordDecl() in ObjectsWithUnnamedStructType test. Can we make it a function, like it is done for StructuralEquivalenceContext?

unittests/AST/StructuralEquivalenceTest.cpp

581 ↗

(On Diff #155386)

Do we really want to test the equivalence of decl to itself, not to its imported version?

[balazske]

lib/AST/ASTStructuralEquivalence.cpp

913 ↗

(On Diff #155386)

The problem was that value of isAnonymousStructOrUnion is not (or not yet) set correctly during the import? But getDeclName is always correct (if it is a false the record is anonymous struct)?

unittests/AST/StructuralEquivalenceTest.cpp

486 ↗

(On Diff #155386)

RecordDecl *

578 ↗

(On Diff #155386)

Is it possible at all that getRecordDecl returns nullptr? It uses cast (nor cast_or_null and not dyn_cast).

581 ↗

(On Diff #155386)

I think this is correct: R0 should be equivalent with itself but not with R1.

[martong]

Address review comments

[martong]

lib/AST/ASTStructuralEquivalence.cpp

913 ↗

(On Diff #155386)

There is a strange thing here: A struct without a name is not necessarily an anonymous struct!
For example the RecordDecl in struct { int i; float f; } obj; is not anonymous.
See the docs for RecordDecl::isAnonymousStructOrUnion:

/// Whether this is an anonymous struct or union. To be an anonymous
/// struct or union, it must have been declared without a name and
/// there must be no objects of this type declared, e.g.,
/// @code
///   union { int i; float f; };
/// @endcode
/// is an anonymous union but neither of the following are:
/// @code
///  union X { int i; float f; };
///  union { int i; float f; } obj;
/// @endcode
bool isAnonymousStructOrUnion() const { return AnonymousStructOrUnion; }

This might seem very perplexed, but this is aligned with the C++17 Standard:

12.3.1 Anonymous unions
para 3. A union for which objects, pointers, or references are declared is not an anonymous union.

Consequently, we should check whether the Decl has a name, isAnonymousStructOrUnion is misleading here.

unittests/AST/ASTImporterTest.cpp

2495 ↗

(On Diff #155386)

OK, I added a new function template to handle both cases.

unittests/AST/StructuralEquivalenceTest.cpp

578 ↗

(On Diff #155386)

Yes, good catch, changed it.

581 ↗

(On Diff #155386)

I think it makes sense, because from the StructuralEquivalence's viewpoint there is no imported version, just two Decls/Types which may or may not have the same ASTContext.

[martong]

RecordDecl* -> RecordDecl *

[a_sidorin]

Thank you Gabor!

[balazske]

unittests/AST/StructuralEquivalenceTest.cpp

615 ↗

(On Diff #155678)

These asserts can be removed too.

[martong]

Remove unneeded assert

cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

if (Context.Complain) {
                         : diag::warn_odr_tag_type_inconsistent)
           << Context.ToCtx.getTypeDeclType(D2);
       Context.Diag1(D1->getLocation(), diag::note_odr_tag_kind_here)
           << D1->getDeclName() << (unsigned)D1->getTagKind();
     }
     return false;
   }
 
-  if (D1->isAnonymousStructOrUnion() && D2->isAnonymousStructOrUnion()) {
+  if (!D1->getDeclName() && !D2->getDeclName()) {
     // If both anonymous structs/unions are in a record context, make sure
     // they occur in the same location in the context records.
     if (Optional<unsigned> Index1 =
             StructuralEquivalenceContext::findUntaggedStructOrUnionIndex(D1)) {
       if (Optional<unsigned> Index2 =
               StructuralEquivalenceContext::findUntaggedStructOrUnionIndex(
                   D2)) {
         if (*Index1 != *Index2)

cfe/trunk/unittests/AST/ASTImporterTest.cpp

void testImportSequence(const CodeFiles &CodeSamples,
     EXPECT_TRUE(FoundDecl.size() == 1);
     const Decl *ToVerify = selectFirst<Decl>(DeclToVerifyID, FoundDecl);
     MatchVerifier<Decl> Verifier;
     EXPECT_TRUE(
         Verifier.match(ToVerify, BindableMatcher<Decl>(VerificationMatcher)));
   }
 };
 
+template <typename T> RecordDecl *getRecordDecl(T *D) {
+  auto *ET = cast<ElaboratedType>(D->getType().getTypePtr());
+  return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
+};
+
 // This class provides generic methods to write tests which can check internal
 // attributes of AST nodes like getPreviousDecl(), isVirtual(), etc. Also,
 // this fixture makes it possible to import from several "From" contexts.
 class ASTImporterTestBase : public ParameterizedTestsFixture {
 
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 

 TEST_P(ASTImporterTestBase, ObjectsWithUnnamedStructType) {
   Decl *FromTU = getTuDecl(
       R"(
       struct { int a; int b; } object0 = { 2, 3 };
       struct { int x; int y; int z; } object1;
       )",
       Lang_CXX, "input0.cc");
 
-  auto getRecordDecl = [](VarDecl *VD) {
-    auto *ET = cast<ElaboratedType>(VD->getType().getTypePtr());
-    return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
-  };
-
   auto *Obj0 =
       FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("object0")));
   auto *From0 = getRecordDecl(Obj0);
   auto *Obj1 =
       FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("object1")));
   auto *From1 = getRecordDecl(Obj1);
 
   auto *To0 = Import(From0, Lang_CXX);
Decl *FromTU = getTuDecl(
         Lang_CXX, "input1.cc");
     auto *FromM = FirstDeclMatcher<FunctionDecl>().match(
         FromTU, functionDecl(hasName("x"), isDefinition()));
     ToM2 = Import(FromM, Lang_CXX);
   }
   EXPECT_NE(ToM1, ToM2);
 }
 
+TEST_P(ASTImporterTestBase, ImportUnnamedStructsWithRecursingField) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct A {
+        struct {
+          struct A *next;
+        } entry0;
+        struct {
+          struct A *next;
+        } entry1;
+      };
+      )",
+      Lang_C, "input0.cc");
+  auto *From =
+      FirstDeclMatcher<RecordDecl>().match(FromTU, recordDecl(hasName("A")));
+
+  Import(From, Lang_C);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+  auto *Entry0 =
+      FirstDeclMatcher<FieldDecl>().match(ToTU, fieldDecl(hasName("entry0")));
+  auto *Entry1 =
+      FirstDeclMatcher<FieldDecl>().match(ToTU, fieldDecl(hasName("entry1")));
+  auto *R0 = getRecordDecl(Entry0);
+  auto *R1 = getRecordDecl(Entry1);
+  EXPECT_NE(R0, R1);
+  EXPECT_TRUE(MatchVerifier<RecordDecl>().match(
+      R0, recordDecl(has(fieldDecl(hasName("next"))))));
+  EXPECT_TRUE(MatchVerifier<RecordDecl>().match(
+      R1, recordDecl(has(fieldDecl(hasName("next"))))));
+}
+
 struct DeclContextTest : ASTImporterTestBase {};
 
 TEST_P(DeclContextTest, removeDeclOfClassTemplateSpecialization) {
   Decl *TU = getTuDecl(
       R"(
       namespace NS {
 
       template <typename T>

cfe/trunk/unittests/AST/StructuralEquivalenceTest.cpp

std::tuple<NodeType *, NodeType *> makeDecls(
     NodeType *D0 = FirstDeclMatcher<NodeType>().match(
         AST0->getASTContext().getTranslationUnitDecl(), Matcher0);
     NodeType *D1 = FirstDeclMatcher<NodeType>().match(
         AST1->getASTContext().getTranslationUnitDecl(), Matcher1);
 
     return std::make_tuple(D0, D1);
   }
 
+  std::tuple<TranslationUnitDecl *, TranslationUnitDecl *> makeTuDecls(
+      const std::string &SrcCode0, const std::string &SrcCode1, Language Lang) {
+    this->Code0 = SrcCode0;
+    this->Code1 = SrcCode1;
+    ArgVector Args = getBasicRunOptionsForLanguage(Lang);
+
+    const char *const InputFileName = "input.cc";
+
+    AST0 = tooling::buildASTFromCodeWithArgs(Code0, Args, InputFileName);
+    AST1 = tooling::buildASTFromCodeWithArgs(Code1, Args, InputFileName);
+
+    return std::make_tuple(AST0->getASTContext().getTranslationUnitDecl(),
+                           AST1->getASTContext().getTranslationUnitDecl());
+  }
+
   // Get a pair of node pointers into the synthesized AST from the given code
   // snippets. The same matcher is used for both snippets.
   template <typename NodeType, typename MatcherType>
   std::tuple<NodeType *, NodeType *> makeDecls(
       const std::string &SrcCode0, const std::string &SrcCode1, Language Lang,
       const MatcherType &AMatcher) {
     return makeDecls<NodeType, MatcherType>(
           SrcCode0, SrcCode1, Lang, AMatcher, AMatcher);
   }
 
   // Get a pair of Decl pointers to the synthesized declarations from the given
   // code snippets. We search for the first NamedDecl with given name in both
   // snippets.
   std::tuple<NamedDecl *, NamedDecl *> makeNamedDecls(
       const std::string &SrcCode0, const std::string &SrcCode1,
       Language Lang, const char *const Identifier = "foo") {
     auto Matcher = namedDecl(hasName(Identifier));
     return makeDecls<NamedDecl>(SrcCode0, SrcCode1, Lang, Matcher);
   }
 
-  bool testStructuralMatch(NamedDecl *D0, NamedDecl *D1) {
+  bool testStructuralMatch(Decl *D0, Decl *D1) {
     llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
     StructuralEquivalenceContext Ctx(
         D0->getASTContext(), D1->getASTContext(), NonEquivalentDecls,
         StructuralEquivalenceKind::Default, false, false);
     return Ctx.IsStructurallyEquivalent(D0, D1);
   }
 
-  bool testStructuralMatch(std::tuple<NamedDecl *, NamedDecl *> t) {
+  bool testStructuralMatch(std::tuple<Decl *, Decl *> t) {
     return testStructuralMatch(get<0>(t), get<1>(t));
   }
 };
 
 TEST_F(StructuralEquivalenceTest, Int) {
   auto Decls = makeNamedDecls("int foo;", "int foo;", Lang_CXX);
   EXPECT_TRUE(testStructuralMatch(Decls));
 }
auto t = makeDecls<FunctionDecl>(
       "struct X { virtual void f(); }; void X::f() { }",
       "struct X { void f(); }; void X::f() { }",
       Lang_CXX,
       functionDecl(allOf(hasName("f"), isDefinition())));
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 struct StructuralEquivalenceRecordTest : StructuralEquivalenceTest {
+  // FIXME Use a common getRecordDecl with ASTImporterTest.cpp!
+  RecordDecl *getRecordDecl(FieldDecl *FD) {
+    auto *ET = cast<ElaboratedType>(FD->getType().getTypePtr());
+    return cast<RecordType>(ET->getNamedType().getTypePtr())->getDecl();
+  };
 };
 
 TEST_F(StructuralEquivalenceRecordTest, Name) {
   auto t = makeDecls<CXXRecordDecl>(
       "struct A{ };",
       "struct B{ };",
       Lang_CXX,
       cxxRecordDecl(hasName("A")),
auto Code = R"(
         void x();
         int a;
       };
       )";
   auto t = makeNamedDecls(Code, Code, Lang_CXX);
   EXPECT_TRUE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceRecordTest, UnnamedRecordsShouldBeInequivalent) {
+  auto t = makeTuDecls(
+      R"(
+      struct A {
+        struct {
+          struct A *next;
+        } entry0;
+        struct {
+          struct A *next;
+        } entry1;
+      };
+      )",
+      "", Lang_C);
+  auto *TU = get<0>(t);
+  auto *Entry0 =
+      FirstDeclMatcher<FieldDecl>().match(TU, fieldDecl(hasName("entry0")));
+  auto *Entry1 =
+      FirstDeclMatcher<FieldDecl>().match(TU, fieldDecl(hasName("entry1")));
+  auto *R0 = getRecordDecl(Entry0);
+  auto *R1 = getRecordDecl(Entry1);
+
+  ASSERT_NE(R0, R1);
+  EXPECT_TRUE(testStructuralMatch(R0, R0));
+  EXPECT_TRUE(testStructuralMatch(R1, R1));
+  EXPECT_FALSE(testStructuralMatch(R0, R1));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+       UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
+  auto Code =
+      R"(
+      struct A {
+        struct {
+          struct A *next;
+        } entry0;
+        struct {
+          struct A *next;
+        } entry1;
+      };
+      )";
+  auto t = makeTuDecls(Code, Code, Lang_C);
+
+  auto *FromTU = get<0>(t);
+  auto *Entry1 =
+      FirstDeclMatcher<FieldDecl>().match(FromTU, fieldDecl(hasName("entry1")));
+
+  auto *ToTU = get<1>(t);
+  auto *Entry0 =
+      FirstDeclMatcher<FieldDecl>().match(ToTU, fieldDecl(hasName("entry0")));
+  auto *A =
+      FirstDeclMatcher<RecordDecl>().match(ToTU, recordDecl(hasName("A")));
+  A->startDefinition(); // Set isBeingDefined, getDefinition() will return a
+                        // nullptr. This may be the case during ASTImport.
+
+  auto *R0 = getRecordDecl(Entry0);
+  auto *R1 = getRecordDecl(Entry1);
+
+  ASSERT_NE(R0, R1);
+  EXPECT_TRUE(testStructuralMatch(R0, R0));
+  EXPECT_TRUE(testStructuralMatch(R1, R1));
+  EXPECT_FALSE(testStructuralMatch(R0, R1));
+}
+
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
       "struct A{ }; struct B{ }; void foo(A a, A b);",
       "struct A{ }; struct B{ }; void foo(A a, B b);",
       Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 } // end namespace ast_matchers
 } // end namespace clang