[modules] When performing redeclaration lookup for a using declaration, prefer

UsingShadowDecls over other declarations of the same entity in the lookup
results. This ensures that we build correct redeclaration chains for the
UsingShadowDecls (otherwise we could see assertions and other misbehavior in
modules builds, when merging combines multiple redeclaration chains for the
same entity from the same module into one chain).

llvm-svn: 243592

clang/lib/Sema/SemaLookup.cpp

@@ -354,13 +354,45 @@ static DeclContext *getContextForScopeMatching(Decl *D) {
   return D->getDeclContext()->getRedeclContext();
 }
 
+/// \brief Determine whether \p D is a better lookup result than \p Existing,
+/// given that they declare the same entity.
+static bool isPreferredLookupResult(Sema::LookupNameKind Kind,
+                                    NamedDecl *D, NamedDecl *Existing) {
+  // When looking up redeclarations of a using declaration, prefer a using
+  // shadow declaration over any other declaration of the same entity.
+  if (Kind == Sema::LookupUsingDeclName && isa<UsingShadowDecl>(D) &&
+      !isa<UsingShadowDecl>(Existing))
+    return true;
+
+  auto *DUnderlying = D->getUnderlyingDecl();
+  auto *EUnderlying = Existing->getUnderlyingDecl();
+
+  // If they have different underlying declarations, pick one arbitrarily
+  // (this happens when two type declarations denote the same type).
+  // FIXME: Should we prefer a struct declaration over a typedef or vice versa?
+  //        If a name could be a typedef-name or a class-name, which is it?
+  if (DUnderlying->getCanonicalDecl() != EUnderlying->getCanonicalDecl()) {
+    assert(isa<TypeDecl>(DUnderlying) && isa<TypeDecl>(EUnderlying));
+    return false;
+  }
+
+  // If D is newer than Existing, prefer it.
+  for (Decl *Prev = DUnderlying->getPreviousDecl(); Prev;
+       Prev = Prev->getPreviousDecl())
+    if (Prev == EUnderlying)
+      return true;
+
+  return false;
+}
+
 /// Resolves the result kind of this lookup.
 void LookupResult::resolveKind() {
   unsigned N = Decls.size();
 
   // Fast case: no possible ambiguity.
   if (N == 0) {
-    assert(ResultKind == NotFound || ResultKind == NotFoundInCurrentInstantiation);
+    assert(ResultKind == NotFound ||
+           ResultKind == NotFoundInCurrentInstantiation);
     return;
   }
 
@@ -379,7 +411,7 @@ void LookupResult::resolveKind() {
   if (ResultKind == Ambiguous) return;
 
   llvm::SmallDenseMap<NamedDecl*, unsigned, 16> Unique;
-  llvm::SmallPtrSet<QualType, 16> UniqueTypes;
+  llvm::SmallDenseMap<QualType, unsigned, 16> UniqueTypes;
 
   bool Ambiguous = false;
   bool HasTag = false, HasFunction = false, HasNonFunction = false;
@@ -398,42 +430,42 @@ void LookupResult::resolveKind() {
       continue;
     }
 
+    llvm::Optional<unsigned> ExistingI;
+
     // Redeclarations of types via typedef can occur both within a scope
     // and, through using declarations and directives, across scopes. There is
     // no ambiguity if they all refer to the same type, so unique based on the
     // canonical type.
     if (TypeDecl *TD = dyn_cast<TypeDecl>(D)) {
+      // FIXME: Why are nested type declarations treated differently?
       if (!TD->getDeclContext()->isRecord()) {
         QualType T = getSema().Context.getTypeDeclType(TD);
-        if (!UniqueTypes.insert(getSema().Context.getCanonicalType(T)).second) {
-          // The type is not unique; pull something off the back and continue
-          // at this index.
-          Decls[I] = Decls[--N];
-          continue;
+        auto UniqueResult = UniqueTypes.insert(
+            std::make_pair(getSema().Context.getCanonicalType(T), I));
+        if (!UniqueResult.second) {
+          // The type is not unique.
+          ExistingI = UniqueResult.first->second;
         }
       }
     }
 
-    auto UniqueResult = Unique.insert(std::make_pair(D, I));
-    if (!UniqueResult.second) {
-      // If it's not unique, pull something off the back (and
-      // continue at this index).
-      auto ExistingI = UniqueResult.first->second;
-      auto *Existing = Decls[ExistingI]->getUnderlyingDecl();
-      for (Decl *Prev = Decls[I]->getUnderlyingDecl()->getPreviousDecl(); /**/;
-           Prev = Prev->getPreviousDecl()) {
-        if (Prev == Existing) {
-          // Existing result is older. Replace it with the new one.
-          Decls[ExistingI] = Decls[I];
-          Decls[I] = Decls[--N];
-          break;
-        }
-        if (!Prev) {
-          // New decl is older. Keep the existing one.
-          Decls[I] = Decls[--N];
-          break;
-        }
+    // For non-type declarations, check for a prior lookup result naming this
+    // canonical declaration.
+    if (!ExistingI) {
+      auto UniqueResult = Unique.insert(std::make_pair(D, I));
+      if (!UniqueResult.second) {
+        // We've seen this entity before.
+        ExistingI = UniqueResult.first->second;
       }
+    }
+
+    if (ExistingI) {
+      // This is not a unique lookup result. Pick one of the results and
+      // discard the other.
+      if (isPreferredLookupResult(getLookupKind(), Decls[I],
+                                  Decls[*ExistingI]))
+        Decls[*ExistingI] = Decls[I];
+      Decls[I] = Decls[--N];
       continue;
     }
 

clang/test/Modules/Inputs/submodules-merge-defs/defs.h

@@ -97,3 +97,9 @@ namespace MergeFunctionTemplateSpecializations {
 
 enum ScopedEnum : int;
 enum ScopedEnum : int { a, b, c };
+
+namespace RedeclDifferentDeclKind {
+  struct X {};
+  typedef X X;
+  using RedeclDifferentDeclKind::X;
+}

clang/test/Modules/Inputs/using-decl-redecl/a.h

@@ -0,0 +1,2 @@
+struct string {};
+namespace N { typedef ::string clstring; }

clang/test/Modules/Inputs/using-decl-redecl/b.h

@@ -0,0 +1,3 @@
+#include "a.h"
+namespace N { using ::N::clstring; }
+extern N::clstring b;

clang/test/Modules/Inputs/using-decl-redecl/c.h

@@ -0,0 +1,2 @@
+#include "b.h"
+namespace N { using ::N::clstring; }

clang/test/Modules/Inputs/using-decl-redecl/module.modulemap

@@ -0,0 +1,3 @@
+module a { header "a.h" }
+module b { header "b.h" export * }
+module c { header "c.h" export * }

clang/test/Modules/using-decl-redecl.cpp

@@ -0,0 +1,11 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t \
+// RUN:            -fmodule-map-file=%S/Inputs/using-decl-redecl/module.modulemap \
+// RUN:            -I%S/Inputs/using-decl-redecl \
+// RUN:            -verify %s
+#include "c.h"
+N::clstring y = b;
+
+// Use a typo to trigger import of all declarations in N.
+N::clstrinh s; // expected-error {{did you mean 'clstring'}}
+// expected-note@a.h:2 {{here}}