Diagnose missing 'template' keywords in contexts where a comma is not a

zygoloid · zygoloid · commit c2dead4d9a5e · 2018-06-27T01:32:04.000Z
binary operator.

Factor out the checking for a comma within potential angle brackets and
also call it from contexts where we parse a comma-separated list of
arguments or initializers.

llvm-svn: 335699
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
@@ -1607,6 +1607,14 @@ class Parser : public CodeCompletionHandler {
   }
 
   bool diagnoseUnknownTemplateId(ExprResult TemplateName, SourceLocation Less);
+  void checkPotentialAngleBracket(ExprResult &PotentialTemplateName);
+  bool checkPotentialAngleBracketDelimiter(const AngleBracketTracker::Loc &,
+                                           const Token &OpToken);
+  bool checkPotentialAngleBracketDelimiter(const Token &OpToken) {
+    if (auto *Info = AngleBrackets.getCurrent(*this))
+      return checkPotentialAngleBracketDelimiter(*Info, OpToken);
+    return false;
+  }
 
   ExprResult ParsePostfixExpressionSuffix(ExprResult LHS);
   ExprResult ParseUnaryExprOrTypeTraitExpression();
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -1830,23 +1830,19 @@ class Sema {
   getTemplateNameKindForDiagnostics(TemplateName Name);
 
   /// Determine whether it's plausible that E was intended to be a
-  /// template-name. Updates E to denote the template name expression.
-  bool mightBeIntendedToBeTemplateName(ExprResult &ER, bool &Dependent) {
-    if (!getLangOpts().CPlusPlus || ER.isInvalid())
+  /// template-name.
+  bool mightBeIntendedToBeTemplateName(ExprResult E, bool &Dependent) {
+    if (!getLangOpts().CPlusPlus || E.isInvalid())
       return false;
-    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))
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E.get()))
       return !DRE->hasExplicitTemplateArgs();
-    if (auto *ME = dyn_cast<MemberExpr>(E))
+    if (auto *ME = dyn_cast<MemberExpr>(E.get()))
       return !ME->hasExplicitTemplateArgs();
     Dependent = true;
-    if (auto *DSDRE = dyn_cast<DependentScopeDeclRefExpr>(E))
+    if (auto *DSDRE = dyn_cast<DependentScopeDeclRefExpr>(E.get()))
       return !DSDRE->hasExplicitTemplateArgs();
-    if (auto *DSME = dyn_cast<CXXDependentScopeMemberExpr>(E))
+    if (auto *DSME = dyn_cast<CXXDependentScopeMemberExpr>(E.get()))
       return !DSME->hasExplicitTemplateArgs();
     // Any additional cases recognized here should also be handled by
     // diagnoseExprIntendedAsTemplateName.
diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
@@ -246,30 +246,6 @@ bool Parser::isNotExpressionStart() {
   return isKnownToBeDeclarationSpecifier();
 }
 
