Diagnose missing 'template' keywords in more cases.

We track when we see a name-shaped expression followed by a '<' token
and parse the '<' as a comparison. Then:

 * if we see a token sequence that cannot possibly be an expression but
   can be a template argument (in particular, a type-id) that follows
   either a ',' or the '<', diagnose that the '<' was supposed to start
   a template argument list, and
 * if we see '>()', diagnose that the '<' was supposed to start a
   template argument list.

This only changes the diagnostic for error cases, and in practice
appears to catch the most common cases where a missing 'template'
keyword leads to parse errors within a template.

Differential Revision: https://reviews.llvm.org/D48571

llvm-svn: 335687

clang/include/clang/Basic/BitmaskEnum.h

@@ -0,0 +1,25 @@
+//===--- BitmaskEnum.h - wrapper of LLVM's bitmask enum facility-*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// Provides LLVM's BitmaskEnum facility to enumeration types declared in
+/// namespace clang.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_BASIC_BITMASKENUM_H
+#define LLVM_CLANG_BASIC_BITMASKENUM_H
+
+#include "llvm/ADT/BitmaskEnum.h"
+
+namespace clang {
+  LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
+}
+
+#endif

clang/include/clang/Parse/Parser.h

@@ -15,6 +15,7 @@
 #define LLVM_CLANG_PARSE_PARSER_H
 
 #include "clang/AST/Availability.h"
+#include "clang/Basic/BitmaskEnum.h"
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Basic/OperatorPrecedence.h"
 #include "clang/Basic/Specifiers.h"
@@ -247,6 +248,90 @@ class Parser : public CodeCompletionHandler {
   /// Identifiers which have been declared within a tentative parse.
   SmallVector<IdentifierInfo *, 8> TentativelyDeclaredIdentifiers;
 
+  /// Tracker for '<' tokens that might have been intended to be treated as an
+  /// angle bracket instead of a less-than comparison.
+  ///
+  /// This happens when the user intends to form a template-id, but typoes the
+  /// template-name or forgets a 'template' keyword for a dependent template
+  /// name.
+  ///
+  /// We track these locations from the point where we see a '<' with a
+  /// name-like expression on its left until we see a '>' or '>>' that might
+  /// match it.
+  struct AngleBracketTracker {
+    /// Flags used to rank candidate template names when there is more than one
+    /// '<' in a scope.
+    enum Priority : unsigned short {
+      /// A non-dependent name that is a potential typo for a template name.
+      PotentialTypo = 0x0,
+      /// A dependent name that might instantiate to a template-name.
+      DependentName = 0x2,
+
+      /// A space appears before the '<' token.
+      SpaceBeforeLess = 0x0,
+      /// No space before the '<' token
+      NoSpaceBeforeLess = 0x1,
+
+      LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue*/ DependentName)
+    };
+
+    struct Loc {
+      Expr *TemplateName;
+      SourceLocation LessLoc;
+      AngleBracketTracker::Priority Priority;
+      unsigned short ParenCount, BracketCount, BraceCount;
+
+      bool isActive(Parser &P) const {
+        return P.ParenCount == ParenCount && P.BracketCount == BracketCount &&
+               P.BraceCount == BraceCount;
+      }
+
+      bool isActiveOrNested(Parser &P) const {
+        return isActive(P) || P.ParenCount > ParenCount ||
+               P.BracketCount > BracketCount || P.BraceCount > BraceCount;
+      }
+    };
+
+    SmallVector<Loc, 8> Locs;
+
+    /// Add an expression that might have been intended to be a template name.
+    /// In the case of ambiguity, we arbitrarily select the innermost such
+    /// expression, for example in 'foo < bar < baz', 'bar' is the current
+    /// candidate. No attempt is made to track that 'foo' is also a candidate
+    /// for the case where we see a second suspicious '>' token.
+    void add(Parser &P, Expr *TemplateName, SourceLocation LessLoc,
+             Priority Prio) {
+      if (!Locs.empty() && Locs.back().isActive(P)) {
+        if (Locs.back().Priority <= Prio) {
+          Locs.back().TemplateName = TemplateName;
+          Locs.back().LessLoc = LessLoc;
+          Locs.back().Priority = Prio;
+        }
+      } else {
+        Locs.push_back({TemplateName, LessLoc, Prio,
+                        P.ParenCount, P.BracketCount, P.BraceCount});
+      }
+    }
+
+    /// Mark the current potential missing template location as having been
+    /// handled (this happens if we pass a "corresponding" '>' or '>>' token
+    /// or leave a bracket scope).
+    void clear(Parser &P) {
+      while (!Locs.empty() && Locs.back().isActiveOrNested(P))
+        Locs.pop_back();
+    }
+
+    /// Get the current enclosing expression that might hve been intended to be
+    /// a template name.
+    Loc *getCurrent(Parser &P) {
+      if (!Locs.empty() && Locs.back().isActive(P))
+        return &Locs.back();
+      return nullptr;
+    }
+  };
+
+  AngleBracketTracker AngleBrackets;
+
   IdentifierInfo *getSEHExceptKeyword();
 
   /// True if we are within an Objective-C container while parsing C-like decls.
