Fix valid-for-expr ellipses eaten as invalid decl

[hubert.reinterpretcast]

Summary

The trial parse for declarative syntax accepts an invalid pack declaration syntax, which is ambiguous with valid pack expansions of expressions. This commit removes the invalid pack declaration syntax to avoid mistaking valid pack expansions as invalid declarator components.

Additionally, the trial parse of a template-argument-list then needs to handle the optional ellipsis that is part of that grammar, as opposed to relying on the trial parse for declarators accepting stray ellipses.

[hubert.reinterpretcast]

lib/Parse/ParseTentative.cpp

542

The change here is primarily for maintainability purposes. This is "morally" where the ellipsis should be checked for, but the ellipsis that we are looking for here is going to be consumed as "stray" anyway.

944

The check for what qualifies as "trailing" is not the strongest, but I find it to be the most straightforward change that should do the job. One alternative is to track whether an ellipsis was consumed within the current loop iteration.

[rsmith]

Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in some cases? If so, do we have tests for that somewhere?

include/clang/Parse/Parser.h

2138

Given that this flag is enabling parsing of things that are *not* valid declarators, I think the default should be the other way around. If some calling code believes it's safe to parse a trailing ellipsis as part of a declarator, it should be explicitly opting into that.

lib/Parse/ParseTentative.cpp

944

Use !NextToken().isOneOf(tok::r_paren, tok::comma) here.

[hubert.reinterpretcast]

In D33339#759125, @rsmith wrote:

Should I assume our "misplaced ellipsis" diagnostic requires that we disambiguate the ill-formed ellipsis-after-declarator-id as a declarator in some cases? If so, do we have tests for that somewhere?

Tests for the diagnostics are in clang/test/FixIt/fixit-cxx0x.cpp and clang/test/Parser/cxx11-templates.cpp.
The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely. The disambiguation process is not needed in many cases, and—when it is—can choose the declarative interpretation for other reasons.

I find the most interesting cases affected by the patch here to be those involving disambiguation between the declaration of a function template and that of a variable template.
The misplaced-ellipsis diagnostic will still trigger (if we accept non-trailing stray ellipses) on a case like

template <typename ...T> int f(T (t) ...(int), int (x));

which, although it works, is not very motivating.

Removing the (int) turns it into a possibly-valid variable template declaration in terms of parsing.
Removing the parentheses around t (whether or not the (int) is there) makes the parser decide to parse as a function declaration (and the misplaced-ellipsis diagnostic is triggered) regardless of the treatment of "stray" ellipses.
The logic implemented here does not generate the misplaced-ellipsis diagnostic for

template <typename ...T> int f(T (t) ..., int x);

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

include/clang/Parse/Parser.h

2138

Okay; will do.

lib/Parse/ParseTentative.cpp

944

Will do.

[rsmith]

In D33339#759146, @hubert.reinterpretcast wrote:

The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely.

[...]

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

I think if we want to keep it, the way to do that would be to carry on through the disambiguation process and treat it as a tiebreaker (that's what we do, for instance, if we see an undeclared identifier in a position where we're looking for a type). I'm not convinced that's worthwhile, especially since our existing testcases do not need this disambiguation rule, but perhaps we could remove the stray ellipsis treatment entirely for now and reconsider adding it back if we find poor diagnostics result from it later?

[hubert.reinterpretcast]

In D33339#759797, @rsmith wrote:

In D33339#759146, @hubert.reinterpretcast wrote:

The check-all target passes even if the ellipsis-after-declarator-id disambiguation as a declarator is removed entirely.

[...]

So, on the whole, the stray ellipsis treatment is both too complicated and not complicated enough.

I think if we want to keep it, the way to do that would be to carry on through the disambiguation process and treat it as a tiebreaker (that's what we do, for instance, if we see an undeclared identifier in a position where we're looking for a type). I'm not convinced that's worthwhile, especially since our existing testcases do not need this disambiguation rule, but perhaps we could remove the stray ellipsis treatment entirely for now and reconsider adding it back if we find poor diagnostics result from it later?

Okay. I'll update the patch to remove the stray ellipsis treatment entirely.

[hubert.reinterpretcast]

Remove stray ellipsis treatment entirely

[rsmith]

LGTM, thanks!

include/clang/Parse/Parser.h

private:
   // They all consume tokens, so backtracking should be used after calling them.
 
   TPResult TryParseSimpleDeclaration(bool AllowForRangeDecl);
   TPResult TryParseTypeofSpecifier();
   TPResult TryParseProtocolQualifiers();
   TPResult TryParsePtrOperatorSeq();
   TPResult TryParseOperatorId();
   TPResult TryParseInitDeclaratorList();
