Ensure code complete with !LoadExternal sees all local decls.

sam-mccall · sam-mccall · commit 091b1efafb93 · 2018-01-16T12:33:46.000Z
Summary: noload_lookups() was too lazy: in addition to avoiding external decls, it avoided populating the lazy lookup structure for internal decls. This is the right behavior for the existing callsite in ASTDumper, but I think it's not a very useful default, so we populate it by default. While here: - remove an unused test file accidentally added in r322371. - remove lookups_begin()/lookups_end() in favor of lookups().begin(), which is more common and more efficient. Reviewers: ilya-biryukov Subscribers: cfe-commits, rsmith Differential Revision: https://reviews.llvm.org/D42077 llvm-svn: 322548
diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
@@ -1823,7 +1823,9 @@ class DeclContext {
   using lookups_range = llvm::iterator_range<all_lookups_iterator>;
 
   lookups_range lookups() const;
-  lookups_range noload_lookups() const;
+  // Like lookups(), but avoids loading external declarations.
+  // If PreserveInternalState, avoids building lookup data structures too.
+  lookups_range noload_lookups(bool PreserveInternalState) const;
 
   /// \brief Iterators over all possible lookups within this context.
   all_lookups_iterator lookups_begin() const;
@@ -1943,6 +1945,7 @@ class DeclContext {
 
   StoredDeclsMap *CreateStoredDeclsMap(ASTContext &C) const;
 
+  void loadLazyLocalLexicalLookups();
   void buildLookupImpl(DeclContext *DCtx, bool Internal);
   void makeDeclVisibleInContextWithFlags(NamedDecl *D, bool Internal,
                                          bool Rediscoverable);
diff --git a/clang/include/clang/AST/DeclLookups.h b/clang/include/clang/AST/DeclLookups.h
@@ -86,16 +86,11 @@ inline DeclContext::lookups_range DeclContext::lookups() const {
   return lookups_range(all_lookups_iterator(), all_lookups_iterator());
 }
 
-inline DeclContext::all_lookups_iterator DeclContext::lookups_begin() const {
-  return lookups().begin();
-}
-
-inline DeclContext::all_lookups_iterator DeclContext::lookups_end() const {
-  return lookups().end();
-}
-
-inline DeclContext::lookups_range DeclContext::noload_lookups() const {
+inline DeclContext::lookups_range
+DeclContext::noload_lookups(bool PreserveInternalState) const {
   DeclContext *Primary = const_cast<DeclContext*>(this)->getPrimaryContext();
+  if (!PreserveInternalState)
+    Primary->loadLazyLocalLexicalLookups();
   if (StoredDeclsMap *Map = Primary->getLookupPtr())
     return lookups_range(all_lookups_iterator(Map->begin(), Map->end()),
                          all_lookups_iterator(Map->end(), Map->end()));
@@ -105,16 +100,6 @@ inline DeclContext::lookups_range DeclContext::noload_lookups() const {
   return lookups_range(all_lookups_iterator(), all_lookups_iterator());
 }
 
-inline
-DeclContext::all_lookups_iterator DeclContext::noload_lookups_begin() const {
-  return noload_lookups().begin();
-}
-
-inline
-DeclContext::all_lookups_iterator DeclContext::noload_lookups_end() const {
-  return noload_lookups().end();
-}
-
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_DECLLOOKUPS_H
diff --git a/clang/lib/AST/ASTDumper.cpp b/clang/lib/AST/ASTDumper.cpp
@@ -809,11 +809,10 @@ void ASTDumper::dumpLookups(const DeclContext *DC, bool DumpDecls) {
 
     bool HasUndeserializedLookups = Primary->hasExternalVisibleStorage();
 
-    for (auto I = Deserialize ? Primary->lookups_begin()
-                              : Primary->noload_lookups_begin(),
-              E = Deserialize ? Primary->lookups_end()
-                              : Primary->noload_lookups_end();
-         I != E; ++I) {
+    auto Range = Deserialize
+                     ? Primary->lookups()
+                     : Primary->noload_lookups(/*PreserveInternalState=*/true);
+    for (auto I = Range.begin(), E = Range.end(); I != E; ++I) {
       DeclarationName Name = I.getLookupName();
       DeclContextLookupResult R = *I;
 
diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
@@ -1588,17 +1588,7 @@ DeclContext::noload_lookup(DeclarationName Name) {
   if (PrimaryContext != this)
     return PrimaryContext->noload_lookup(Name);
 
-  // If we have any lazy lexical declarations not in our lookup map, add them
-  // now. Don't import any external declarations, not even if we know we have
-  // some missing from the external visible lookups.
-  if (HasLazyLocalLexicalLookups) {
-    SmallVector<DeclContext *, 2> Contexts;
-    collectAllContexts(Contexts);
-    for (unsigned I = 0, N = Contexts.size(); I != N; ++I)
-      buildLookupImpl(Contexts[I], hasExternalVisibleStorage());
-    HasLazyLocalLexicalLookups = false;
-  }
-
+  loadLazyLocalLexicalLookups();
   StoredDeclsMap *Map = LookupPtr;
   if (!Map)
     return lookup_result();
@@ -1608,6 +1598,19 @@ DeclContext::noload_lookup(DeclarationName Name) {
                          : lookup_result();
 }
 
+// If we have any lazy lexical declarations not in our lookup map, add them
+// now. Don't import any external declarations, not even if we know we have
+// some missing from the external visible lookups.
+void DeclContext::loadLazyLocalLexicalLookups() {
+  if (HasLazyLocalLexicalLookups) {
+    SmallVector<DeclContext *, 2> Contexts;
+    collectAllContexts(Contexts);
+    for (unsigned I = 0, N = Contexts.size(); I != N; ++I)
+      buildLookupImpl(Contexts[I], hasExternalVisibleStorage());
+    HasLazyLocalLexicalLookups = false;
+  }
+}
+
 void DeclContext::localUncachedLookup(DeclarationName Name,
                                       SmallVectorImpl<NamedDecl *> &Results) {
   Results.clear();
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
@@ -3543,7 +3543,8 @@ static void LookupVisibleDecls(DeclContext *Ctx, LookupResult &Result,
 
   // Enumerate all of the results in this context.
   for (DeclContextLookupResult R :
-       LoadExternal ? Ctx->lookups() : Ctx->noload_lookups()) {
+       LoadExternal ? Ctx->lookups()
+                    : Ctx->noload_lookups(/*PreserveInternalState=*/false)) {
     for (auto *D : R) {
       if (auto *ND = Result.getAcceptableDecl(D)) {
         Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
diff --git a/clang/test/Index/complete-pch-skip.cpp b/clang/test/Index/complete-pch-skip.cpp
@@ -16,3 +16,10 @@ int main() { return ns:: }
 // SKIP-PCH: {TypedText bar}
 // SKIP-PCH-NOT: foo
 
+// Verify that with *no* preamble (no -include flag) we still get local results.
+// SkipPreamble used to break this, by making lookup *too* lazy.
+// RUN: env CINDEXTEST_COMPLETION_SKIP_PREAMBLE=1 c-index-test -code-completion-at=%s:5:26 %s | FileCheck -check-prefix=NO-PCH %s
+// NO-PCH-NOT: foo
+// NO-PCH: {TypedText bar}
+// NO-PCH-NOT: foo
+
diff --git a/clang/test/Index/complete-pch-skip.h b/clang/test/Index/complete-pch-skip.h
