Fix implementation of [temp.local]p4.

When a template-name is looked up, we need to give injected-class-name
declarations of class templates special treatment, as they denote a
template rather than a type.

Previously we achieved this by applying a filter to the lookup results
after completing name lookup, but that is incorrect in various ways, not
least of which is that it lost all information about access and how
members were named, and the filtering caused us to generally lose
all ambiguity errors between templates and non-templates.

We now preserve the lookup results exactly, and the few places that need
to map from a declaration found by name lookup into a declaration of a
template do so explicitly. Deduplication of repeated lookup results of
the same injected-class-name declaration is done by name lookup instead
of after the fact.

This reinstates r354091, which was previously reverted in r354097
because it exposed bugs in lldb and compiler-rt. Those bugs were fixed
in r354173 and r354174 respectively.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@354176 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Sema/Lookup.h

@@ -172,7 +172,8 @@ public:
       : SemaPtr(Other.SemaPtr), NameInfo(Other.NameInfo),
         LookupKind(Other.LookupKind), IDNS(Other.IDNS), Redecl(Other.Redecl),
         ExternalRedecl(Other.ExternalRedecl), HideTags(Other.HideTags),
-        AllowHidden(Other.AllowHidden) {}
+        AllowHidden(Other.AllowHidden),
+        TemplateNameLookup(Other.TemplateNameLookup) {}
 
   // FIXME: Remove these deleted methods once the default build includes
   // -Wdeprecated.
@@ -193,7 +194,8 @@ public:
         HideTags(std::move(Other.HideTags)),
         Diagnose(std::move(Other.Diagnose)),
         AllowHidden(std::move(Other.AllowHidden)),
-        Shadowed(std::move(Other.Shadowed)) {
+        Shadowed(std::move(Other.Shadowed)),
+        TemplateNameLookup(std::move(Other.TemplateNameLookup)) {
     Other.Paths = nullptr;
     Other.Diagnose = false;
   }
@@ -216,6 +218,7 @@ public:
     Diagnose = std::move(Other.Diagnose);
     AllowHidden = std::move(Other.AllowHidden);
     Shadowed = std::move(Other.Shadowed);
+    TemplateNameLookup = std::move(Other.TemplateNameLookup);
     Other.Paths = nullptr;
     Other.Diagnose = false;
     return *this;
@@ -286,6 +289,15 @@ public:
     HideTags = Hide;
   }
 
+  /// Sets whether this is a template-name lookup. For template-name lookups,
+  /// injected-class-names are treated as naming a template rather than a
+  /// template specialization.
+  void setTemplateNameLookup(bool TemplateName) {
+    TemplateNameLookup = TemplateName;
+  }
+
+  bool isTemplateNameLookup() const { return TemplateNameLookup; }
+
   bool isAmbiguous() const {
     return getResultKind() == Ambiguous;
   }
@@ -739,6 +751,9 @@ private:
   /// declaration that we skipped. This only happens when \c LookupKind
   /// is \c LookupRedeclarationWithLinkage.
   bool Shadowed = false;
+
+  /// True if we're looking up a template-name.
+  bool TemplateNameLookup = false;
 };
 
 /// Consumes visible declarations found when searching for

include/clang/Sema/Sema.h

