[ASTImporter] Fix duplicate class template definitions problem

[martong]

Summary

We fail to import a ClassTemplateDecl if the "To" context already contains a definition and then a forward decl.
This is because localUncachedLookup does not find the definition.
This is not a lookup error, the parser behaves differently than assumed in the importer code.
A DeclContext contains one DenseMap (LookupPtr) which maps names to lists.
The list is a special list StoredDeclsList which is optimized to have one element.
During building the initial AST, the parser first adds the definition to the DeclContext.
Then during parsing the second declaration (the forward decl) the parser again calls DeclContext::addDecl but that will not add a new element to the StoredDeclsList rarther it simply overwrites the old element with the most recent one.
This patch fixes the error by finding the definition in the redecl chain.
Added tests for the same issue with CXXRecordDecl and with ClassTemplateSpecializationDecl.
These tests pass and they pass because in VisitRecordDecl and in VisitClassTemplateSpecializationDecl we already use D->getDefinition() after the lookup.

[a.sidorin]

Hello Gabor,

The patch is fine, I have found only some style issues.

lib/AST/ASTImporter.cpp

4073

forward

4121–4127

is a definition?

4124

let's; if it is available

4126

• is misplaced here.

unittests/AST/ASTImporterTest.cpp

1550

Although long names make me happy, this one exceeds 80-char limit. We can abbreviate "Specialization" to "Spec".

unittests/AST/DeclMatcher.h

52

Predicate, Count (member names should start with capital).

[martong]

• Addressing Aleksei's comments

[a.sidorin]

LGTM, thanks!

[a.sidorin]

Hm. Should we test -fms-compatibility in addition to -fdelayed-template-parsing?

[martong]

@a.sidorin
Yes, that is a very good idea.
Just created a new patch which adds that switch for the tests which use the TestBase.
https://reviews.llvm.org/D47367

lib/AST/ASTImporter.cpp

ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
   return TemplateTemplateParmDecl::Create(Importer.getToContext(), 
                               Importer.getToContext().getTranslationUnitDecl(), 
                                           Loc, D->getDepth(), D->getPosition(),
                                           D->isParameterPack(),
                                           Name.getAsIdentifierInfo(), 
                                           TemplateParams);
 }
 
+// Returns the definition for a (forward) declaration of a ClassTemplateDecl, if
+// it has any definition in the redecl chain.
+static ClassTemplateDecl *getDefinition(ClassTemplateDecl *D) {
+  CXXRecordDecl *ToTemplatedDef = D->getTemplatedDecl()->getDefinition();
+  if (!ToTemplatedDef)
+    return nullptr;
+  ClassTemplateDecl *TemplateWithDef =
+      ToTemplatedDef->getDescribedClassTemplate();
+  return TemplateWithDef;
+}
+
 Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
   // If this record has a definition in the translation unit we're coming from,
   // but this particular declaration is not that definition, import the
   // definition and map to that.
   auto *Definition =
       cast_or_null<CXXRecordDecl>(D->getTemplatedDecl()->getDefinition());
   if (Definition && Definition != D->getTemplatedDecl()) {
     Decl *ImportedDef
       = Importer.Import(Definition->getDescribedClassTemplate());
     if (!ImportedDef)
       return nullptr;
 
     return Importer.Imported(D, ImportedDef);
   }
-  
   // Import the major distinguishing characteristics of this class template.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return nullptr;
   if (ToD)
     return ToD;
 
   // We may already have a template of the same name; try to find and match it.
   if (!DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     SmallVector<NamedDecl *, 2> FoundDecls;
     DC->getRedeclContext()->localUncachedLookup(Name, FoundDecls);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(Decl::IDNS_Ordinary))
         continue;
