Separate the notion of 'context' when recursing down in the parser and actual errors.

Summary:
Change how error messages are constructed and stored in Diagnostics.
Separate the notion of 'context' when recursing down in the parser and actual errors.
This will simplify adding some new features, like argument overloading and error recovery.

Reviewers: klimek

CC: cfe-commits, revane

Differential Revision: http://llvm-reviews.chandlerc.com/D1168

llvm-svn: 186602

5 files changed, 172 insertions, 82 deletions

diff --git a/clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h b/clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
index 973d54cead3..49527e059e4 100644
--- a/clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
+++ b/clang/include/clang/ASTMatchers/Dynamic/Diagnostics.h
@@ -49,7 +49,13 @@ struct ParserValue {
 
 /// \brief Helper class to manage error messages.
 class Diagnostics {
- public:
+public:
+  /// \brief Parser context types.
+  enum ContextType {
+    CT_MatcherArg = 0,
+    CT_MatcherConstruct = 1
+  };
+
   /// \brief All errors from the system.
   enum ErrorType {
     ET_None = 0,
@@ -60,51 +66,96 @@ class Diagnostics {
     ET_RegistryNotBindable = 4,
 
     ET_ParserStringError = 100,
-    ET_ParserMatcherArgFailure = 101,
-    ET_ParserMatcherFailure = 102,
-    ET_ParserNoOpenParen = 103,
-    ET_ParserNoCloseParen = 104,
-    ET_ParserNoComma = 105,
-    ET_ParserNoCode = 106,
-    ET_ParserNotAMatcher = 107,
-    ET_ParserInvalidToken = 108,
-    ET_ParserMalformedBindExpr = 109,
-    ET_ParserTrailingCode = 110,
-    ET_ParserUnsignedError = 111,
-    ET_ParserOverloadedType = 112
+    ET_ParserNoOpenParen = 101,
+    ET_ParserNoCloseParen = 102,
+    ET_ParserNoComma = 103,
+    ET_ParserNoCode = 104,
+    ET_ParserNotAMatcher = 105,
+    ET_ParserInvalidToken = 106,
+    ET_ParserMalformedBindExpr = 107,
+    ET_ParserTrailingCode = 108,
+    ET_ParserUnsignedError = 109,
+    ET_ParserOverloadedType = 110
   };
 
   /// \brief Helper stream class.
-  struct ArgStream {
+  class ArgStream {
+  public:
+    ArgStream(std::vector<std::string> *Out) : Out(Out) {}
     template <class T> ArgStream &operator<<(const T &Arg) {
       return operator<<(Twine(Arg));
     }
     ArgStream &operator<<(const Twine &Arg);
+
+  private:
     std::vector<std::string> *Out;
   };
 
-  /// \brief Push a frame to the beginning of the list
+  /// \brief Class defining a parser context.
+  ///
+  /// Used by the parser to specify (possibly recursive) contexts where the
+  /// parsing/construction can fail. Any error triggered within a context will
+  /// keep information about the context chain.
+  /// This class should be used as a RAII instance in the stack.
+  struct Context {
+  public:
+    /// \brief About to call the constructor for a matcher.
+    enum ConstructMatcherEnum { ConstructMatcher };
+    Context(ConstructMatcherEnum, Diagnostics *Error, StringRef MatcherName,
+            const SourceRange &MatcherRange);
+    /// \brief About to recurse into parsing one argument for a matcher.
+    enum MatcherArgEnum { MatcherArg };
+    Context(MatcherArgEnum, Diagnostics *Error, StringRef MatcherName,
+            const SourceRange &MatcherRange, unsigned ArgNumber);
+    ~Context();
+
+  private:
+    Diagnostics *const Error;
+  };
+
+  /// \brief Add an error to the diagnostics.
   ///
-  /// Returns a helper class to allow the caller to pass the arguments for the
+  /// All the context information will be kept on the error message.
+  /// \return a helper class to allow the caller to pass the arguments for the
   /// error message, using the << operator.
-  ArgStream pushErrorFrame(const SourceRange &Range, ErrorType Error);
+  ArgStream addError(const SourceRange &Range, ErrorType Error);
+
+  /// \brief Information stored for one frame of the context.
+  struct ContextFrame {
+    ContextType Type;
+    SourceRange Range;
+    std::vector<std::string> Args;
+
+    std::string ToString() const;
+  };
 
-  struct ErrorFrame {
+  /// \brief Information stored for each error found.
+  struct ErrorContent {
+    std::vector<ContextFrame> ContextStack;
     SourceRange Range;
     ErrorType Type;
     std::vector<std::string> Args;
 
     std::string ToString() const;
   };
-  ArrayRef<ErrorFrame> frames() const { return Frames; }
+  ArrayRef<ErrorContent> errors() const { return Errors; }
 
-  /// \brief Returns a string representation of the last frame.
+  /// \brief Returns a simple string representation of each error.
+  ///
+  /// Each error only shows the error message without any context.
   std::string ToString() const;
-  /// \brief Returns a string representation of the whole frame stack.
+
+  /// \brief Returns the full string representation of each error.
+  ///
+  /// Each error message contains the full context.
   std::string ToStringFull() const;
 
- private:
-   std::vector<ErrorFrame> Frames;
+private:
+  /// \brief Helper function used by the constructors of ContextFrame.
+  ArgStream pushContextFrame(ContextType Type, SourceRange Range);
+
+  std::vector<ContextFrame> ContextStack;
+  std::vector<ErrorContent> Errors;
 };
 
 }  // namespace dynamic
diff --git a/clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp b/clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
index 1ca1f365398..71aeb9a7540 100644
--- a/clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
+++ b/clang/lib/ASTMatchers/Dynamic/Diagnostics.cpp
@@ -13,20 +13,56 @@ namespace clang {
 namespace ast_matchers {
 namespace dynamic {
 
-Diagnostics::ArgStream &
-Diagnostics::ArgStream::operator<<(const Twine &Arg) {
+Diagnostics::ArgStream Diagnostics::pushContextFrame(ContextType Type,
+                                                     SourceRange Range) {
+  ContextStack.push_back(ContextFrame());
+  ContextFrame& data = ContextStack.back();
+  data.Type = Type;
+  data.Range = Range;
+  return ArgStream(&data.Args);
+}
+
+Diagnostics::Context::Context(ConstructMatcherEnum, Diagnostics *Error,
+                              StringRef MatcherName,
+                              const SourceRange &MatcherRange)
+    : Error(Error) {
+  Error->pushContextFrame(CT_MatcherConstruct, MatcherRange) << MatcherName;
+}
+
+Diagnostics::Context::Context(MatcherArgEnum, Diagnostics *Error,
+                              StringRef MatcherName,
+                              const SourceRange &MatcherRange,
+                              unsigned ArgNumber)
+    : Error(Error) {
+  Error->pushContextFrame(CT_MatcherArg, MatcherRange) << ArgNumber
+                                                       << MatcherName;
+}
+
+Diagnostics::Context::~Context() { Error->ContextStack.pop_back(); }
+
+Diagnostics::ArgStream &Diagnostics::ArgStream::operator<<(const Twine &Arg) {
   Out->push_back(Arg.str());
   return *this;
 }
 
-Diagnostics::ArgStream Diagnostics::pushErrorFrame(const SourceRange &Range,
-                                                   ErrorType Error) {
-  Frames.insert(Frames.begin(), ErrorFrame());
-  ErrorFrame &Last = Frames.front();
+Diagnostics::ArgStream Diagnostics::addError(const SourceRange &Range,
+                                             ErrorType Error) {
+  Errors.push_back(ErrorContent());
+  ErrorContent &Last = Errors.back();
+  Last.ContextStack = ContextStack;
   Last.Range = Range;
   Last.Type = Error;
-  ArgStream Out = { &Last.Args };
-  return Out;
+  return ArgStream(&Last.Args);
+}
+
+StringRef ContextTypeToString(Diagnostics::ContextType Type) {
+  switch (Type) {
+    case Diagnostics::CT_MatcherConstruct:
+      return "Error building matcher $0.";
+    case Diagnostics::CT_MatcherArg:
+      return "Error parsing argument $0 for matcher $1.";
+  }
+  llvm_unreachable("Unknown ContextType value.");
 }
 
 StringRef ErrorTypeToString(Diagnostics::ErrorType Type) {
@@ -42,10 +78,6 @@ StringRef ErrorTypeToString(Diagnostics::ErrorType Type) {
 
   case Diagnostics::ET_ParserStringError:
     return "Error parsing string token: <$0>";
-  case Diagnostics::ET_ParserMatcherArgFailure:
-    return "Error parsing argument $0 for matcher $1.";
-  case Diagnostics::ET_ParserMatcherFailure:
-    return "Error building matcher $0.";
   case Diagnostics::ET_ParserNoOpenParen:
     return "Error parsing matcher. Found token <$0> while looking for '('.";
   case Diagnostics::ET_ParserNoCloseParen:
@@ -95,25 +127,42 @@ std::string FormatErrorString(StringRef FormatString,
   return Out;
 }
 
-std::string Diagnostics::ErrorFrame::ToString() const {
-  StringRef FormatString = ErrorTypeToString(Type);
-  std::string ErrorOut = FormatErrorString(FormatString, Args);
+static std::string MaybeAddLineAndColumn(Twine Input,
+                                         const SourceRange &Range) {
   if (Range.Start.Line > 0 && Range.Start.Column > 0)
     return (Twine(Range.Start.Line) + ":" + Twine(Range.Start.Column) + ": " +
-            ErrorOut).str();
-  return ErrorOut;
+            Input).str();
+  return Input.str();
+}
+
+std::string Diagnostics::ContextFrame::ToString() const {
+  return MaybeAddLineAndColumn(
+      FormatErrorString(ContextTypeToString(Type), Args), Range);
+}
+
+std::string Diagnostics::ErrorContent::ToString() const {
+  return MaybeAddLineAndColumn(FormatErrorString(ErrorTypeToString(Type), Args),
+                               Range);
 }
 
 std::string Diagnostics::ToString() const {
-  if (Frames.empty()) return "";
-  return Frames[Frames.size() - 1].ToString();
+  std::string Result;
+  for (size_t i = 0, e = Errors.size(); i != e; ++i) {
+    if (i != 0) Result += "\n";
+    Result += Errors[i].ToString();
+  }
+  return Result;
 }
 
 std::string Diagnostics::ToStringFull() const {
   std::string Result;
-  for (size_t i = 0, end = Frames.size(); i != end; ++i) {
-    if (i > 0) Result += "\n";
-    Result += Frames[i].ToString();
+  for (size_t i = 0, e = Errors.size(); i != e; ++i) {
+    if (i != 0) Result += "\n";
+    const ErrorContent &Error = Errors[i];
+    for (size_t i = 0, e = Error.ContextStack.size(); i != e; ++i) {
+      Result += Error.ContextStack[i].ToString() + "\n";
+    }
+    Result += Error.ToString();
   }
   return Result;
 }
diff --git a/clang/lib/ASTMatchers/Dynamic/Marshallers.h b/clang/lib/ASTMatchers/Dynamic/Marshallers.h
index 3aa55d0a5a5..9f1cfe3443f 100644
--- a/clang/lib/ASTMatchers/Dynamic/Marshallers.h
+++ b/clang/lib/ASTMatchers/Dynamic/Marshallers.h
@@ -152,14 +152,14 @@ private:
 /// \brief Helper macros to check the arguments on all marshaller functions.
 #define CHECK_ARG_COUNT(count)                                                 \
   if (Args.size() != count) {                                                  \
-    Error->pushErrorFrame(NameRange, Error->ET_RegistryWrongArgCount)          \
+    Error->addError(NameRange, Error->ET_RegistryWrongArgCount)                \
         << count << Args.size();                                               \
     return MatcherList();                                                      \
   }
 
 #define CHECK_ARG_TYPE(index, type)                                            \
   if (!ArgTypeTraits<type>::is(Args[index].Value)) {                           \
-    Error->pushErrorFrame(Args[index].Range, Error->ET_RegistryWrongArgType)   \
+    Error->addError(Args[index].Range, Error->ET_RegistryWrongArgType)         \
         << (index + 1) << ArgTypeTraits<type>::asString()                      \
         << Args[index].Value.getTypeAsString();                                \
     return MatcherList();                                                      \
@@ -254,7 +254,7 @@ MatcherList variadicMatcherCreateCallback(StringRef MatcherName,
     const ParserValue &Arg = Args[i];
     const VariantValue &Value = Arg.Value;
     if (!ArgTraits::is(Value)) {
-      Error->pushErrorFrame(Arg.Range, Error->ET_RegistryWrongArgType)
+      Error->addError(Arg.Range, Error->ET_RegistryWrongArgType)
           << (i + 1) << ArgTraits::asString() << Value.getTypeAsString();
       HasError = true;
       break;
diff --git a/clang/lib/ASTMatchers/Dynamic/Parser.cpp b/clang/lib/ASTMatchers/Dynamic/Parser.cpp
index fc09a30ddd9..54424cea7e2 100644
--- a/clang/lib/ASTMatchers/Dynamic/Parser.cpp
+++ b/clang/lib/ASTMatchers/Dynamic/Parser.cpp
@@ -162,8 +162,7 @@ private:
       SourceRange Range;
       Range.Start = Result->Range.Start;
       Range.End = currentLocation();
-      Error->pushErrorFrame(Range, Error->ET_ParserUnsignedError)
-          << Result->Text;
+      Error->addError(Range, Error->ET_ParserUnsignedError) << Result->Text;
       Result->Kind = TokenInfo::TK_Error;
     }
   }
@@ -198,8 +197,7 @@ private:
     SourceRange Range;
     Range.Start = Result->Range.Start;
     Range.End = currentLocation();
-    Error->pushErrorFrame(Range, Error->ET_ParserStringError)
-        << ErrorText;
+    Error->addError(Range, Error->ET_ParserStringError) << ErrorText;
     Result->Kind = TokenInfo::TK_Error;
   }
 
@@ -239,7 +237,7 @@ bool Parser::parseMatcherExpressionImpl(VariantValue *Value) {
   assert(NameToken.Kind == TokenInfo::TK_Ident);
   const TokenInfo OpenToken = Tokenizer->consumeNextToken();
   if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
-    Error->pushErrorFrame(OpenToken.Range, Error->ET_ParserNoOpenParen)
+    Error->addError(OpenToken.Range, Error->ET_ParserNoOpenParen)
         << OpenToken.Text;
     return false;
   }
@@ -256,27 +254,24 @@ bool Parser::parseMatcherExpressionImpl(VariantValue *Value) {
       // We must find a , token to continue.
       const TokenInfo CommaToken = Tokenizer->consumeNextToken();
       if (CommaToken.Kind != TokenInfo::TK_Comma) {
-        Error->pushErrorFrame(CommaToken.Range, Error->ET_ParserNoComma)
+        Error->addError(CommaToken.Range, Error->ET_ParserNoComma)
             << CommaToken.Text;
         return false;
       }
     }
 
+    Diagnostics::Context Ctx(Diagnostics::Context::MatcherArg, Error,
+                             NameToken.Text, NameToken.Range, Args.size() + 1);
     ParserValue ArgValue;
     ArgValue.Text = Tokenizer->peekNextToken().Text;
     ArgValue.Range = Tokenizer->peekNextToken().Range;
-    if (!parseExpressionImpl(&ArgValue.Value)) {
-      Error->pushErrorFrame(NameToken.Range,
-                            Error->ET_ParserMatcherArgFailure)
-          << (Args.size() + 1) << NameToken.Text;
-      return false;
-    }
+    if (!parseExpressionImpl(&ArgValue.Value)) return false;
 
     Args.push_back(ArgValue);
   }
 
   if (EndToken.Kind == TokenInfo::TK_Eof) {
-    Error->pushErrorFrame(OpenToken.Range, Error->ET_ParserNoCloseParen);
+    Error->addError(OpenToken.Range, Error->ET_ParserNoCloseParen);
     return false;
   }
 
@@ -293,35 +288,32 @@ bool Parser::parseMatcherExpressionImpl(VariantValue *Value) {
     //       explicit about the syntax error.
     if (BindToken.Kind != TokenInfo::TK_Ident ||
         BindToken.Text != TokenInfo::ID_Bind) {
-      Error->pushErrorFrame(BindToken.Range, Error->ET_ParserMalformedBindExpr);
+      Error->addError(BindToken.Range, Error->ET_ParserMalformedBindExpr);
       return false;
     }
     if (OpenToken.Kind != TokenInfo::TK_OpenParen) {
-      Error->pushErrorFrame(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
+      Error->addError(OpenToken.Range, Error->ET_ParserMalformedBindExpr);
       return false;
     }
     if (IDToken.Kind != TokenInfo::TK_Literal || !IDToken.Value.isString()) {
-      Error->pushErrorFrame(IDToken.Range, Error->ET_ParserMalformedBindExpr);
+      Error->addError(IDToken.Range, Error->ET_ParserMalformedBindExpr);
       return false;
     }
     if (CloseToken.Kind != TokenInfo::TK_CloseParen) {
-      Error->pushErrorFrame(CloseToken.Range,
-                            Error->ET_ParserMalformedBindExpr);
+      Error->addError(CloseToken.Range, Error->ET_ParserMalformedBindExpr);
       return false;
     }
     BindID = IDToken.Value.getString();
   }
 
   // Merge the start and end infos.
+  Diagnostics::Context Ctx(Diagnostics::Context::ConstructMatcher, Error,
+                           NameToken.Text, NameToken.Range);
   SourceRange MatcherRange = NameToken.Range;
   MatcherRange.End = EndToken.Range.End;
   MatcherList Result = S->actOnMatcherExpression(
       NameToken.Text, MatcherRange, BindID, Args, Error);
-  if (Result.empty()) {
-    Error->pushErrorFrame(NameToken.Range, Error->ET_ParserMatcherFailure)
-        << NameToken.Text;
-    return false;
-  }
+  if (Result.empty()) return false;
 
   *Value = Result;
   return true;
@@ -338,8 +330,8 @@ bool Parser::parseExpressionImpl(VariantValue *Value) {
     return parseMatcherExpressionImpl(Value);
 
   case TokenInfo::TK_Eof:
-    Error->pushErrorFrame(Tokenizer->consumeNextToken().Range,
-                          Error->ET_ParserNoCode);
+    Error->addError(Tokenizer->consumeNextToken().Range,
+                    Error->ET_ParserNoCode);
     return false;
 
   case TokenInfo::TK_Error:
@@ -352,8 +344,7 @@ bool Parser::parseExpressionImpl(VariantValue *Value) {
   case TokenInfo::TK_Period:
   case TokenInfo::TK_InvalidChar:
     const TokenInfo Token = Tokenizer->consumeNextToken();
-    Error->pushErrorFrame(Token.Range, Error->ET_ParserInvalidToken)
-        << Token.Text;
+    Error->addError(Token.Range, Error->ET_ParserInvalidToken) << Token.Text;
     return false;
   }
 
@@ -392,8 +383,8 @@ bool Parser::parseExpression(StringRef Code, Sema *S,
   CodeTokenizer Tokenizer(Code, Error);
   if (!Parser(&Tokenizer, S, Error).parseExpressionImpl(Value)) return false;
   if (Tokenizer.peekNextToken().Kind != TokenInfo::TK_Eof) {
-    Error->pushErrorFrame(Tokenizer.peekNextToken().Range,
-                          Error->ET_ParserTrailingCode);
+    Error->addError(Tokenizer.peekNextToken().Range,
+                    Error->ET_ParserTrailingCode);
     return false;
   }
   return true;
@@ -412,11 +403,11 @@ DynTypedMatcher *Parser::parseMatcherExpression(StringRef Code,
   if (!parseExpression(Code, S, &Value, Error))
     return NULL;
   if (!Value.isMatchers()) {
-    Error->pushErrorFrame(SourceRange(), Error->ET_ParserNotAMatcher);
+    Error->addError(SourceRange(), Error->ET_ParserNotAMatcher);
     return NULL;
   }
   if (Value.getMatchers().matchers().size() != 1) {
-    Error->pushErrorFrame(SourceRange(), Error->ET_ParserOverloadedType)
+    Error->addError(SourceRange(), Error->ET_ParserOverloadedType)
         << Value.getTypeAsString();
     return NULL;
   }
diff --git a/clang/lib/ASTMatchers/Dynamic/Registry.cpp b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
index f523cc4abb0..6cab4073b96 100644
--- a/clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ b/clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -298,8 +298,7 @@ MatcherList Registry::constructMatcher(StringRef MatcherName,
   ConstructorMap::const_iterator it =
       RegistryData->constructors().find(MatcherName);
   if (it == RegistryData->constructors().end()) {
-    Error->pushErrorFrame(NameRange, Error->ET_RegistryNotFound)
-        << MatcherName;
+    Error->addError(NameRange, Error->ET_RegistryNotFound) << MatcherName;
     return MatcherList();
   }
 
@@ -322,7 +321,7 @@ MatcherList Registry::constructBoundMatcher(StringRef MatcherName,
       return *Bound;
     }
   }
-  Error->pushErrorFrame(NameRange, Error->ET_RegistryNotBindable);
+  Error->addError(NameRange, Error->ET_RegistryNotBindable);
   return MatcherList();
 }