Ensure code complete with !LoadExternal sees all local decls.

[sammccall]

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.

[ilya-biryukov]

LGTM with a few NITs (see the comments)

include/clang/AST/DeclBase.h

1826–1828

Maybe we should consider removing the default argument and specifying it explicitly at each call site?
The difference seems subtle enough to not leave it out in the callers of the function.

include/clang/AST/DeclLookups.h

89–90

Maybe move the doc to the declaration site?
Or is it common in these files to put comments at the definition?

[sammccall]

Remove default value of PreserveInternalState, and move doc comment.

include/clang/AST/DeclBase.h

public:
 
   /// all_lookups_iterator - An iterator that provides a view over the results
   /// of looking up every possible name.
   class all_lookups_iterator;
 
   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;
   all_lookups_iterator lookups_end() const;
 
   /// \brief Iterators over all possible lookups within this context that are
   /// currently loaded; don't attempt to retrieve anything from an external
   /// source.
private:
   /// name.
   ///
   /// Analogous to makeDeclVisibleInContext, but for the exclusive
   /// use of addDeclInternal().
   void makeDeclVisibleInContextInternal(NamedDecl *D);
 
   StoredDeclsMap *CreateStoredDeclsMap(ASTContext &C) const;
 
+  void loadLazyLocalLexicalLookups();
   void buildLookupImpl(DeclContext *DCtx, bool Internal);
   void makeDeclVisibleInContextWithFlags(NamedDecl *D, bool Internal,
                                          bool Rediscoverable);
   void makeDeclVisibleInContextImpl(NamedDecl *D, bool Internal);
 };
 
 inline bool Decl::isTemplateParameter() const {
   return getKind() == TemplateTypeParm || getKind() == NonTypeTemplateParm ||

include/clang/AST/DeclLookups.h

if (StoredDeclsMap *Map = Primary->buildLookup())
     return lookups_range(all_lookups_iterator(Map->begin(), Map->end()),
                          all_lookups_iterator(Map->end(), Map->end()));
 
   // Synthesize an empty range. This requires that two default constructed
   // versions of these iterators form a valid empty range.
   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()));
 
   // Synthesize an empty range. This requires that two default constructed
   // versions of these iterators form a valid empty range.
   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

lib/AST/ASTDumper.cpp

dumpChild([=] {
     const DeclContext *Primary = DC->getPrimaryContext();
     if (Primary != DC) {
       OS << " primary";
       dumpPointer(cast<Decl>(Primary));
     }
 
     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;
 
       dumpChild([=] {
         OS << "DeclarationName ";
         {
           ColorScope Color(*this, DeclNameColor);
           OS << '\'' << Name << '\'';

lib/AST/DeclBase.cpp

 DeclContext::noload_lookup(DeclarationName Name) {
   assert(DeclKind != Decl::LinkageSpec && DeclKind != Decl::Export &&
          "should not perform lookups into transparent contexts");
 
   DeclContext *PrimaryContext = getPrimaryContext();
   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.
+  loadLazyLocalLexicalLookups();
+  StoredDeclsMap *Map = LookupPtr;
+  if (!Map)
+    return lookup_result();
+
+  StoredDeclsMap::iterator I = Map->find(Name);
+  return I != Map->end() ? I->second.getLookupResult()
+                         : lookup_result();
+}
+
+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;
   }
-
-  StoredDeclsMap *Map = LookupPtr;
-  if (!Map)
-    return lookup_result();
-
-  StoredDeclsMap::iterator I = Map->find(Name);
-  return I != Map->end() ? I->second.getLookupResult()
-                         : lookup_result();
 }
 
 void DeclContext::localUncachedLookup(DeclarationName Name,
                                       SmallVectorImpl<NamedDecl *> &Results) {
   Results.clear();
   
   // If there's no external storage, just perform a normal lookup and copy
   // the results.

lib/Sema/SemaLookup.cpp

if (isa<TranslationUnitDecl>(Ctx) &&
     return;
   }
 
   if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx))
     Result.getSema().ForceDeclarationOfImplicitMembers(Class);
 
   // 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);
         Visited.add(ND);
       }
     }
   }
 

test/Index/complete-pch-skip.h

-namespace ns {
-int foo;
-}

test/Index/complete-pch-skip.cpp

 // WITH-PCH: {TypedText bar}
 // WITH-PCH: {TypedText foo}
 
 // RUN: env CINDEXTEST_COMPLETION_SKIP_PREAMBLE=1 c-index-test -code-completion-at=%s:5:26 -include %t.h %s | FileCheck -check-prefix=SKIP-PCH %s
 // SKIP-PCH-NOT: foo
 // 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
+