-      
       Decl *Found = FoundDecl;
       if (auto *FoundTemplate = dyn_cast<ClassTemplateDecl>(Found)) {
-        if (IsStructuralMatch(D, FoundTemplate)) {
-          // The class templates structurally match; call it the same template.
 
-          // We found a forward declaration but the class to be imported has a
-          // definition.
-          // FIXME Add this forward declaration to the redeclaration chain.
-          if (D->isThisDeclarationADefinition() &&
-              !FoundTemplate->isThisDeclarationADefinition())
+        // The class to be imported is a definition.
+        if (D->isThisDeclarationADefinition()) {
+          // Lookup will find the fwd decl only if that is more recent than the
+          // definition. So, try to get the definition if that is available in
+          // the redecl chain.
+          ClassTemplateDecl *TemplateWithDef = getDefinition(FoundTemplate);
+          if (!TemplateWithDef)
             continue;
+          FoundTemplate = TemplateWithDef; // Continue with the definition.
+        }
+
+        if (IsStructuralMatch(D, FoundTemplate)) {
+          // The class templates structurally match; call it the same template.
 
-          Importer.Imported(D->getTemplatedDecl(), 
+          Importer.Imported(D->getTemplatedDecl(),
                             FoundTemplate->getTemplatedDecl());
           return Importer.Imported(D, FoundTemplate);
-        }         
+        }
       }
-      
       ConflictingDecls.push_back(FoundDecl);
     }
-    
     if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, Decl::IDNS_Ordinary,
-                                         ConflictingDecls.data(), 
+                                         ConflictingDecls.data(),
                                          ConflictingDecls.size());
     }
-    
     if (!Name)
       return nullptr;
   }
 
   CXXRecordDecl *FromTemplated = D->getTemplatedDecl();
 
   // Create the declaration that is being templated.
   auto *ToTemplated = cast_or_null<CXXRecordDecl>(

unittests/AST/ASTImporterTest.cpp

getImportedDecl(StringRef FromSrcCode, Language FromLang, StringRef ToSrcCode,
 
     assert(FoundDecls.size() == 1);
 
     Decl *Imported = Importer.Import(FoundDecls.front());
     assert(Imported);
     return std::make_tuple(*FoundDecls.begin(), Imported);
   }
 
-  // Creates a TU decl for the given source code.
-  // May be called several times in a given test.
+  // Creates a TU decl for the given source code which can be used as a From
+  // context.  May be called several times in a given test (with different file
+  // name).
   TranslationUnitDecl *getTuDecl(StringRef SrcCode, Language Lang,
                                  StringRef FileName = "input.cc") {
     assert(
         std::find_if(FromTUs.begin(), FromTUs.end(), [FileName](const TU &E) {
           return E.FileName == FileName;
         }) == FromTUs.end());
 
     ArgVector Args = getArgVectorForLanguage(Lang);
     FromTUs.emplace_back(SrcCode, FileName, Args);
     TU &Tu = FromTUs.back();
 
     return Tu.TUDecl;
   }
 
+  // Creates the To context with the given source code and returns the TU decl.
+  TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
+    ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+    ToCode = ToSrcCode;
+    assert(!ToAST);
+    ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+
+    return ToAST->getASTContext().getTranslationUnitDecl();
+  }
+
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
   Decl *Import(Decl *From, Language ToLang) {
     if (!ToAST) {
       ArgVector ToArgs = getArgVectorForLanguage(ToLang);
       // Build the AST from an empty file.
       ToAST =
TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
     Import(FromD, Lang_CXX);
     auto *FromCTD = FirstDeclMatcher<ClassTemplateDecl>().match(
         FromTU, classTemplateDecl(hasName("B")));
     auto *ToCTD = cast<ClassTemplateDecl>(Import(FromCTD, Lang_CXX));
     EXPECT_TRUE(ToCTD->isThisDeclarationADefinition());
   }
 }
 
