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

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

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

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

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

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

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

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

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

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 ↗

(On Diff #10117)

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/Basic/DiagnosticParseKinds.td

 
 def err_expected_parameter_pack : Error<
   "expected the name of a parameter pack">;
+def err_misplaced_ellipsis_in_parameter_pack : Error<
+  "ellipsis comes before the name of template parameter pack">;
 def err_paren_sizeof_parameter_pack : Error<
   "missing parentheses around the size of parameter pack %0">;
 def err_sizeof_parameter_pack : Error<

lib/Parse/ParseTemplate.cpp

       // Somebody probably forgot to close the template. Skip ahead and
       // try to get out of the expression. This error is currently
       // subsumed by whatever goes on in ParseTemplateParameter.
-      Diag(Tok.getLocation(), diag::err_expected_comma_greater);
+      if (Tok.is(tok::ellipsis)) {
+        FixItHint FixIt;
+        if (!TemplateParams.back()->isParameterPack())
+          FixIt = FixItHint::CreateInsertion(PrevTokLocation, "... ");
+        Diag(Tok.getLocation(), diag::err_misplaced_ellipsis_in_parameter_pack)
+          << FixItHint::CreateRemoval(Tok.getLocation()) << FixIt;
+      }
+      else
+        Diag(Tok.getLocation(), diag::err_expected_comma_greater);
       SkipUntil(tok::comma, tok::greater, tok::greatergreater,
                 StopAtSemi | StopBeforeMatch);
       return false;

lib/Sema/SemaTemplateVariadic.cpp

 #include "clang/AST/Expr.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TypeLoc.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/ScopeInfo.h"

     : Diag(Loc, diag::err_unexpanded_parameter_pack_3_or_more)
         << (int)UPPC << Names[0] << Names[1];
 
-  for (unsigned I = 0, N = Locations.size(); I != N; ++I)
-    DB << SourceRange(Locations[I]);
+  for (auto Location : Locations)
+    DB << SourceRange(Location);
+  for (auto Location : Locations)
+    DB << FixItHint::CreateInsertion(PP.getLocForEndOfToken(Location), "...");
   return true;
 }
 

test/FixIt/fixit-parameter-pack.cpp

+// RUN: %clang_cc1 -verify -x c++ -std=c++11 %s
+// RUN: %clang_cc1 -fdiagnostics-parseable-fixits -x c++ -std=c++11 %s 2>&1 | FileCheck %s
+
+// CHECK: fix-it:"{{.*}}":{10:6-10:8}:"{}"
+// CHECK: fix-it:"{{.*}}":{10:6-10:8}:"{}"
+template<typename Args...> // expected-error {{ellipsis comes before the name of template parameter pack}}
+void foo(Args...);
+
+// CHECK: fix-it:"{{.*}}":{10:6-10:8}:"{}"
+template<typename... Args...> // expected-error {{ellipsis comes before the name of template parameter pack}}
+void bar(Args...);
+
+// CHECK: fix-it:"{{.*}}":{10:6-10:8}:"{}"
+template<typename... Args>
+void baz(Args args...); // expected-error {{unexpanded parameter pack}}
+