-/// We've parsed something that could plausibly be intended to be a template
-/// name (\p LHS) followed by a '<' token, and the following code can't possibly
-/// be an expression. Determine if this is likely to be a template-id and if so,
-/// diagnose it.
-bool Parser::diagnoseUnknownTemplateId(ExprResult LHS, SourceLocation Less) {
-  TentativeParsingAction TPA(*this);
-  // FIXME: We could look at the token sequence in a lot more detail here.
-  if (SkipUntil(tok::greater, tok::greatergreater, tok::greatergreatergreater,
-                StopAtSemi | StopBeforeMatch)) {
-    TPA.Commit();
-
-    SourceLocation Greater;
-    ParseGreaterThanInTemplateList(Greater, true, false);
-    Actions.diagnoseExprIntendedAsTemplateName(getCurScope(), LHS,
-                                               Less, Greater);
-    return true;
-  }
-
-  // There's no matching '>' token, this probably isn't supposed to be
-  // interpreted as a template-id. Parse it as an (ill-formed) comparison.
-  TPA.Revert();
-  return false;
-}
-
 bool Parser::isFoldOperator(prec::Level Level) const {
   return Level > prec::Unknown && Level != prec::Conditional &&
          Level != prec::Spaceship;
@@ -303,57 +279,12 @@ Parser::ParseRHSOfBinaryExpression(ExprResult LHS, prec::Level MinPrec) {
       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);
+    if (OpToken.isOneOf(tok::comma, tok::greater, tok::greatergreater,
+                        tok::greatergreatergreater) &&
+        checkPotentialAngleBracketDelimiter(OpToken))
+      return ExprError();
 
     // Bail out when encountering a comma followed by a token which can't
     // possibly be the start of an expression. For instance:
@@ -879,6 +810,8 @@ ExprResult Parser::ParseCastExpression(bool isUnaryExpression,
     assert(Res.get() == nullptr && "Stray primary-expression annotation?");
     Res = getExprAnnotation(Tok);
     ConsumeAnnotationToken();
+    if (!Res.isInvalid() && Tok.is(tok::less))
+      checkPotentialAngleBracket(Res);
     break;
 
   case tok::kw___super:
@@ -1098,11 +1031,13 @@ ExprResult Parser::ParseCastExpression(bool isUnaryExpression,
         isAddressOfOperand, std::move(Validator),
         /*IsInlineAsmIdentifier=*/false,
         Tok.is(tok::r_paren) ? nullptr : &Replacement);
-    if (!Res.isInvalid() && !Res.get()) {
+    if (!Res.isInvalid() && Res.isUnset()) {
       UnconsumeToken(Replacement);
       return ParseCastExpression(isUnaryExpression, isAddressOfOperand,
                                  NotCastExpr, isTypeCast);
     }
+    if (!Res.isInvalid() && Tok.is(tok::less))
+      checkPotentialAngleBracket(Res);
     break;
   }
   case tok::char_constant:     // constant: character-constant
@@ -1841,6 +1776,8 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {
                                             OpKind, SS, TemplateKWLoc, Name,
                                  CurParsedObjCImpl ? CurParsedObjCImpl->Dcl
                                                    : nullptr);
+      if (!LHS.isInvalid() && Tok.is(tok::less))
+        checkPotentialAngleBracket(LHS);
       break;
     }
     case tok::plusplus:    // postfix-expression: postfix-expression '++'
@@ -2878,7 +2815,10 @@ bool Parser::ParseExpressionList(SmallVectorImpl<Expr *> &Exprs,
     if (Tok.isNot(tok::comma))
       break;
     // Move to the next argument, remember where the comma was.
+    Token Comma = Tok;
     CommaLocs.push_back(ConsumeToken());
+
+    checkPotentialAngleBracketDelimiter(Comma);
   }
   if (SawError) {
     // Ensure typos get diagnosed when errors were encountered while parsing the
@@ -2913,7 +2853,10 @@ Parser::ParseSimpleExpressionList(SmallVectorImpl<Expr*> &Exprs,
       return false;
 
     // Move to the next argument, remember where the comma was.
+    Token Comma = Tok;
     CommaLocs.push_back(ConsumeToken());
+
+    checkPotentialAngleBracketDelimiter(Comma);
   }
 }
 
diff --git a/clang/lib/Parse/ParseExprCXX.cpp b/clang/lib/Parse/ParseExprCXX.cpp
@@ -561,10 +561,13 @@ ExprResult Parser::tryParseCXXIdExpression(CXXScopeSpec &SS, bool isAddressOfOpe
   if (isAddressOfOperand && isPostfixExpressionSuffixStart())
     isAddressOfOperand = false;
 
-  return Actions.ActOnIdExpression(getCurScope(), SS, TemplateKWLoc, Name,
-                                   Tok.is(tok::l_paren), isAddressOfOperand,
-                                   nullptr, /*IsInlineAsmIdentifier=*/false,
-                                   &Replacement);
+  ExprResult E = Actions.ActOnIdExpression(
+      getCurScope(), SS, TemplateKWLoc, Name, Tok.is(tok::l_paren),
+      isAddressOfOperand, nullptr, /*IsInlineAsmIdentifier=*/false,
+      &Replacement);
+  if (!E.isInvalid() && !E.isUnset() && Tok.is(tok::less))
+    checkPotentialAngleBracket(E);
+  return E;
 }
 
 /// ParseCXXIdExpression - Handle id-expression.
diff --git a/clang/lib/Parse/ParseTemplate.cpp b/clang/lib/Parse/ParseTemplate.cpp
@@ -1474,3 +1474,111 @@ void Parser::LexTemplateFunctionForLateParsing(CachedTokens &Toks) {
     }
   }
 }