@@ -426,8 +511,10 @@ private:
     assert(isTokenParen() && "wrong consume method");
     if (Tok.getKind() == tok::l_paren)
       ++ParenCount;
-    else if (ParenCount)
+    else if (ParenCount) {
+      AngleBrackets.clear(*this);
       --ParenCount;       // Don't let unbalanced )'s drive the count negative.
+    }
     PrevTokLocation = Tok.getLocation();
     PP.Lex(Tok);
     return PrevTokLocation;
@@ -439,8 +526,10 @@ private:
     assert(isTokenBracket() && "wrong consume method");
     if (Tok.getKind() == tok::l_square)
       ++BracketCount;
-    else if (BracketCount)
+    else if (BracketCount) {
+      AngleBrackets.clear(*this);
       --BracketCount;     // Don't let unbalanced ]'s drive the count negative.
+    }
 
     PrevTokLocation = Tok.getLocation();
     PP.Lex(Tok);
@@ -453,8 +542,10 @@ private:
     assert(isTokenBrace() && "wrong consume method");
     if (Tok.getKind() == tok::l_brace)
       ++BraceCount;
-    else if (BraceCount)
+    else if (BraceCount) {
+      AngleBrackets.clear(*this);
       --BraceCount;     // Don't let unbalanced }'s drive the count negative.
+    }
 
     PrevTokLocation = Tok.getLocation();
     PP.Lex(Tok);

clang/include/clang/Parse/RAIIObjectsForParser.h

