Fix hole in our enforcement of rule requiring 'typename' prior to a dependent

name. If the dependent name happened to end in a template-id (X<T>::Y<U>), we
would fail to notice that the 'typename' keyword is missing when resolving it
to a type.

It turns out that GCC has a similar bug. If this shows up in much real code, we
can easily downgrade this to an ExtWarn.


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

Author: zygoloid

include/clang/Basic/DiagnosticSemaKinds.td

@@ -4324,6 +4324,8 @@ def note_typename_refers_here : Note<
     "referenced member %0 is declared here">;
 def err_typename_missing : Error<
   "missing 'typename' prior to dependent type name '%0%1'">;
+def err_typename_missing_template : Error<
+  "missing 'typename' prior to dependent type template name '%0%1'">;
 def ext_typename_missing : ExtWarn<
   "missing 'typename' prior to dependent type name '%0%1'">,
   InGroup<DiagGroup<"typename-missing">>;

include/clang/Parse/Parser.h

@@ -2694,7 +2694,7 @@ class Parser : public CodeCompletionHandler {
                                SourceLocation TemplateKWLoc,
                                UnqualifiedId &TemplateName,
                                bool AllowTypeAnnotation = true);
-  void AnnotateTemplateIdTokenAsType();
+  void AnnotateTemplateIdTokenAsType(bool IsClassName = false);
   bool IsTemplateArgumentList(unsigned Skip = 0);
   bool ParseTemplateArgumentList(TemplateArgList &TemplateArgs);
   ParsedTemplateArgument ParseTemplateTemplateArgument();

include/clang/Sema/Sema.h

@@ -5931,7 +5931,8 @@ class Sema {
                       SourceLocation LAngleLoc,
                       ASTTemplateArgsPtr TemplateArgs,
                       SourceLocation RAngleLoc,
-                      bool IsCtorOrDtorName = false);
+                      bool IsCtorOrDtorName = false,
+                      bool IsClassName = false);
 
   /// \brief Parsed an elaborated-type-specifier that refers to a template-id,
   /// such as \c class T::template apply<U>.

lib/Parse/ParseDeclCXX.cpp

