[ASTImporter] Extend lookup logic in class templates

[martong]

Summary

During import of a class template, lookup may find a forward declaration and structural match falsely reports equivalency in between a fwd decl and a definition. This results that some definitions are not imported if we had imported a fwd decl previously. This patch gives a fix.

[martong]

During import of a class template, lookup may find a forward declaration and structural match falsely reports equivalency in between a fwd decl and a definition.

This can happen when the class to be imported does not have any data members. Structural equivalency check the data members only (and not any of the member functions).

[a.sidorin]

Hello Gabor,

Thank you for the patch. It looks mostly good, but I have a question: should we add this new declaration to the redeclaration chain like we do it for RecordDecls?

[martong]

should we add this new declaration to the redeclaration chain like we do it for RecordDecls?

I think, on a long term we should. Otherwise we could loose e.g. C++11 attributes which are attached to the forward declaration only.
However, I'd do that as a separate commit, because that would require some independent changes and tests, also other decl kinds like ClassTemplateSepcializationDecl may be affected as well by that.

[a.sidorin]

In D46353#1086456, @martong wrote:

should we add this new declaration to the redeclaration chain like we do it for RecordDecls?

I think, on a long term we should. Otherwise we could loose e.g. C++11 attributes which are attached to the forward declaration only.
However, I'd do that as a separate commit, because that would require some independent changes and tests, also other decl kinds like ClassTemplateSepcializationDecl may be affected as well by that.

I'm fine with this. But a FIXME would be very appreciated.

[martong]

• Add FIXME

[martong]

Hi Aleksei,

Added the FIXME, can you help me with committing this?

lib/AST/ASTImporter.cpp

if (!DC->isFunctionOrMethod()) {
     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.
-          // FIXME: We may be filling in a forward declaration here. Handle
-          // this case!
+
+          // 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())
+            continue;
+
           Importer.Imported(D->getTemplatedDecl(), 
                             FoundTemplate->getTemplatedDecl());
           return Importer.Imported(D, FoundTemplate);
         }         
       }
       
       ConflictingDecls.push_back(FoundDecl);
     }

unittests/AST/ASTImporterTest.cpp

auto Pattern = translationUnitDecl(has(classTemplateDecl(
       hasName("Base"),
       has(classTemplateSpecializationDecl(has(fieldDecl(hasName("a"))))))));
   ASSERT_TRUE(
       MatchVerifier<Decl>{}.match(From->getTranslationUnitDecl(), Pattern));
   EXPECT_TRUE(
       MatchVerifier<Decl>{}.match(To->getTranslationUnitDecl(), Pattern));
 }
 
+TEST_P(ASTImporterTestBase, ImportDefinitionOfClassTemplateAfterFwdDecl) {
+  {
+    Decl *FromTU = getTuDecl(
+        R"(
+            template <typename T>
+            struct B;
+            )",
+        Lang_CXX, "input0.cc");
+    auto *FromD = FirstDeclMatcher<ClassTemplateDecl>().match(
+        FromTU, classTemplateDecl(hasName("B")));
+
+    Import(FromD, Lang_CXX);
+  }
+
+  {
+    Decl *FromTU = getTuDecl(
+        R"(
+            template <typename T>
+            struct B {
+              void f();
+            };
+            )",
+        Lang_CXX, "input1.cc");
+    FunctionDecl *FromD = FirstDeclMatcher<FunctionDecl>().match(
+        FromTU, functionDecl(hasName("f")));
+    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());
+  }
+}
+
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ASTImporterTestBase,
     ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
 
 struct ImportFunctions : ASTImporterTestBase {};
 
 TEST_P(ImportFunctions,
        PrototypeShouldBeImportedAsAPrototypeWhenThereIsNoDefinition) {