FileCheck: Return parse error w/ Error & Expected

[thopre]

Summary

Make use of Error and Expected to bubble up diagnostics and force
checking of errors in the callers.

[arichardson]

Looks good to me. Just two minor comments:

This adds quite a lot of return make_error<ParseErrorInfo>(SM.GetMessage(SMLoc::getFromPointer(Foo.data()), SourceMgr::DK_Error, "...")));
Maybe adding a helper would make sense? Something like:

Error ParseErrorInfo::get(const SourceManager& SM, const char* LocPtr, const Twine& ErrMsg) {
   return make_error<ParseErrorInfo>(SM.GetMessage(SMLoc::getFromPointer(LocPtr), SourceMgr::DK_Error,  ErrMsg)));
}

Then the above can just be return ParseErrorInfo::get(SM, Foo.data(), "...");

[thopre]

• Fix bugs in unit test when running with Error checks enabled
• Make eval() functions return an Expected instead of Optional
• Create helper functions to create ParseError and its associated diagnostic
• Rename ParseErrorInfo into FileCheckParseError

[thopre]

llvm/include/llvm/Support/FileCheck.h

351–352

This can be simplified to Diagnostic.print(nullptr, OS);

llvm/lib/Support/FileCheck.cpp

619–622

Perhaps worth making match() return an error via an Expected<size_t> return value.

[thopre]

• Convert match to return Expected as well
• Simplify FileCheckErrorDiagnostic logging

[thopre]

Fix unit test

[thopre]

Replace assert(false) by llvm_unreachable

[thopre]

Fix code style issues

[thopre]

Remove "represent value" -> "store value" change

[probinson]

LGTM.

[thopre]

• Remove useless llvm_unreachable from handleAllErrors
• Move comment to apply to both ignored errors

llvm/include/llvm/Support/FileCheck.h

public:
 
   /// \returns the line number where this variable is defined.
   size_t getDefLineNumber() { return DefLineNumber; }
 };
 
 /// Type of functions evaluating a given binary operation.
 using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
 
