C++ modules: fix a bug where loading a declaration with some name would prevent

name lookup from lazily deserializing the other declarations with the same
name, by tracking a bit to indicate whether a name in a DeclContext might have
additional external results. This also allows lazier reconciling of the lookup
table if a module import adds decls to a pre-existing DC.

However, this exposes a pre-existing bug, which causes a regression in
test/Modules/decldef.mm: if we have a reference to a declaration, and a
later-imported module adds a redeclaration, nothing causes us to load that
redeclaration when we use or emit the reference (which can manifest as a
reference to an undefined inline function, a use of an incomplete type, and so
on). decldef.mm has been extended with an additional testcase which fails with
or without this change.

llvm-svn: 190293

clang/include/clang/AST/DeclContextInternals.h

@@ -18,6 +18,7 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclarationName.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
 #include <algorithm>
@@ -26,23 +27,29 @@ namespace clang {
 
 class DependentDiagnostic;
 
-/// StoredDeclsList - This is an array of decls optimized a common case of only
-/// containing one entry.
+/// \brief An array of decls optimized for the common case of only containing
+/// one entry.
 struct StoredDeclsList {
 
-  /// DeclsTy - When in vector form, this is what the Data pointer points to.
+  /// \brief When in vector form, this is what the Data pointer points to.
   typedef SmallVector<NamedDecl *, 4> DeclsTy;
 
+  /// \brief A collection of declarations, with a flag to indicate if we have
+  /// further external declarations.
+  typedef llvm::PointerIntPair<DeclsTy *, 1, bool> DeclsAndHasExternalTy;
+
   /// \brief The stored data, which will be either a pointer to a NamedDecl,
-  /// or a pointer to a vector.
-  llvm::PointerUnion<NamedDecl *, DeclsTy *> Data;
+  /// or a pointer to a vector with a flag to indicate if there are further
+  /// external declarations.
+  llvm::PointerUnion<NamedDecl*, DeclsAndHasExternalTy> Data;
 
 public:
   StoredDeclsList() {}
 
   StoredDeclsList(const StoredDeclsList &RHS) : Data(RHS.Data) {
     if (DeclsTy *RHSVec = RHS.getAsVector())
-      Data = new DeclsTy(*RHSVec);
+      Data = DeclsAndHasExternalTy(new DeclsTy(*RHSVec),
+                                   RHS.hasExternalDecls());
   }
 
   ~StoredDeclsList() {
@@ -56,7 +63,7 @@ public:
       delete Vector;
     Data = RHS.Data;
     if (DeclsTy *RHSVec = RHS.getAsVector())
-      Data = new DeclsTy(*RHSVec);
+      Data = DeclsAndHasExternalTy(new DeclsTy(*RHSVec), hasExternalDecls());
     return *this;
   }
 
@@ -66,8 +73,27 @@ public:
     return Data.dyn_cast<NamedDecl *>();
   }
 
+  DeclsAndHasExternalTy getAsVectorAndHasExternal() const {
+    return Data.dyn_cast<DeclsAndHasExternalTy>();
+  }
+
   DeclsTy *getAsVector() const {
-    return Data.dyn_cast<DeclsTy *>();
+    return getAsVectorAndHasExternal().getPointer();
+  }
+
+  bool hasExternalDecls() const {
+    return getAsVectorAndHasExternal().getInt();
+  }
+
+  void setHasExternalDecls() {
+    if (DeclsTy *Vec = getAsVector())
+      Data = DeclsAndHasExternalTy(Vec, true);
+    else {
+      DeclsTy *VT = new DeclsTy();
+      if (NamedDecl *OldD = getAsDecl())
+        VT->push_back(OldD);
+      Data = DeclsAndHasExternalTy(VT, true);
+    }
   }
 
   void setOnlyValue(NamedDecl *ND) {
@@ -110,6 +136,8 @@ public:
       Vec.erase(std::remove_if(Vec.begin(), Vec.end(),
                                std::mem_fun(&Decl::isFromASTFile)),
                 Vec.end());
+      // Don't have any external decls any more.
+      Data = DeclsAndHasExternalTy(&Vec, false);
     }
   }
 