@@ -6212,9 +6212,21 @@ public:
   // C++ Templates [C++ 14]
   //
   void FilterAcceptableTemplateNames(LookupResult &R,
-                                     bool AllowFunctionTemplates = true);
+                                     bool AllowFunctionTemplates = true,
+                                     bool AllowDependent = true);
   bool hasAnyAcceptableTemplateNames(LookupResult &R,
-                                     bool AllowFunctionTemplates = true);
+                                     bool AllowFunctionTemplates = true,
+                                     bool AllowDependent = true);
+  /// Try to interpret the lookup result D as a template-name.
+  ///
+  /// \param D A declaration found by name lookup.
+  /// \param AllowFunctionTemplates Whether function templates should be
+  ///        considered valid results.
+  /// \param AllowDependent Whether unresolved using declarations (that might
+  ///        name templates) should be considered valid results.
+  NamedDecl *getAsTemplateNameDecl(NamedDecl *D,
+                                   bool AllowFunctionTemplates = true,
+                                   bool AllowDependent = true);
 
   bool LookupTemplateName(LookupResult &R, Scope *S, CXXScopeSpec &SS,
                           QualType ObjectType, bool EnteringContext,

lib/Sema/SemaDecl.cpp

@@ -1017,7 +1017,8 @@ Corrected:
 
   case LookupResult::Ambiguous:
     if (getLangOpts().CPlusPlus && NextToken.is(tok::less) &&
-        hasAnyAcceptableTemplateNames(Result)) {
+        hasAnyAcceptableTemplateNames(Result, /*AllowFunctionTemplates=*/true,
+                                      /*AllowDependent=*/false)) {
       // C++ [temp.local]p3:
       //   A lookup that finds an injected-class-name (10.2) can result in an
       //   ambiguity in certain cases (for example, if it is found in more than
@@ -1041,7 +1042,9 @@ Corrected:
   }
 
   if (getLangOpts().CPlusPlus && NextToken.is(tok::less) &&
-      (IsFilteredTemplateName || hasAnyAcceptableTemplateNames(Result))) {
+      (IsFilteredTemplateName ||
+       hasAnyAcceptableTemplateNames(Result, /*AllowFunctionTemplates=*/true,
+                                     /*AllowDependent=*/false))) {
     // C++ [temp.names]p3:
     //   After name lookup (3.4) finds that a name is a template-name or that
     //   an operator-function-id or a literal- operator-id refers to a set of
@@ -1060,15 +1063,16 @@ Corrected:
         Template = Context.getOverloadedTemplateName(Result.begin(),
                                                      Result.end());
       } else {
-        TemplateDecl *TD
-          = cast<TemplateDecl>((*Result.begin())->getUnderlyingDecl());
+        auto *TD = cast<TemplateDecl>(getAsTemplateNameDecl(
+            *Result.begin(), /*AllowFunctionTemplates=*/true,
+            /*AllowDependent=*/false));
         IsFunctionTemplate = isa<FunctionTemplateDecl>(TD);
         IsVarTemplate = isa<VarTemplateDecl>(TD);
 
         if (SS.isSet() && !SS.isInvalid())
-          Template = Context.getQualifiedTemplateName(SS.getScopeRep(),
-                                                    /*TemplateKeyword=*/false,
-                                                      TD);
+          Template =
+              Context.getQualifiedTemplateName(SS.getScopeRep(),
+                                               /*TemplateKeyword=*/false, TD);
         else
           Template = TemplateName(TD);
       }

lib/Sema/SemaDeclCXX.cpp

@@ -1128,7 +1128,6 @@ static bool checkTupleLikeDecomposition(Sema &S,
         }
       }
     }
-    S.FilterAcceptableTemplateNames(MemberGet);
   }
 
   unsigned I = 0;

lib/Sema/SemaLookup.cpp

@@ -2172,11 +2172,27 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
         DeclContext::lookup_iterator FirstD = FirstPath->Decls.begin();
         DeclContext::lookup_iterator CurrentD = Path->Decls.begin();
 