+
+/// We've parsed something that could plausibly be intended to be a template
+/// name (\p LHS) followed by a '<' token, and the following code can't possibly
+/// be an expression. Determine if this is likely to be a template-id and if so,
+/// diagnose it.
+bool Parser::diagnoseUnknownTemplateId(ExprResult LHS, SourceLocation Less) {
+  TentativeParsingAction TPA(*this);
+  // FIXME: We could look at the token sequence in a lot more detail here.
+  if (SkipUntil(tok::greater, tok::greatergreater, tok::greatergreatergreater,
+                StopAtSemi | StopBeforeMatch)) {
+    TPA.Commit();
+
+    SourceLocation Greater;
+    ParseGreaterThanInTemplateList(Greater, true, false);
+    Actions.diagnoseExprIntendedAsTemplateName(getCurScope(), LHS,
+                                               Less, Greater);
+    return true;
+  }
+
+  // There's no matching '>' token, this probably isn't supposed to be
+  // interpreted as a template-id. Parse it as an (ill-formed) comparison.
+  TPA.Revert();
+  return false;
+}
+
+void Parser::checkPotentialAngleBracket(ExprResult &PotentialTemplateName) {
+  assert(Tok.is(tok::less) && "not at a potential angle bracket");
+
+  bool DependentTemplateName = false;
+  if (!Actions.mightBeIntendedToBeTemplateName(PotentialTemplateName,
+                                               DependentTemplateName))
+    return;
+
+  // OK, this might be a name that the user intended to be parsed as a
+  // template-name, followed by a '<' token. Check for some easy cases.
+
+  // If we have potential_template<>, then it's supposed to be a template-name.
+  if (NextToken().is(tok::greater) ||
+      (getLangOpts().CPlusPlus11 &&
+       NextToken().isOneOf(tok::greatergreater, tok::greatergreatergreater))) {
+    SourceLocation Less = ConsumeToken();
+    SourceLocation Greater;
+    ParseGreaterThanInTemplateList(Greater, true, false);
+    Actions.diagnoseExprIntendedAsTemplateName(
+        getCurScope(), PotentialTemplateName, Less, Greater);
+    // FIXME: Perform error recovery.
+    PotentialTemplateName = ExprError();
+    return;
+  }
+
+  // If we have 'potential_template<type-id', assume it's supposed to be a
+  // template-name if there's a matching '>' later on.
+  {
+    // FIXME: Avoid the tentative parse when NextToken() can't begin a type.
+    TentativeParsingAction TPA(*this);
+    SourceLocation Less = ConsumeToken();
+    if (isTypeIdUnambiguously() &&
+        diagnoseUnknownTemplateId(PotentialTemplateName, Less)) {
+      TPA.Commit();
+      // FIXME: Perform error recovery.
+      PotentialTemplateName = ExprError();
+      return;
+    }
+    TPA.Revert();
+  }
+
+  // Otherwise, remember that we saw this in case we see a potentially-matching
+  // '>' token later on.
+  AngleBracketTracker::Priority Priority =
+      (DependentTemplateName ? AngleBracketTracker::DependentName
+                             : AngleBracketTracker::PotentialTypo) |
+      (Tok.hasLeadingSpace() ? AngleBracketTracker::SpaceBeforeLess
+                             : AngleBracketTracker::NoSpaceBeforeLess);
+  AngleBrackets.add(*this, PotentialTemplateName.get(), Tok.getLocation(),
+                    Priority);
+}
+
+bool Parser::checkPotentialAngleBracketDelimiter(
+    const AngleBracketTracker::Loc &LAngle, const Token &OpToken) {
+  // If a comma in an expression context 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.
+  if (OpToken.is(tok::comma) && isTypeIdUnambiguously() &&
+      diagnoseUnknownTemplateId(LAngle.TemplateName, LAngle.LessLoc)) {
+    AngleBrackets.clear(*this);
+    return true;
+  }
+
+  // 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(), LAngle.TemplateName, LAngle.LessLoc,
+        OpToken.getLocation());
+    AngleBrackets.clear(*this);
+    return true;
+  }
+
+  // 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);
+  return false;
+}
diff --git a/clang/test/SemaTemplate/dependent-template-recover.cpp b/clang/test/SemaTemplate/dependent-template-recover.cpp
@@ -32,6 +32,16 @@ struct X {
     // 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'}}
+
+    sizeof T::foo < 123 > (); // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+    f(t->foo<1, 2>(), // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+      t->bar<3, 4>()); // expected-error{{missing 'template' keyword prior to dependent template name 'bar'}}
+
+    int arr[] = {
+      t->baz<1, 2>(1 + 1), // ok, two comparisons
+      t->foo<1, 2>(), // expected-error{{missing 'template' keyword prior to dependent template name 'foo'}}
+      t->bar<3, 4>()  // FIXME: we don't recover from the previous error so don't diagnose this
+    };
   }
 
   int xyz;