@@ -165,12 +193,14 @@ public:
   /// not a redeclaration to merge it into the appropriate place in our list.
   ///
   void AddSubsequentDecl(NamedDecl *D) {
+    assert(!isNull() && "don't AddSubsequentDecl when we have no decls");
+
     // If this is the second decl added to the list, convert this to vector
     // form.
     if (NamedDecl *OldD = getAsDecl()) {
       DeclsTy *VT = new DeclsTy();
       VT->push_back(OldD);
-      Data = VT;
+      Data = DeclsAndHasExternalTy(VT, false);
     }
 
     DeclsTy &Vec = *getAsVector();

clang/lib/AST/DeclBase.cpp

@@ -926,11 +926,8 @@ void DeclContext::reconcileExternalVisibleStorage() {
   NeedToReconcileExternalVisibleStorage = false;
 
   StoredDeclsMap &Map = *LookupPtr.getPointer();
-  ExternalASTSource *Source = getParentASTContext().getExternalSource();
-  for (StoredDeclsMap::iterator I = Map.begin(); I != Map.end(); ++I) {
-    I->second.removeExternalDecls();
-    Source->FindExternalVisibleDeclsByName(this, I->first);
-  }
+  for (StoredDeclsMap::iterator I = Map.begin(); I != Map.end(); ++I)
+    I->second.setHasExternalDecls();
 }
 
 /// \brief Load the declarations within this lexical storage from an
@@ -983,8 +980,7 @@ ExternalASTSource::SetNoExternalVisibleDeclsForName(const DeclContext *DC,
   if (!(Map = DC->LookupPtr.getPointer()))
     Map = DC->CreateStoredDeclsMap(Context);
 
-  // Add an entry to the map for this name, if it's not already present.
-  (*Map)[Name];
+  (*Map)[Name].removeExternalDecls();
 
   return DeclContext::lookup_result();
 }
@@ -1246,16 +1242,14 @@ DeclContext::lookup(DeclarationName Name) {
     if (!Map)
       Map = CreateStoredDeclsMap(getParentASTContext());
 
-    // If a PCH/module has a result for this name, and we have a local
-    // declaration, we will have imported the PCH/module result when adding the
-    // local declaration or when reconciling the module.
+    // If we have a lookup result with no external decls, we are done.
     std::pair<StoredDeclsMap::iterator, bool> R =
         Map->insert(std::make_pair(Name, StoredDeclsList()));
-    if (!R.second)
+    if (!R.second && !R.first->second.hasExternalDecls())
       return R.first->second.getLookupResult();
 
     ExternalASTSource *Source = getParentASTContext().getExternalSource();
-    if (Source->FindExternalVisibleDeclsByName(this, Name)) {
+    if (Source->FindExternalVisibleDeclsByName(this, Name) || R.second) {
       if (StoredDeclsMap *Map = LookupPtr.getPointer()) {
         StoredDeclsMap::iterator I = Map->find(Name);
         if (I != Map->end())
@@ -1304,7 +1298,8 @@ DeclContext::noload_lookup(DeclarationName Name) {
     LookupPtr.setInt(false);
 
     // There may now be names for which we have local decls but are
-    // missing the external decls.
+    // missing the external decls. FIXME: Just set the hasExternalDecls
+    // flag on those names that have external decls.
     NeedToReconcileExternalVisibleStorage = true;
 
     Map = LookupPtr.getPointer();
@@ -1461,7 +1456,18 @@ void DeclContext::makeDeclVisibleInContextImpl(NamedDecl *D, bool Internal) {
 
   // Insert this declaration into the map.
   StoredDeclsList &DeclNameEntries = (*Map)[D->getDeclName()];
-  if (DeclNameEntries.isNull()) {
+
+  if (Internal) {
+    // If this is being added as part of loading an external declaration,
+    // this may not be the only external declaration with this name.
+    // In this case, we never try to replace an existing declaration; we'll
+    // handle that when we finalize the list of declarations for this name.
+    DeclNameEntries.setHasExternalDecls();
+    DeclNameEntries.AddSubsequentDecl(D);
+    return;
+  }
+
+  else if (DeclNameEntries.isNull()) {
     DeclNameEntries.setOnlyValue(D);
     return;
   }

clang/lib/Sema/SemaType.cpp

@@ -5070,6 +5070,14 @@ bool Sema::RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
     return false;
   }
 
+  // FIXME: If there's an unimported definition of this type in a module (for
+  // instance, because we forward declared it, then imported the definition),
+  // import that definition now.
+  // FIXME: What about other cases where an import extends a redeclaration
+  // chain for a declaration that can be accessed through a mechanism other
+  // than name lookup (eg, referenced in a template, or a variable whose type
+  // could be completed by the module)?
+
   const TagType *Tag = T->getAs<TagType>();
   const ObjCInterfaceType *IFace = 0;
 

clang/test/Modules/Inputs/def.h

@@ -17,4 +17,11 @@ class Def2 {
 public:
   void func();
 };
+
+namespace Def3NS {
+  class Def3 {
+  public:
+   void func();
+  };
+}
 #endif

clang/test/Modules/Inputs/namespaces-top.h

@@ -12,3 +12,8 @@ namespace N3 {
 
 namespace N12 { }
 
+namespace N13 {
+  void f();
+  int f(int);
+  void (*p)() = &f;
+}

clang/test/Modules/cxx-templates.cpp

@@ -82,11 +82,8 @@ typedef SomeTemplate<int&> SomeTemplateIntRef;
 SomeTemplate<char*> some_template_char_ptr;
 SomeTemplate<char&> some_template_char_ref;
 
-// FIXME: There should only be two 'f's here.
 // CHECK-GLOBAL:      DeclarationName 'f'
 // CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
-// CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
-// CHECK-GLOBAL-NEXT: |-FunctionTemplate {{.*}} 'f'
 // CHECK-GLOBAL-NEXT: `-FunctionTemplate {{.*}} 'f'
 
 // CHECK-NAMESPACE-N:      DeclarationName 'f'

clang/test/Modules/decldef.mm

@@ -6,8 +6,10 @@
 
 @class Def;
 Def *def;
-class Def2;
+class Def2; // expected-note {{forward decl}}
 Def2 *def2;
+namespace Def3NS { class Def3; } // expected-note {{forward decl}}
+Def3NS::Def3 *def3;
 
 @interface Unrelated
 - defMethod;
@@ -39,5 +41,9 @@ void testDef() {
 }
 
 void testDef2() {
-  def2->func();
+  // FIXME: These should both work, since we've (implicitly) imported
+  // decldef.Def here, but they don't, because nothing has triggered the lazy
+  // loading of the definitions of these classes.
+  def2->func(); // expected-error {{incomplete}}
+  def3->func(); // expected-error {{incomplete}}
 }

clang/test/Modules/namespaces.cpp

@@ -75,3 +75,10 @@ void testAnonymousNotMerged() {
 
 // expected-note@Inputs/namespaces-right.h:60 {{passing argument to parameter here}}
 // expected-note@Inputs/namespaces-right.h:67 {{passing argument to parameter here}}
+
+// Test that bringing in one name from an overload set does not hide the rest.
+void testPartialImportOfOverloadSet() {
+  void (*p)() = N13::p;
+  p();
+  N13::f(0);
+}