PR11306 - Variadic template fix-it suggestion

[nikola]

Summary

This patch adds new error for misplaced ellipsis in template parameter pack. Fixit is suggested for this error and unexpanded parameter pack error. Is new error here acceptable or should this be a note? This will also impact the wording.

[nikola]

Ping

[ismailp]

include/clang/Basic/DiagnosticParseKinds.td

716 ↗

(On Diff #9062)

My 2 cents about this:

• IMO, using '...' is clearer than "ellipsis". I think all occurrences of "ellipsis" diagnostics can be replaced with '...' to make messages clearer.
• I think you can emphasize that ... must follow typename or class in type parameter declaration to form a parameter pack, something like: "'...' must follow %select{typename|class} in template parameter pack" or "'...' must precede '<identifier>'" (the latter one is closer to err_misplaced_ellipsis_in_declaration)

test/FixIt/fixit-parameter-pack.cpp

1 ↗

(On Diff #9062)

Can we use an existing test instead of adding a new file? e.g. test/FixIt/fixit-cxx0x.cpp? That file already contains similar diagnostics and FixIts.

[nikola]

include/clang/Basic/DiagnosticParseKinds.td

716 ↗

(On Diff #9062)

I'll go with '...' instead of ellipsis as it does seem to be clearer and more consistent.

I am having some trouble with the exact spelling:
We don't seem to select between typename and class in any of the existing diagnostics and it seems to be hard to get this information. Now printing the exact identifier is achievable by casting to NamedDecl but is this really necessary? I'd say that it's not any more useful than if we said "must precede declared identifier". And this brings me to the idea of reusing err_misplaced_ellipsis_in_declaration in which case the message would say "'...' must immediately precede declared" or if we want to be more specific:

'...' must immediately precede declared parameter pack
'...' must immediately precede the name of parameter pack

test/FixIt/fixit-parameter-pack.cpp

1 ↗

(On Diff #9062)

I think that we can, I'll also add tests that use a struct to make sure we have the same result for template and function parameter packs.

[jordan_rose]

lib/Parse/ParseTemplate.cpp

358–366

This one looks fine, but since you're putting the fix-it on the error itself you have to then recover as if the user wrote the pack correctly.

lib/Sema/SemaTemplateVariadic.cpp

253–254 ↗

(On Diff #9062)

This I'm not so sure about. The place to put the ... might not be directly after the parameter pack, and pretending it is seems questionable.

[nikola]

lib/Parse/ParseTemplate.cpp

358–366

Thanks, I was thinking about this but I wasn't sure how recovery in this case is done? Do I have to modify the existing Decl? Could you point me to code that does something like this?

I have a test for this:

template<typename Args...> // this would give misplaced ellipsis error
void baz(Args args...);  // this should give unexpanded parameter pack error but doesn't as Args is not a parameter pack

lib/Sema/SemaTemplateVariadic.cpp

253–254 ↗

(On Diff #9062)

Could you be a bit more specific, what do you mean by "might not be directly after"?. I'm looking at [temp.variadic] p5 but the only example there is:

template<typename... Args>
void g(Args ... args)
{
  f(args);
}

[jordan_rose]

By the way, it's considered good form to add cfe-commits as a CC on all Phabricator reviews. Not everyone uses Phabricator.

lib/Parse/ParseTemplate.cpp

358–366

You could modify the existing decl by marking it as a pack (something that currently isn't allowed), but you might have better luck just creating a new decl and replacing the existing one.

I'm not quite sure what's the right way to go here. +Richard for comment.

lib/Sema/SemaTemplateVariadic.cpp

253–254 ↗

(On Diff #9062)

Whatever's before the ... gets duplicated. This, for example, is entirely legal:

template<typename T>
int f(T arg);

template<typename... Args>
std::vector<int> g(Args ... args)
{
  return { f(args)... };
}

g(1, "2", 3.0);

[rsmith]

include/clang/Basic/DiagnosticParseKinds.td

716 ↗

(On Diff #9062)

Please just reuse err_misplaced_ellipsis_in_declaration. "declared identifier" seems fine here; I don't think we need to be more specific.

lib/Parse/ParseTemplate.cpp

358–366

I'd prefer for the misplaced ellipsis detection/recovery to be done earlier. If you do this in ParseTypeParameter / ParseTemplateTemplateParameter, you should be able to create the parameter with the right variadicness (and also do the right thing if the ellipsis is between the parameter and a default argument).

lib/Sema/SemaTemplateVariadic.cpp

253–254 ↗

(On Diff #9062)

Right. I don't think we can assume we know where the ellipsis should go here. (Well, we could try all possible places and pick one that works, but that seems like massively overkill.)

[nikola]

I'm not too happy with DiagnoseMisplacedEllipsis but it's still better than having it in two places. The rest should be good.

[rsmith]

lib/Parse/ParseTemplate.cpp

674

Please move this check to the callers in ParseDecl.cpp, and change this to an assert.

676–679

I don't really like the interface of this function:

• It's very Declarator-specific, and yet is called in non-Declarator cases
• It gives the wrong fixit in the template-type-parameter and template-template-parameter cases because it can't tell whether there was already an ellipsis.

Maybe split it into:

DiagnoseMisplacedEllipsis(SourceLocation NewLoc, SourceLocation CorrectLoc, bool AlreadyHasEllipsis);
DiagnoseMisplacedEllipsisInDeclarator(SourceLocation NewLoc, SourceLocation CorrectLoc, Declarator *D);

... with the second function calling the first?

test/FixIt/fixit-cxx0x.cpp

142

Typo Missplaced

[nikola]

That's sooo much better. I'm still not checking if ellipsis exist on template parameter and template template paremeter, but I don't expect anyone to write something like

template <typename... T...>

Then again... :)

[rsmith]

Thanks, this looks much nicer.

lib/Parse/ParseTemplate.cpp

503

I don't think your AlreadyHasEllipsis argument here is correct; we might have parsed one a few lines above.

588

Likewise here.

[nikola]

You might have missed the last part of my comment, I clicked submit too soon and had to edit it. In any case I think you're right, it's not hard to detect redundant ellipsis and provide more accurate recovery. Here it is, with tests for this scenario.

[rsmith]

LGTM, thanks.

(Yes, I did miss the last part of your comment and then forgot about the part I had seen...)

[nikola]

Thanks for your valuable guidance. Committed in r210304.

include/clang/Parse/Parser.h

   Decl *ParseTypeParameter(unsigned Depth, unsigned Position);
   Decl *ParseTemplateTemplateParameter(unsigned Depth, unsigned Position);
   Decl *ParseNonTypeTemplateParameter(unsigned Depth, unsigned Position);
+  void DiagnoseMisplacedEllipsis(SourceLocation EllipsisLoc,
+                                 SourceLocation CorrectLoc,
+                                 bool AlreadyHasEllipsis,
+                                 bool IdentifierHasName);
+  void DiagnoseMisplacedEllipsisInDeclarator(SourceLocation EllipsisLoc,
+                                             Declarator &D);
   // C++ 14.3: Template arguments [temp.arg]
   typedef SmallVector<ParsedTemplateArgument, 16> TemplateArgList;
 

include/clang/Sema/Sema.h

   void DiagnoseTemplateParameterShadow(SourceLocation Loc, Decl *PrevDecl);
   TemplateDecl *AdjustDeclIfTemplate(Decl *&Decl);
 
-  Decl *ActOnTypeParameter(Scope *S, bool Typename, bool Ellipsis,
+  Decl *ActOnTypeParameter(Scope *S, bool Typename,
                            SourceLocation EllipsisLoc,
                            SourceLocation KeyLoc,
                            IdentifierInfo *ParamName,

lib/Parse/ParseDecl.cpp

   }
 }
 
-static void diagnoseMisplacedEllipsis(Parser &P, Declarator &D,
-                                      SourceLocation EllipsisLoc) {
-  if (EllipsisLoc.isValid()) {
-    FixItHint Insertion;
-    if (!D.getEllipsisLoc().isValid()) {
-      Insertion = FixItHint::CreateInsertion(D.getIdentifierLoc(), "...");
-      D.setEllipsisLoc(EllipsisLoc);
-    }
-    P.Diag(EllipsisLoc, diag::err_misplaced_ellipsis_in_declaration)
-      << FixItHint::CreateRemoval(EllipsisLoc) << Insertion << !D.hasName();
-  }
-}
-
 /// ParseDirectDeclarator
 ///       direct-declarator: [C99 6.7.5]
 /// [C99]   identifier

         // The ellipsis was put in the wrong place. Recover, and explain to
         // the user what they should have done.
         ParseDeclarator(D);
-        diagnoseMisplacedEllipsis(*this, D, EllipsisLoc);
+        if (EllipsisLoc.isValid())
+          DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, D);
         return;
       } else
         D.setEllipsisLoc(EllipsisLoc);

 
     // An ellipsis cannot be placed outside parentheses.
     if (EllipsisLoc.isValid())
-      diagnoseMisplacedEllipsis(*this, D, EllipsisLoc);
+      DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, D);
 
     return;
   }

lib/Parse/ParseTemplate.cpp

     return nullptr;
   }
 
+  // Recover from misplaced ellipsis.
+  if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+    DiagnoseMisplacedEllipsis(EllipsisLoc, NameLoc, false, true);
+
   // Grab a default argument (if available).
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the type parameter into the local scope.

     DefaultArg = ParseTypeName(/*Range=*/nullptr,
                                Declarator::TemplateTypeArgContext).get();
 
-  return Actions.ActOnTypeParameter(getCurScope(), TypenameKeyword, Ellipsis, 
-                                    EllipsisLoc, KeyLoc, ParamName, NameLoc,
-                                    Depth, Position, EqualLoc, DefaultArg);
+  return Actions.ActOnTypeParameter(getCurScope(), TypenameKeyword, EllipsisLoc,
+                                    KeyLoc, ParamName, NameLoc, Depth, Position,
+                                    EqualLoc, DefaultArg);
 }
 
 /// ParseTemplateTemplateParameter - Handle the parsing of template

     return nullptr;
   }
 
+  // Recover from misplaced ellipsis.
+  if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+    DiagnoseMisplacedEllipsis(EllipsisLoc, NameLoc, false, true);
+
   TemplateParameterList *ParamList =
     Actions.ActOnTemplateParameterList(Depth, SourceLocation(),
                                        TemplateLoc, LAngleLoc,

     return nullptr;
   }
 
+  // Recover from misplaced ellipsis.
+  SourceLocation EllipsisLoc;
+  if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
+    DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, ParamDecl);
+
   // If there is a default value, parse it.
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the template parameter into the local scope.

                                                DefaultArg.get());
 }
 