+/// Class to represent an undefined variable error which prints that variable's
+/// name between quotes when printed.
+class FileCheckUndefVarError : public ErrorInfo<FileCheckUndefVarError> {
+private:
+  StringRef VarName;
+
+public:
+  static char ID;
+
+  FileCheckUndefVarError(StringRef VarName) : VarName(VarName) {}
+
+  StringRef getVarName() const { return VarName; }
+
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
+
+  /// Print name of variable associated with this error.
+  void log(raw_ostream &OS) const override {
+    OS << "\"";
+    OS.write_escaped(VarName) << "\"";
+  }
+};
+
 /// Class representing a numeric expression consisting of either a single
 /// numeric variable or a binary operation between a numeric variable and an
 /// immediate.
 class FileCheckNumExpr {
 private:
   /// Left operand.
   FileCheckNumericVariable *LeftOp;
 
   /// Right operand.
   uint64_t RightOp;
 
   /// Pointer to function that can evaluate this binary operation.
   binop_eval_t EvalBinop;
 
 public:
   FileCheckNumExpr(binop_eval_t EvalBinop,
                    FileCheckNumericVariable *OperandLeft, uint64_t OperandRight)
       : LeftOp(OperandLeft), RightOp(OperandRight), EvalBinop(EvalBinop) {}
 
   /// Evaluates the value of this numeric expression, using EvalBinop to
-  /// perform the binary operation it consists of. \returns None if the numeric
-  /// variable used is undefined, or the expression value otherwise.
-  Optional<uint64_t> eval() const;
-
-  /// \returns the name of the undefined variable used in this expression if
-  /// any or an empty string otherwise.
-  StringRef getUndefVarName() const;
+  /// perform the binary operation it consists of. \returns an error if the
+  /// numeric variable used is undefined, or the expression value otherwise.
+  Expected<uint64_t> eval() const;
 };
 
 class FileCheckPatternContext;
 
 /// Class representing a substitution to perform in the RegExStr string.
 class FileCheckSubstitution {
 protected:
   /// Pointer to a class instance holding, among other things, the table with
public:
 
   /// \returns the string to be substituted for something else.
   StringRef getFromString() const { return FromStr; }
 
   /// \returns the index where the substitution is to be performed in RegExStr.
   size_t getIndex() const { return InsertIdx; }
 
   /// \returns a string containing the result of the substitution represented
-  /// by this class instance or None if substitution failed.
-  virtual Optional<std::string> getResult() const = 0;
-
-  /// \returns the name of the variable used in this substitution if undefined,
-  /// or an empty string otherwise.
-  virtual StringRef getUndefVarName() const = 0;
+  /// by this class instance or an error if substitution failed.
+  virtual Expected<std::string> getResult() const = 0;
 };
 
 class FileCheckStringSubstitution : public FileCheckSubstitution {
 public:
   FileCheckStringSubstitution(FileCheckPatternContext *Context,
                               StringRef VarName, size_t InsertIdx)
       : FileCheckSubstitution(Context, VarName, InsertIdx) {}
 
   /// \returns the text that the string variable in this substitution matched
-  /// when defined, or None if the variable is undefined.
-  Optional<std::string> getResult() const override;
-
-  /// \returns the name of the string variable used in this substitution if
-  /// undefined, or an empty string otherwise.
-  StringRef getUndefVarName() const override;
+  /// when defined, or an error if the variable is undefined.
+  Expected<std::string> getResult() const override;
 };
 
 class FileCheckNumericSubstitution : public FileCheckSubstitution {
 private:
   /// Pointer to the class representing the numeric expression whose value is
   /// to be substituted.
   FileCheckNumExpr *NumExpr;
 
 public:
   FileCheckNumericSubstitution(FileCheckPatternContext *Context, StringRef Expr,
                                FileCheckNumExpr *NumExpr, size_t InsertIdx)
       : FileCheckSubstitution(Context, Expr, InsertIdx), NumExpr(NumExpr) {}
 
   /// \returns a string containing the result of evaluating the numeric
-  /// expression in this substitution, or None if evaluation failed.
-  Optional<std::string> getResult() const override;
-
-  /// \returns the name of the numeric variable used in this substitution if
-  /// undefined, or an empty string otherwise.
-  StringRef getUndefVarName() const override;
+  /// expression in this substitution, or an error if evaluation failed.
+  Expected<std::string> getResult() const override;
 };
 
 //===----------------------------------------------------------------------===//
 // Pattern handling code.
 //===----------------------------------------------------------------------===//
 
 namespace Check {
 
private:
   /// automatically free them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckNumericVariable>> NumericVariables;
 
   /// Vector holding pointers to all substitutions. Used to automatically free
   /// them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckSubstitution>> Substitutions;
 
 public:
-  /// \returns the value of string variable \p VarName or None if no such
+  /// \returns the value of string variable \p VarName or an error if no such
   /// variable has been defined.
-  Optional<StringRef> getPatternVarValue(StringRef VarName);
+  Expected<StringRef> getPatternVarValue(StringRef VarName);
 
   /// Defines string and numeric variables from definitions given on the
   /// command line, passed as a vector of [#]VAR=VAL strings in
-  /// \p CmdlineDefines. Reports any error to \p SM and \returns whether an
-  /// error occured.
-  bool defineCmdlineVariables(std::vector<std::string> &CmdlineDefines,
+  /// \p CmdlineDefines. \returns an error list containing diagnostics against
+  /// \p SM for all definition parsing failures, if any, or Success otherwise.
+  Error defineCmdlineVariables(std::vector<std::string> &CmdlineDefines,
                                SourceMgr &SM);
 
   /// Undefines local variables (variables whose name does not start with a '$'
   /// sign), i.e. removes them from GlobalVariableTable and from
   /// GlobalNumericVariableTable and also clears the value of numeric
   /// variables.
   void clearLocalVars();
 
 private:
private:
 
   /// Makes a new numeric substitution and registers it for destruction when
   /// the context is destroyed.
   FileCheckSubstitution *makeNumericSubstitution(StringRef Expr,
                                                  FileCheckNumExpr *NumExpr,
                                                  size_t InsertIdx);
 };
 
+/// Class to represent a parsing error, holding a diagnostic with location
+/// information used when printing it.
+class FileCheckParseError : public ErrorInfo<FileCheckParseError> {
+private:
+  SMDiagnostic Diagnostic;
+
+public:
+  static char ID;
+
+  FileCheckParseError(SMDiagnostic &&Diag) : Diagnostic(Diag) {}
+
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
+
+  /// Print diagnostic associated with this error when printing the error.
+  void log(raw_ostream &OS) const override {
+    Diagnostic.getSourceMgr()->PrintMessage(
+        OS, Diagnostic.getLoc(), Diagnostic.getKind(), Diagnostic.getMessage());
+  }
+
+  static Error get(const SourceMgr &SM, SMLoc Loc, const Twine &ErrMsg) {
+    return make_error<FileCheckParseError>(
+        SM.GetMessage(Loc, SourceMgr::DK_Error, ErrMsg));
+  }
+
+  static Error get(const SourceMgr &SM, StringRef Buffer, const Twine &ErrMsg) {
+    return get(SM, SMLoc::getFromPointer(Buffer.data()), ErrMsg);
+  }
+};
+
 class FileCheckPattern {
   SMLoc PatternLoc;
 
   /// A fixed string to match as the pattern or empty if this pattern requires
   /// a regex match.
   StringRef FixedStr;
 
   /// A regex string to match as the pattern or empty if this pattern requires
public:
   SMLoc getLoc() const { return PatternLoc; }
 
   /// \returns the pointer to the global state for all patterns in this
   /// FileCheck instance.
   FileCheckPatternContext *getContext() const { return Context; }
 
   /// \returns whether \p C is a valid first character for a variable name.
   static bool isValidVarNameStart(char C);
-  /// Parses the string at the start of \p Str for a variable name and \returns
-  /// whether the variable name is ill-formed. If parsing succeeded, sets
-  /// \p IsPseudo to indicate if it is a pseudo variable, sets \p Name to the
-  /// parsed variable name and strips \p Str from the variable name.
-  static bool parseVariable(StringRef &Str, StringRef &Name, bool &IsPseudo);
-  /// Parses \p Expr for the definition of a numeric variable, returning an
-  /// error if \p Context already holds a string variable with the same name.
-  /// \returns whether parsing fails, in which case errors are reported on
-  /// \p SM. Otherwise, sets \p Name to the name of the parsed numeric
-  /// variable.
-  static bool parseNumericVariableDefinition(StringRef &Expr, StringRef &Name,
+  /// Parses the string at the start of \p Str for a variable name. \returns
+  /// an error holding a diagnostic against \p SM if parsing fail, or the
+  /// name of the variable otherwise. In the latter case, sets \p IsPseudo to
+  /// indicate if it is a pseudo variable and strips \p Str from the variable
+  /// name.
+  static Expected<StringRef> parseVariable(StringRef &Str, bool &IsPseudo,
+                                           const SourceMgr &SM);
+  /// Parses \p Expr for the name of a numeric variable to be defined. \returns
+  /// an error holding a diagnostic against \p SM should defining such a
+  /// variable be invalid, or Success otherwise. In the latter case, sets
+  /// \p Name to the name of the parsed numeric variable name.
+  static Error parseNumericVariableDefinition(StringRef &Expr, StringRef &Name,
                                               FileCheckPatternContext *Context,
                                               const SourceMgr &SM);
   /// Parses \p Expr for a numeric substitution block. \returns the class
   /// representing the AST of the numeric expression whose value must be
-  /// substituted, or nullptr if parsing fails, in which case errors are
-  /// reported on \p SM. Sets \p DefinedNumericVariable to point to the class
-  /// representing the numeric variable defined in this numeric substitution
-  /// block, or nullptr if this block does not define any variable.
-  FileCheckNumExpr *parseNumericSubstitutionBlock(
-      StringRef Expr, FileCheckNumericVariable *&DefinedNumericVariable,
+  /// substituted, or an error holding a diagnostic against \p SM if parsing
+  /// fails. If substitution was successful, sets \p DefinedNumericVariable to
+  /// point to the class representing the numeric variable defined in this
+  /// numeric substitution block, or None if this block does not define any
+  /// variable.
+  Expected<FileCheckNumExpr *> parseNumericSubstitutionBlock(
+      StringRef Expr,
+      Optional<FileCheckNumericVariable *> &DefinedNumericVariable,
       const SourceMgr &SM) const;
   /// Parses the pattern in \p PatternStr and initializes this FileCheckPattern
   /// instance accordingly.
   ///
   /// \p Prefix provides which prefix is being matched, \p Req describes the
   /// global options that influence the parsing such as whitespace
   /// canonicalization, \p SM provides the SourceMgr used for error reports.
   /// \returns true in case of an error, false otherwise.
private:
   ///
   /// \p Str has to point in the beginning of the definition (right after the
   /// opening sequence). \p SM holds the SourceMgr used for error repporting.
   ///  \returns the offset of the closing sequence within Str, or npos if it
   /// was not found.
   size_t FindRegexVarEnd(StringRef Str, SourceMgr &SM);
 
   /// Parses \p Expr for the use of a numeric variable. \returns the pointer to
-  /// the class instance representing that variable if successful, or nullptr
-  /// otherwise, in which case errors are reported on \p SM.
-  FileCheckNumericVariable *parseNumericVariableUse(StringRef &Expr,
-                                                    const SourceMgr &SM) const;
+  /// the class instance representing that variable if successful, or an error
+  /// holding a diagnostic against \p SM otherwise.
+  Expected<FileCheckNumericVariable *>
+  parseNumericVariableUse(StringRef &Expr, const SourceMgr &SM) const;
   /// Parses \p Expr for a binary operation.
   /// \returns the class representing the binary operation of the numeric
-  /// expression, or nullptr if parsing fails, in which case errors are
-  /// reported on \p SM.
-  FileCheckNumExpr *parseBinop(StringRef &Expr, const SourceMgr &SM) const;
+  /// expression, or an error holding a diagnostic against \p SM otherwise.
+  Expected<FileCheckNumExpr *> parseBinop(StringRef &Expr,
+                                          const SourceMgr &SM) const;
 };
 
 //===----------------------------------------------------------------------===//
 /// Summary of a FileCheck diagnostic.
 //===----------------------------------------------------------------------===//
 
 struct FileCheckDiag {
   /// What is the FileCheck directive for this diagnostic?

llvm/lib/Support/FileCheck.cpp

 
 bool FileCheckNumericVariable::clearValue() {
   if (!Value)
     return true;
   Value = None;
   return false;
 }
 
-Optional<uint64_t> FileCheckNumExpr::eval() const {
+Expected<uint64_t> FileCheckNumExpr::eval() const {
   assert(LeftOp && "Evaluating an empty numeric expression");
   Optional<uint64_t> LeftOpValue = LeftOp->getValue();
   // Variable is undefined.
   if (!LeftOpValue)
-    return None;
+    return make_error<FileCheckUndefVarError>(LeftOp->getName());
   return EvalBinop(*LeftOpValue, RightOp);
 }
 
-StringRef FileCheckNumExpr::getUndefVarName() const {
-  if (!LeftOp->getValue())
-    return LeftOp->getName();
-  return StringRef();
-}
-
-Optional<std::string> FileCheckNumericSubstitution::getResult() const {
-  Optional<uint64_t> EvaluatedValue = NumExpr->eval();
+Expected<std::string> FileCheckNumericSubstitution::getResult() const {
+  Expected<uint64_t> EvaluatedValue = NumExpr->eval();
   if (!EvaluatedValue)
-    return None;
+    return EvaluatedValue.takeError();
   return utostr(*EvaluatedValue);
 }
 
-Optional<std::string> FileCheckStringSubstitution::getResult() const {
+Expected<std::string> FileCheckStringSubstitution::getResult() const {
   // Look up the value and escape it so that we can put it into the regex.
-  Optional<StringRef> VarVal = Context->getPatternVarValue(FromStr);
+  Expected<StringRef> VarVal = Context->getPatternVarValue(FromStr);
   if (!VarVal)
-    return None;
+    return VarVal.takeError();
   return Regex::escape(*VarVal);
 }
 
-StringRef FileCheckNumericSubstitution::getUndefVarName() const {
-  // Although a use of an undefined numeric variable is detected at parse
-  // time, a numeric variable can be undefined later by ClearLocalVariables.
-  return NumExpr->getUndefVarName();
-}
-
-StringRef FileCheckStringSubstitution::getUndefVarName() const {
-  if (!Context->getPatternVarValue(FromStr))
-    return FromStr;
-
-  return StringRef();
-}
-
 bool FileCheckPattern::isValidVarNameStart(char C) {
   return C == '_' || isalpha(C);
 }
 
-bool FileCheckPattern::parseVariable(StringRef &Str, StringRef &Name,
-                                     bool &IsPseudo) {
+Expected<StringRef> FileCheckPattern::parseVariable(StringRef &Str,
+                                                    bool &IsPseudo,
+                                                    const SourceMgr &SM) {
   if (Str.empty())
-    return true;
+    return FileCheckParseError::get(SM, Str, "empty variable name");
 
   bool ParsedOneChar = false;
   unsigned I = 0;
   IsPseudo = Str[0] == '@';
 
   // Global vars start with '$'.
   if (Str[0] == '$' || IsPseudo)
     ++I;
 
   for (unsigned E = Str.size(); I != E; ++I) {
     if (!ParsedOneChar && !isValidVarNameStart(Str[I]))
-      return true;
+      return FileCheckParseError::get(SM, Str, "invalid variable name");
 
     // Variable names are composed of alphanumeric characters and underscores.
     if (Str[I] != '_' && !isalnum(Str[I]))
       break;
     ParsedOneChar = true;
   }
 
-  Name = Str.take_front(I);
+  StringRef Name = Str.take_front(I);
   Str = Str.substr(I);
-  return false;
+  return Name;
 }
 
 // StringRef holding all characters considered as horizontal whitespaces by
 // FileCheck input canonicalization.
 StringRef SpaceChars = " \t";
 
 // Parsing helper function that strips the first character in S and returns it.
 static char popFront(StringRef &S) {
   char C = S.front();
   S = S.drop_front();
   return C;
 }
 
-bool FileCheckPattern::parseNumericVariableDefinition(
+char FileCheckParseError::ID = 0;
+char FileCheckUndefVarError::ID = 0;
+
+Error FileCheckPattern::parseNumericVariableDefinition(
     StringRef &Expr, StringRef &Name, FileCheckPatternContext *Context,
     const SourceMgr &SM) {
   bool IsPseudo;
-  if (parseVariable(Expr, Name, IsPseudo)) {
-    SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
-                    "invalid variable name");
-    return true;
-  }
-
-  if (IsPseudo) {
-    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                    "definition of pseudo numeric variable unsupported");
-    return true;
-  }
+  Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+  if (!ParseVarResult)
+    return ParseVarResult.takeError();
+  Name = *ParseVarResult;
+
+  if (IsPseudo)
+    return FileCheckParseError::get(
+        SM, Name, "definition of pseudo numeric variable unsupported");
 
   // Detect collisions between string and numeric variables when the latter
   // is created later than the former.
   if (Context->DefinedVariableTable.find(Name) !=
-      Context->DefinedVariableTable.end()) {
-    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                    "string variable with name '" + Name + "' already exists");
-    return true;
-  }
+      Context->DefinedVariableTable.end())
+    return FileCheckParseError::get(
+        SM, Name, "string variable with name '" + Name + "' already exists");
 
-  return false;
+  return Error::success();
 }
 
-FileCheckNumericVariable *
+Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
                                           const SourceMgr &SM) const {
   bool IsPseudo;
-  StringRef Name;
-  if (parseVariable(Expr, Name, IsPseudo)) {
-    SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
-                    "invalid variable name");
-    return nullptr;
-  }
-
-  if (IsPseudo && !Name.equals("@LINE")) {
-    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                    "invalid pseudo numeric variable '" + Name + "'");
-    return nullptr;
-  }
+  Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+  if (!ParseVarResult)
+    return ParseVarResult.takeError();
+  StringRef Name = *ParseVarResult;
+
+  if (IsPseudo && !Name.equals("@LINE"))
+    return FileCheckParseError::get(
+        SM, Name, "invalid pseudo numeric variable '" + Name + "'");
 
   // This method is indirectly called from parsePattern for all numeric
   // variable definitions and uses in the order in which they appear in the
   // CHECK pattern. For each definition, the pointer to the class instance of
   // the corresponding numeric variable definition is stored in
   // GlobalNumericVariableTable. Therefore, the pointer we get below is for the
   // class instance corresponding to the last definition of this variable use.
   auto VarTableIter = Context->GlobalNumericVariableTable.find(Name);
-  if (VarTableIter == Context->GlobalNumericVariableTable.end()) {
-    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                    "using undefined numeric variable '" + Name + "'");
-    return nullptr;
-  }
+  if (VarTableIter == Context->GlobalNumericVariableTable.end())
+    return FileCheckParseError::get(
+        SM, Name, "using undefined numeric variable '" + Name + "'");
 
   FileCheckNumericVariable *NumericVariable = VarTableIter->second;
-  if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber) {
-    SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
+  if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber)
+    return FileCheckParseError::get(SM, Name,
                                     "numeric variable '" + Name +
                                         "' defined on the same line as used");
-    return nullptr;
-  }
 
   return NumericVariable;
 }
 
 static uint64_t add(uint64_t LeftOp, uint64_t RightOp) {
   return LeftOp + RightOp;
 }
 
 static uint64_t sub(uint64_t LeftOp, uint64_t RightOp) {
   return LeftOp - RightOp;
 }
 