@@ -1076,7 +1076,7 @@ TypeResult Parser::ParseBaseTypeSpecifier(SourceLocation &BaseLoc,
     TemplateIdAnnotation *TemplateId = takeTemplateIdAnnotation(Tok);
     if (TemplateId->Kind == TNK_Type_template ||
         TemplateId->Kind == TNK_Dependent_template_name) {
-      AnnotateTemplateIdTokenAsType();
+      AnnotateTemplateIdTokenAsType(/*IsClassName*/true);
 
       assert(Tok.is(tok::annot_typename) && "template-id -> type failed");
       ParsedType Type = getTypeAnnotation(Tok);
@@ -1124,10 +1124,10 @@ TypeResult Parser::ParseBaseTypeSpecifier(SourceLocation &BaseLoc,
 
     // Parse the full template-id, then turn it into a type.
     if (AnnotateTemplateIdToken(Template, TNK, SS, SourceLocation(),
-                                TemplateName, true))
+                                TemplateName))
       return true;
-    if (TNK == TNK_Dependent_template_name)
-      AnnotateTemplateIdTokenAsType();
+    if (TNK == TNK_Type_template || TNK == TNK_Dependent_template_name)
+      AnnotateTemplateIdTokenAsType(/*IsClassName*/true);
 
     // If we didn't end up with a typename token, there's nothing more we
     // can do.
@@ -1145,7 +1145,7 @@ TypeResult Parser::ParseBaseTypeSpecifier(SourceLocation &BaseLoc,
   // We have an identifier; check whether it is actually a type.
   IdentifierInfo *CorrectedII = nullptr;
   ParsedType Type = Actions.getTypeName(
-      *Id, IdLoc, getCurScope(), &SS, true, false, nullptr,
+      *Id, IdLoc, getCurScope(), &SS, /*IsClassName=*/true, false, nullptr,
       /*IsCtorOrDtorName=*/false,
       /*NonTrivialTypeSourceInfo=*/true,
       /*IsClassTemplateDeductionContext*/ false, &CorrectedII);
@@ -3377,7 +3377,7 @@ MemInitResult Parser::ParseMemInitializer(Decl *ConstructorDecl) {
     TemplateIdAnnotation *TemplateId = takeTemplateIdAnnotation(Tok);
     if (TemplateId->Kind == TNK_Type_template ||
         TemplateId->Kind == TNK_Dependent_template_name) {
-      AnnotateTemplateIdTokenAsType();
+      AnnotateTemplateIdTokenAsType(/*IsClassName*/true);
       assert(Tok.is(tok::annot_typename) && "template-id -> type failed");
       TemplateTypeTy = getTypeAnnotation(Tok);
     }

lib/Parse/ParseTemplate.cpp

@@ -1064,7 +1064,12 @@ bool Parser::AnnotateTemplateIdToken(TemplateTy Template, TemplateNameKind TNK,
 /// If there was a failure when forming the type from the template-id,
 /// a type annotation token will still be created, but will have a
 /// NULL type pointer to signify an error.
-void Parser::AnnotateTemplateIdTokenAsType() {
+///
+/// \param IsClassName Is this template-id appearing in a context where we
+/// know it names a class, such as in an elaborated-type-specifier or
+/// base-specifier? ('typename' and 'template' are unneeded and disallowed
+/// in those contexts.)
+void Parser::AnnotateTemplateIdTokenAsType(bool IsClassName) {
   assert(Tok.is(tok::annot_template_id) && "Requires template-id tokens");
 
   TemplateIdAnnotation *TemplateId = takeTemplateIdAnnotation(Tok);
@@ -1083,7 +1088,9 @@ void Parser::AnnotateTemplateIdTokenAsType() {
                                   TemplateId->TemplateNameLoc,
                                   TemplateId->LAngleLoc,
                                   TemplateArgsPtr,
-                                  TemplateId->RAngleLoc);
+                                  TemplateId->RAngleLoc,
+                                  /*IsCtorOrDtorName*/false,
+                                  IsClassName);
   // Create the new "type" annotation token.
   Tok.setKind(tok::annot_typename);
   setTypeAnnotation(Tok, Type.isInvalid() ? nullptr : Type.get());

lib/Sema/SemaTemplate.cpp

@@ -2426,17 +2426,36 @@ Sema::ActOnTemplateIdType(CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
                           SourceLocation LAngleLoc,
                           ASTTemplateArgsPtr TemplateArgsIn,
                           SourceLocation RAngleLoc,
-                          bool IsCtorOrDtorName) {
+                          bool IsCtorOrDtorName, bool IsClassName) {
   if (SS.isInvalid())
     return true;
 
-  // Per C++ [class.qual]p2, if the template-id was an injected-class-name,
-  // it's not actually allowed to be used as a type in most cases. Because
-  // we annotate it before we know whether it's valid, we have to check for
-  // this case here.
-  if (!IsCtorOrDtorName) {
-    auto *LookupRD =
-        dyn_cast_or_null<CXXRecordDecl>(computeDeclContext(SS, true));
+  if (!IsCtorOrDtorName && !IsClassName && SS.isSet()) {
+    DeclContext *LookupCtx = computeDeclContext(SS, /*EnteringContext*/false);
+
+    // C++ [temp.res]p3:
+    //   A qualified-id that refers to a type and in which the
+    //   nested-name-specifier depends on a template-parameter (14.6.2)
+    //   shall be prefixed by the keyword typename to indicate that the
+    //   qualified-id denotes a type, forming an
+    //   elaborated-type-specifier (7.1.5.3).
+    if (!LookupCtx && isDependentScopeSpecifier(SS)) {
+      Diag(TemplateIILoc, diag::err_typename_missing_template)
+        << SS.getScopeRep() << TemplateII->getName();
+      // Recover as if 'typename' were specified.
+      // FIXME: This is not quite correct recovery as we don't transform SS
+      // into the corresponding dependent form (and we don't diagnose missing
+      // 'template' keywords within SS as a result).
+      return ActOnTypenameType(nullptr, SourceLocation(), SS, TemplateKWLoc,
+                               TemplateD, TemplateII, TemplateIILoc, LAngleLoc,
+                               TemplateArgsIn, RAngleLoc);
+    }
+
+    // Per C++ [class.qual]p2, if the template-id was an injected-class-name,
+    // it's not actually allowed to be used as a type in most cases. Because
+    // we annotate it before we know whether it's valid, we have to check for
+    // this case here.
+    auto *LookupRD = dyn_cast_or_null<CXXRecordDecl>(LookupCtx);
     if (LookupRD && LookupRD->getIdentifier() == TemplateII) {
       Diag(TemplateIILoc,
            TemplateKWLoc.isInvalid()
@@ -8588,13 +8607,15 @@ Sema::ActOnTypenameType(Scope *S,
 
   // Strangely, non-type results are not ignored by this lookup, so the
   // program is ill-formed if it finds an injected-class-name.
-  auto *LookupRD =
-      dyn_cast_or_null<CXXRecordDecl>(computeDeclContext(SS, true));
-  if (LookupRD && LookupRD->getIdentifier() == TemplateII) {
-    Diag(TemplateIILoc,
-         diag::ext_out_of_line_qualified_id_type_names_constructor)
-      << TemplateII << 0 /*injected-class-name used as template name*/
-      << (TemplateKWLoc.isValid() ? 1 : 0 /*'template'/'typename' keyword*/);
+  if (TypenameLoc.isValid()) {
+    auto *LookupRD =
+        dyn_cast_or_null<CXXRecordDecl>(computeDeclContext(SS, false));
+    if (LookupRD && LookupRD->getIdentifier() == TemplateII) {
+      Diag(TemplateIILoc,
+           diag::ext_out_of_line_qualified_id_type_names_constructor)
+        << TemplateII << 0 /*injected-class-name used as template name*/
+        << (TemplateKWLoc.isValid() ? 1 : 0 /*'template'/'typename' keyword*/);
+    }
   }
 
   // Translate the parser's template argument list in our AST format.

test/CXX/basic/basic.lookup/basic.lookup.qual/class.qual/p2.cpp

@@ -18,6 +18,8 @@ struct X1 : X0 {
   X1 f2(int);
   X1 f2(float);
   X1 f2(double);
+  X1 f2(short);
+  X1 f2(long);
 };
 
 // Error recovery: out-of-line constructors whose names have template arguments.
@@ -29,10 +31,12 @@ X0::X0 X0::f1() { return X0(); } // expected-error{{qualified reference to 'X0'
 
 struct X0::X0 X0::f2() { return X0(); }
 
-template<typename T> X1<T>::X1<T> X1<T>::f2() { } // expected-error{{qualified reference to 'X1' is a constructor name rather than a template name in this context}}
-template<typename T> X1<T>::X1<T> (X1<T>::f2)(int) { } // expected-error{{qualified reference to 'X1' is a constructor name rather than a template name in this context}}
+template<typename T> X1<T>::X1<T> X1<T>::f2() { } // expected-error{{missing 'typename'}}
+template<typename T> X1<T>::X1<T> (X1<T>::f2)(int) { } // expected-error{{missing 'typename'}}
 template<typename T> struct X1<T>::X1<T> (X1<T>::f2)(float) { }
 template<typename T> struct X1<T>::X1 (X1<T>::f2)(double) { }
+template<typename T> typename X1<T>::template X1<T> X1<T>::f2(short) { } // expected-warning {{qualified reference to 'X1' is a constructor name rather than a template name in this context}}
+template<typename T> typename X1<T>::template X1<T> (X1<T>::f2)(long) { } // expected-warning {{qualified reference to 'X1' is a constructor name rather than a template name in this context}}
 
 void x1test(X1<int> x1i) {
   x1i.f2();

test/CXX/temp/temp.decls/temp.variadic/sizeofpack.cpp

@@ -61,7 +61,7 @@ struct X {
 
 template<class... Members>
 template<int i>
-X<Members...>::get_t<i> X<Members...>::get()
+typename X<Members...>::template get_t<i> X<Members...>::get()
 {
   return 0;
 }

test/CXX/temp/temp.res/p3.cpp

@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -verify %s -std=c++11
+
+template<typename T> struct A {
+  template<typename U> struct B;
+  template<typename U> using C = U; // expected-note {{here}}
+};
+
+struct X {
+  template<typename T> X(T);
+  struct Y {
+    template<typename T> Y(T);
+  };
+};
+
+template<typename T> A<T>::B<T> f1(); // expected-error {{missing 'typename' prior to dependent type template name 'A<T>::B'}}
+template<typename T> A<T>::C<T> f2(); // expected-error {{missing 'typename' prior to dependent type template name 'A<T>::C'}}
+
+// FIXME: Should these cases really be valid? There doesn't appear to be a rule prohibiting them...
+template<typename T> A<T>::C<X>::X(T) {}
+template<typename T> A<T>::C<X>::X::Y::Y(T) {}
+
+// FIXME: This is ill-formed
+template<typename T> int A<T>::B<T>::*f3() {}
+template<typename T> int A<T>::C<X>::*f4() {}
+
+// FIXME: This is valid
+template<typename T> int A<T>::template C<int>::*f5() {} // expected-error {{has no members}}
+
+template<typename T> template<typename U> struct A<T>::B {
+  friend A<T>::C<T> f6(); // ok, same as 'friend T f6();'
+
+  // FIXME: Error recovery here is awful; we decide that the template-id names
+  // a type, and then complain about the rest of the tokens, and then complain
+  // that we didn't get a function declaration.
+  friend A<U>::C<T> f7(); // expected-error {{use 'template' keyword to treat 'C' as a dependent template name}} expected-error 3{{}}
+  friend A<U>::template C<T> f8(); // expected-error 3{{}}
+};