+void Parser::DiagnoseMisplacedEllipsis(SourceLocation EllipsisLoc,
+                                       SourceLocation CorrectLoc,
+                                       bool AlreadyHasEllipsis,
+                                       bool IdentifierHasName) {
+  FixItHint Insertion;
+  if (!AlreadyHasEllipsis)
+    Insertion = FixItHint::CreateInsertion(CorrectLoc, "...");
+  Diag(EllipsisLoc, diag::err_misplaced_ellipsis_in_declaration)
+      << FixItHint::CreateRemoval(EllipsisLoc) << Insertion
+      << !IdentifierHasName;
+}
+
+void Parser::DiagnoseMisplacedEllipsisInDeclarator(SourceLocation EllipsisLoc,
+                                                   Declarator &D) {
+  assert(EllipsisLoc.isValid());
+  bool AlreadyHasEllipsis = D.getEllipsisLoc().isValid();
+  if (!AlreadyHasEllipsis)
+    D.setEllipsisLoc(EllipsisLoc);
+  DiagnoseMisplacedEllipsis(EllipsisLoc, D.getIdentifierLoc(),
+                            AlreadyHasEllipsis, D.hasName());
+}
+
 /// \brief Parses a '>' at the end of a template list.
 ///
 /// If this function encounters '>>', '>>>', '>=', or '>>=', it tries

