[ASTImporter] Typedef import brings in the complete type

[martong]

Summary

When we already have an incomplete underlying type of a typedef in the
"To" context, and the "From" context has the same typedef, but the
underlying type is complete, then the imported type should be complete.

Fixes an assertion in Xerces:
Assertion `DD && "queried property of class with no definition"' failed.
This assert is happening in the analyzer engine, because that attempts
to query an underlying type of a typedef, which happens to be
incomplete.

[a_sidorin]

Hi Gabor,
Thank you for the patch. I looks reasonable but I have some questions.

lib/AST/ASTImporter.cpp

2309

We can move these two vars upper and use them in the condition.

2313

This condition omits the case where both types are complete. Should we use FromUT->isIncompleteType == FoundUT-isIncompleteType() instead?

[martong]

• Move FromUT upper, add break

[martong]

lib/AST/ASTImporter.cpp

2309

Good point, changed it.

2313

I think you wanted to say "both types are INcomplete".
The condition indeed omits the case where both types are incomplete and that is a potential bug, thanks for finding it!
I think the best way to handle this case is to do the something similar what is done in case of functions, i.e break the for loop once we know that the found and the to be imported Decl are structurally equivalent.
So, I added a break to do that and to be similar what we do in case of functions.

This is what we have with functions:

if (IsStructuralMatch(D, FoundFunction)) {
  const FunctionDecl *Definition = nullptr;
  if (D->doesThisDeclarationHaveABody() &&
      FoundFunction->hasBody(Definition)) {
    return Importer.MapImported(
        D, const_cast<FunctionDecl *>(Definition));
  }
  FoundByLookup = FoundFunction;
  break;
}

I'd like to have something similar with typedefs. The goal is to have only one TypedefNameDecl with a complete type. A TypedefNameDecl with a complete type is analogous to a FunctionDecl with a definition, we want to have only one in the "To" context.
A TypedefNameDecl is also redeclarable, so on a long term we should connect a new Decl to the found previous one.

[a_sidorin]

Thanks!

[shafik]

Apologies for my late comments.

cfe/trunk/unittests/AST/ASTImporterTest.cpp

3780 ↗

(On Diff #175422)

As far as I can tell S<int> is complete in Code, am I missing something?

3792 ↗

(On Diff #175422)

Given my previous comment, I don't see why the addition code would change where S<int> was complete or not.

[martong]

cfe/trunk/unittests/AST/ASTImporterTest.cpp

3780 ↗

(On Diff #175422)

No, S<int> is incomplete at this point. The specialization is indeed created, but its type is incomplete. There is no need for the type to be complete because there is not any operation which would require the size of the type.
This is how the AST looks like:

|-ClassTemplateDecl 0x1f5b378 <output.cc:2:7, line:5:7> line:3:14 S
| |-TemplateTypeParmDecl 0x1f5b228 <line:2:17, col:26> col:26 typename depth 0 index 0 T
| |-CXXRecordDecl 0x1f5b2e0 <line:3:7, line:5:7> line:3:14 struct S definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x1f5b5e0 <col:7, col:14> col:14 implicit struct S
| | `-CXXMethodDecl 0x1f5b700 <line:4:9, col:18> col:14 foo 'void ()'
| `-ClassTemplateSpecializationDecl 0x1f5b7d8 <line:2:7, line:5:7> line:3:14 struct S
|   `-TemplateArgument type 'int'
`-TypeAliasDecl 0x1f5b988 <line:6:7, col:22> col:13 U 'S<int>':'S<int>'
  `-TemplateSpecializationType 0x1f5b8e0 'S<int>' sugar S
    |-TemplateArgument type 'int'
    `-RecordType 0x1f5b8c0 'S<int>'
      `-ClassTemplateSpecialization 0x1f5b7d8 'S'

However, below by using the member access operator (->) we require the completion of the type. Alas, the specialization has a definition there, not just a fwd declaration.
The AST of FromTU looks like this:

|-ClassTemplateDecl 0x2033c28 <input.cc:2:7, line:5:7> line:3:14 S
| |-TemplateTypeParmDecl 0x2033ad8 <line:2:17, col:26> col:26 typename depth 0 index 0 T
| |-CXXRecordDecl 0x2033b90 <line:3:7, line:5:7> line:3:14 struct S definition
| | |-DefinitionData empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
| | | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
| | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveConstructor exists simple trivial needs_implicit
| | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
| | | |-MoveAssignment exists simple trivial needs_implicit
| | | `-Destructor simple irrelevant trivial needs_implicit
| | |-CXXRecordDecl 0x2033e90 <col:7, col:14> col:14 implicit struct S
| | `-CXXMethodDecl 0x2033fb0 <line:4:9, col:18> col:14 foo 'void ()'
| `-ClassTemplateSpecializationDecl 0x2034088 <line:2:7, line:5:7> line:3:14 struct S definition
|   |-DefinitionData pass_in_registers empty aggregate standard_layout trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor can_const_default_init
|   | |-DefaultConstructor exists trivial constexpr needs_implicit defaulted_is_constexpr
|   | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveConstructor exists simple trivial needs_implicit
|   | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
|   | |-MoveAssignment exists simple trivial needs_implicit
|   | `-Destructor simple irrelevant trivial needs_implicit
|   |-TemplateArgument type 'int'
|   |-CXXRecordDecl 0x2034598 prev 0x2034088 <col:7, col:14> col:14 implicit struct S
|   `-CXXMethodDecl 0x2034630 <line:4:9, col:18> col:14 used foo 'void ()'
|-TypeAliasDecl 0x2034238 <line:6:7, col:22> col:13 referenced U 'S<int>':'S<int>'
| `-TemplateSpecializationType 0x2034190 'S<int>' sugar S
|   |-TemplateArgument type 'int'
|   `-RecordType 0x2034170 'S<int>'
|     `-ClassTemplateSpecialization 0x2034088 'S'
`-FunctionDecl 0x2034420 <line:8:7, line:10:7> line:8:12 foo 'void (U *)'
  |-ParmVarDecl 0x2034318 <col:16, col:19> col:19 used u 'U *'
  `-CompoundStmt 0x2061a38 <col:22, line:10:7>
    `-CXXMemberCallExpr 0x2061a10 <line:9:9, col:16> 'void'
      `-MemberExpr 0x2034718 <col:9, col:12> '<bound member function type>' ->foo 0x2034630
        `-ImplicitCastExpr 0x2034700 <col:9> 'U *' <LValueToRValue>
          `-DeclRefExpr 0x2034508 <col:9> 'U *' lvalue ParmVar 0x2034318 'u' 'U *'

lib/AST/ASTImporter.cpp

if (!DC->isFunctionOrMethod()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     unsigned IDNS = Decl::IDNS_Ordinary;
     SmallVector<NamedDecl *, 2> FoundDecls;
     DC->getRedeclContext()->localUncachedLookup(Name, FoundDecls);
     for (auto *FoundDecl : FoundDecls) {
       if (!FoundDecl->isInIdentifierNamespace(IDNS))
         continue;
       if (auto *FoundTypedef = dyn_cast<TypedefNameDecl>(FoundDecl)) {
-        if (Importer.IsStructurallyEquivalent(
-                D->getUnderlyingType(), FoundTypedef->getUnderlyingType()))
+        QualType FromUT = D->getUnderlyingType();
+        QualType FoundUT = FoundTypedef->getUnderlyingType();
+        if (Importer.IsStructurallyEquivalent(FromUT, FoundUT)) {
+          // If the "From" context has a complete underlying type but we
+          // already have a complete underlying type then return with that.
+          if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
             return Importer.MapImported(D, FoundTypedef);
         }
+        // FIXME Handle redecl chain.
+        break;
+      }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, IDNS,
                                          ConflictingDecls.data(),
                                          ConflictingDecls.size());

unittests/AST/ASTImporterTest.cpp

TEST_P(ImportFunctionTemplateSpecializations, DefinitionThenPrototype) {
   auto *To1 = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
   EXPECT_TRUE(ImportedD == To0);
   EXPECT_TRUE(ImportedD != To1);
   EXPECT_TRUE(To0->doesThisDeclarationHaveABody());
   EXPECT_FALSE(To1->doesThisDeclarationHaveABody());
   EXPECT_EQ(To1->getPreviousDecl(), To0);
 }
 
+TEST_P(ASTImporterTestBase, ImportingTypedefShouldImportTheCompleteType) {
+  // We already have an incomplete underlying type in the "To" context.
+  auto Code =
+      R"(
+      template <typename T>
+      struct S {
+        void foo();
+      };
+      using U = S<int>;
+      )";
+  Decl *ToTU = getToTuDecl(Code, Lang_CXX11);
+  auto *ToD = FirstDeclMatcher<TypedefNameDecl>().match(ToTU,
+      typedefNameDecl(hasName("U")));
+  ASSERT_TRUE(ToD->getUnderlyingType()->isIncompleteType());
+
+  // The "From" context has the same typedef, but the underlying type is
+  // complete this time.
+  Decl *FromTU = getTuDecl(std::string(Code) +
+      R"(
+      void foo(U* u) {
+        u->foo();
+      }
+      )", Lang_CXX11);
+  auto *FromD = FirstDeclMatcher<TypedefNameDecl>().match(FromTU,
+      typedefNameDecl(hasName("U")));
+  ASSERT_FALSE(FromD->getUnderlyingType()->isIncompleteType());
+
+  // The imported type should be complete.
+  auto *ImportedD = cast<TypedefNameDecl>(Import(FromD, Lang_CXX11));
+  EXPECT_FALSE(ImportedD->getUnderlyingType()->isIncompleteType());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, CanonicalRedeclChain,
     ::testing::Values(ArgVector()),);
 
 auto DefaultTestValuesForRunOptions = ::testing::Values(