[clang-import-test] Lookup inside entities

[spyffe]

Summary

clang-import-test has until now been only able to report top-level entities. This is clearly insufficient; we should be able to look inside structs and namespaces also.
This patch adds new test cases for a variety of lookups inside existing entities, and adds the functionality necessary to make most of these testcases work.
One testcase is known to fail because of ASTImporter limitations when importing templates; this will be addressed separately.

[aprantl]

A few stylistic issues inline.
It also wouldn't hurt to document what is being tested, either in the clang-import-test sources or in the input files. Otherwise this looks fine. More tests are always great!

tools/clang-import-test/clang-import-test.cpp

213 ↗

(On Diff #89949)

Sort of an irrelevant nitpick, but we don't typically use curly braces on single statements.

228 ↗

(On Diff #89949)

->

// Couldn't find the name, so we have to give up.
return nullptr;

[spyffe]

• Applied Adrian's suggestion to move comments before the line they apply to.
• Got to 100% coverage by changing a test and making an assert for something I couldn't make happen in a test.

I did not take Adrian's suggestion to remove braces around a single-statement if because (as far as I can tell) the LLVM coding standard is silent on the topic and I prefer to have the braces there.

[a.sidorin]

Hello Sean,

It is good to have the ability of recursive lookup. But for me (and I'm sorry), the code becomes very hard to understand: I need to track if the Decl/DC is in the source AST or in the AST being imported, if it was imported or if was in the AST before across lambdas and calls. Can we make this more clear?

tools/clang-import-test/clang-import-test.cpp

201 ↗

(On Diff #89957)

Can we make the usage of qualified and non-qualified casts consistent? Usually, casts are used as unqualified.

298 ↗

(On Diff #89957)

I guess we can use SmallVectors here too.

303 ↗

(On Diff #89957)

Nit: to make the code cleaner, we can extract this std::none_of to a separate function like IsFoundAsForward(). Nested lambdas are harder to read.

333 ↗

(On Diff #89957)

The loop:

Decls.reserve(DeclsToReport.size());
for (Candidate &C : DeclsToReport)
  Decls.push_back(cast<NamedDecl>(C.second->Import(C.first)));

will look a bit nicer here. What do you think?

[spyffe]

• Removed namespace qualification of cast across the board.
• Standardized on SmallVector
• Factored out the filtering of forward declarations
• Fixed a loop as requested.
• Made a Origin<> wrapper for arbitrary AST objects that makes it clear where they live, so importation and searching gets a little clearer.

[spyffe]

• Broke all the ASTImporter and ExternalASTSource subclassing logic into its own source file, with its API defined in AST/ExternalASTMerger.h.
• Cleaned up the API for LLDB's consumption: now rather than a CompilerInstance a client just has to provide an ASTContext and a FileManager (just like when setting up an ASTImporter). This allows e.g. DWARF to be a valid source.

[spyffe]

Added the ExternalASTMerger implementation/interface, which I hadn't svn added before generating the last diff.

[a.sidorin]

Looks excellent now. Thank you Sean!

cfe/trunk/include/clang/AST/ExternalASTMerger.h

+//===--- ExternalASTMerger.h - Merging External AST Interface ---*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file declares the ExternalASTMerger, which vends a combination of ASTs
+//  from several different ASTContext/FileManager pairs
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_CLANG_AST_EXTERNALASTMERGER_H
+#define LLVM_CLANG_AST_EXTERNALASTMERGER_H
+
+#include "clang/AST/ASTImporter.h"
+#include "clang/AST/ExternalASTSource.h"
+
+namespace clang {
+
+class ExternalASTMerger : public ExternalASTSource {
+public:
+  struct ImporterPair {
+    std::unique_ptr<ASTImporter> Forward;
+    std::unique_ptr<ASTImporter> Reverse;
+  };
+
+private:
+  std::vector<ImporterPair> Importers;
+
+public:
+  struct ImporterEndpoint {
+    ASTContext &AST;
+    FileManager &FM;
+  };
+  ExternalASTMerger(const ImporterEndpoint &Target,
+                    llvm::ArrayRef<ImporterEndpoint> Sources);
+
+  bool FindExternalVisibleDeclsByName(const DeclContext *DC,
+                                      DeclarationName Name) override;
+
+  void
+  FindExternalLexicalDecls(const DeclContext *DC,
+                           llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
+                           SmallVectorImpl<Decl *> &Result) override;
+};
+
+} // end namespace clang
+
+#endif

cfe/trunk/lib/AST/CMakeLists.txt

add_clang_library(clangAST
   DeclOpenMP.cpp
   DeclPrinter.cpp
   DeclTemplate.cpp
   Expr.cpp
   ExprClassification.cpp
   ExprConstant.cpp
   ExprCXX.cpp
   ExprObjC.cpp
+  ExternalASTMerger.cpp
   ExternalASTSource.cpp
   InheritViz.cpp
   ItaniumCXXABI.cpp
   ItaniumMangle.cpp
   Mangle.cpp
   MicrosoftCXXABI.cpp
   MicrosoftMangle.cpp
   NestedNameSpecifier.cpp

cfe/trunk/lib/AST/ExternalASTMerger.cpp

+//===- ExternalASTMerger.cpp - Merging External AST Interface ---*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements the ExternalASTMerger, which vends a combination of
+//  ASTs from several different ASTContext/FileManager pairs
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/AST/ExternalASTMerger.h"
+
+using namespace clang;
+
+namespace {
+
+template <typename T> struct Source {
+  T t;
+  Source(T &&t) : t(std::move(t)) {}
+  operator T() { return t; }
+  template <typename U = T> U &get() { return t; }
+  template <typename U = T> const U &get() const { return t; }
+  template <typename U> operator Source<U>() { return Source<U>(t); }
+};
+
+typedef std::pair<Source<NamedDecl *>, ASTImporter *> Candidate;
+
+class LazyASTImporter : public ASTImporter {
+public:
+  LazyASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
+                  ASTContext &FromContext, FileManager &FromFileManager)
+      : ASTImporter(ToContext, ToFileManager, FromContext, FromFileManager,
+                    /*MinimalImport=*/true) {}
+  Decl *Imported(Decl *From, Decl *To) override {
+    if (auto ToTag = dyn_cast<TagDecl>(To)) {
+      ToTag->setHasExternalLexicalStorage();
+    } else if (auto ToNamespace = dyn_cast<NamespaceDecl>(To)) {
+      ToNamespace->setHasExternalVisibleStorage();
+    }
+    return ASTImporter::Imported(From, To);
+  }
+};
+
+Source<const DeclContext *>
+LookupSameContext(Source<TranslationUnitDecl *> SourceTU, const DeclContext *DC,
+                  ASTImporter &ReverseImporter) {
+  if (DC->isTranslationUnit()) {
+    return SourceTU;
+  }
+  Source<const DeclContext *> SourceParentDC =
+      LookupSameContext(SourceTU, DC->getParent(), ReverseImporter);
+  if (!SourceParentDC) {
+    // If we couldn't find the parent DC in this TranslationUnit, give up.
+    return nullptr;
+  }
+  auto ND = cast<NamedDecl>(DC);
+  DeclarationName Name = ND->getDeclName();
+  Source<DeclarationName> SourceName = ReverseImporter.Import(Name);
+  DeclContext::lookup_result SearchResult =
+      SourceParentDC.get()->lookup(SourceName.get());
+  size_t SearchResultSize = SearchResult.size();
+  // Handle multiple candidates once we have a test for it.
+  // This may turn up when we import template specializations correctly.
+  assert(SearchResultSize < 2);
+  if (SearchResultSize == 0) {
+    // couldn't find the name, so we have to give up
+    return nullptr;
+  } else {
+    NamedDecl *SearchResultDecl = SearchResult[0];
+    return dyn_cast<DeclContext>(SearchResultDecl);
+  }
+}
+
+bool IsForwardDeclaration(Decl *D) {
+  assert(!isa<ObjCInterfaceDecl>(D)); // TODO handle this case
+  if (auto TD = dyn_cast<TagDecl>(D)) {
+    return !TD->isThisDeclarationADefinition();
+  } else if (auto FD = dyn_cast<FunctionDecl>(D)) {
+    return !FD->isThisDeclarationADefinition();
+  } else {
+    return false;
+  }
+}
+
+void ForEachMatchingDC(
+    const DeclContext *DC,
+    llvm::ArrayRef<ExternalASTMerger::ImporterPair> Importers,
+    std::function<void(const ExternalASTMerger::ImporterPair &IP,
+                       Source<const DeclContext *> SourceDC)>
+        Callback) {
+  for (const ExternalASTMerger::ImporterPair &IP : Importers) {
+    Source<TranslationUnitDecl *> SourceTU(
+        IP.Forward->getFromContext().getTranslationUnitDecl());
+    Source<const DeclContext *> SourceDC =
+        LookupSameContext(SourceTU, DC, *IP.Reverse);
+    if (SourceDC.get()) {
+      Callback(IP, SourceDC);
+    }
+  }
+}
+
+bool HasDeclOfSameType(llvm::ArrayRef<Candidate> Decls, const Candidate &C) {
+  return std::any_of(Decls.begin(), Decls.end(), [&C](const Candidate &D) {
+    return C.first.get()->getKind() == D.first.get()->getKind();
+  });
+}
+} // end namespace
+
+ExternalASTMerger::ExternalASTMerger(const ImporterEndpoint &Target,
+                                     llvm::ArrayRef<ImporterEndpoint> Sources) {
+  for (const ImporterEndpoint &S : Sources) {
+    Importers.push_back(
+        {llvm::make_unique<LazyASTImporter>(Target.AST, Target.FM, S.AST, S.FM),
+         llvm::make_unique<ASTImporter>(S.AST, S.FM, Target.AST, Target.FM,
+                                        /*MinimalImport=*/true)});
+  }
+}
+
+bool ExternalASTMerger::FindExternalVisibleDeclsByName(const DeclContext *DC,
+                                                       DeclarationName Name) {
+  llvm::SmallVector<NamedDecl *, 1> Decls;
+  llvm::SmallVector<Candidate, 4> CompleteDecls;
+  llvm::SmallVector<Candidate, 4> ForwardDecls;
+
+  auto FilterFoundDecl = [&CompleteDecls, &ForwardDecls](const Candidate &C) {
+    if (IsForwardDeclaration(C.first.get())) {
+      if (!HasDeclOfSameType(ForwardDecls, C)) {
+        ForwardDecls.push_back(C);
+      }
+    } else {
+      CompleteDecls.push_back(C);
+    }
+  };
+
+  ForEachMatchingDC(DC, Importers, [Name, &FilterFoundDecl](
+                                       const ImporterPair &IP,
+                                       Source<const DeclContext *> SourceDC) {
+    DeclarationName FromName = IP.Reverse->Import(Name);
+    DeclContextLookupResult Result = SourceDC.get()->lookup(FromName);
+    for (NamedDecl *FromD : Result) {
+      FilterFoundDecl(std::make_pair(FromD, IP.Forward.get()));
+    }
+  });
+
+  llvm::ArrayRef<Candidate> DeclsToReport =
+      CompleteDecls.empty() ? ForwardDecls : CompleteDecls;
+
+  if (DeclsToReport.empty()) {
+    return false;
+  }
+
+  Decls.reserve(DeclsToReport.size());
+  for (const Candidate &C : DeclsToReport) {
+    NamedDecl *d = cast<NamedDecl>(C.second->Import(C.first.get()));
+    assert(d);
+    Decls.push_back(d);
+  }
+  SetExternalVisibleDeclsForName(DC, Name, Decls);
+  return true;
+}
+
+void ExternalASTMerger::FindExternalLexicalDecls(
+    const DeclContext *DC, llvm::function_ref<bool(Decl::Kind)> IsKindWeWant,
+    SmallVectorImpl<Decl *> &Result) {
+  ForEachMatchingDC(
+      DC, Importers, [DC, IsKindWeWant](const ImporterPair &IP,
+                                        Source<const DeclContext *> SourceDC) {
+        for (Source<const Decl *> SourceDecl : SourceDC.get()->decls()) {
+          if (IsKindWeWant(SourceDecl.get()->getKind())) {
+            Decl *ImportedDecl =
+                IP.Forward->Import(const_cast<Decl *>(SourceDecl.get()));
+            assert(ImportedDecl->getDeclContext() == DC);
+          }
+        }
+      });
+}

cfe/trunk/test/Import/forward-declared-struct/Inputs/S1.c

+struct S;

cfe/trunk/test/Import/forward-declared-struct/Inputs/S2.c

+struct S {
+  int a;
+};

cfe/trunk/test/Import/forward-declared-struct/test.c

+// RUN: clang-import-test -import %S/Inputs/S1.c --import %S/Inputs/S2.c -expression %s
+void expr() {
+  struct S MyS;
+  MyS.a = 3;
+}

cfe/trunk/test/Import/member-in-struct/Inputs/S.c

+struct S {
+  int a;
+};

cfe/trunk/test/Import/member-in-struct/test.c

+// RUN: clang-import-test -import %S/Inputs/S.c -expression %s
+void expr() {
+  struct S MyS;
+  MyS.a = 3;
+}

cfe/trunk/test/Import/multiple-forward-declarations/Inputs/S1.c

+struct S;

cfe/trunk/test/Import/multiple-forward-declarations/Inputs/S2.c

+struct S;

cfe/trunk/test/Import/multiple-forward-declarations/test.c

+// RUN: clang-import-test -import %S/Inputs/S1.c --import %S/Inputs/S2.c -expression %s
+void expr() {
+  struct S *MySPtr;
+}

cfe/trunk/test/Import/overloaded-function/Inputs/F1.c

+void f(int arg) { }

cfe/trunk/test/Import/overloaded-function/Inputs/F2.c

+struct S { int a; };
+
+void f(const char *arg) { }
+void f(S arg) { }

cfe/trunk/test/Import/overloaded-function/test.c

+// RUN: clang-import-test -import %S/Inputs/F1.c -import %S/Inputs/F2.c -expression %s
+void expr() {
+  f(2);
+  f("world");
+  S s;
+  f(s);
+}

cfe/trunk/test/Import/struct-in-namespace/Inputs/N1.cpp

+namespace N {
+  struct S {
+    int a;
+  };
+}
+
+namespace N {
+  struct T {
+    int b;
+  };
+}

cfe/trunk/test/Import/struct-in-namespace/Inputs/N2.cpp

+namespace N {
+  struct U {
+    int c;
+  };
+}

cfe/trunk/test/Import/struct-in-namespace/Inputs/N3.cpp

+namespace M {
+  struct V {
+    int d;
+  };
+}

cfe/trunk/test/Import/struct-in-namespace/test.cpp

+// RUN: clang-import-test -import %S/Inputs/N1.cpp -import %S/Inputs/N2.cpp -import %S/Inputs/N3.cpp -expression %s
+void expr() {
+  N::S s;
+  N::T t;
+  N::U u;
+  int d = s.a + t.b + u.c;
+}

cfe/trunk/test/Import/template-specialization/Inputs/T.cpp

+template <typename T> struct A {
+};
+
+template <> struct A<int> {
+  struct B {
+    int f;
+  };
+};
+
+template <> struct A<bool> {
+  struct B {
+    int g;
+  };
+};

cfe/trunk/test/Import/template-specialization/test.cpp

+// RUN: clang-import-test -import %S/Inputs/T.cpp -expression %s
+// XFAIL: *
+void expr() {
+  A<int>::B b1;
+  A<bool>::B b2;
+  b1.f + b2.g;
+}

cfe/trunk/tools/clang-import-test/clang-import-test.cpp

 //===-- import-test.cpp - ASTImporter/ExternalASTSource testbed -----------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/AST/ExternalASTMerger.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/CodeGen/ModuleBuilder.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
std::unique_ptr<CodeGenerator> BuildCodeGen(CompilerInstance &CI,
   StringRef ModuleName("$__module");
   return std::unique_ptr<CodeGenerator>(CreateLLVMCodeGen(
       CI.getDiagnostics(), ModuleName, CI.getHeaderSearchOpts(),
       CI.getPreprocessorOpts(), CI.getCodeGenOpts(), LLVMCtx));
 }
 } // end namespace
 
 namespace {
-class TestExternalASTSource : public ExternalASTSource {
-private:
-  llvm::ArrayRef<std::unique_ptr<CompilerInstance>> ImportCIs;
-  std::map<CompilerInstance *, std::unique_ptr<ASTImporter>> ForwardImporters;
-  std::map<CompilerInstance *, std::unique_ptr<ASTImporter>> ReverseImporters;
-
-public:
-  TestExternalASTSource(
-      CompilerInstance &ExpressionCI,
-      llvm::ArrayRef<std::unique_ptr<CompilerInstance>> ImportCIs)
-      : ImportCIs(ImportCIs) {
-    for (const std::unique_ptr<CompilerInstance> &ImportCI : ImportCIs) {
-      ForwardImporters[ImportCI.get()] = llvm::make_unique<ASTImporter>(
-          ExpressionCI.getASTContext(), ExpressionCI.getFileManager(),
-          ImportCI->getASTContext(), ImportCI->getFileManager(),
-          /*MinimalImport=*/true);
-      ReverseImporters[ImportCI.get()] = llvm::make_unique<ASTImporter>(
-          ImportCI->getASTContext(), ImportCI->getFileManager(),
-          ExpressionCI.getASTContext(), ExpressionCI.getFileManager(),
-          /*MinimalImport=*/true);
-    }
-  }
-
-  bool FindExternalVisibleDeclsByName(const DeclContext *DC,
-                                      DeclarationName Name) override {
-    llvm::SmallVector<NamedDecl *, 1> Decls;
-
-    if (isa<TranslationUnitDecl>(DC)) {
-      for (const std::unique_ptr<CompilerInstance> &I : ImportCIs) {
-        DeclarationName FromName = ReverseImporters[I.get()]->Import(Name);
-        DeclContextLookupResult Result =
-            I->getASTContext().getTranslationUnitDecl()->lookup(FromName);
-        for (NamedDecl *FromD : Result) {
-          NamedDecl *D =
-              llvm::cast<NamedDecl>(ForwardImporters[I.get()]->Import(FromD));
-          Decls.push_back(D);
-        }
-      }
-    }
-    if (Decls.empty()) {
-      return false;
-    } else {
-      SetExternalVisibleDeclsForName(DC, Name, Decls);
-      return true;
-    }
-  }
-};
  
 void AddExternalSource(
     CompilerInstance &CI,
     llvm::ArrayRef<std::unique_ptr<CompilerInstance>> Imports) {
-  ASTContext &AST = CI.getASTContext();
-  auto ES = llvm::make_unique<TestExternalASTSource>(CI, Imports);
-  AST.setExternalSource(ES.release());
-  AST.getTranslationUnitDecl()->setHasExternalVisibleStorage();
+  ExternalASTMerger::ImporterEndpoint Target({CI.getASTContext(), CI.getFileManager()});
+  llvm::SmallVector<ExternalASTMerger::ImporterEndpoint, 3> Sources;
+  for (const std::unique_ptr<CompilerInstance> &CI : Imports) {
+    Sources.push_back({CI->getASTContext(), CI->getFileManager()});
+  }
+  auto ES = llvm::make_unique<ExternalASTMerger>(Target, Sources);
+  CI.getASTContext().setExternalSource(ES.release());
+  CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
 }
 
 llvm::Error ParseSource(const std::string &Path, CompilerInstance &CI,
                         CodeGenerator &CG) {
   SourceManager &SM = CI.getSourceManager();
   const FileEntry *FE = CI.getFileManager().getFile(Path);
   if (!FE) {
     return llvm::make_error<llvm::StringError>(
Parse(const std::string &Path,
   CI->getDiagnosticClient().EndSourceFile();
   if (CI->getDiagnosticClient().getNumErrors()) {
     return llvm::make_error<llvm::StringError>(
         "Errors occured while parsing the expression.", std::error_code());
   } else {
     return std::move(CI);
   }
 }
+
 } // end namespace
 
 int main(int argc, const char **argv) {
   const bool DisableCrashReporting = true;
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0], DisableCrashReporting);
   llvm::cl::ParseCommandLineOptions(argc, argv);
   std::vector<std::unique_ptr<CompilerInstance>> ImportCIs;
   for (auto I : Imports) {