[WIP][Parser][FixIt] Better diagnostics for omitted template keyword (type dependent template names)

[jkorous-apple]

Summary

What I would love to have is FixIt for this kind of situation:

template <class T> void foo() {
    T::bar<int*>(); /* missing template keyword */
}

which currently gets not that helpful diagnostics:

> clang misleading.cpp
misleading.cpp:2:15: error: expected '(' for function-style cast or type construction
    T::bar<int*>();
           ~~~^
misleading.cpp:2:16: error: expected expression
    T::bar<int*>();
               ^
misleading.cpp:2:18: error: expected expression
    T::bar<int*>();
                 ^
3 errors generated.

while

template <class T> void foo() {
    T::bar<int>();
}

gets a good one:

good.cpp:2:8: error: use 'template' keyword to treat 'bar' as a dependent template name
    T::bar<int>();
       ^
       template
1 error generated.

If I understand it correctly than current implementation of

bool Parser::IsTemplateArgumentList(unsigned Skip)

relies on the fact that if there's a '<' token followed by declaration specifier it has to be a template. Which is true but produces false negatives for non-type parameters (array<1>) or pointer type parameters (vector<int*>) (among others).

One particular thing to keep on mind is that there's the [over.over] rule about resolving names to function pointers - best explained by test test/SemaTemplate/dependent-template-recover.cpp (look for snippet below):

// Note: no diagnostic here, this is actually valid as a comparison between
// the decayed pointer to Y::g<> and mb!
T::g<mb>(0);

What I did so far is that in case the simple approach doesn't validate there's a template I try to use speculative parsing. The only problem remaining is diagnostics - I am able to use TentativeParsingAction which is later reverted but I still get all the diagnostics which is incorrect in case my speculation about how to parse the code was wrong.

FixIt/fixit-template-for-dependent-name.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+template <class T> void foo() {
+    T::bar<int>(); // expected-error {{use 'template' keyword to treat 'bar' as a dependent template name}}
+    T::baz<int*>(); // expected-error {{use 'template' keyword to treat 'baz' as a dependent template name}}
+}
+
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// CHECK: fix-it:{{.*}}:{4:8-4:8}:"template "
+// CHECK: fix-it:{{.*}}:{5:8-5:8}:"template "

Parse/ParseTemplate.cpp

ParsedTemplateArgument Parser::ParseTemplateArgument() {
   return ParsedTemplateArgument(ParsedTemplateArgument::NonType, 
                                 ExprArg.get(), Loc);
 }
 
 /// \brief Determine whether the current tokens can only be parsed as a 
 /// template argument list (starting with the '<') and never as a '<' 
 /// expression.
 bool Parser::IsTemplateArgumentList(unsigned Skip) {
-  struct AlwaysRevertAction : TentativeParsingAction {
-    AlwaysRevertAction(Parser &P) : TentativeParsingAction(P) { }
-    ~AlwaysRevertAction() { Revert(); }
-  } Tentative(*this);
-  
-  while (Skip) {
-    ConsumeAnyToken();
-    --Skip;
+  {
+    RevertingTentativeParsingAction AutoRevertScopeGuard(*this);
+    for (; Skip > 0; --Skip) {
+      ConsumeToken();
     }
-  
-  // '<'
     if (!TryConsumeToken(tok::less))
       return false;
 
-  // An empty template argument list.
-  if (Tok.is(tok::greater))
-    return true;
-  
+    // If the first wannabe template argument consists only of decl specs
+    // it's a template argument list indeed.
     // See whether we have declaration specifiers, which indicate a type.
     while (isCXXDeclarationSpecifier() == TPResult::True)
       ConsumeAnyToken();
-  
     // If we have a '>' or a ',' then this is a template argument list.
-  return Tok.isOneOf(tok::greater, tok::comma);
+    if (Tok.isOneOf(tok::greater, tok::comma))
+      return true;
+  }
+
+  RevertingTentativeParsingAction AutoRevertScopeGuard(*this);
+
+  for (; Skip > 0; --Skip) {
+    ConsumeToken();
+  }
+
+  if (!TryConsumeToken(tok::less))
+    return false;
+
+  TemplateArgList ignore;
+  if (ParseTemplateArgumentList(ignore))
+    return false;
+
+  if (!TryConsumeToken(tok::greater))
+    return false;
+
+  return true;
 }
 
 /// ParseTemplateArgumentList - Parse a C++ template-argument-list
 /// (C++ [temp.names]). Returns true if there was an error.
 ///
 ///       template-argument-list: [C++ 14.2]
 ///         template-argument
 ///         template-argument-list ',' template-argument

SemaTemplate/dependent-template-recover.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 template<typename T, typename U, int N>
 struct X {
   void f(T* t) {
     t->f0<U>(); // expected-error{{use 'template' keyword to treat 'f0' as a dependent template name}}
     t->f0<int>(); // expected-error{{use 'template' keyword to treat 'f0' as a dependent template name}}
 
     t->operator+<U const, 1>(); // expected-error{{use 'template' keyword to treat 'operator +' as a dependent template name}}
+    t->operator+<1, U const>(); // expected-error{{use 'template' keyword to treat 'operator +' as a dependent template name}}
     t->f1<int const, 2>(); // expected-error{{use 'template' keyword to treat 'f1' as a dependent template name}}
 
     T::getAs<U>(); // expected-error{{use 'template' keyword to treat 'getAs' as a dependent template name}}
     t->T::getAs<U>(); // expected-error{{use 'template' keyword to treat 'getAs' as a dependent template name}}
 
     // FIXME: We can't recover from these yet
     (*t).f2<N>(); // expected-error{{expected expression}}
     (*t).f2<0>(); // expected-error{{expected expression}}