-FileCheckNumExpr *FileCheckPattern::parseBinop(StringRef &Expr,
-                                               const SourceMgr &SM) const {
-  FileCheckNumericVariable *LeftOp = parseNumericVariableUse(Expr, SM);
-  if (!LeftOp) {
-    // Error reporting done in parseNumericVariableUse().
-    return nullptr;
+Expected<FileCheckNumExpr *>
+FileCheckPattern::parseBinop(StringRef &Expr, const SourceMgr &SM) const {
+  Expected<FileCheckNumericVariable *> LeftParseResult =
+      parseNumericVariableUse(Expr, SM);
+  if (!LeftParseResult) {
+    return LeftParseResult.takeError();
   }
+  FileCheckNumericVariable *LeftOp = *LeftParseResult;
 
   // Check if this is a supported operation and select a function to perform
   // it.
   Expr = Expr.ltrim(SpaceChars);
   if (Expr.empty())
     return Context->makeNumExpr(add, LeftOp, 0);
   SMLoc OpLoc = SMLoc::getFromPointer(Expr.data());
   char Operator = popFront(Expr);
   binop_eval_t EvalBinop;
   switch (Operator) {
   case '+':
     EvalBinop = add;
     break;
   case '-':
     EvalBinop = sub;
     break;
   default:
-    SM.PrintMessage(OpLoc, SourceMgr::DK_Error,
-                    Twine("unsupported numeric operation '") + Twine(Operator) +
-                        "'");
-    return nullptr;
+    return FileCheckParseError::get(SM, OpLoc,
+                                    Twine("unsupported numeric operation '") +
+                                        Twine(Operator) + "'");
   }
 
   // Parse right operand.
   Expr = Expr.ltrim(SpaceChars);
-  if (Expr.empty()) {
-    SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
+  if (Expr.empty())
+    return FileCheckParseError::get(SM, Expr,
                                     "missing operand in numeric expression");
-    return nullptr;
-  }
   uint64_t RightOp;
-  if (Expr.consumeInteger(10, RightOp)) {
-    SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
-                    "invalid offset in numeric expression '" + Expr + "'");
-    return nullptr;
-  }
+  if (Expr.consumeInteger(10, RightOp))
+    return FileCheckParseError::get(
+        SM, Expr, "invalid offset in numeric expression '" + Expr + "'");
   Expr = Expr.ltrim(SpaceChars);
-  if (!Expr.empty()) {
-    SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
-                    "unexpected characters at end of numeric expression '" +
-                        Expr + "'");
-    return nullptr;
-  }
+  if (!Expr.empty())
+    return FileCheckParseError::get(
+        SM, Expr,
+        "unexpected characters at end of numeric expression '" + Expr + "'");
 
   return Context->makeNumExpr(EvalBinop, LeftOp, RightOp);
 }
 
