Fix PR8170: Clang does not check constructor declaration that uses a template-id

[faisalv]

Summary

Clang currently does no real checking of the template argument list when a template-id is used to declare a constructor:

template<class T> struct X {

X<T*, T>(); // Clang erroneously accepts this.

};

Both gcc and edg emit an error on the above code (though one could claim the spec temp.local p1 is perhaps not as explicit as it could be in this regard)

See: https://llvm.org/bugs/show_bug.cgi?id=8170

[faisalv]

*ping*
My main question is, does it make sense to suppress diagnostics when checking the template-id, and then rewording it as I do. If it does make sense to suppress and then report a new diagnostic - is my method of suppression a reasonable one (as opposed to SFINAETrap or passing a flag around that's checked prior to emitting a diagnostic)
Thanks!

[majnemer]

lib/Sema/SemaTemplate.cpp

528

You could use StringifiedTemplateArgs.empty() instead.

529

You could hoist this bit out of the std::for_each

[faisalv]

lib/Sema/SemaTemplate.cpp

528

Aah yes - Will do - Thanks!

529

Well - I would still need some way to know that this is not the first iteration. I was trying to avoid declaring another variable or checking that the string was equal to another string (I thought the empty check would be reasonable) -do you feel strongly about the hoist?

[faisalv]

*ping*

[faisalv]

*ping*

[martell]

In case anyone comes here looking for rL300555 that should be D15006 no D15005.

Thanks

[martell]

It won't let me revert the automatically updated diff to the previous one because I am not the author.
@faisalv are you able todo this and Sorry for the confusion.

include/clang/Basic/DiagnosticSemaKinds.td

def ext_function_specialization_in_class : ExtWarn<
   "explicit specialization of %0 within class scope is a Microsoft extension">,
   InGroup<MicrosoftTemplate>;
 def ext_explicit_specialization_storage_class : ExtWarn<
   "explicit specialization cannot have a storage class">;
 def err_explicit_specialization_inconsistent_storage_class : Error<
   "explicit specialization has extraneous, inconsistent storage class "
   "'%select{none|extern|static|__private_extern__|auto|register}0'">;
 
+// C++ class template constructor
+def err_expected_constructor_template_id : Error<
+  "constructor template-id has inconsistent (and extraneous) template "
+  "argument list: expected %0 or '%1'">;
+
 // C++ class template specializations and out-of-line definitions
 def err_template_spec_needs_header : Error<
   "template specialization requires 'template<>'">;
 def err_template_spec_needs_template_parameters : Error<
   "template specialization or definition requires a template parameter list "
   "corresponding to the nested type %0">;
 def err_template_param_list_matches_nontemplate : Error<
   "template parameter list matching the non-templated nested type %0 should "

include/clang/Parse/Parser.h

private:
   ///
   OMPClause *ParseOpenMPVarListClause(OpenMPClauseKind Kind);
 public:
   bool ParseUnqualifiedId(CXXScopeSpec &SS, bool EnteringContext,
                           bool AllowDestructorName,
                           bool AllowConstructorName,
                           ParsedType ObjectType,
                           SourceLocation& TemplateKWLoc,
-                          UnqualifiedId &Result);
+                          UnqualifiedId &Result,
+                          const DeclSpec *DS = nullptr);
 
 private:
   //===--------------------------------------------------------------------===//
   // C++ 14: Templates [temp]
 
   // C++ 14.1: Template Parameters [temp.param]
   Decl *ParseDeclarationStartingWithTemplate(unsigned Context,
                                           SourceLocation &DeclEnd,

include/clang/Sema/Sema.h

Decl *ActOnFinishLinkageSpecification(Scope *S,
                                         Decl *LinkageSpec,
                                         SourceLocation RBraceLoc);
 
 
   //===--------------------------------------------------------------------===//
   // C++ Classes
   //
   bool isCurrentClassName(const IdentifierInfo &II, Scope *S,
-                          const CXXScopeSpec *SS = nullptr);
+                          const CXXScopeSpec *SS = nullptr, 
+                          const CXXRecordDecl **CurClassOut = nullptr);
+
   bool isCurrentClassNameTypo(IdentifierInfo *&II, const CXXScopeSpec *SS);
 
   bool ActOnAccessSpecifier(AccessSpecifier Access,
                             SourceLocation ASLoc,
                             SourceLocation ColonLoc,
                             AttributeList *Attrs = nullptr);
 
   NamedDecl *ActOnCXXMemberDeclarator(Scope *S, AccessSpecifier AS,
void translateTemplateArguments(const ASTTemplateArgsPtr &In,
                                   TemplateArgumentListInfo &Out);
 
   void NoteAllFoundTemplates(TemplateName Name);
 
   QualType CheckTemplateIdType(TemplateName Template,
                                SourceLocation TemplateLoc,
                               TemplateArgumentListInfo &TemplateArgs);
 
+  // Represent the template arguments of the template specialization as a
+  // string.
+  std::string
+  Sema::stringifyTemplateArguments(const TemplateSpecializationType *TST);
+
+  // Given a template-id and the templated class, check whether the template id
+  // represents the class itself (i.e. the injected class name)
+  void
+  checkUnqualifiedTemplateIdIsConstructor(TemplateIdAnnotation *TemplateId,
+                                          const CXXRecordDecl *TemplatedClass);
+
   TypeResult
   ActOnTemplateIdType(CXXScopeSpec &SS, SourceLocation TemplateKWLoc,
                       TemplateTy Template, SourceLocation TemplateLoc,
                       SourceLocation LAngleLoc,
                       ASTTemplateArgsPtr TemplateArgs,
                       SourceLocation RAngleLoc,
                       bool IsCtorOrDtorName = false);
 

lib/Parse/ParseDecl.cpp

if (Tok.isOneOf(tok::identifier, tok::kw_operator, tok::annot_template_id,
       SourceLocation TemplateKWLoc;
       bool HadScope = D.getCXXScopeSpec().isValid();
       if (ParseUnqualifiedId(D.getCXXScopeSpec(),
                              /*EnteringContext=*/true,
                              /*AllowDestructorName=*/true,
                              AllowConstructorName,
                              ParsedType(),
                              TemplateKWLoc,
-                             D.getName()) ||
+                             D.getName(), 
+                             &D.getDeclSpec()) ||
           // Once we're past the identifier, if the scope was bad, mark the
           // whole declarator bad.
           D.getCXXScopeSpec().isInvalid()) {
         D.SetIdentifier(nullptr, Tok.getLocation());
         D.setInvalidType(true);
       } else {
         // ParseUnqualifiedId might have parsed a scope specifier during error
         // recovery. If it did so, enter that scope.

lib/Parse/ParseExprCXX.cpp

 /// \param Result on a successful parse, contains the parsed unqualified-id.
 ///
 /// \returns true if parsing fails, false otherwise.
 bool Parser::ParseUnqualifiedId(CXXScopeSpec &SS, bool EnteringContext,
                                 bool AllowDestructorName,
                                 bool AllowConstructorName,
                                 ParsedType ObjectType,
                                 SourceLocation& TemplateKWLoc,
-                                UnqualifiedId &Result) {
+                                UnqualifiedId &Result,
+                                const DeclSpec *DS) {
 
   // Handle 'A::template B'. This is for template-ids which have not
   // already been annotated by ParseOptionalCXXScopeSpecifier().
   bool TemplateSpecified = false;
   if (getLangOpts().CPlusPlus && Tok.is(tok::kw_template) &&
       (ObjectType || SS.isSet())) {
     TemplateSpecified = true;
     TemplateKWLoc = ConsumeToken();
if (Tok.is(tok::identifier)) {
     
     return false;
   }
   
   // unqualified-id:
   //   template-id (already parsed and annotated)
   if (Tok.is(tok::annot_template_id)) {
     TemplateIdAnnotation *TemplateId = takeTemplateIdAnnotation(Tok);
-
+    const CXXRecordDecl *CurClass = nullptr;
     // If the template-name names the current class, then this is a constructor 
     if (AllowConstructorName && TemplateId->Name &&
-        Actions.isCurrentClassName(*TemplateId->Name, getCurScope(), &SS)) {
+        Actions.isCurrentClassName(*TemplateId->Name, getCurScope(), &SS, &CurClass)) {
       if (SS.isSet()) {
         // C++ [class.qual]p2 specifies that a qualified template-name
         // is taken as the constructor name where a constructor can be
         // declared. Thus, the template arguments are extraneous, so
         // complain about them and remove them entirely.
         Diag(TemplateId->TemplateNameLoc, 
              diag::err_out_of_line_constructor_template_id)
           << TemplateId->Name
           << FixItHint::CreateRemoval(
                     SourceRange(TemplateId->LAngleLoc, TemplateId->RAngleLoc));
         ParsedType Ty = Actions.getTypeName(*TemplateId->Name,
                                             TemplateId->TemplateNameLoc,
                                             getCurScope(),
                                             &SS, false, false,
                                             ParsedType(),
                                             /*IsCtorOrDtorName=*/true,
                                             /*NontrivialTypeSourceInfo=*/true);
         Result.setConstructorName(Ty, TemplateId->TemplateNameLoc,
                                   TemplateId->RAngleLoc);
         ConsumeToken();
         return false;
       }
 
+      // Only perform the template-id check if not declaring a friend here. If a
+      // template-id involves the current class in a friend declaration
+      // referring to a constructor, it must be a qualified id - and is
+      // diagnosed as an error when semantically checking friend function
+      // declarations for such things downstream.
+
+      // 
+      // For e.g. 
+      //   template<class T> struct X { 
+      //     X<T,T>() { }         <-- check template args here
+      //     friend X<T,T>() { }  <-- ignore template args, bound to fail later
+      //   };
+      //  
+      if (!DS || !DS->isFriendSpecified())
+        Actions.checkUnqualifiedTemplateIdIsConstructor(TemplateId, CurClass);
+   
       Result.setConstructorTemplateId(TemplateId);
       ConsumeToken();
       return false;
     }
 
     // We have already parsed a template-id; consume the annotation token as
     // our unqualified-id.
     Result.setTemplateId(TemplateId);

lib/Sema/SemaDeclCXX.cpp

bool Sema::CheckConstexprFunctionBody(const FunctionDecl *Dcl, Stmt *Body) {
   return true;
 }
 
 /// isCurrentClassName - Determine whether the identifier II is the
 /// name of the class type currently being defined. In the case of
 /// nested classes, this will only return true if II is the name of
 /// the innermost class.
 bool Sema::isCurrentClassName(const IdentifierInfo &II, Scope *,
-                              const CXXScopeSpec *SS) {
+                              const CXXScopeSpec *SS, 
+                              const CXXRecordDecl **CurClassOut) {
   assert(getLangOpts().CPlusPlus && "No class names in C!");
 
   CXXRecordDecl *CurDecl;
+  if (CurClassOut) *CurClassOut = nullptr;
   if (SS && SS->isSet() && !SS->isInvalid()) {
     DeclContext *DC = computeDeclContext(*SS, true);
     CurDecl = dyn_cast_or_null<CXXRecordDecl>(DC);
   } else
     CurDecl = dyn_cast_or_null<CXXRecordDecl>(CurContext);
 
   if (CurDecl && CurDecl->getIdentifier())
-    return &II == CurDecl->getIdentifier();
+    if (&II == CurDecl->getIdentifier()) {
+      if (CurClassOut) *CurClassOut = CurDecl;
+      return true;
+    }
   return false;
 }
 
 /// \brief Determine whether the identifier II is a typo for the name of
 /// the class type currently being defined. If so, update it to the identifier
 /// that should have been used.
 bool Sema::isCurrentClassNameTypo(IdentifierInfo *&II, const CXXScopeSpec *SS) {
   assert(getLangOpts().CPlusPlus && "No class names in C!");

lib/Sema/SemaTemplate.cpp

return TemplateArgumentLoc(TArg,
                                Arg.getLocation(),
                                Arg.getEllipsisLoc());
   }
   }
 
   llvm_unreachable("Unhandled parsed template argument");
 }
 
+std::string
+Sema::stringifyTemplateArguments(const TemplateSpecializationType *TST) {
+  ArrayRef<TemplateArgument> TemplateArgs(TST->getArgs(), TST->getNumArgs());
+
+  llvm::SmallString<256> StringifiedTemplateArgs;
+  std::for_each(TemplateArgs.begin(), TemplateArgs.end(),
+                [&StringifiedTemplateArgs, this](const TemplateArgument &TA) {
+    if (const bool IsFirst = !StringifiedTemplateArgs.size())
+      StringifiedTemplateArgs = "<";
+    else
+      StringifiedTemplateArgs += ",";
+    llvm::raw_svector_ostream OS(StringifiedTemplateArgs);
+    TA.print(this->getPrintingPolicy(), OS);
+  });
+
+  StringifiedTemplateArgs += ">";
+  return StringifiedTemplateArgs.c_str();
+}
+
+void Sema::checkUnqualifiedTemplateIdIsConstructor(
+    TemplateIdAnnotation *TemplateId, const CXXRecordDecl *TemplatedClass) {
+  assert(TemplatedClass->getDescribedClassTemplate());
+  assert(TemplateId->Name == TemplatedClass->getIdentifier() &&
+         "TemplateId must have the same identifier as that of the class!");
+
+  // Get the type that corresponds to this template id, while suppressing all
+  // diagnostics.
+  const bool PrevSuppress = Diags.getSuppressAllDiagnostics();
+  Diags.setSuppressAllDiagnostics(true);
+  TypeResult TemplateIdTy = ActOnTemplateIdType(
+      TemplateId->SS, TemplateId->TemplateKWLoc,
+      TemplateTy::make(
+          TemplateName(TemplatedClass->getDescribedClassTemplate())),
+      TemplateId->TemplateNameLoc, TemplateId->LAngleLoc,
+      ASTTemplateArgsPtr(TemplateId->getTemplateArgs(), TemplateId->NumArgs),
+      TemplateId->RAngleLoc, /*IsCtorOrDtorName*/ true);
+  Diags.setSuppressAllDiagnostics(PrevSuppress);
+
+  // If the template-id type is the same type as the type of the
+  // templated-class, all is ok - else emit a diagnostic.
+  if (TemplateIdTy.get() &&
+      Context.hasSameType(GetTypeFromParser(TemplateIdTy.get()).getTypePtr(),
+                          Context.getTypeDeclType(TemplatedClass).getTypePtr()))
+    return;
+
+  // Emit a diagnostic, indicating that the template arguments do not match up
+  // to the injected class name.
+  std::string ExpectedTemplateId = TemplateId->Name->getName();
+  ExpectedTemplateId += stringifyTemplateArguments(
+      cast<InjectedClassNameType>(Context.getTypeDeclType(TemplatedClass))
+          ->getInjectedTST());
+
+  Diag(TemplateId->TemplateNameLoc,
+       diag::err_expected_constructor_template_id)
+      << TemplateId->Name << ExpectedTemplateId.c_str()
+      << FixItHint::CreateRemoval(
+             SourceRange(TemplateId->LAngleLoc, TemplateId->RAngleLoc));
+}
+
 /// \brief Translates template arguments as provided by the parser
 /// into template arguments used by semantic analysis.
 void Sema::translateTemplateArguments(const ASTTemplateArgsPtr &TemplateArgsIn,
                                       TemplateArgumentListInfo &TemplateArgs) {
  for (unsigned I = 0, Last = TemplateArgsIn.size(); I != Last; ++I)
    TemplateArgs.addArgument(translateTemplateArgument(*this,
                                                       TemplateArgsIn[I]));
 }

test/CXX/temp/temp.res/temp.local/p1.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
+
 
 // C++0x [temp.local]p1:
 //   Like normal (non-template) classes, class templates have an
 //   injected-class-name (Clause 9). The injected-class-name can be used with
 //   or without a template-argument-list. When it is used without
 //   a template-argument-list, it is equivalent to the injected-class-name
 //   followed by the template-parameters of the class template enclosed in <>.
 
 template <typename T> struct X0 {
   X0();
   ~X0();
+  X0<T>(int); 
+  X0<T, T>(int*); //expected-error{{expected 'X0' or 'X0<T>'}}
   X0 f(const X0&);
 };
 
 // Test non-type template parameters.
 template <int N1, const int& N2, const int* N3> struct X1 {
   X1();
+  template<class U> X1<U,N1,"wtf">(char); //expected-error{{expected 'X1' or 'X1<N1,N2,N3>'}}
   ~X1();
   X1 f(const X1& x1a) { X1 x1b(x1a); return x1b; }
 };
 
 //   When it is used with a template-argument-list, it refers to the specified
 //   class template specialization, which could be the current specialization
 //   or another specialization.
 // FIXME: Test this clause.
 
 int i = 42;
 void test() {
   X0<int> x0; (void)x0;
   X1<42, i, &i> x1; (void)x1;
 }