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

[Jan Korous via Phabricator via cfe-commits]

jkorous-apple created this revision.

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.


https://reviews.llvm.org/D43331

Files:
  FixIt/fixit-template-for-dependent-name.cpp
  Parse/ParseTemplate.cpp
  SemaTemplate/dependent-template-recover.cpp


Index: SemaTemplate/dependent-template-recover.cpp
===================================================================
--- SemaTemplate/dependent-template-recover.cpp
+++ SemaTemplate/dependent-template-recover.cpp
@@ -6,6 +6,7 @@
     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}}
Index: FixIt/fixit-template-for-dependent-name.cpp
===================================================================
--- /dev/null
+++ FixIt/fixit-template-for-dependent-name.cpp
@@ -0,0 +1,10 @@
+// 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 "
Index: Parse/ParseTemplate.cpp
===================================================================
--- Parse/ParseTemplate.cpp
+++ Parse/ParseTemplate.cpp
@@ -1247,30 +1247,45 @@
 /// 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;
+
+    // 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.
+    if (Tok.isOneOf(tok::greater, tok::comma))
+      return true;
   }
-  
-  // '<'
+
+  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;
-  
-  // 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);
+  TemplateArgList ignore;
+  if (ParseTemplateArgumentList(ignore))
+    return false;
+
+  if (!TryConsumeToken(tok::greater))
+    return false;
+
+  return true;
 }
 
 /// ParseTemplateArgumentList - Parse a C++ template-argument-list


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43331.134397.patch
Type: text/x-patch
Size: 3511 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180215/4a91f703/attachment.bin>