-  TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier=true);
+  TPResult TryParseDeclarator(bool mayBeAbstract, bool mayHaveIdentifier = true,
+                              bool mayHaveTrailingStrayEllipsis = true);
   TPResult
   TryParseParameterDeclarationClause(bool *InvalidAsDeclaration = nullptr,
                                      bool VersusTemplateArg = false);
   TPResult TryParseFunctionDeclarator();
   TPResult TryParseBracketDeclarator();
   TPResult TryConsumeDeclarationSpecifier();
 
 public:

lib/Parse/ParseTentative.cpp

Parser::isCXXConditionDeclarationOrInitStatement(bool CanBeInitStatement) {
   ///
   /// The context parameter states what context we're parsing right
   /// now, which affects how this routine copes with the token
   /// following the type-id. If the context is TypeIdInParens, we have
   /// already parsed the '(' and we will cease lookahead when we hit
   /// the corresponding ')'. If the context is
   /// TypeIdAsTemplateArgument, we've already parsed the '<' or ','
   /// before this template argument, and will cease lookahead when we
-  /// hit a '>', '>>' (in C++0x), or ','. Returns true for a type-id
-  /// and false for an expression.  If during the disambiguation
-  /// process a parsing error is encountered, the function returns
-  /// true to let the declaration parsing code handle it.
+  /// hit a '>', '>>' (in C++0x), or ','; or, in C++0x, an ellipsis immediately
+  /// preceding such. Returns true for a type-id and false for an expression.
+  /// If during the disambiguation process a parsing error is encountered,
+  /// the function returns true to let the declaration parsing code handle it.
   ///
   /// type-id:
   ///   type-specifier-seq abstract-declarator[opt]
   ///
 bool Parser::isCXXTypeId(TentativeCXXTypeIdContext Context, bool &isAmbiguous) {
 
   isAmbiguous = false;
 
if (TPR == TPResult::Ambiguous) {
     // We are supposed to be inside parens, so if after the abstract declarator
     // we encounter a ')' this is a type-id, otherwise it's an expression.
     if (Context == TypeIdInParens && Tok.is(tok::r_paren)) {
       TPR = TPResult::True;
       isAmbiguous = true;
 
     // We are supposed to be inside a template argument, so if after
     // the abstract declarator we encounter a '>', '>>' (in C++0x), or
-    // ',', this is a type-id. Otherwise, it's an expression.
+    // ','; or, in C++0x, an ellipsis immediately preceding such, this
+    // is a type-id. Otherwise, it's an expression.
     } else if (Context == TypeIdAsTemplateArgument &&
                (Tok.isOneOf(tok::greater, tok::comma) ||
-                (getLangOpts().CPlusPlus11 && Tok.is(tok::greatergreater)))) {
+                (getLangOpts().CPlusPlus11 &&
+                 (Tok.is(tok::greatergreater) ||
+                  (Tok.is(tok::ellipsis) &&
+                   NextToken().isOneOf(tok::greater, tok::greatergreater,
+                                       tok::comma)))))) {
       TPR = TPResult::True;
       isAmbiguous = true;
 
     } else
       TPR = TPResult::False;
   }
 
   assert(TPR == TPResult::True || TPR == TPResult::False);

 ///                 cv-qualifier-seq[opt] exception-specification[opt]
 ///           direct-declarator '[' constant-expression[opt] ']'
 ///           '(' declarator ')'
 /// [GNU]     '(' attributes declarator ')'
 ///
 ///         abstract-declarator:
 ///           ptr-operator abstract-declarator[opt]
 ///           direct-abstract-declarator
-///           ...
 ///
 ///         direct-abstract-declarator:
 ///           direct-abstract-declarator[opt]
-///           '(' parameter-declaration-clause ')' cv-qualifier-seq[opt]
+///                 '(' parameter-declaration-clause ')' cv-qualifier-seq[opt]
 ///                 exception-specification[opt]
 ///           direct-abstract-declarator[opt] '[' constant-expression[opt] ']'
 ///           '(' abstract-declarator ')'
+/// [C++0x]   ...
 ///
 ///         ptr-operator:
 ///           '*' cv-qualifier-seq[opt]
 ///           '&'
 /// [C++0x]   '&&'                                                        [TODO]
 ///           '::'[opt] nested-name-specifier '*' cv-qualifier-seq[opt]
 ///
 ///         cv-qualifier-seq:

 ///           operator-function-id
 ///           conversion-function-id
 ///           literal-operator-id
 ///           '~' class-name                                              [TODO]
 ///           '~' decltype-specifier                                      [TODO]
 ///           template-id                                                 [TODO]
 ///
 Parser::TPResult Parser::TryParseDeclarator(bool mayBeAbstract,
-                                            bool mayHaveIdentifier) {
+                                            bool mayHaveIdentifier,
+                                            bool mayHaveTrailingStrayEllipsis) {
   // declarator:
   //   direct-declarator
   //   ptr-operator declarator
   if (TryParsePtrOperatorSeq() == TPResult::Error)
     return TPResult::Error;
 
   // direct-declarator:
   // direct-abstract-declarator:
if ((Tok.isOneOf(tok::identifier, tok::kw_operator) ||
     } else {
       // '(' declarator ')'
       // '(' attributes declarator ')'
       // '(' abstract-declarator ')'
       if (Tok.isOneOf(tok::kw___attribute, tok::kw___declspec, tok::kw___cdecl,
                       tok::kw___stdcall, tok::kw___fastcall, tok::kw___thiscall,
                       tok::kw___regcall, tok::kw___vectorcall))
         return TPResult::True; // attributes indicate declaration
-      TPResult TPR = TryParseDeclarator(mayBeAbstract, mayHaveIdentifier);
+
+      // Disallow trailing stray ellipses since they can parse successfully in
+      // an initializer-list.
+      TPResult TPR = TryParseDeclarator(mayBeAbstract, mayHaveIdentifier,
+                                        /*mayHaveTrailingStrayEllipsis*/ false);
       if (TPR != TPResult::Ambiguous)
         return TPR;
       if (Tok.isNot(tok::r_paren))
         return TPResult::False;
       ConsumeParen();
     }
   } else if (!mayBeAbstract) {
     return TPResult::False;
   }
 
   while (1) {
     TPResult TPR(TPResult::Ambiguous);
 
-    // abstract-declarator: ...
-    if (Tok.is(tok::ellipsis))
+    // Allow stray ellipses as attempts to declare parameter packs.
+    if (Tok.is(tok::ellipsis) &&
+        (mayHaveTrailingStrayEllipsis ||
+         !(NextToken().is(tok::r_paren) || NextToken().is(tok::comma))))
       ConsumeToken();
 
     if (Tok.is(tok::l_paren)) {
       // Check whether we have a function declarator or a possible ctor-style
       // initializer that follows the declarator. Note that ctor-style
       // initializers are not possible in contexts where abstract declarators
       // are allowed.
       if (!mayBeAbstract && !isCXXFunctionDeclarator())
if (VersusTemplateArgument && TPR == TPResult::True) {
       // Disambiguate what follows the decl-specifier.
       if (TryConsumeDeclarationSpecifier() == TPResult::Error)
         return TPResult::Error;
     } else
       return TPR;
 
     // declarator
     // abstract-declarator[opt]
-    TPR = TryParseDeclarator(true/*mayBeAbstract*/);
+
+    // Disallow trailing stray ellipses since they can parse successfully in an
+    // initializer-list.
+    TPR = TryParseDeclarator(/*mayBeAbstract*/ true, /*mayHaveIdentifier*/ true,
+                             /*mayHaveTrailingStrayEllipsis*/ false);
     if (TPR != TPResult::Ambiguous)
       return TPR;
 
     // [GNU] attributes[opt]
     if (Tok.is(tok::kw___attribute))
       return TPResult::True;
 
     // If we're disambiguating a template argument in a default argument in

test/Parser/cxx0x-ambig.cpp

struct S {
     void j(T(int...)); // expected-error {{unexpanded parameter pack}}
     void j(T(T...)); // expected-error {{unexpanded parameter pack}}
     void k(int(...)(T)); // expected-error {{cannot return function type}}
     void k(int ...(T));
     void l(int(&...)(T)); // expected-warning {{ISO C++11 requires a parenthesized pack declaration to have a name}}
     void l(int(*...)(T)); // expected-warning {{ISO C++11 requires a parenthesized pack declaration to have a name}}
     void l(int(S<int>::*...)(T)); // expected-warning {{ISO C++11 requires a parenthesized pack declaration to have a name}}
   };
+
+  struct CtorSink {
+    template <typename ...T> constexpr CtorSink(T &&...t) { }
+    constexpr operator int() const { return 42; }
+  };
+
+  template <unsigned ...N> struct UnsignedTmplArgSink;
+
+  template <typename ...T>
+  void foo(int x, T ...t) {
+    // Have a variety of cases where the syntax is technically unambiguous, but hinges on careful treatment of ellipses.
+    CtorSink(t ...), x; // ok, expression; expected-warning 2{{expression result unused}}
+
+    int x0(CtorSink(t ...)); // ok, declares object x0
+    int *p0 = &x0;
+    (void)p0;
+
+    CtorSink x1(int(t) ..., int(x)); // ok, declares object x1
+    CtorSink *p1 = &x1;
+    (void)p1;
+
+    UnsignedTmplArgSink<T(CtorSink(t ...)) ...> *t0; // ok
+    UnsignedTmplArgSink<((T *)0, 42u) ...> **t0p = &t0;
+  }
+
+  template void foo(int, int, int); // expected-note {{in instantiation of function template specialization 'ellipsis::foo<int, int>' requested here}}
 }
 
 namespace braced_init_list {
   struct X {
     void foo() {}
   };
 
   void (*pf1)() {};