-FileCheckNumExpr *FileCheckPattern::parseNumericSubstitutionBlock(
-    StringRef Expr, FileCheckNumericVariable *&DefinedNumericVariable,
+Expected<FileCheckNumExpr *> FileCheckPattern::parseNumericSubstitutionBlock(
+    StringRef Expr,
+    Optional<FileCheckNumericVariable *> &DefinedNumericVariable,
     const SourceMgr &SM) const {
   // Parse the numeric variable definition.
-  DefinedNumericVariable = nullptr;
+  DefinedNumericVariable = None;
   size_t DefEnd = Expr.find(':');
   if (DefEnd != StringRef::npos) {
     StringRef DefExpr = Expr.substr(0, DefEnd);
     StringRef UseExpr = Expr = Expr.substr(DefEnd + 1);
 
     DefExpr = DefExpr.ltrim(SpaceChars);
     StringRef Name;
-    if (parseNumericVariableDefinition(DefExpr, Name, Context, SM)) {
-      // Invalid variable definition. Error reporting done in parsing function.
-      return nullptr;
-    }
+    Error ParseError =
+        parseNumericVariableDefinition(DefExpr, Name, Context, SM);
+    if (ParseError)
+      return std::move(ParseError);
 
     DefinedNumericVariable =
         Context->makeNumericVariable(this->LineNumber, Name);
 
     DefExpr = DefExpr.ltrim(SpaceChars);
-    if (!DefExpr.empty()) {
-      SM.PrintMessage(SMLoc::getFromPointer(DefExpr.data()),
-                      SourceMgr::DK_Error,
+    if (!DefExpr.empty())
+      return FileCheckParseError::get(SM, DefExpr,
                                       "invalid numeric variable definition");
-      return nullptr;
-    }
     UseExpr = UseExpr.ltrim(SpaceChars);
-    if (!UseExpr.empty()) {
-      SM.PrintMessage(
-          SMLoc::getFromPointer(UseExpr.data()), SourceMgr::DK_Error,
+    if (!UseExpr.empty())
+      return FileCheckParseError::get(
+          SM, UseExpr,
           "unexpected string after variable definition: '" + UseExpr + "'");
-      return nullptr;
-    }
     return Context->makeNumExpr(add, nullptr, 0);
   }
 
   // Parse the numeric expression itself.
   Expr = Expr.ltrim(SpaceChars);
   return parseBinop(Expr, SM);
 }
 
if (PatternStr.startswith("[[")) {
         if (SpacePos != StringRef::npos) {
           SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data() + SpacePos),
                           SourceMgr::DK_Error, "unexpected whitespace");
           return true;
         }
 
         // Get the name (e.g. "foo") and verify it is well formed.
         bool IsPseudo;
-        StringRef Name;
         StringRef OrigMatchStr = MatchStr;
-        if (parseVariable(MatchStr, Name, IsPseudo)) {
-          SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data()),
-                          SourceMgr::DK_Error, "invalid variable name");
+        Expected<StringRef> ParseVarResult =
+            parseVariable(MatchStr, IsPseudo, SM);
+        if (!ParseVarResult) {
+          logAllUnhandledErrors(ParseVarResult.takeError(), errs());
           return true;
         }
+        StringRef Name = *ParseVarResult;
 
         IsDefinition = (VarEndIdx != StringRef::npos);
         if (IsDefinition) {
           if ((IsPseudo || !MatchStr.consume_front(":"))) {
             SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
                             SourceMgr::DK_Error,
                             "invalid name in string variable definition");
             return true;
if (PatternStr.startswith("[[")) {
             IsNumBlock = true;
           } else
             SubstStr = Name;
         }
       }
 
       // Parse numeric substitution block.
       FileCheckNumExpr *NumExpr;