lib/Sema/SemaTemplate.cpp

 /// ParamNameLoc is the location of the parameter name (if any).
 /// If the type parameter has a default argument, it will be added
 /// later via ActOnTypeParameterDefault.
-Decl *Sema::ActOnTypeParameter(Scope *S, bool Typename, bool Ellipsis,
+Decl *Sema::ActOnTypeParameter(Scope *S, bool Typename,
                                SourceLocation EllipsisLoc,
                                SourceLocation KeyLoc,
                                IdentifierInfo *ParamName,

   if (!ParamName)
     Loc = KeyLoc;
 
+  bool IsParameterPack = EllipsisLoc.isValid();
   TemplateTypeParmDecl *Param
     = TemplateTypeParmDecl::Create(Context, Context.getTranslationUnitDecl(),
                                    KeyLoc, Loc, Depth, Position, ParamName,
-                                   Typename, Ellipsis);
+                                   Typename, IsParameterPack);
   Param->setAccess(AS_public);
   if (Invalid)
     Param->setInvalidDecl();

   // C++0x [temp.param]p9:
   //   A default template-argument may be specified for any kind of
   //   template-parameter that is not a template parameter pack.
-  if (DefaultArg && Ellipsis) {
+  if (DefaultArg && IsParameterPack) {
     Diag(EqualLoc, diag::err_template_param_pack_default_arg);
     DefaultArg = ParsedType();
   }

test/FixIt/fixit-cxx0x.cpp

   register int n; // expected-warning {{'register' storage class specifier is deprecated}}
   return n;
 }
+
+namespace MisplacedParameterPack {
+  template <typename Args...> // expected-error {{'...' must immediately precede declared identifier}}
+  void typeParameter(Args...);
+
+  template <template <typename> class Args...> // expected-error {{'...' must immediately precede declared identifier}}
+  void templateTypeParameter(Args<int>...);
+
+  template <int N...> // expected-error {{'...' must immediately precede declared identifier}}
+  void nonTypeTemplateParameter();
+}