Fix duplicate class template definitions problem

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.

Reviewers: a.sidorin, xazax.hun, szepet

Subscribers: rnkovacs, dkrupp, cfe-commits

Differential Revision: https://reviews.llvm.org/D46950

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@333082 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/AST/ASTImporter.cpp b/lib/AST/ASTImporter.cpp
index 6fa4617cf0614bec377dd9e6d50ac882e4422ff4..3c69897436425f390bd74994143f3e4be713d237 100644 (file)
--- a/lib/AST/ASTImporter.cpp
+++ b/lib/AST/ASTImporter.cpp
@@ -4070,6 +4070,17 @@ ASTNodeImporter::VisitTemplateTemplateParmDecl(TemplateTemplateParmDecl *D) {
                                           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
@@ -4084,7 +4095,7 @@ Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
 
     return Importer.Imported(D, ImportedDef);
   }
-  
+
   // Import the major distinguishing characteristics of this class template.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
@@ -4103,34 +4114,39 @@ Decl *ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
     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;
   }

diff --git a/unittests/AST/ASTImporterTest.cpp b/unittests/AST/ASTImporterTest.cpp
index 7d6748e550920b0b44387fae37a35856de921381..18c8f4cc02c3f70874824613e18805578c284309 100644 (file)
--- a/unittests/AST/ASTImporterTest.cpp
+++ b/unittests/AST/ASTImporterTest.cpp
@@ -281,8 +281,9 @@ public:
     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(
@@ -297,6 +298,16 @@ public:
     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.
@@ -1464,6 +1475,121 @@ TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
   }
 }
 
+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"}),);

diff --git a/unittests/AST/DeclMatcher.h b/unittests/AST/DeclMatcher.h
index c73a4cb49f26b8c63a356aeaee26b8ebce6e59a6..602f8dff07feae6a5a1180ce22d3063adf798277 100644 (file)
--- a/unittests/AST/DeclMatcher.h
+++ b/unittests/AST/DeclMatcher.h
@@ -44,15 +44,23 @@ 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;
@@ -62,6 +70,9 @@ public:
   }
 };
 
+template <typename NodeType>
+using DeclCounter = DeclCounterWithPredicate<NodeType>;
+
 } // end namespace ast_matchers
 } // end namespace clang