-      FileCheckNumericVariable *DefinedNumericVariable;
+      Optional<FileCheckNumericVariable *> DefinedNumericVariable;
       if (IsNumBlock) {
-        NumExpr =
+        Expected<FileCheckNumExpr *> ParseResult =
             parseNumericSubstitutionBlock(MatchStr, DefinedNumericVariable, SM);
-        if (NumExpr == nullptr)
+        if (!ParseResult) {
+          logAllUnhandledErrors(ParseResult.takeError(), errs());
           return true;
+        }
+        NumExpr = *ParseResult;
         if (DefinedNumericVariable) {
           IsDefinition = true;
-          DefName = DefinedNumericVariable->getName();
+          DefName = (*DefinedNumericVariable)->getName();
           MatchRegexp = StringRef("[0-9]+");
         } else
           SubstStr = MatchStr;
       }
 
       // Handle substitutions: [[foo]] and [[#<foo expr>]].
       if (!IsDefinition) {
         // Handle substitution of string variables that were defined earlier on
if (PatternStr.startswith("[[")) {
           Substitutions.push_back(Substitution);
         }
         continue;
       }
 
       // Handle variable definitions: [[<def>:(...)]] and
       // [[#(...)<def>:(...)]].
       if (IsNumBlock) {
-        FileCheckNumExprMatch NumExprDef = {DefinedNumericVariable, CurParen};
+        FileCheckNumExprMatch NumExprDef = {*DefinedNumericVariable, CurParen};
         NumericVariableDefs[DefName] = NumExprDef;
         // This store is done here rather than in match() to allow
         // parseNumericVariableUse() to get the pointer to the class instance
         // of the right variable definition corresponding to a given numeric
         // variable use.
-        Context->GlobalNumericVariableTable[DefName] = DefinedNumericVariable;
+        Context->GlobalNumericVariableTable[DefName] = *DefinedNumericVariable;
       } else {
         VariableDefs[DefName] = CurParen;
         // Mark the string variable as defined to detect collisions between
         // string and numeric variables in parseNumericVariableUse() and
         // DefineCmdlineVariables() when the latter is created later than the
         // former. We cannot reuse GlobalVariableTable for this by populating
         // it with an empty string since we would then lose the ability to
         // detect the use of an undefined variable in match().
if (!Substitutions.empty()) {
     TmpStr = RegExStr;
 
     size_t InsertOffset = 0;
     // Substitute all string variables and numeric expressions whose values are
     // only now known. Use of string variables defined on the same line are
     // handled by back-references.
     for (const auto &Substitution : Substitutions) {
       // Substitute and check for failure (e.g. use of undefined variable).
-      Optional<std::string> Value = Substitution->getResult();
-      if (!Value)
+      Expected<std::string> Value = Substitution->getResult();
+      if (!Value) {
+        // Errors are handled in printSubstitutions().
+        Error SubstError = Value.takeError();
+        assert(SubstError.isA<FileCheckUndefVarError>() && "Unexpected error");
+        consumeError(std::move(SubstError));
         return StringRef::npos;
+      }
 
       // Plop it into the regex at the adjusted offset.
       TmpStr.insert(TmpStr.begin() + Substitution->getIndex() + InsertOffset,
                     Value->begin(), Value->end());
       InsertOffset += Value->size();
     }
 
     // Match the newly constructed regex.
const FileCheckNumExprMatch &NumericVariableMatch =
         NumericVariableDef.getValue();
     unsigned CaptureParenGroup = NumericVariableMatch.CaptureParenGroup;
     assert(CaptureParenGroup < MatchInfo.size() && "Internal paren error");
     FileCheckNumericVariable *DefinedNumericVariable =
         NumericVariableMatch.DefinedNumericVariable;
 
     StringRef MatchedValue = MatchInfo[CaptureParenGroup];
     uint64_t Val;
     if (MatchedValue.getAsInteger(10, Val)) {
       SM.PrintMessage(SMLoc::getFromPointer(MatchedValue.data()),
                       SourceMgr::DK_Error, "Unable to represent numeric value");
     }
     if (DefinedNumericVariable->setValue(Val))
       assert(false && "Numeric variable redefined");
   }
 
   // Like CHECK-NEXT, CHECK-EMPTY's match range is considered to start after
   // the required preceding newline, which is consumed by the pattern in the
   // case of CHECK-EMPTY but not CHECK-NEXT.
   size_t MatchStartSkip = CheckTy == Check::CheckEmpty;

 
 void FileCheckPattern::printSubstitutions(const SourceMgr &SM, StringRef Buffer,
                                           SMRange MatchRange) const {
   // Print what we know about substitutions.
   if (!Substitutions.empty()) {
     for (const auto &Substitution : Substitutions) {
       SmallString<256> Msg;
       raw_svector_ostream OS(Msg);
-      Optional<std::string> MatchedValue = Substitution->getResult();
+      Expected<std::string> MatchedValue = Substitution->getResult();
 
       // Substitution failed or is not known at match time, print the undefined
       // variable it uses.
       if (!MatchedValue) {
-        StringRef UndefVarName = Substitution->getUndefVarName();
-        if (UndefVarName.empty())
-          continue;
-        OS << "uses undefined variable \"";
-        OS.write_escaped(UndefVarName) << "\"";
+        Error UndefVarError = MatchedValue.takeError();
+        assert(UndefVarError.isA<FileCheckUndefVarError>() &&
+               "Unexpected error");
+        logAllUnhandledErrors(std::move(UndefVarError), OS,
+                              "uses undefined variable ");
       } else {
         // Substitution succeeded. Print substituted value.
         OS << "with \"";
         OS.write_escaped(Substitution->getFromString()) << "\" equal to \"";
         OS.write_escaped(*MatchedValue) << "\"";
       }
 
       if (MatchRange.isValid())
if (Best && Best != StringRef::npos && BestQuality < 50) {
     SM.PrintMessage(MatchRange.Start, SourceMgr::DK_Note,
                     "possible intended match here");
 
     // FIXME: If we wanted to be really friendly we would show why the match
     // failed, as it can be hard to spot simple one character differences.
   }
 }
 
-Optional<StringRef>
+Expected<StringRef>
 FileCheckPatternContext::getPatternVarValue(StringRef VarName) {
   auto VarIter = GlobalVariableTable.find(VarName);
   if (VarIter == GlobalVariableTable.end())
-    return None;
+    return make_error<FileCheckUndefVarError>(VarName);
 
   return VarIter->second;
 }
 
 FileCheckNumExpr *
 FileCheckPatternContext::makeNumExpr(binop_eval_t EvalBinop,
                                      FileCheckNumericVariable *OperandLeft,
                                      uint64_t OperandRight) {
FindFirstMatchingPrefix(Regex &PrefixRE, StringRef &Buffer,
   }
 
   // We ran out of buffer while skipping partial matches so give up.
   return {StringRef(), StringRef()};
 }
 
 bool FileCheck::ReadCheckFile(SourceMgr &SM, StringRef Buffer, Regex &PrefixRE,
                               std::vector<FileCheckString> &CheckStrings) {
-  if (PatternContext.defineCmdlineVariables(Req.GlobalDefines, SM))
+  Error DefineError =
+      PatternContext.defineCmdlineVariables(Req.GlobalDefines, SM);
+  if (DefineError) {
+    logAllUnhandledErrors(std::move(DefineError), errs());
     return true;
+  }
 
   std::vector<FileCheckPattern> ImplicitNegativeChecks;
   for (const auto &PatternString : Req.ImplicitCheckNot) {
     // Create a buffer with fake command line content in order to display the
     // command line option responsible for the specific implicit CHECK-NOT.
     std::string Prefix = "-implicit-check-not='";
     std::string Suffix = "'";
     std::unique_ptr<MemoryBuffer> CmdLine = MemoryBuffer::getMemBufferCopy(
if (Prefix != Req.CheckPrefixes.front())
       PrefixRegexStr.push_back('|');
 
     PrefixRegexStr.append(Prefix);
   }
 
   return Regex(PrefixRegexStr);
 }
 
-bool FileCheckPatternContext::defineCmdlineVariables(
+Error FileCheckPatternContext::defineCmdlineVariables(
     std::vector<std::string> &CmdlineDefines, SourceMgr &SM) {
   assert(GlobalVariableTable.empty() && GlobalNumericVariableTable.empty() &&
          "Overriding defined variable with command-line variable definitions");
 
   if (CmdlineDefines.empty())
-    return false;
+    return Error::success();
 
   // Create a string representing the vector of command-line definitions. Each
   // definition is on its own line and prefixed with a definition number to
   // clarify which definition a given diagnostic corresponds to.
   unsigned I = 0;
-  bool ErrorFound = false;
+  Error Errs = Error::success();
   std::string CmdlineDefsDiag;
   StringRef Prefix1 = "Global define #";
   StringRef Prefix2 = ": ";
   for (StringRef CmdlineDef : CmdlineDefines)
     CmdlineDefsDiag +=
         (Prefix1 + Twine(++I) + Prefix2 + CmdlineDef + "\n").str();
 
   // Create a buffer with fake command line content in order to display
   // parsing diagnostic with location information and point to the
   // global definition with invalid syntax.
   std::unique_ptr<MemoryBuffer> CmdLineDefsDiagBuffer =
       MemoryBuffer::getMemBufferCopy(CmdlineDefsDiag, "Global defines");
   StringRef CmdlineDefsDiagRef = CmdLineDefsDiagBuffer->getBuffer();
   SM.AddNewSourceBuffer(std::move(CmdLineDefsDiagBuffer), SMLoc());
 
   SmallVector<StringRef, 4> CmdlineDefsDiagVec;
   CmdlineDefsDiagRef.split(CmdlineDefsDiagVec, '\n', -1 /*MaxSplit*/,
                            false /*KeepEmpty*/);
   for (StringRef CmdlineDefDiag : CmdlineDefsDiagVec) {
     unsigned DefStart = CmdlineDefDiag.find(Prefix2) + Prefix2.size();
     StringRef CmdlineDef = CmdlineDefDiag.substr(DefStart);
     size_t EqIdx = CmdlineDef.find('=');
     if (EqIdx == StringRef::npos) {
-      SM.PrintMessage(SMLoc::getFromPointer(CmdlineDef.data()),
-                      SourceMgr::DK_Error,
-                      "Missing equal sign in global definition");
-      ErrorFound = true;
+      Errs = joinErrors(
+          std::move(Errs),
+          FileCheckParseError::get(SM, CmdlineDef,
+                                   "missing equal sign in global definition"));
       continue;
     }
 
     // Numeric variable definition.
     if (CmdlineDef[0] == '#') {
       StringRef CmdlineName = CmdlineDef.substr(1, EqIdx - 1);
       StringRef VarName;
       SMLoc CmdlineNameLoc = SMLoc::getFromPointer(CmdlineName.data());
-      bool ParseError = FileCheckPattern::parseNumericVariableDefinition(
+      Error ParseError = FileCheckPattern::parseNumericVariableDefinition(
           CmdlineName, VarName, this, SM);
-      // Check that CmdlineName starts with a valid numeric variable to be
-      // defined and that it is not followed that anything. That second check
-      // detects cases like "FOO+2" in a "FOO+2=10" definition.
-      if (ParseError || !CmdlineName.empty()) {
-        if (!ParseError)
-          SM.PrintMessage(CmdlineNameLoc, SourceMgr::DK_Error,
-                          "invalid variable name");
-        ErrorFound = true;
+      if (ParseError) {
+        Errs = joinErrors(std::move(Errs), std::move(ParseError));
+        continue;
+      }
+      // Check that CmdlineName is only composed of the parsed numeric
+      // variable. This catches cases like "FOO+2" in a "FOO+2=10" definition.
+      if (!CmdlineName.empty()) {
+        Errs = joinErrors(std::move(Errs),
+                          FileCheckParseError::get(SM, CmdlineNameLoc,
+                                                   "invalid variable name"));
         continue;
       }
 
       // Detect collisions between string and numeric variables when the latter
       // is created later than the former.
       if (DefinedVariableTable.find(VarName) != DefinedVariableTable.end()) {
-        SM.PrintMessage(
-            SMLoc::getFromPointer(VarName.data()), SourceMgr::DK_Error,
-            "string variable with name '" + VarName + "' already exists");
-        ErrorFound = true;
+        Errs = joinErrors(
+            std::move(Errs),
+            FileCheckParseError::get(SM, VarName,
+                                     "string variable with name '" + VarName +
+                                         "' already exists"));
         continue;
       }
 
       StringRef CmdlineVal = CmdlineDef.substr(EqIdx + 1);
       uint64_t Val;
       if (CmdlineVal.getAsInteger(10, Val)) {
-        SM.PrintMessage(SMLoc::getFromPointer(CmdlineVal.data()),
-                        SourceMgr::DK_Error,
+        Errs = joinErrors(std::move(Errs),
+                          FileCheckParseError::get(
+                              SM, CmdlineVal,
                               "invalid value in numeric variable definition '" +
-                            CmdlineVal + "'");
-        ErrorFound = true;
+                                  CmdlineVal + "'"));
         continue;
       }
       auto DefinedNumericVariable = makeNumericVariable(0, VarName);
       DefinedNumericVariable->setValue(Val);
 
       // Record this variable definition.
       GlobalNumericVariableTable[DefinedNumericVariable->getName()] =
           DefinedNumericVariable;
     } else {
       // String variable definition.
       std::pair<StringRef, StringRef> CmdlineNameVal = CmdlineDef.split('=');
       StringRef CmdlineName = CmdlineNameVal.first;
       StringRef OrigCmdlineName = CmdlineName;
-      StringRef Name;
       bool IsPseudo;
-      if (FileCheckPattern::parseVariable(CmdlineName, Name, IsPseudo) ||
-          IsPseudo || !CmdlineName.empty()) {
-        SM.PrintMessage(SMLoc::getFromPointer(OrigCmdlineName.data()),
-                        SourceMgr::DK_Error,
+      Expected<StringRef> ParseVarResult =
+          FileCheckPattern::parseVariable(CmdlineName, IsPseudo, SM);
+      if (!ParseVarResult) {
+        Errs = joinErrors(std::move(Errs), ParseVarResult.takeError());
+        continue;
+      }
+      // Check that CmdlineName does not denote a pseudo variable is only
+      // composed of the parsed numeric variable. This catches cases like
+      // "FOO+2" in a "FOO+2=10" definition.
+      if (IsPseudo || !CmdlineName.empty()) {
+        Errs = joinErrors(std::move(Errs),
+                          FileCheckParseError::get(
+                              SM, OrigCmdlineName,
                               "invalid name in string variable definition '" +
-                            OrigCmdlineName + "'");
-        ErrorFound = true;
+                                  OrigCmdlineName + "'"));
         continue;
       }
+      StringRef Name = *ParseVarResult;
 
       // Detect collisions between string and numeric variables when the former
       // is created later than the latter.
       if (GlobalNumericVariableTable.find(Name) !=
           GlobalNumericVariableTable.end()) {
-        SM.PrintMessage(SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
-                        "numeric variable with name '" + Name +
-                            "' already exists");
-        ErrorFound = true;
+        Errs =
+            joinErrors(std::move(Errs),
+                       FileCheckParseError::get(SM, Name,
+                                                "numeric variable with name '" +
+                                                    Name + "' already exists"));
         continue;
       }
       GlobalVariableTable.insert(CmdlineNameVal);
       // Mark the string variable as defined to detect collisions between
       // string and numeric variables in DefineCmdlineVariables when the latter
       // is created later than the former. We cannot reuse GlobalVariableTable
       // for this by populating it with an empty string since we would then
       // lose the ability to detect the use of an undefined variable in
       // match().
       DefinedVariableTable[Name] = true;
     }
   }
 
-  return ErrorFound;
+  return Errs;
 }
 
 void FileCheckPatternContext::clearLocalVars() {
   SmallVector<StringRef, 16> LocalPatternVars, LocalNumericVars;
   for (const StringMapEntry<StringRef> &Var : GlobalVariableTable)
     if (Var.first()[0] != '$')
       LocalPatternVars.push_back(Var.first());
 

llvm/test/FileCheck/string-defines-diagnostics.txt

 RUN:   | FileCheck %s --check-prefix ERRCLIVAR2
 
 ERRCLIVAR2: Missing variable name in command-line definition '-D='
 
 ; Invalid variable name: starts with a digit.
 RUN: not FileCheck -D10VALUE=10 --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix ERRCLIFMT
 
-ERRCLIFMT: Global defines:1:19: error: invalid name in string variable definition '10VALUE'
+ERRCLIFMT: Global defines:1:19: error: invalid variable name
 ERRCLIFMT-NEXT: Global define #1: 10VALUE=10
 ERRCLIFMT-NEXT: {{^                  \^$}}
 
 ; Invalid definition of pseudo variable.
 RUN: not FileCheck -D@VALUE=10 --input-file %s %s 2>&1 \
 RUN:   | FileCheck %s --strict-whitespace --check-prefix ERRCLIPSEUDO
 
 ERRCLIPSEUDO: Global defines:1:19: error: invalid name in string variable definition '@VALUE'

llvm/unittests/Support/FileCheckTest.cpp

TEST_F(FileCheckTest, NumericVariable) {
   EXPECT_FALSE(FooVar.clearValue());
   Value = FooVar.getValue();
   EXPECT_FALSE(Value);
   EXPECT_TRUE(FooVar.clearValue());
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
+static void expectUndefError(const Twine &ExpectedStr, Error Err) {
+  handleAllErrors(std::move(Err), [&](const FileCheckUndefVarError &E) {
+    EXPECT_EQ(ExpectedStr.str(), E.getVarName());
+  });
+}
+
 TEST_F(FileCheckTest, NumExpr) {
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
   FileCheckNumExpr NumExpr = FileCheckNumExpr(doAdd, &FooVar, 18);
 
-  // Defined variable: eval returns right value, no undefined variable
-  // returned.
-  llvm::Optional<uint64_t> Value = NumExpr.eval();
-  EXPECT_TRUE(Value);
+  // Defined variable: eval returns right value.
+  Expected<uint64_t> Value = NumExpr.eval();
+  EXPECT_TRUE(static_cast<bool>(Value));
   EXPECT_EQ(60U, *Value);
-  StringRef UndefVar = NumExpr.getUndefVarName();
-  EXPECT_EQ("", UndefVar);
 
   // Undefined variable: eval fails, undefined variable returned. We call
   // getUndefVarName first to check that it can be called without calling
   // eval() first.
   FooVar.clearValue();
-  UndefVar = NumExpr.getUndefVarName();
-  EXPECT_EQ("FOO", UndefVar);
-  Value = NumExpr.eval();
-  EXPECT_FALSE(Value);
+  Error EvalError = NumExpr.eval().takeError();
+  EXPECT_TRUE(errorToBool(std::move(EvalError)));
+  expectUndefError("FOO", std::move(EvalError));
 }
 
 TEST_F(FileCheckTest, ValidVarNameStart) {
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('a'));
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('G'));
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('_'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('2'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('$'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('@'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('+'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('-'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart(':'));
 }
 
+static StringRef bufferize(SourceMgr &SM, StringRef Str) {
+  std::unique_ptr<MemoryBuffer> Buffer =
+      MemoryBuffer::getMemBufferCopy(Str, "TestBuffer");
+  StringRef StrBufferRef = Buffer->getBuffer();
+  SM.AddNewSourceBuffer(std::move(Buffer), SMLoc());
+  return StrBufferRef;
+}
+
 TEST_F(FileCheckTest, ParseVar) {
-  StringRef OrigVarName = "GoodVar42";
+  SourceMgr SM;
+  StringRef OrigVarName = bufferize(SM, "GoodVar42");
   StringRef VarName = OrigVarName;
-  StringRef ParsedName;
   bool IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
-  EXPECT_EQ(ParsedName, OrigVarName);
+  Expected<StringRef> ParsedName =
+      FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
+  EXPECT_EQ(*ParsedName, OrigVarName);
   EXPECT_TRUE(VarName.empty());
   EXPECT_FALSE(IsPseudo);
 
-  VarName = OrigVarName = "$GoodGlobalVar";
+  VarName = OrigVarName = bufferize(SM, "$GoodGlobalVar");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
-  EXPECT_EQ(ParsedName, OrigVarName);
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
+  EXPECT_EQ(*ParsedName, OrigVarName);
   EXPECT_TRUE(VarName.empty());
   EXPECT_FALSE(IsPseudo);
 
-  VarName = OrigVarName = "@GoodPseudoVar";
+  VarName = OrigVarName = bufferize(SM, "@GoodPseudoVar");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
-  EXPECT_EQ(ParsedName, OrigVarName);
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
+  EXPECT_EQ(*ParsedName, OrigVarName);
   EXPECT_TRUE(VarName.empty());
   EXPECT_TRUE(IsPseudo);
 
-  VarName = "42BadVar";
-  EXPECT_TRUE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  VarName = bufferize(SM, "42BadVar");
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(errorToBool(ParsedName.takeError()));
+
+  VarName = bufferize(SM, "$@");
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(errorToBool(ParsedName.takeError()));
 
-  VarName = "$@";
-  EXPECT_TRUE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
-
-  VarName = OrigVarName = "B@dVar";
+  VarName = OrigVarName = bufferize(SM, "B@dVar");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
   EXPECT_EQ(VarName, OrigVarName.substr(1));
-  EXPECT_EQ(ParsedName, "B");
+  EXPECT_EQ(*ParsedName, "B");
   EXPECT_FALSE(IsPseudo);
 
-  VarName = OrigVarName = "B$dVar";
+  VarName = OrigVarName = bufferize(SM, "B$dVar");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
   EXPECT_EQ(VarName, OrigVarName.substr(1));
-  EXPECT_EQ(ParsedName, "B");
+  EXPECT_EQ(*ParsedName, "B");
   EXPECT_FALSE(IsPseudo);
 
-  VarName = "BadVar+";
+  VarName = bufferize(SM, "BadVar+");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
   EXPECT_EQ(VarName, "+");
-  EXPECT_EQ(ParsedName, "BadVar");
+  EXPECT_EQ(*ParsedName, "BadVar");
   EXPECT_FALSE(IsPseudo);
 
-  VarName = "BadVar-";
+  VarName = bufferize(SM, "BadVar-");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
   EXPECT_EQ(VarName, "-");
-  EXPECT_EQ(ParsedName, "BadVar");
+  EXPECT_EQ(*ParsedName, "BadVar");
   EXPECT_FALSE(IsPseudo);
 
-  VarName = "BadVar:";
+  VarName = bufferize(SM, "BadVar:");
   IsPseudo = true;
-  EXPECT_FALSE(FileCheckPattern::parseVariable(VarName, ParsedName, IsPseudo));
+  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
+  EXPECT_TRUE(static_cast<bool>(ParsedName));
   EXPECT_EQ(VarName, ":");
-  EXPECT_EQ(ParsedName, "BadVar");
+  EXPECT_EQ(*ParsedName, "BadVar");
   EXPECT_FALSE(IsPseudo);
 }
 
-static StringRef bufferize(SourceMgr &SM, StringRef Str) {
-  std::unique_ptr<MemoryBuffer> Buffer =
-      MemoryBuffer::getMemBufferCopy(Str, "TestBuffer");
-  StringRef StrBufferRef = Buffer->getBuffer();
-  SM.AddNewSourceBuffer(std::move(Buffer), SMLoc());
-  return StrBufferRef;
-}
-
 class PatternTester {
 private:
   size_t LineNumber = 1;
   SourceMgr SM;
   FileCheckRequest Req;
   FileCheckPatternContext Context;
   FileCheckPattern P =
       FileCheckPattern(Check::CheckPlain, &Context, LineNumber++);
 
 public:
   PatternTester() {
     std::vector<std::string> GlobalDefines;
     GlobalDefines.emplace_back(std::string("#FOO=42"));
     GlobalDefines.emplace_back(std::string("BAR=BAZ"));
-    Context.defineCmdlineVariables(GlobalDefines, SM);
+    EXPECT_FALSE(
+        errorToBool(Context.defineCmdlineVariables(GlobalDefines, SM)));
     // Call parsePattern to have @LINE defined.
     P.parsePattern("N/A", "CHECK", SM, Req);
     // parsePattern does not expect to be called twice for the same line and
     // will set FixedStr and RegExStr incorrectly if it is. Therefore prepare
     // a pattern for a different line.
     initNextPattern();
   }
 
   void initNextPattern() {
     P = FileCheckPattern(Check::CheckPlain, &Context, LineNumber++);
   }
 
   bool parseNumVarDefExpect(StringRef Expr) {
     StringRef ExprBufferRef = bufferize(SM, Expr);
     StringRef Name;
-    return FileCheckPattern::parseNumericVariableDefinition(ExprBufferRef, Name,
-                                                            &Context, SM);
+    return errorToBool(FileCheckPattern::parseNumericVariableDefinition(
+        ExprBufferRef, Name, &Context, SM));
   }
 
   bool parseSubstExpect(StringRef Expr) {
     StringRef ExprBufferRef = bufferize(SM, Expr);
-    FileCheckNumericVariable *DefinedNumericVariable;
-    return P.parseNumericSubstitutionBlock(
-               ExprBufferRef, DefinedNumericVariable, SM) == nullptr;
+    Optional<FileCheckNumericVariable *> DefinedNumericVariable;
+    return errorToBool(P.parseNumericSubstitutionBlock(
+                            ExprBufferRef, DefinedNumericVariable, SM)
+                           .takeError());
   }
 
   bool parsePatternExpect(StringRef Pattern) {
     StringRef PatBufferRef = bufferize(SM, Pattern);
     return P.parsePattern(PatBufferRef, "CHECK", SM, Req);
   }
 
   bool matchExpect(StringRef Buffer) {
TEST_F(FileCheckTest, Match) {
   EXPECT_FALSE(Tester.matchExpect("18 20"));
 }
 
 TEST_F(FileCheckTest, Substitution) {
   SourceMgr SM;
   FileCheckPatternContext Context;
   std::vector<std::string> GlobalDefines;
   GlobalDefines.emplace_back(std::string("FOO=BAR"));
-  Context.defineCmdlineVariables(GlobalDefines, SM);
+  EXPECT_FALSE(errorToBool(Context.defineCmdlineVariables(GlobalDefines, SM)));
 
-  // Substitution of an undefined string variable fails.
+  // Substitution of an undefined string variable fails and error holds that
+  // variable's name.
   FileCheckStringSubstitution StringSubstitution =
       FileCheckStringSubstitution(&Context, "VAR404", 42);
-  EXPECT_FALSE(StringSubstitution.getResult());
+  Expected<std::string> SubstValue = StringSubstitution.getResult();
+  EXPECT_FALSE(static_cast<bool>(SubstValue));
+  expectUndefError("VAR404", SubstValue.takeError());
 
   // Substitutions of defined pseudo and non-pseudo numeric variables return
   // the right value.
   FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE", 42);
   FileCheckNumericVariable NVar = FileCheckNumericVariable("N", 10);
   FileCheckNumExpr NumExprLine = FileCheckNumExpr(doAdd, &LineVar, 0);
   FileCheckNumExpr NumExprN = FileCheckNumExpr(doAdd, &NVar, 3);
   FileCheckNumericSubstitution SubstitutionLine =
       FileCheckNumericSubstitution(&Context, "@LINE", &NumExprLine, 12);
   FileCheckNumericSubstitution SubstitutionN =
       FileCheckNumericSubstitution(&Context, "N", &NumExprN, 30);
-  llvm::Optional<std::string> Value = SubstitutionLine.getResult();
-  EXPECT_TRUE(Value);
+  Expected<std::string> Value = SubstitutionLine.getResult();
+  EXPECT_TRUE(static_cast<bool>(Value));
   EXPECT_EQ("42", *Value);
   Value = SubstitutionN.getResult();
-  EXPECT_TRUE(Value);
+  EXPECT_TRUE(static_cast<bool>(Value));
   EXPECT_EQ("13", *Value);
 
   // Substitution of an undefined numeric variable fails.
   LineVar.clearValue();
-  EXPECT_FALSE(SubstitutionLine.getResult());
+  SubstValue = SubstitutionLine.getResult().takeError();
+  EXPECT_FALSE(static_cast<bool>(SubstValue));
+  expectUndefError("@LINE", SubstValue.takeError());
   NVar.clearValue();
-  EXPECT_FALSE(SubstitutionN.getResult());
+  SubstValue = SubstitutionN.getResult().takeError();
+  EXPECT_FALSE(static_cast<bool>(SubstValue));
+  expectUndefError("N", SubstValue.takeError());
 
   // Substitution of a defined string variable returns the right value.
   FileCheckPattern P = FileCheckPattern(Check::CheckPlain, &Context, 1);
   StringSubstitution = FileCheckStringSubstitution(&Context, "FOO", 42);
   Value = StringSubstitution.getResult();
-  EXPECT_TRUE(Value);
+  EXPECT_TRUE(static_cast<bool>(Value));
   EXPECT_EQ("BAR", *Value);
 }
 
-TEST_F(FileCheckTest, UndefVars) {
-  SourceMgr SM;
-  FileCheckPatternContext Context;
-  std::vector<std::string> GlobalDefines;
-  GlobalDefines.emplace_back(std::string("FOO=BAR"));
-  Context.defineCmdlineVariables(GlobalDefines, SM);
-
-  // getUndefVarName() on a string substitution with an undefined variable
-  // returns that variable.
-  FileCheckStringSubstitution StringSubstitution =
-      FileCheckStringSubstitution(&Context, "VAR404", 42);
-  StringRef UndefVar = StringSubstitution.getUndefVarName();
-  EXPECT_EQ("VAR404", UndefVar);
-
-  // getUndefVarName() on a string substitution with a defined variable returns
-  // an empty string.
-  StringSubstitution = FileCheckStringSubstitution(&Context, "FOO", 42);
-  UndefVar = StringSubstitution.getUndefVarName();
-  EXPECT_EQ("", UndefVar);
-
-  // getUndefVarName() on a numeric substitution with a defined variable
-  // returns an empty string.
-  FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE", 42);
-  FileCheckNumExpr NumExpr = FileCheckNumExpr(doAdd, &LineVar, 0);
-  FileCheckNumericSubstitution NumericSubstitution =
-      FileCheckNumericSubstitution(&Context, "@LINE", &NumExpr, 12);
-  UndefVar = NumericSubstitution.getUndefVarName();
-  EXPECT_EQ("", UndefVar);
-
-  // getUndefVarName() on a numeric substitution with an undefined variable
-  // returns that variable.
-  LineVar.clearValue();
-  UndefVar = NumericSubstitution.getUndefVarName();
-  EXPECT_EQ("@LINE", UndefVar);
-}
-
 TEST_F(FileCheckTest, FileCheckContext) {
   FileCheckPatternContext Cxt = FileCheckPatternContext();
   std::vector<std::string> GlobalDefines;
   SourceMgr SM;
 
   // Missing equal sign.
   GlobalDefines.emplace_back(std::string("LocalVar"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("#LocalNumVar"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
 
   // Empty variable name.
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("=18"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("#=18"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
 
   // Invalid variable name.
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("18LocalVar=18"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("#18LocalNumVar=18"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
 
   // Name conflict between pattern and numeric variable.
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("LocalVar=18"));
   GlobalDefines.emplace_back(std::string("#LocalVar=36"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   Cxt = FileCheckPatternContext();
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("#LocalNumVar=18"));
   GlobalDefines.emplace_back(std::string("LocalNumVar=36"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   Cxt = FileCheckPatternContext();
 
   // Invalid numeric value for numeric variable.
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("#LocalNumVar=x"));
-  EXPECT_TRUE(Cxt.defineCmdlineVariables(GlobalDefines, SM));
+  EXPECT_TRUE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
 
   // Define local variables from command-line.
   GlobalDefines.clear();
   GlobalDefines.emplace_back(std::string("LocalVar=FOO"));
   GlobalDefines.emplace_back(std::string("EmptyVar="));
   GlobalDefines.emplace_back(std::string("#LocalNumVar=18"));
-  bool GotError = Cxt.defineCmdlineVariables(GlobalDefines, SM);
-  EXPECT_FALSE(GotError);
+  EXPECT_FALSE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
 
   // Check defined variables are present and undefined is absent.
   StringRef LocalVarStr = "LocalVar";
   StringRef LocalNumVarRef = bufferize(SM, "LocalNumVar");
   StringRef EmptyVarStr = "EmptyVar";
   StringRef UnknownVarStr = "UnknownVar";
-  llvm::Optional<StringRef> LocalVar = Cxt.getPatternVarValue(LocalVarStr);
+  Expected<StringRef> LocalVar = Cxt.getPatternVarValue(LocalVarStr);
   FileCheckPattern P = FileCheckPattern(Check::CheckPlain, &Cxt, 1);
-  FileCheckNumericVariable *DefinedNumericVariable;
-  FileCheckNumExpr *NumExpr = P.parseNumericSubstitutionBlock(
+  Optional<FileCheckNumericVariable *> DefinedNumericVariable;
+  Expected<FileCheckNumExpr *> NumExpr = P.parseNumericSubstitutionBlock(
       LocalNumVarRef, DefinedNumericVariable, SM);
-  llvm::Optional<StringRef> EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
-  llvm::Optional<StringRef> UnknownVar = Cxt.getPatternVarValue(UnknownVarStr);
-  EXPECT_TRUE(LocalVar);
+  Expected<StringRef> EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
+  Expected<StringRef> UnknownVar = Cxt.getPatternVarValue(UnknownVarStr);
+  EXPECT_TRUE(static_cast<bool>(LocalVar));
   EXPECT_EQ(*LocalVar, "FOO");
-  EXPECT_TRUE(NumExpr);
-  llvm::Optional<uint64_t> NumExprVal = NumExpr->eval();
-  EXPECT_TRUE(NumExprVal);
+  EXPECT_TRUE(static_cast<bool>(NumExpr));
+  Expected<uint64_t> NumExprVal = (*NumExpr)->eval();
+  EXPECT_TRUE(static_cast<bool>(NumExprVal));
   EXPECT_EQ(*NumExprVal, 18U);
-  EXPECT_TRUE(EmptyVar);
+  EXPECT_TRUE(static_cast<bool>(EmptyVar));
   EXPECT_EQ(*EmptyVar, "");
-  EXPECT_FALSE(UnknownVar);
+  EXPECT_TRUE(errorToBool(UnknownVar.takeError()));
 
   // Clear local variables and check they become absent.
   Cxt.clearLocalVars();
   LocalVar = Cxt.getPatternVarValue(LocalVarStr);
-  EXPECT_FALSE(LocalVar);
+  EXPECT_TRUE(errorToBool(LocalVar.takeError()));
   // Check a numeric expression's evaluation fails if called after clearing of
   // local variables, if it was created before. This is important because local
   // variable clearing due to --enable-var-scope happens after numeric
   // expressions are linked to the numeric variables they use.
-  EXPECT_FALSE(NumExpr->eval());
+  EXPECT_TRUE(errorToBool((*NumExpr)->eval().takeError()));
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 2);
   NumExpr = P.parseNumericSubstitutionBlock(LocalNumVarRef,
                                             DefinedNumericVariable, SM);
-  EXPECT_FALSE(NumExpr);
+  EXPECT_TRUE(errorToBool(NumExpr.takeError()));
   EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
-  EXPECT_FALSE(EmptyVar);
+  EXPECT_TRUE(errorToBool(EmptyVar.takeError()));
 
   // Redefine global variables and check variables are defined again.
   GlobalDefines.emplace_back(std::string("$GlobalVar=BAR"));
   GlobalDefines.emplace_back(std::string("#$GlobalNumVar=36"));
-  GotError = Cxt.defineCmdlineVariables(GlobalDefines, SM);
-  EXPECT_FALSE(GotError);
+  EXPECT_FALSE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   StringRef GlobalVarStr = "$GlobalVar";
   StringRef GlobalNumVarRef = bufferize(SM, "$GlobalNumVar");
-  llvm::Optional<StringRef> GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);
-  EXPECT_TRUE(GlobalVar);
+  Expected<StringRef> GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);
+  EXPECT_TRUE(static_cast<bool>(GlobalVar));
   EXPECT_EQ(*GlobalVar, "BAR");
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 3);
   NumExpr = P.parseNumericSubstitutionBlock(GlobalNumVarRef,
                                             DefinedNumericVariable, SM);
-  EXPECT_TRUE(NumExpr);
-  NumExprVal = NumExpr->eval();
-  EXPECT_TRUE(NumExprVal);
+  EXPECT_TRUE(static_cast<bool>(NumExpr));
+  NumExprVal = (*NumExpr)->eval();
+  EXPECT_TRUE(static_cast<bool>(NumExprVal));
   EXPECT_EQ(*NumExprVal, 36U);
 
   // Clear local variables and check global variables remain defined.
   Cxt.clearLocalVars();
-  GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);
-  EXPECT_TRUE(GlobalVar);
+  EXPECT_FALSE(errorToBool(Cxt.getPatternVarValue(GlobalVarStr).takeError()));
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 4);
   NumExpr = P.parseNumericSubstitutionBlock(GlobalNumVarRef,
                                             DefinedNumericVariable, SM);
-  EXPECT_TRUE(NumExpr);
-  NumExprVal = NumExpr->eval();
-  EXPECT_TRUE(NumExprVal);
+  EXPECT_TRUE(static_cast<bool>(NumExpr));
+  NumExprVal = (*NumExpr)->eval();
+  EXPECT_TRUE(static_cast<bool>(NumExprVal));
   EXPECT_EQ(*NumExprVal, 36U);
 }
 } // namespace