[ASTImporter] Support importing UnresolvedMemberExpr, DependentNameType, DependentScopeDeclRefExpr

[szepet]

Summary

The visit callback implementations for the 3 C++ AST Node added to the ASTImporter.

Note: Based on: https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp

[xazax.hun]

lib/AST/ASTImporter.cpp

6220

According to phabricator this code is very similar to a snippet starting from line 4524 and some code bellow. Maybe it would be worth to have a function instead of duplicating?

[szepet]

lib/AST/ASTImporter.cpp

6220

Good point, I would do it in a separate patch and add it as a dependency if it is OK.

[xazax.hun]

lib/AST/ASTImporter.cpp

6220

Sure, that would be awesome. :)

[a.sidorin]

Hello Peter,

Looks mostly good, but there are some minor comments.

lib/AST/ASTImporter.cpp

6220

This is a template I have prepared for a patch still non-commited yet:

template<typename InContainerTy>
bool ImportTemplateArgumentListInfo(const InContainerTy &Container,
                                    TemplateArgumentListInfo &ToTAInfo) {
  for (const auto &FromLoc : Container) {
    if (auto ToLoc = ImportTemplateArgumentLoc(FromLoc))
      ToTAInfo.addArgument(*ToLoc);
    else
      return true;
  }
  return false;
}

6293

Needed space after if.

unittests/AST/ASTImporterTest.cpp

1732

Could you please align the code as conventions require?

[szepet]

Updated based on review comments.

Hello Aleksei,
Thank for the review and the code snippet as well!

[a.sidorin]

Hello Peter,

This looks good to me. But could you please check if test works if we add -fms-compatibility and -fdelayed-template-parsing to Args?

unittests/AST/ASTImporterTest.cpp

1731

Uninstantiated templates make me worry about Windows buildbots. If they will start to fail, we should test these matchers with -fms-compatibility and -fdelayed-template-parsing options enabled.

[szepet]

unittests/AST/ASTImporterTest.cpp

1731

Not sure about Windows but I can ensure that this test has passed with these args added as well.

[a.sidorin]

Hello Peter. Please set the dependencies for the patch - it cannot be applied clearly and even if I add ImportTemplateArgumentListInfo, tests still fail - looks like FunctionTemplateDecl patch should be applied first.

[xazax.hun]

This should be rebased to latest master.

[a.sidorin]

Hi Peter,
The changes needed for rebase are too large - they contain both fixes for compilation failures and fixes for test errors. Could you please upgrade the patch?

[szepet]

Hello Aleksei,

Thanks for carrying the ASTImporter improvements (in general)! I will rebase it and upload for sure, but probably just next week. (After EuroLLVM.)

[szepet]

Rewritten the tests using the newly added TEST_P method.
This patch failed earlier when -fdelayed-template-parsing flag was enabled, however, the actual import process was OK but the original AST havent included the checked nodes.
TEST_P made possible to check whether the original code contains the node we would like to import (and we test only in this case, since otherwise it does not make sense).

[martong]

unittests/AST/ASTImporterTest.cpp

1716

Instead of having 4 FromTUs, perhaps a simpler alternative would be to have only one, which contains all the code snippets with declToImport0 ... declToImport3 (Or just skip declToImport since we no longer need that phabricated name, so we could just have a simple name like X) and importing them one by one later with ASTImporterTestBase::Import.
This would spare the need for the transform and the for cycle below.

1729

I think, this if would be needed only if the test suite would be parameterized also with ArgVector{"-fdelayed-template-parsing"}, but that is not.

1733

It will be hard to notice based on the test output which DSDRE's import was unsuccessful. Therefore I'd unroll the for loop instead, also please see the above comment that it would be more practical to have one FromTU.

[szepet]

unittests/AST/ASTImporterTest.cpp

1729

Its necessary, since on windows platforms we have that flag by default (so, it does not matter that we dont include here, it will be used anyway).

1733

I understand the concern, but I am really against the mentioned design. The main point of this code is that whenever we would like to add one more test cases, then we need to add it to the vector and everything else is done. Other than that, I would say that debugging a broken test case is already a "time expensive" job, so in that case it wouldnt make much of a different, since you need to look at the code closely anyway.

[a.sidorin]

unittests/AST/ASTImporterTest.cpp

1729

The declaration should be always present independently of flags so if is invalid here. To achieve this, we use explicit template instantiations in the test code. See ImportCXXDependentScopeMemberExpr test for reference. Explicit instantiations also allow us to find errors in the template code itself.

1737

Please don't do this - always test the code with both flag options. Explicit instantiations will help you to do this. Otherwise, windows buildbot will add the flag on its own causing the test to crash after the commit.

[szepet]