+TEST_P(ASTImporterTestBase,
+       ImportDefinitionOfClassTemplateIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <typename T>
+      struct B {
+        void f();
+      };
+
+      template <typename T>
+      struct B;
+      )",
+      Lang_CXX);
+  ASSERT_EQ(1u, DeclCounterWithPredicate<ClassTemplateDecl>(
+                    [](const ClassTemplateDecl *T) {
+                      return T->isThisDeclarationADefinition();
+                    })
+                    .match(ToTU, classTemplateDecl()));
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <typename T>
+      struct B {
+        void f();
+      };
+      )",
+      Lang_CXX, "input1.cc");
+  ClassTemplateDecl *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+      FromTU, classTemplateDecl(hasName("B")));
+
+  Import(FromD, Lang_CXX);
+
+  // We should have only one definition.
+  EXPECT_EQ(1u, DeclCounterWithPredicate<ClassTemplateDecl>(
+                    [](const ClassTemplateDecl *T) {
+                      return T->isThisDeclarationADefinition();
+                    })
+                    .match(ToTU, classTemplateDecl()));
+}
+
+TEST_P(ASTImporterTestBase,
+       ImportDefinitionOfClassIfThereIsAnExistingFwdDeclAndDefinition) {
+  Decl *ToTU = getToTuDecl(
+      R"(
+      struct B {
+        void f();
+      };
+
+      struct B;
+      )",
+      Lang_CXX);
+  ASSERT_EQ(2u, DeclCounter<CXXRecordDecl>().match(
+                    ToTU, cxxRecordDecl(hasParent(translationUnitDecl()))));
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      struct B {
+        void f();
+      };
+      )",
+      Lang_CXX, "input1.cc");
+  auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
+      FromTU, cxxRecordDecl(hasName("B")));
+
+  Import(FromD, Lang_CXX);
+
+  EXPECT_EQ(2u, DeclCounter<CXXRecordDecl>().match(
+                    ToTU, cxxRecordDecl(hasParent(translationUnitDecl()))));
+}
+
+TEST_P(
+    ASTImporterTestBase,
+    ImportDefinitionOfClassTemplateSpecIfThereIsAnExistingFwdDeclAndDefinition)
+{
+  Decl *ToTU = getToTuDecl(
+      R"(
+      template <typename T>
+      struct B;
+
+      template <>
+      struct B<int> {};
+
+      template <>
+      struct B<int>;
+      )",
+      Lang_CXX);
+  // We should have only one definition.
+  ASSERT_EQ(1u, DeclCounterWithPredicate<ClassTemplateSpecializationDecl>(
+                    [](const ClassTemplateSpecializationDecl *T) {
+                      return T->isThisDeclarationADefinition();
+                    })
+                    .match(ToTU, classTemplateSpecializationDecl()));
+
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <typename T>
+      struct B;
+
+      template <>
+      struct B<int> {};
+      )",
+      Lang_CXX, "input1.cc");
+  auto *FromD = FirstDeclMatcher<ClassTemplateSpecializationDecl>().match(
+      FromTU, classTemplateSpecializationDecl(hasName("B")));
+
+  Import(FromD, Lang_CXX);
+
+  // We should have only one definition.
+  EXPECT_EQ(1u, DeclCounterWithPredicate<ClassTemplateSpecializationDecl>(
+                    [](const ClassTemplateSpecializationDecl *T) {
+                      return T->isThisDeclarationADefinition();
+                    })
+                    .match(ToTU, classTemplateSpecializationDecl()));
+}
+
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ASTImporterTestBase,
     ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
 
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
        PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition) {

unittests/AST/DeclMatcher.h

public:
   }
 };
 template <typename NodeType>
 using LastDeclMatcher = DeclMatcher<NodeType, DeclMatcherKind::Last>;
 template <typename NodeType>
 using FirstDeclMatcher = DeclMatcher<NodeType, DeclMatcherKind::First>;
 
 template <typename NodeType>
-class DeclCounter : public MatchFinder::MatchCallback {
+class DeclCounterWithPredicate : public MatchFinder::MatchCallback {
+  using UnaryPredicate = std::function<bool(const NodeType *)>;
+  UnaryPredicate Predicate;
   unsigned Count = 0;
   void run(const MatchFinder::MatchResult &Result) override {
-      if(Result.Nodes.getNodeAs<NodeType>("")) {
+    if (auto N = Result.Nodes.getNodeAs<NodeType>("")) {
+      if (Predicate(N))
         ++Count;
-      }
   }
+
 public:
-  // Returns the number of matched nodes under the tree rooted in `D`.
+  DeclCounterWithPredicate()
+      : Predicate([](const NodeType *) { return true; }) {}
+  DeclCounterWithPredicate(UnaryPredicate P) : Predicate(P) {}
+  // Returns the number of matched nodes which satisfy the predicate under the
+  // tree rooted in `D`.
   template <typename MatcherType>
   unsigned match(const Decl *D, const MatcherType &AMatcher) {
     MatchFinder Finder;
     Finder.addMatcher(AMatcher.bind(""), this);
     Finder.matchAST(D->getASTContext());
     return Count;
   }
 };
 
+template <typename NodeType>
+using DeclCounter = DeclCounterWithPredicate<NodeType>;
+
 } // end namespace ast_matchers
 } // end namespace clang
 
 #endif