+        // Get the decl that we should use for deduplicating this lookup.
+        auto GetRepresentativeDecl = [&](NamedDecl *D) -> Decl * {
+          // C++ [temp.local]p3:
+          //   A lookup that finds an injected-class-name (10.2) can result in
+          //   an ambiguity in certain cases (for example, if it is found in
+          //   more than one base class). If all of the injected-class-names
+          //   that are found refer to specializations of the same class
+          //   template, and if the name is used as a template-name, the
+          //   reference refers to the class template itself and not a
+          //   specialization thereof, and is not ambiguous.
+          if (R.isTemplateNameLookup())
+            if (auto *TD = getAsTemplateNameDecl(D))
+              D = TD;
+          return D->getUnderlyingDecl()->getCanonicalDecl();
+        };
+
         while (FirstD != FirstPath->Decls.end() &&
                CurrentD != Path->Decls.end()) {
-         if ((*FirstD)->getUnderlyingDecl()->getCanonicalDecl() !=
-             (*CurrentD)->getUnderlyingDecl()->getCanonicalDecl())
-           break;
+          if (GetRepresentativeDecl(*FirstD) !=
+              GetRepresentativeDecl(*CurrentD))
+            break;
 
           ++FirstD;
           ++CurrentD;

lib/Sema/SemaTemplate.cpp

@@ -66,17 +66,20 @@ static Expr *clang::formAssociatedConstraints(TemplateParameterList *Params,
 
 /// Determine whether the declaration found is acceptable as the name
 /// of a template and, if so, return that template declaration. Otherwise,
-/// returns NULL.
-static NamedDecl *isAcceptableTemplateName(ASTContext &Context,
-                                           NamedDecl *Orig,
-                                           bool AllowFunctionTemplates) {
-  NamedDecl *D = Orig->getUnderlyingDecl();
+/// returns null.
+///
+/// Note that this may return an UnresolvedUsingValueDecl if AllowDependent
+/// is true. In all other cases it will return a TemplateDecl (or null).
+NamedDecl *Sema::getAsTemplateNameDecl(NamedDecl *D,
+                                       bool AllowFunctionTemplates,
+                                       bool AllowDependent) {
+  D = D->getUnderlyingDecl();
 
   if (isa<TemplateDecl>(D)) {
     if (!AllowFunctionTemplates && isa<FunctionTemplateDecl>(D))
       return nullptr;
 
-    return Orig;
+    return D;
   }
 
   if (CXXRecordDecl *Record = dyn_cast<CXXRecordDecl>(D)) {
@@ -107,54 +110,29 @@ static NamedDecl *isAcceptableTemplateName(ASTContext &Context,
   // 'using Dependent::foo;' can resolve to a template name.
   // 'using typename Dependent::foo;' cannot (not even if 'foo' is an
   // injected-class-name).
-  if (isa<UnresolvedUsingValueDecl>(D))
+  if (AllowDependent && isa<UnresolvedUsingValueDecl>(D))
     return D;
 
   return nullptr;
 }
 
 void Sema::FilterAcceptableTemplateNames(LookupResult &R,
-                                         bool AllowFunctionTemplates) {
-  // The set of class templates we've already seen.
-  llvm::SmallPtrSet<ClassTemplateDecl *, 8> ClassTemplates;
+                                         bool AllowFunctionTemplates,
+                                         bool AllowDependent) {
   LookupResult::Filter filter = R.makeFilter();
   while (filter.hasNext()) {
     NamedDecl *Orig = filter.next();
-    NamedDecl *Repl = isAcceptableTemplateName(Context, Orig,
-                                               AllowFunctionTemplates);
-    if (!Repl)
+    if (!getAsTemplateNameDecl(Orig, AllowFunctionTemplates, AllowDependent))
       filter.erase();
-    else if (Repl != Orig) {
-
-      // C++ [temp.local]p3:
-      //   A lookup that finds an injected-class-name (10.2) can result in an
-      //   ambiguity in certain cases (for example, if it is found in more than
-      //   one base class). If all of the injected-class-names that are found
-      //   refer to specializations of the same class template, and if the name
-      //   is used as a template-name, the reference refers to the class
-      //   template itself and not a specialization thereof, and is not
-      //   ambiguous.
-      if (ClassTemplateDecl *ClassTmpl = dyn_cast<ClassTemplateDecl>(Repl))
-        if (!ClassTemplates.insert(ClassTmpl).second) {
-          filter.erase();
-          continue;
-        }
-
-      // FIXME: we promote access to public here as a workaround to
-      // the fact that LookupResult doesn't let us remember that we
-      // found this template through a particular injected class name,
-      // which means we end up doing nasty things to the invariants.
-      // Pretending that access is public is *much* safer.
-      filter.replace(Repl, AS_public);
-    }
   }
   filter.done();
 }
 
 bool Sema::hasAnyAcceptableTemplateNames(LookupResult &R,
-                                         bool AllowFunctionTemplates) {
+                                         bool AllowFunctionTemplates,
+                                         bool AllowDependent) {
   for (LookupResult::iterator I = R.begin(), IEnd = R.end(); I != IEnd; ++I)
-    if (isAcceptableTemplateName(Context, *I, AllowFunctionTemplates))
+    if (getAsTemplateNameDecl(*I, AllowFunctionTemplates, AllowDependent))
       return true;
 
   return false;
@@ -198,20 +176,45 @@ TemplateNameKind Sema::isTemplateName(Scope *S,
                          MemberOfUnknownSpecialization))
     return TNK_Non_template;
   if (R.empty()) return TNK_Non_template;
+
+  NamedDecl *D = nullptr;
   if (R.isAmbiguous()) {
-    // Suppress diagnostics;  we'll redo this lookup later.
-    R.suppressDiagnostics();
+    // If we got an ambiguity involving a non-function template, treat this
+    // as a template name, and pick an arbitrary template for error recovery.
+    bool AnyFunctionTemplates = false;
+    for (NamedDecl *FoundD : R) {
+      if (NamedDecl *FoundTemplate = getAsTemplateNameDecl(FoundD)) {
+        if (isa<FunctionTemplateDecl>(FoundTemplate))
+          AnyFunctionTemplates = true;
+        else {
+          D = FoundTemplate;
+          break;
+        }
+      }
+    }
 
-    // FIXME: we might have ambiguous templates, in which case we
-    // should at least parse them properly!
-    return TNK_Non_template;
+    // If we didn't find any templates at all, this isn't a template name.
+    // Leave the ambiguity for a later lookup to diagnose.
+    if (!D && !AnyFunctionTemplates) {
+      R.suppressDiagnostics();
+      return TNK_Non_template;
+    }
+
+    // If the only templates were function templates, filter out the rest.
+    // We'll diagnose the ambiguity later.
+    if (!D)
+      FilterAcceptableTemplateNames(R);
   }
 
+  // At this point, we have either picked a single template name declaration D
+  // or we have a non-empty set of results R containing either one template name
+  // declaration or a set of function templates.
+
   TemplateName Template;
   TemplateNameKind TemplateKind;
 
   unsigned ResultCount = R.end() - R.begin();
-  if (ResultCount > 1) {
+  if (!D && ResultCount > 1) {
     // We assume that we'll preserve the qualifier from a function
     // template name in other ways.
     Template = Context.getOverloadedTemplateName(R.begin(), R.end());
@@ -219,12 +222,19 @@ TemplateNameKind Sema::isTemplateName(Scope *S,
 
     // We'll do this lookup again later.
     R.suppressDiagnostics();
-  } else if (isa<UnresolvedUsingValueDecl>((*R.begin())->getUnderlyingDecl())) {
-    // We don't yet know whether this is a template-name or not.
-    MemberOfUnknownSpecialization = true;
-    return TNK_Non_template;
   } else {
-    TemplateDecl *TD = cast<TemplateDecl>((*R.begin())->getUnderlyingDecl());
+    if (!D) {
+      D = getAsTemplateNameDecl(*R.begin());
+      assert(D && "unambiguous result is not a template name");
+    }
+
+    if (isa<UnresolvedUsingValueDecl>(D)) {
+      // We don't yet know whether this is a template-name or not.
+      MemberOfUnknownSpecialization = true;
+      return TNK_Non_template;
+    }
+
+    TemplateDecl *TD = cast<TemplateDecl>(D);
 
     if (SS.isSet() && !SS.isInvalid()) {
       NestedNameSpecifier *Qualifier = SS.getScopeRep();
@@ -316,6 +326,8 @@ bool Sema::LookupTemplateName(LookupResult &Found,
                               bool EnteringContext,
                               bool &MemberOfUnknownSpecialization,
                               SourceLocation TemplateKWLoc) {
+  Found.setTemplateNameLookup(true);
+
   // Determine where to perform name lookup
   MemberOfUnknownSpecialization = false;
   DeclContext *LookupCtx = nullptr;
@@ -390,6 +402,9 @@ bool Sema::LookupTemplateName(LookupResult &Found,
     IsDependent |= Found.wasNotFoundInCurrentInstantiation();
   }
 
+  if (Found.isAmbiguous())
+    return false;
+
   if (Found.empty() && !IsDependent) {
     // If we did not find any names, attempt to correct any typos.
     DeclarationName Name = Found.getLookupName();
@@ -407,7 +422,9 @@ bool Sema::LookupTemplateName(LookupResult &Found,
       if (auto *ND = Corrected.getFoundDecl())
         Found.addDecl(ND);
       FilterAcceptableTemplateNames(Found);
-      if (!Found.empty()) {
+      if (Found.isAmbiguous()) {
+        Found.clear();
+      } else if (!Found.empty()) {
         if (LookupCtx) {
           std::string CorrectedStr(Corrected.getAsString(getLangOpts()));
           bool DroppedSpecifier = Corrected.WillReplaceSpecifier() &&
@@ -457,14 +474,19 @@ bool Sema::LookupTemplateName(LookupResult &Found,
     // Note: C++11 does not perform this second lookup.
     LookupResult FoundOuter(*this, Found.getLookupName(), Found.getNameLoc(),
                             LookupOrdinaryName);
+    FoundOuter.setTemplateNameLookup(true);
     LookupName(FoundOuter, S);
+    // FIXME: We silently accept an ambiguous lookup here, in violation of
+    // [basic.lookup]/1.
     FilterAcceptableTemplateNames(FoundOuter, /*AllowFunctionTemplates=*/false);
 
+    NamedDecl *OuterTemplate;
     if (FoundOuter.empty()) {
       //   - if the name is not found, the name found in the class of the
       //     object expression is used, otherwise
-    } else if (!FoundOuter.getAsSingle<ClassTemplateDecl>() ||
-               FoundOuter.isAmbiguous()) {
+    } else if (FoundOuter.isAmbiguous() || !FoundOuter.isSingleResult() ||
+               !(OuterTemplate =
+                     getAsTemplateNameDecl(FoundOuter.getFoundDecl()))) {
       //   - if the name is found in the context of the entire
       //     postfix-expression and does not name a class template, the name
       //     found in the class of the object expression is used, otherwise
@@ -474,8 +496,8 @@ bool Sema::LookupTemplateName(LookupResult &Found,
       //     entity as the one found in the class of the object expression,
       //     otherwise the program is ill-formed.
       if (!Found.isSingleResult() ||
-          Found.getFoundDecl()->getCanonicalDecl()
-            != FoundOuter.getFoundDecl()->getCanonicalDecl()) {
+          getAsTemplateNameDecl(Found.getFoundDecl())->getCanonicalDecl() !=
+              OuterTemplate->getCanonicalDecl()) {
         Diag(Found.getNameLoc(),
              diag::ext_nested_name_member_ref_lookup_ambiguous)
           << Found.getLookupName()
@@ -545,7 +567,8 @@ void Sema::diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
 
   // Try to correct the name by looking for templates and C++ named casts.
   struct TemplateCandidateFilter : CorrectionCandidateCallback {
-    TemplateCandidateFilter() {
+    Sema &S;
+    TemplateCandidateFilter(Sema &S) : S(S) {
       WantTypeSpecifiers = false;
       WantExpressionKeywords = false;
       WantRemainingKeywords = false;
@@ -553,7 +576,7 @@ void Sema::diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
     };
     bool ValidateCandidate(const TypoCorrection &Candidate) override {
       if (auto *ND = Candidate.getCorrectionDecl())
-        return isAcceptableTemplateName(ND->getASTContext(), ND, true);
+        return S.getAsTemplateNameDecl(ND);
       return Candidate.isKeyword();
     }
   };
@@ -561,12 +584,11 @@ void Sema::diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
   DeclarationName Name = NameInfo.getName();
   if (TypoCorrection Corrected =
           CorrectTypo(NameInfo, LookupKind, S, &SS,
-                      llvm::make_unique<TemplateCandidateFilter>(),
+                      llvm::make_unique<TemplateCandidateFilter>(*this),
                       CTK_ErrorRecovery, LookupCtx)) {
     auto *ND = Corrected.getFoundDecl();
     if (ND)
-      ND = isAcceptableTemplateName(Context, ND,
-                                    /*AllowFunctionTemplates*/ true);
+      ND = getAsTemplateNameDecl(ND);
     if (ND || Corrected.isKeyword()) {
       if (LookupCtx) {
         std::string CorrectedStr(Corrected.getAsString(getLangOpts()));
@@ -4262,7 +4284,7 @@ TemplateNameKind Sema::ActOnDependentTemplateName(Scope *S,
                      LookupOrdinaryName);
       bool MOUS;
       if (!LookupTemplateName(R, S, SS, ObjectType.get(), EnteringContext,
-                              MOUS, TemplateKWLoc))
+                              MOUS, TemplateKWLoc) && !R.isAmbiguous())
         Diag(Name.getBeginLoc(), diag::err_no_member)
             << DNI.getName() << LookupCtx << SS.getRange();
       return TNK_Non_template;

test/CXX/class.access/p4.cpp

@@ -514,16 +514,12 @@ namespace test17 {
 }
 
 namespace test18 {
-  template <class T> class A {};
-  class B : A<int> {
+  template <class T> class A {}; // expected-note {{member is declared here}}
+  class B : A<int> { // expected-note {{constrained by implicitly private inheritance here}}
     A<int> member;
   };
-
-  // FIXME: this access to A should be forbidden (because C++ is dumb),
-  // but LookupResult can't express the necessary information to do
-  // the check, so we aggressively suppress access control.
   class C : B {
-    A<int> member;
+    A<int> member; // expected-error {{'A' is a private member of 'test18::A<int>'}}
   };
 }
 

test/CXX/temp/temp.decls/temp.friend/p1.cpp

@@ -380,10 +380,10 @@ template <class T> struct A {
 namespace test18 {
 namespace ns1 { template <class T> struct foo {}; } // expected-note{{candidate ignored: not a function template}}
 namespace ns2 { void foo() {} } // expected-note{{candidate ignored: not a function template}}
-using ns1::foo;
-using ns2::foo;
+using ns1::foo; // expected-note {{found by name lookup}}
+using ns2::foo; // expected-note {{found by name lookup}}
 
 template <class T> class A {
-  friend void foo<T>() {} // expected-error{{no candidate function template was found for dependent friend function template specialization}}
+  friend void foo<T>() {} // expected-error {{ambiguous}} expected-error{{no candidate function template was found for dependent friend function template specialization}}
 };
 }

test/SemaTemplate/temp.cpp

@@ -8,12 +8,43 @@ namespace test0 {
 
 // PR7252
 namespace test1 {
-  namespace A { template<typename T> struct Base { typedef T t; }; } // expected-note {{member found}}
+  namespace A { template<typename T> struct Base { typedef T t; }; } // expected-note 3{{member}}
   namespace B { template<typename T> struct Base { typedef T t; }; } // expected-note {{member found}}
 
   template<typename T> struct Derived : A::Base<char>, B::Base<int> {
-    // FIXME: the syntax error here is unfortunate
-    typename Derived::Base<float>::t x; // expected-error {{found in multiple base classes of different types}} \
-                                        // expected-error {{expected member name or ';'}}
+    typename Derived::Base<float>::t x; // expected-error {{found in multiple base classes of different types}}
   };
+
+  class X : A::Base<int> {}; // expected-note 2{{private}}
+  class Y : A::Base<float> {};
+  struct Z : A::Base<double> {};
+  struct Use1 : X, Y {
+    Base<double> b1; // expected-error {{private}}
+    Use1::Base<double> b2; // expected-error {{private}}
+  };
+  struct Use2 : Z, Y {
+    Base<double> b1;
+    Use2::Base<double> b2;
+  };
+  struct Use3 : X, Z {
+    Base<double> b1;
+    Use3::Base<double> b2;
+  };
+}
+
+namespace test2 {
+  struct A { static int x; }; // expected-note 4{{member}}
+  struct B { template<typename T> static T x(); }; // expected-note 4{{member}}
+  struct C { template<typename T> struct x {}; }; // expected-note 3{{member}}
+  struct D { template<typename T> static T x(); }; // expected-note {{member}}
+
+  template<typename ...T> struct X : T... {};
+
+  void f() {
+    X<A, B>::x<int>(); // expected-error {{found in multiple base classes of different types}}
+    X<A, C>::x<int>(); // expected-error {{found in multiple base classes of different types}}
+    X<B, C>::x<int>(); // expected-error {{found in multiple base classes of different types}}
+    X<A, B, C>::x<int>(); // expected-error {{found in multiple base classes of different types}}
+    X<A, B, D>::x<int>(); // expected-error {{found in multiple base classes of different types}}
+  }
 }