Yepp, good point, Aleksei, rewritten the tests using explicit instantiation.

[martong]

LGTM!

unittests/AST/ASTImporterTest.cpp

1665

Perhaps a newline before would make it more independent from the test case instantiation.

[martong]

unittests/AST/ASTImporterTest.cpp

1692

Perhaps a newline before would make it more independent from the Matcher, as you did it at line 1544 .

[a.sidorin]

Hello Peter! This looks almost OK, just some minor formatting issues.

unittests/AST/ASTImporterTest.cpp

1669

Please add spaces after and before {}.

1670

Indentation is a bit off here and in the samples below.

[szepet]

Format changes added based on comments.

[a.sidorin]

LGTM!

lib/AST/ASTImporter.cpp

public:
     // FIXME: DependentDecltypeType
     QualType VisitRecordType(const RecordType *T);
     QualType VisitEnumType(const EnumType *T);
     QualType VisitAttributedType(const AttributedType *T);
     QualType VisitTemplateTypeParmType(const TemplateTypeParmType *T);
     QualType VisitSubstTemplateTypeParmType(const SubstTemplateTypeParmType *T);
     QualType VisitTemplateSpecializationType(const TemplateSpecializationType *T);
     QualType VisitElaboratedType(const ElaboratedType *T);
-    // FIXME: DependentNameType
+    QualType VisitDependentNameType(const DependentNameType *T);
     QualType VisitPackExpansionType(const PackExpansionType *T);
     QualType VisitDependentTemplateSpecializationType(
         const DependentTemplateSpecializationType *T);
     QualType VisitObjCInterfaceType(const ObjCInterfaceType *T);
     QualType VisitObjCObjectType(const ObjCObjectType *T);
     QualType VisitObjCObjectPointerType(const ObjCObjectPointerType *T);
                             
     // Importing declarations                            
public:
     Expr *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *E);
     Expr *VisitPackExpansionExpr(PackExpansionExpr *E);
     Expr *VisitSizeOfPackExpr(SizeOfPackExpr *E);
     Expr *VisitCXXNewExpr(CXXNewExpr *CE);
     Expr *VisitCXXDeleteExpr(CXXDeleteExpr *E);
     Expr *VisitCXXConstructExpr(CXXConstructExpr *E);
     Expr *VisitCXXMemberCallExpr(CXXMemberCallExpr *E);
     Expr *VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E);
+    Expr *VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E);
     Expr *VisitCXXUnresolvedConstructExpr(CXXUnresolvedConstructExpr *CE);
     Expr *VisitUnresolvedLookupExpr(UnresolvedLookupExpr *E);
+    Expr *VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E);
     Expr *VisitExprWithCleanups(ExprWithCleanups *EWC);
     Expr *VisitCXXThisExpr(CXXThisExpr *E);
     Expr *VisitCXXBoolLiteralExpr(CXXBoolLiteralExpr *E);
     Expr *VisitCXXPseudoDestructorExpr(CXXPseudoDestructorExpr *E);
     Expr *VisitMemberExpr(MemberExpr *E);
     Expr *VisitCallExpr(CallExpr *E);
     Expr *VisitLambdaExpr(LambdaExpr *LE);
     Expr *VisitInitListExpr(InitListExpr *E);
QualType ASTNodeImporter::VisitDependentTemplateSpecializationType(
   ToPack.reserve(T->getNumArgs());
   if (ImportTemplateArguments(T->getArgs(), T->getNumArgs(), ToPack))
     return {};
 
   return Importer.getToContext().getDependentTemplateSpecializationType(
         T->getKeyword(), Qualifier, Name, ToPack);
 }
 
+QualType ASTNodeImporter::VisitDependentNameType(const DependentNameType *T) {
+  NestedNameSpecifier *NNS = Importer.Import(T->getQualifier());
+  if (!NNS && T->getQualifier())
+    return QualType();
+
+  IdentifierInfo *Name = Importer.Import(T->getIdentifier());
+  if (!Name && T->getIdentifier())
+    return QualType();
+
+  QualType Canon = (T == T->getCanonicalTypeInternal().getTypePtr())
+                       ? QualType()
+                       : Importer.Import(T->getCanonicalTypeInternal());
+  if (!Canon.isNull())
+    Canon = Canon.getCanonicalType();
+
+  return Importer.getToContext().getDependentNameType(T->getKeyword(), NNS,
+                                                      Name, Canon);
+}
+
 QualType ASTNodeImporter::VisitObjCInterfaceType(const ObjCInterfaceType *T) {
   auto *Class =
       dyn_cast_or_null<ObjCInterfaceDecl>(Importer.Import(T->getDecl()));
   if (!Class)
     return {};
 
   return Importer.getToContext().getObjCInterfaceType(Class);
 }