@@ -331,6 +331,7 @@ namespace clang {
         BraceCount(p.BraceCount) { }
     
     ~ParenBraceBracketBalancer() {
+      P.AngleBrackets.clear(P);
       P.ParenCount = ParenCount;
       P.BracketCount = BracketCount;
       P.BraceCount = BraceCount;

clang/include/clang/Sema/Sema.h

@@ -21,6 +21,7 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/LocInfoType.h"
@@ -1829,14 +1830,24 @@ public:
   getTemplateNameKindForDiagnostics(TemplateName Name);
 
   /// Determine whether it's plausible that E was intended to be a
-  /// template-name.
-  bool mightBeIntendedToBeTemplateName(ExprResult E) {
-    if (!getLangOpts().CPlusPlus || E.isInvalid())
+  /// template-name. Updates E to denote the template name expression.
+  bool mightBeIntendedToBeTemplateName(ExprResult &ER, bool &Dependent) {
+    if (!getLangOpts().CPlusPlus || ER.isInvalid())
       return false;
-    if (auto *DRE = dyn_cast<DeclRefExpr>(E.get()))
+    Expr *E = ER.get()->IgnoreImplicit();
+    while (auto *BO = dyn_cast<BinaryOperator>(E))
+      E = BO->getRHS()->IgnoreImplicit();
+    ER = E;
+    Dependent = false;
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E))
       return !DRE->hasExplicitTemplateArgs();
-    if (auto *ME = dyn_cast<MemberExpr>(E.get()))
+    if (auto *ME = dyn_cast<MemberExpr>(E))
       return !ME->hasExplicitTemplateArgs();
+    Dependent = true;
+    if (auto *DSDRE = dyn_cast<DependentScopeDeclRefExpr>(E))
+      return !DSDRE->hasExplicitTemplateArgs();
+    if (auto *DSME = dyn_cast<CXXDependentScopeMemberExpr>(E))
+      return !DSME->hasExplicitTemplateArgs();
     // Any additional cases recognized here should also be handled by
     // diagnoseExprIntendedAsTemplateName.
     return false;

clang/lib/CodeGen/CGOpenMPRuntime.cpp

@@ -19,8 +19,8 @@
 #include "clang/CodeGen/ConstantInitBuilder.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/StmtOpenMP.h"
+#include "clang/Basic/BitmaskEnum.h"
 #include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/BitmaskEnum.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DerivedTypes.h"

clang/lib/Parse/ParseCXXInlineMethods.cpp

@@ -312,6 +312,8 @@ void Parser::ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM) {
     Actions.ActOnDelayedCXXMethodParameter(getCurScope(), Param);
     std::unique_ptr<CachedTokens> Toks = std::move(LM.DefaultArgs[I].Toks);
     if (Toks) {
+      ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
       // Mark the end of the default argument so that we know when to stop when
       // we parse it later on.
       Token LastDefaultArgToken = Toks->back();
@@ -384,6 +386,8 @@ void Parser::ParseLexedMethodDeclaration(LateParsedMethodDeclaration &LM) {
 
   // Parse a delayed exception-specification, if there is one.
   if (CachedTokens *Toks = LM.ExceptionSpecTokens) {
+    ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
     // Add the 'stop' token.
     Token LastExceptionSpecToken = Toks->back();
     Token ExceptionSpecEnd;
@@ -489,6 +493,8 @@ void Parser::ParseLexedMethodDef(LexedMethod &LM) {
     ++CurTemplateDepthTracker;
   }
 
+  ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
   assert(!LM.Toks.empty() && "Empty body!");
   Token LastBodyToken = LM.Toks.back();
   Token BodyEnd;
@@ -609,6 +615,8 @@ void Parser::ParseLexedMemberInitializer(LateParsedMemberInitializer &MI) {
   if (!MI.Field || MI.Field->isInvalidDecl())
     return;
 
+  ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
   // Append the current token at the end of the new token stream so that it
   // doesn't get lost.
   MI.Toks.push_back(Tok);

clang/lib/Parse/ParseDeclCXX.cpp

@@ -3009,6 +3009,8 @@ void Parser::SkipCXXMemberSpecification(SourceLocation RecordLoc,
 Parser::DeclGroupPtrTy Parser::ParseCXXClassMemberDeclarationWithPragmas(
     AccessSpecifier &AS, ParsedAttributesWithRange &AccessAttrs,
     DeclSpec::TST TagType, Decl *TagDecl) {
+  ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
   switch (Tok.getKind()) {
   case tok::kw___if_exists:
   case tok::kw___if_not_exists:

clang/lib/Parse/ParseExpr.cpp

@@ -302,6 +302,59 @@ Parser::ParseRHSOfBinaryExpression(ExprResult LHS, prec::Level MinPrec) {
     if (OpToken.is(tok::caretcaret)) {
       return ExprError(Diag(Tok, diag::err_opencl_logical_exclusive_or));
     }
+
+    // If we have a name-shaped expression followed by '<', track it in case we
+    // later find we're probably supposed to be in a template-id.
+    ExprResult TemplateName = LHS;
+    bool DependentTemplateName = false;
+    if (OpToken.is(tok::less) && Actions.mightBeIntendedToBeTemplateName(
+                                     TemplateName, DependentTemplateName)) {
+      AngleBracketTracker::Priority Priority =
+          (DependentTemplateName ? AngleBracketTracker::DependentName
+                                 : AngleBracketTracker::PotentialTypo) |
+          (OpToken.hasLeadingSpace() ? AngleBracketTracker::SpaceBeforeLess
+                                     : AngleBracketTracker::NoSpaceBeforeLess);
+      AngleBrackets.add(*this, TemplateName.get(), OpToken.getLocation(),
+                        Priority);
+    }
+
+    // If we're potentially in a template-id, we may now be able to determine
+    // whether we're actually in one or not.
+    if (auto *Info = AngleBrackets.getCurrent(*this)) {
+      // If an operator is followed by a type that can be a template argument
+      // and cannot be an expression, then this is ill-formed, but might be
+      // intended to be part of a template-id. Likewise if this is <>.
+      if ((OpToken.isOneOf(tok::less, tok::comma) &&
+            isKnownToBeDeclarationSpecifier()) ||
+           (OpToken.is(tok::less) &&
+            Tok.isOneOf(tok::greater, tok::greatergreater,
+                        tok::greatergreatergreater))) {
+        if (diagnoseUnknownTemplateId(Info->TemplateName, Info->LessLoc)) {
+          AngleBrackets.clear(*this);
+          return ExprError();
+        }
+      }
+
+      // If a context that looks like a template-id is followed by '()', then
+      // this is ill-formed, but might be intended to be a template-id followed
+      // by '()'.
+      if (OpToken.is(tok::greater) && Tok.is(tok::l_paren) &&
+          NextToken().is(tok::r_paren)) {
+        Actions.diagnoseExprIntendedAsTemplateName(
+            getCurScope(), Info->TemplateName, Info->LessLoc,
+            OpToken.getLocation());
+        AngleBrackets.clear(*this);
+        return ExprError();
+      }
+    }
+
+    // After a '>' (etc), we're no longer potentially in a construct that's
+    // intended to be treated as a template-id.
+    if (OpToken.is(tok::greater) ||
+        (getLangOpts().CPlusPlus11 &&
+         OpToken.isOneOf(tok::greatergreater, tok::greatergreatergreater)))
+      AngleBrackets.clear(*this);
+
     // Bail out when encountering a comma followed by a token which can't
     // possibly be the start of an expression. For instance:
     //   int f() { return 1, }
@@ -313,16 +366,6 @@ Parser::ParseRHSOfBinaryExpression(ExprResult LHS, prec::Level MinPrec) {
       return LHS;
     }
 
-    // If a '<' token is followed by a type that can be a template argument and
-    // cannot be an expression, then this is ill-formed, but might be intended
-    // to be a template-id.
-    if (OpToken.is(tok::less) && Actions.mightBeIntendedToBeTemplateName(LHS) &&
-        (isKnownToBeDeclarationSpecifier() ||
-         Tok.isOneOf(tok::greater, tok::greatergreater,
-                     tok::greatergreatergreater)) &&
-        diagnoseUnknownTemplateId(LHS, OpToken.getLocation()))
-      return ExprError();
-
     // If the next token is an ellipsis, then this is a fold-expression. Leave
     // it alone so we can handle it in the paren expression.
     if (isFoldOperator(NextTokPrec) && Tok.is(tok::ellipsis)) {

clang/lib/Parse/ParseExprCXX.cpp

@@ -1735,6 +1735,8 @@ Parser::ParseCXXTypeConstructExpression(const DeclSpec &DS) {
 Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
                                                 SourceLocation Loc,
                                                 Sema::ConditionKind CK) {
+  ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteOrdinaryName(getCurScope(), Sema::PCC_Condition);
     cutOffParsing();

clang/lib/Parse/ParseStmt.cpp

@@ -1621,6 +1621,8 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
                                                    attrs, attrs.Range.getEnd());
     ForRange = true;
   } else if (isForInitDeclaration()) {  // for (int X = 4;
+    ParenBraceBracketBalancer BalancerRAIIObj(*this);
+
     // Parse declaration, which eats the ';'.
     if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
       Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);

clang/lib/Sema/SemaTemplate.cpp

@@ -508,20 +508,41 @@ void Sema::diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
 
   DeclContext *LookupCtx = nullptr;
   NamedDecl *Found = nullptr;
+  bool MissingTemplateKeyword = false;
 
   // Figure out what name we looked up.
-  if (auto *ME = dyn_cast<MemberExpr>(TemplateName.get())) {
+  if (auto *DRE = dyn_cast<DeclRefExpr>(TemplateName.get())) {
+    NameInfo = DRE->getNameInfo();
+    SS.Adopt(DRE->getQualifierLoc());
+    LookupKind = LookupOrdinaryName;
+    Found = DRE->getFoundDecl();
+  } else if (auto *ME = dyn_cast<MemberExpr>(TemplateName.get())) {
     NameInfo = ME->getMemberNameInfo();
     SS.Adopt(ME->getQualifierLoc());
     LookupKind = LookupMemberName;
     LookupCtx = ME->getBase()->getType()->getAsCXXRecordDecl();
     Found = ME->getMemberDecl();
+  } else if (auto *DSDRE =
+                 dyn_cast<DependentScopeDeclRefExpr>(TemplateName.get())) {
+    NameInfo = DSDRE->getNameInfo();
+    SS.Adopt(DSDRE->getQualifierLoc());
+    MissingTemplateKeyword = true;
+  } else if (auto *DSME =
+                 dyn_cast<CXXDependentScopeMemberExpr>(TemplateName.get())) {
+    NameInfo = DSME->getMemberNameInfo();
+    SS.Adopt(DSME->getQualifierLoc());
+    MissingTemplateKeyword = true;
   } else {
-    auto *DRE = cast<DeclRefExpr>(TemplateName.get());
-    NameInfo = DRE->getNameInfo();
-    SS.Adopt(DRE->getQualifierLoc());
-    LookupKind = LookupOrdinaryName;
-    Found = DRE->getFoundDecl();
+    llvm_unreachable("unexpected kind of potential template name");
+  }
+
+  // If this is a dependent-scope lookup, diagnose that the 'template' keyword
+  // was missing.
+  if (MissingTemplateKeyword) {
+    Diag(NameInfo.getLocStart(), diag::err_template_kw_missing)
+        << "" << NameInfo.getName().getAsString()
+        << SourceRange(Less, Greater);
+    return;
   }
 
   // Try to correct the name by looking for templates and C++ named casts.

clang/test/SemaTemplate/dependent-template-recover.cpp

@@ -5,18 +5,59 @@ struct X {
     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->f1<int const, 2>(); // expected-error{{use 'template' keyword to treat 'f1' as a dependent template name}}
+    t->operator+<U const, 1>(1); // expected-error{{use 'template' keyword to treat 'operator +' as a dependent template name}}
+    t->f1<int const, 2>(1); // expected-error{{use 'template' keyword to treat 'f1' as a dependent template name}}
+    t->f1<3, int const>(1); // expected-error{{missing 'template' keyword prior to dependent template name 'f1'}}
 
     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}}
+    (*t).f2<N>(); // expected-error{{missing 'template' keyword prior to dependent template name 'f2'}}
+    (*t).f2<0>(); // expected-error{{missing 'template' keyword prior to dependent template name 'f2'}}
+    T::f2<0>(); // expected-error{{missing 'template' keyword prior to dependent template name 'f2'}}
+    T::f2<0, int>(0); // expected-error{{missing 'template' keyword prior to dependent template name 'f2'}}
+
+    T::foo<N < 2 || N >= 4>(); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+
+    // If there are multiple potential template names, pick the one where there
+    // is no whitespace between the name and the '<'.
+    T::foo<T::bar < 1>(); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+    T::foo < T::bar<1>(); // expected-error{{missing 'template' keyword prior to dependent template name 'bar'}}
+
+    // Prefer to diagonse a missing 'template' keyword rather than finding a non-template name.
+    xyz < T::foo < 1 > (); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+    T::foo < xyz < 1 > (); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+
+    // ... even if the whitespace suggests the other name is the template name.
+    // FIXME: Is this the right heuristic?
+    xyz<T::foo < 1>(); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+    T::foo < xyz<1>(); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
   }
+
+  int xyz;
 };
 
+template <typename T> void not_missing_template(T t) {
+  (T::n < 0) > (
+     ) // expected-error {{expected expression}}
+    ;
+
+  int a = T::x < 3;
+  int b = T::y > (); // expected-error {{expected expression}}
+
+  void c(int = T::x < 3);
+  void d(int = T::y > ()); // expected-error {{expected expression}}
+
+  for (int x = t < 3 ? 1 : 2; t > (); ++t) { // expected-error {{expected expression}}
+  }
+
+  // FIXME: We shouldn't treat 'T::t' as a potential template-name here,
+  // because that would leave a '?' with no matching ':'.
+  // We should probably generally treat '?' ... ':' as a bracket-like
+  // construct.
+  bool k = T::t < 3 ? 1 > () : false; // expected-error {{missing 'template' keyword}} expected-error +{{}} expected-note +{{}}
+}
+
 struct MrsBadcrumble {
   friend MrsBadcrumble operator<(void (*)(int), MrsBadcrumble);
   friend void operator>(MrsBadcrumble, int);
@@ -30,6 +71,9 @@ template<int N, typename T> void f(T t) {
   // Note: no diagnostic here, this is actually valid as a comparison between
   // the decayed pointer to Y::g<> and mb!
   T::g<mb>(0);
+
+  // ... but this one must be a template-id.
+  T::g<mb, int>(0); // expected-error {{missing 'template' keyword prior to dependent template name 'g'}}
 }
 
 struct Y {