Expr *ASTNodeImporter::VisitCXXDependentScopeMemberExpr(
   return CXXDependentScopeMemberExpr::Create(
       Importer.getToContext(), Base, BaseType, E->isArrow(),
       Importer.Import(E->getOperatorLoc()),
       Importer.Import(E->getQualifierLoc()),
       Importer.Import(E->getTemplateKeywordLoc()),
       cast_or_null<NamedDecl>(ToFQ), MemberNameInfo, ResInfo);
 }
 
+Expr *
+ASTNodeImporter::VisitDependentScopeDeclRefExpr(DependentScopeDeclRefExpr *E) {
+  DeclarationName Name = Importer.Import(E->getDeclName());
+  if (!E->getDeclName().isEmpty() && Name.isEmpty())
+    return nullptr;
+
+  DeclarationNameInfo NameInfo(Name, Importer.Import(E->getExprLoc()));
+  ImportDeclarationNameLoc(E->getNameInfo(), NameInfo);
+
+  TemplateArgumentListInfo ToTAInfo(Importer.Import(E->getLAngleLoc()),
+                                    Importer.Import(E->getRAngleLoc()));
+  TemplateArgumentListInfo *ResInfo = nullptr;
+  if (E->hasExplicitTemplateArgs()) {
+    if (ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+      return nullptr;
+    ResInfo = &ToTAInfo;
+  }
+
+  return DependentScopeDeclRefExpr::Create(
+      Importer.getToContext(), Importer.Import(E->getQualifierLoc()),
+      Importer.Import(E->getTemplateKeywordLoc()), NameInfo, ResInfo);
+}
+
 Expr *ASTNodeImporter::VisitCXXUnresolvedConstructExpr(
     CXXUnresolvedConstructExpr *CE) {
   unsigned NumArgs = CE->arg_size();
 
   SmallVector<Expr *, 8> ToArgs(NumArgs);
   if (ImportArrayChecked(CE->arg_begin(), CE->arg_end(), ToArgs.begin()))
     return nullptr;
 
return UnresolvedLookupExpr::Create(
         ResInfo, ToDecls.begin(), ToDecls.end());
 
   return UnresolvedLookupExpr::Create(
       Importer.getToContext(), NamingClass,
       Importer.Import(E->getQualifierLoc()), NameInfo, E->requiresADL(),
       E->isOverloaded(), ToDecls.begin(), ToDecls.end());
 }
 
+Expr *ASTNodeImporter::VisitUnresolvedMemberExpr(UnresolvedMemberExpr *E) {
+  DeclarationName Name = Importer.Import(E->getName());
+  if (!E->getName().isEmpty() && Name.isEmpty())
+    return nullptr;
+  DeclarationNameInfo NameInfo(Name, Importer.Import(E->getNameLoc()));
+  // Import additional name location/type info.
+  ImportDeclarationNameLoc(E->getNameInfo(), NameInfo);
+
+  QualType BaseType = Importer.Import(E->getType());
+  if (!E->getType().isNull() && BaseType.isNull())
+    return nullptr;
+
+  UnresolvedSet<8> ToDecls;
+  for (Decl *D : E->decls()) {
+    if (NamedDecl *To = cast_or_null<NamedDecl>(Importer.Import(D)))
+      ToDecls.addDecl(To);
+    else
+      return nullptr;
+  }
+
+  TemplateArgumentListInfo ToTAInfo;
+  TemplateArgumentListInfo *ResInfo = nullptr;
+  if (E->hasExplicitTemplateArgs()) {
+    if (ImportTemplateArgumentListInfo(E->template_arguments(), ToTAInfo))
+      return nullptr;
+    ResInfo = &ToTAInfo;
+  }
+
+  Expr *BaseE = E->isImplicitAccess() ? nullptr : Importer.Import(E->getBase());
+  if (!BaseE && !E->isImplicitAccess() && E->getBase()) {
+    return nullptr;
+  }
+
+  return UnresolvedMemberExpr::Create(
+      Importer.getToContext(), E->hasUnresolvedUsing(), BaseE, BaseType,
+      E->isArrow(), Importer.Import(E->getOperatorLoc()),
+      Importer.Import(E->getQualifierLoc()),
+      Importer.Import(E->getTemplateKeywordLoc()), NameInfo, ResInfo,
+      ToDecls.begin(), ToDecls.end());
+}
+
 Expr *ASTNodeImporter::VisitCallExpr(CallExpr *E) {
   QualType T = Importer.Import(E->getType());
   if (T.isNull())
     return nullptr;
 
   Expr *ToCallee = Importer.Import(E->getCallee());
   if (!ToCallee && E->getCallee())
     return nullptr;

unittests/AST/ASTImporterTest.cpp

TEST_P(ImportFunctions, VirtualFlagShouldBePreservedWhenImportingPrototype) {
   ASSERT_TRUE(Def->isVirtual());
   CXXMethodDecl *To = cast<CXXMethodDecl>(Import(Proto, Lang_CXX));
   EXPECT_TRUE(To->isVirtual());
 }
 
 INSTANTIATE_TEST_CASE_P(
     ParameterizedTests, ImportFunctions,
     ::testing::Values(ArgVector(), ArgVector{"-fdelayed-template-parsing"}),);
 
 AST_MATCHER_P(TagDecl, hasTypedefForAnonDecl, Matcher<TypedefNameDecl>,
               InnerMatcher) {
   if (auto *Typedef = Node.getTypedefNameForAnonDecl())
     return InnerMatcher.matches(*Typedef, Finder, Builder);
   return false;
 }
 
 TEST(ImportDecl, ImportEnumSequential) {
   CodeFiles Samples{{"main.c",
                      {"void foo();"
                       "void moo();"
                       "int main() { foo(); moo(); }",
                       Lang_C}},
 
                     {"foo.c",
                      {"typedef enum { THING_VALUE } thing_t;"
                       "void conflict(thing_t type);"
                       "void foo() { (void)THING_VALUE; }"
                       "void conflict(thing_t type) {}",
                       Lang_C}},
 
                     {"moo.c",
                      {"typedef enum { THING_VALUE } thing_t;"
                       "void conflict(thing_t type);"
                       "void moo() { conflict(THING_VALUE); }",
                       Lang_C}}};
 
   auto VerificationMatcher =
       enumDecl(has(enumConstantDecl(hasName("THING_VALUE"))),
                hasTypedefForAnonDecl(hasName("thing_t")));
 
   ImportAction ImportFoo{"foo.c", "main.c", functionDecl(hasName("foo"))},
       ImportMoo{"moo.c", "main.c", functionDecl(hasName("moo"))};
 
   testImportSequence(
       Samples, {ImportFoo, ImportMoo}, // "foo", them "moo".
       // Just check that there is only one enum decl in the result AST.
       "main.c", enumDecl(), VerificationMatcher);
 
   // For different import order, result should be the same.
   testImportSequence(
       Samples, {ImportMoo, ImportFoo}, // "moo", them "foo".
       // Check that there is only one enum decl in the result AST.
       "main.c", enumDecl(), VerificationMatcher);
 }
 
+const internal::VariadicDynCastAllOfMatcher<Expr, DependentScopeDeclRefExpr>
+    dependentScopeDeclRefExpr;
+
+TEST(ImportExpr, DependentScopeDeclRefExpr) {
+  MatchVerifier<Decl> Verifier;
+  testImport("template <typename T> struct S { static T foo; };"
+             "template <typename T> void declToImport() {"
+             "  (void) S<T>::foo;"
+             "}"
+             "void instantiate() { declToImport<int>(); }",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             functionTemplateDecl(has(functionDecl(has(compoundStmt(
+                 has(cStyleCastExpr(has(dependentScopeDeclRefExpr())))))))));
+
+  testImport("template <typename T> struct S {"
+             "template<typename S> static void foo(){};"
+             "};"
+             "template <typename T> void declToImport() {"
+             "  S<T>::template foo<T>();"
+             "}"
+             "void instantiate() { declToImport<int>(); }",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             functionTemplateDecl(has(functionDecl(has(compoundStmt(
+                 has(callExpr(has(dependentScopeDeclRefExpr())))))))));
+}
+
+const internal::VariadicDynCastAllOfMatcher<Type, DependentNameType>
+    dependentNameType;
+
+TEST(ImportExpr, DependentNameType) {
+  MatchVerifier<Decl> Verifier;
+  testImport("template <typename T> struct declToImport {"
+             "  typedef typename T::type dependent_name;"
+             "};",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             classTemplateDecl(has(
+                 cxxRecordDecl(has(typedefDecl(has(dependentNameType())))))));
+}
+
+const internal::VariadicDynCastAllOfMatcher<Expr, UnresolvedMemberExpr>
+    unresolvedMemberExpr;
+
+TEST(ImportExpr, UnresolvedMemberExpr) {
+  MatchVerifier<Decl> Verifier;
+  testImport("struct S { template <typename T> void mem(); };"
+             "template <typename U> void declToImport() {"
+             "  S s;"
+             "  s.mem<U>();"
+             "}"
+             "void instantiate() { declToImport<int>(); }",
+             Lang_CXX11, "", Lang_CXX11, Verifier,
+             functionTemplateDecl(has(functionDecl(has(
+                 compoundStmt(has(callExpr(has(unresolvedMemberExpr())))))))));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang