[FileCheck] Better diagnostic for format conflict

[thopre]

Summary

Improve error message in case of conflict between several implicit
format to mention the operand that conflict.

[jdenny]

I like the idea of the change, but there are some parts I don't understand yet. I've added inline comments.

llvm/lib/Support/FileCheck.cpp

112–115

Is a diagnostic lost when both !LeftFormat and !RightFormat?

If that's not possible, I think an assert would help.

If it is possible, I think a comment explaining why we don't care would help.

118–120

Format is unused if the following if condition is true. I think the logic would be clearer if you move the assigned expression to the final return statement.

407

I'm not sure what UseExpr means. Does OuterBinOpExpr make sense?

435–442

Is there a logic change here for the case of !ExpressionASTPointer?

That used to mean ImplicitFormat = NoFormat and then Format = ImplicitFormat.

Now it means Format = Unsigned.

Did I read that correctly? Why the change?

llvm/lib/Support/FileCheckImpl.h

238–239

Is Name now being stored in both ExpressionAST and NumericVariableUse?

675–683

update -> updates

[thopre]

Address all feedback.

[jdenny]

llvm/lib/Support/FileCheck.cpp

435–442

I don't see where this was addressed in your update.

[thopre]

llvm/lib/Support/FileCheck.cpp

435–442

My bad, forgot to submit to send this reply:

NoFormat evaluates to false, hence the final else would have applied instead and set Format to Unsigned. Therefore the behavior is unchanged.

[grimar]

I am not well familar with the code of FileCheck,
the code looks good to me (I've found no issues with how errors are handled either),
Two observations/suggestions are below.

llvm/lib/Support/FileCheck.cpp

30

I'd put it to the end, after other cases. It is more consistent with the resto of the code and also,
default is the most unlikely case here.

297

Perhaps, I'd inline:

if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0,
                         LiteralValue))
  return std::make_unique<ExpressionLiteral>(
      OperandExpr.drop_back(Expr.size()), LiteralValue);

[thopre]

Address review feedback

[thopre]

Rebase

[thopre]

Rebase

[jdenny]

Other than the missing test case, LGTM.

llvm/lib/Support/FileCheck.cpp

112–115

Is there a test for this case?

435–442

Ah, got it. Thanks for explaining.

[jdenny]

llvm/lib/Support/FileCheck.cpp

39

One more thing: clang-tidy is saying you're violating this:

http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

[thopre]

• Remove default case in ExplicitFormat::toString() and move llvm_unreachable outside the switch block
• Add testcase for getImplicitFormat with errors on both operands

[thopre]

llvm/unittests/Support/FileCheckTest.cpp

45

clang-tidy complains about the first letter of this variable being lowercase but since it's a lambda I think it's justified.

[jhenderson]

llvm/unittests/Support/FileCheckTest.cpp

45

I believe the general consensus is that lamdbas are variables, not functions (akin to a variable storing a function pointer or a class instance with an operator()), and therefore should be labelled as such with an upper-case first letter. I can dig up conversations on the topic from elsewhere if you want.

[thopre]

Fix code style for lambda variable.

llvm/unittests/Support/FileCheckTest.cpp

45

No need, I'm happy to take your word for it. :-)

llvm/lib/Support/FileCheck.cpp

 #include "llvm/Support/FormatVariadic.h"
 #include <cstdint>
 #include <list>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 
+StringRef ExpressionFormat::toString() const {
+  switch (Value) {
+  case Kind::NoFormat:
+    return StringRef("<none>");
+  case Kind::Unsigned:
+    return StringRef("%u");
+  case Kind::HexUpper:
+    return StringRef("%X");
+  case Kind::HexLower:
+    return StringRef("%x");
+  }
+  llvm_unreachable("unknown expression format");
+}
+
 Expected<StringRef> ExpressionFormat::getWildcardRegex() const {
   switch (Value) {
   case Kind::Unsigned:
     return StringRef("[0-9]+");
   case Kind::HexUpper:
     return StringRef("[0-9A-F]+");
   case Kind::HexLower:
     return StringRef("[0-9a-f]+");
ExpressionFormat::valueFromStringRepr(StringRef StrVal,
   return IntegerValue;
 }
 
 Expected<uint64_t> NumericVariableUse::eval() const {
   Optional<uint64_t> Value = Variable->getValue();
   if (Value)
     return *Value;
 
-  return make_error<UndefVarError>(Name);
+  return make_error<UndefVarError>(getExpressionStr());
 }
 
 Expected<uint64_t> BinaryOperation::eval() const {
   Expected<uint64_t> LeftOp = LeftOperand->eval();
   Expected<uint64_t> RightOp = RightOperand->eval();
 
   // Bubble up any error (e.g. undefined variables) in the recursive
   // evaluation.
   if (!LeftOp || !RightOp) {
     Error Err = Error::success();
     if (!LeftOp)
       Err = joinErrors(std::move(Err), LeftOp.takeError());
     if (!RightOp)
       Err = joinErrors(std::move(Err), RightOp.takeError());
     return std::move(Err);
   }
 
   return EvalBinop(*LeftOp, *RightOp);
 }
 
-ExpressionFormat BinaryOperation::getImplicitFormat() const {
-  ExpressionFormat LeftFormat = LeftOperand->getImplicitFormat();
-  ExpressionFormat RightFormat = RightOperand->getImplicitFormat();
-
-  ExpressionFormat Format =
-      LeftFormat != ExpressionFormat::Kind::NoFormat ? LeftFormat : RightFormat;
-  if (LeftFormat != ExpressionFormat::Kind::NoFormat &&
-      RightFormat != ExpressionFormat::Kind::NoFormat &&
-      LeftFormat != RightFormat)
-    Format = ExpressionFormat(ExpressionFormat::Kind::Conflict);
+Expected<ExpressionFormat>
+BinaryOperation::getImplicitFormat(const SourceMgr &SM) const {
+  Expected<ExpressionFormat> LeftFormat = LeftOperand->getImplicitFormat(SM);
+  Expected<ExpressionFormat> RightFormat = RightOperand->getImplicitFormat(SM);
+  if (!LeftFormat || !RightFormat) {
+    Error Err = Error::success();
+    if (!LeftFormat)
+      Err = joinErrors(std::move(Err), LeftFormat.takeError());
+    if (!RightFormat)
+      Err = joinErrors(std::move(Err), RightFormat.takeError());
+    return std::move(Err);
+  }
+
+  if (*LeftFormat != ExpressionFormat::Kind::NoFormat &&
+      *RightFormat != ExpressionFormat::Kind::NoFormat &&
+      *LeftFormat != *RightFormat)
+    return ErrorDiagnostic::get(
+        SM, getExpressionStr(),
+        "implicit format conflict between '" + LeftOperand->getExpressionStr() +
+            "' (" + LeftFormat->toString() + ") and '" +
+            RightOperand->getExpressionStr() + "' (" + RightFormat->toString() +
+            "), need an explicit format specifier");
 
-  return Format;
+  return *LeftFormat != ExpressionFormat::Kind::NoFormat ? *LeftFormat
+                                                         : *RightFormat;
 }
 
 Expected<std::string> NumericSubstitution::getResult() const {
   assert(ExpressionPointer->getAST() != nullptr &&
          "Substituting empty expression");
   Expected<uint64_t> EvaluatedValue = ExpressionPointer->getAST()->eval();
   if (!EvaluatedValue)
     return EvaluatedValue.takeError();
if (AO == AllowedOperand::LineVar || AO == AllowedOperand::Any) {
     if (AO == AllowedOperand::LineVar)
       return ParseVarResult.takeError();
     // Ignore the error and retry parsing as a literal.
     consumeError(ParseVarResult.takeError());
   }
 
   // Otherwise, parse it as a literal.
   uint64_t LiteralValue;
+  StringRef OperandExpr = Expr;
   if (!Expr.consumeInteger((AO == AllowedOperand::LegacyLiteral) ? 10 : 0,
-                           LiteralValue))
-    return std::make_unique<ExpressionLiteral>(LiteralValue);
+                           LiteralValue)) {
+    return std::make_unique<ExpressionLiteral>(
+        OperandExpr.drop_back(Expr.size()), LiteralValue);
+  }
 
   return ErrorDiagnostic::get(SM, Expr,
                               "invalid operand format '" + Expr + "'");
 }
 
 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;
 }
 
 Expected<std::unique_ptr<ExpressionAST>>
-Pattern::parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
+Pattern::parseBinop(StringRef Expr, StringRef &RemainingExpr,
+                    std::unique_ptr<ExpressionAST> LeftOp,
                     bool IsLegacyLineExpr, Optional<size_t> LineNumber,
                     FileCheckPatternContext *Context, const SourceMgr &SM) {
-  Expr = Expr.ltrim(SpaceChars);
-  if (Expr.empty())
+  RemainingExpr = RemainingExpr.ltrim(SpaceChars);
+  if (RemainingExpr.empty())
     return std::move(LeftOp);
 
   // Check if this is a supported operation and select a function to perform
   // it.
-  SMLoc OpLoc = SMLoc::getFromPointer(Expr.data());
-  char Operator = popFront(Expr);
+  SMLoc OpLoc = SMLoc::getFromPointer(RemainingExpr.data());
+  char Operator = popFront(RemainingExpr);
   binop_eval_t EvalBinop;
   switch (Operator) {
   case '+':
     EvalBinop = add;
     break;
   case '-':
     EvalBinop = sub;
     break;
   default:
     return ErrorDiagnostic::get(
         SM, OpLoc, Twine("unsupported operation '") + Twine(Operator) + "'");
   }
 
   // Parse right operand.
-  Expr = Expr.ltrim(SpaceChars);
-  if (Expr.empty())
-    return ErrorDiagnostic::get(SM, Expr, "missing operand in expression");
+  RemainingExpr = RemainingExpr.ltrim(SpaceChars);
+  if (RemainingExpr.empty())
+    return ErrorDiagnostic::get(SM, RemainingExpr,
+                                "missing operand in expression");
   // The second operand in a legacy @LINE expression is always a literal.
   AllowedOperand AO =
       IsLegacyLineExpr ? AllowedOperand::LegacyLiteral : AllowedOperand::Any;
   Expected<std::unique_ptr<ExpressionAST>> RightOpResult =
-      parseNumericOperand(Expr, AO, LineNumber, Context, SM);
+      parseNumericOperand(RemainingExpr, AO, LineNumber, Context, SM);
   if (!RightOpResult)
     return RightOpResult;
 
-  Expr = Expr.ltrim(SpaceChars);
-  return std::make_unique<BinaryOperation>(EvalBinop, std::move(LeftOp),
+  Expr = Expr.drop_back(RemainingExpr.size());
+  return std::make_unique<BinaryOperation>(Expr, EvalBinop, std::move(LeftOp),
                                            std::move(*RightOpResult));
 }
 
 Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(
     StringRef Expr, Optional<NumericVariable *> &DefinedNumericVariable,
     bool IsLegacyLineExpr, Optional<size_t> LineNumber,
     FileCheckPatternContext *Context, const SourceMgr &SM) {
   std::unique_ptr<ExpressionAST> ExpressionASTPointer = nullptr;
Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(
   size_t DefEnd = Expr.find(':');
   if (DefEnd != StringRef::npos) {
     DefExpr = Expr.substr(0, DefEnd);
     Expr = Expr.substr(DefEnd + 1);
   }
 
   // Parse the expression itself.
   Expr = Expr.ltrim(SpaceChars);
-  StringRef UseExpr = Expr;
   if (!Expr.empty()) {
+    Expr = Expr.rtrim(SpaceChars);
+    StringRef OuterBinOpExpr = Expr;
     // The first operand in a legacy @LINE expression is always the @LINE
     // pseudo variable.
     AllowedOperand AO =
         IsLegacyLineExpr ? AllowedOperand::LineVar : AllowedOperand::Any;
     Expected<std::unique_ptr<ExpressionAST>> ParseResult =
         parseNumericOperand(Expr, AO, LineNumber, Context, SM);
     while (ParseResult && !Expr.empty()) {
-      ParseResult = parseBinop(Expr, std::move(*ParseResult), IsLegacyLineExpr,
-                               LineNumber, Context, SM);
+      ParseResult = parseBinop(OuterBinOpExpr, Expr, std::move(*ParseResult),
+                               IsLegacyLineExpr, LineNumber, Context, SM);
       // Legacy @LINE expressions only allow 2 operands.
       if (ParseResult && IsLegacyLineExpr && !Expr.empty())
         return ErrorDiagnostic::get(
             SM, Expr,
             "unexpected characters at end of expression '" + Expr + "'");
     }
     if (!ParseResult)
       return ParseResult.takeError();
     ExpressionASTPointer = std::move(*ParseResult);
   }
 
   // Select format of the expression, i.e. (i) its explicit format, if any,
   // otherwise (ii) its implicit format, if any, otherwise (iii) the default
   // format (unsigned). Error out in case of conflicting implicit format
   // without explicit format.
-  ExpressionFormat Format,
-      ImplicitFormat = ExpressionASTPointer
-                           ? ExpressionASTPointer->getImplicitFormat()
-                           : ExpressionFormat(ExpressionFormat::Kind::NoFormat);
-  if (bool(ExplicitFormat))
+  ExpressionFormat Format;
+  if (ExplicitFormat)
     Format = ExplicitFormat;
-  else if (ImplicitFormat == ExpressionFormat::Kind::Conflict)
-    return ErrorDiagnostic::get(
-        SM, UseExpr,
-        "variables with conflicting format specifier: need an explicit one");
-  else if (bool(ImplicitFormat))
-    Format = ImplicitFormat;
-  else
+  else if (ExpressionASTPointer) {
+    Expected<ExpressionFormat> ImplicitFormat =
+        ExpressionASTPointer->getImplicitFormat(SM);
+    if (!ImplicitFormat)
+      return ImplicitFormat.takeError();
+    Format = *ImplicitFormat;
+  }
+  if (!Format)
     Format = ExpressionFormat(ExpressionFormat::Kind::Unsigned);
 
   std::unique_ptr<Expression> ExpressionPointer =
       std::make_unique<Expression>(std::move(ExpressionASTPointer), Format);
 
   // Parse the numeric variable definition.
   if (DefEnd != StringRef::npos) {
     DefExpr = DefExpr.ltrim(SpaceChars);

llvm/lib/Support/FileCheckImpl.h

 /// Type representing the format an expression value should be textualized into
 /// for matching. Used to represent both explicit format specifiers as well as
 /// implicit format from using numeric variables.
 struct ExpressionFormat {
   enum class Kind {
     /// Denote absence of format. Used for implicit format of literals and
     /// empty expressions.
     NoFormat,
-    /// Used when there are several conflicting implicit formats in an
-    /// expression.
-    Conflict,
     /// Value is an unsigned integer and should be printed as a decimal number.
     Unsigned,
     /// Value should be printed as an uppercase hex number.
     HexUpper,
     /// Value should be printed as a lowercase hex number.
     HexLower
   };
 
 private:
   Kind Value;
 
 public:
   /// Evaluates a format to true if it can be used in a match.
-  explicit operator bool() const {
-    return Value != Kind::NoFormat && Value != Kind::Conflict;
-  }
+  explicit operator bool() const { return Value != Kind::NoFormat; }
 
   /// Define format equality: formats are equal if neither is NoFormat and
   /// their kinds are the same.
   bool operator==(const ExpressionFormat &Other) const {
     return Value != Kind::NoFormat && Value == Other.Value;
   }
 
   bool operator!=(const ExpressionFormat &other) const {
     return !(*this == other);
   }
 
   bool operator==(Kind OtherValue) const { return Value == OtherValue; }
 
   bool operator!=(Kind OtherValue) const { return !(*this == OtherValue); }
 
+  /// \returns the format specifier corresponding to this format as a string.
+  StringRef toString() const;
+
   ExpressionFormat() : Value(Kind::NoFormat){};
   explicit ExpressionFormat(Kind Value) : Value(Value){};
 
   /// \returns a wildcard regular expression StringRef that matches any value
   /// in the format represented by this instance, or an error if the format is
-  /// NoFormat or Conflict.
+  /// NoFormat.
   Expected<StringRef> getWildcardRegex() const;
 
   /// \returns the string representation of \p Value in the format represented
-  /// by this instance, or an error if the format is NoFormat or Conflict.
+  /// by this instance, or an error if the format is NoFormat.
   Expected<std::string> getMatchingString(uint64_t Value) const;
 
   /// \returns the value corresponding to string representation \p StrVal
   /// according to the matching format represented by this instance or an error
   /// with diagnostic against \p SM if \p StrVal does not correspond to a valid
   /// and representable value.
   Expected<uint64_t> valueFromStringRepr(StringRef StrVal,
                                          const SourceMgr &SM) const;
 };
 
 /// Base class representing the AST of a given expression.
 class ExpressionAST {
+private:
+  StringRef ExpressionStr;
+
 public:
+  ExpressionAST(StringRef ExpressionStr) : ExpressionStr(ExpressionStr) {}
+
   virtual ~ExpressionAST() = default;
 
+  StringRef getExpressionStr() const { return ExpressionStr; }
+
   /// Evaluates and \returns the value of the expression represented by this
   /// AST or an error if evaluation fails.
   virtual Expected<uint64_t> eval() const = 0;
 
-  /// \returns either the implicit format of this AST, FormatConflict if
-  /// implicit formats of the AST's components conflict, or NoFormat if the AST
-  /// has no implicit format (e.g. AST is made up of a single literal).
-  virtual ExpressionFormat getImplicitFormat() const {
+  /// \returns either the implicit format of this AST, a diagnostic against
+  /// \p SM if implicit formats of the AST's components conflict, or NoFormat
+  /// if the AST has no implicit format (e.g. AST is made up of a single
+  /// literal).
+  virtual Expected<ExpressionFormat>
+  getImplicitFormat(const SourceMgr &SM) const {
     return ExpressionFormat();
   }
 };
 
 /// Class representing an unsigned literal in the AST of an expression.
 class ExpressionLiteral : public ExpressionAST {
 private:
   /// Actual value of the literal.
   uint64_t Value;
 
 public:
-  /// Constructs a literal with the specified value.
-  ExpressionLiteral(uint64_t Val) : Value(Val) {}
+  /// Constructs a literal with the specified value parsed from
+  /// \p ExpressionStr.
+  ExpressionLiteral(StringRef ExpressionStr, uint64_t Val)
+      : ExpressionAST(ExpressionStr), Value(Val) {}
 
   /// \returns the literal's value.
   Expected<uint64_t> eval() const override { return Value; }
 };
 
 /// Class to represent an undefined variable error, which quotes that
 /// variable's name when printed.
 class UndefVarError : public ErrorInfo<UndefVarError> {
public:
   /// if defined before input is parsed.
   Optional<size_t> getDefLineNumber() const { return DefLineNumber; }
 };
 
 /// Class representing the use of a numeric variable in the AST of an
 /// expression.
 class NumericVariableUse : public ExpressionAST {
 private:
-  /// Name of the numeric variable.
-  StringRef Name;
-
   /// Pointer to the class instance for the variable this use is about.
   NumericVariable *Variable;
 
 public:
   NumericVariableUse(StringRef Name, NumericVariable *Variable)
-      : Name(Name), Variable(Variable) {}
-
+      : ExpressionAST(Name), Variable(Variable) {}
   /// \returns the value of the variable referenced by this instance.
   Expected<uint64_t> eval() const override;
 
   /// \returns implicit format of this numeric variable.
-  ExpressionFormat getImplicitFormat() const override {
+  Expected<ExpressionFormat>
+  getImplicitFormat(const SourceMgr &SM) const override {
     return Variable->getImplicitFormat();
   }
 };
 
 /// Type of functions evaluating a given binary operation.
 using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
 
 /// Class representing a single binary operation in the AST of an expression.
 class BinaryOperation : public ExpressionAST {
 private:
   /// Left operand.
   std::unique_ptr<ExpressionAST> LeftOperand;
 
   /// Right operand.
   std::unique_ptr<ExpressionAST> RightOperand;
 
   /// Pointer to function that can evaluate this binary operation.
   binop_eval_t EvalBinop;
 
 public:
-  BinaryOperation(binop_eval_t EvalBinop, std::unique_ptr<ExpressionAST> LeftOp,
+  BinaryOperation(StringRef ExpressionStr, binop_eval_t EvalBinop,
+                  std::unique_ptr<ExpressionAST> LeftOp,
                   std::unique_ptr<ExpressionAST> RightOp)
-      : EvalBinop(EvalBinop) {
+      : ExpressionAST(ExpressionStr), EvalBinop(EvalBinop) {
     LeftOperand = std::move(LeftOp);
     RightOperand = std::move(RightOp);
   }
 
   /// Evaluates the value of the binary operation represented by this AST,
   /// using EvalBinop on the result of recursively evaluating the operands.
   /// \returns the expression value or an error if an undefined numeric
   /// variable is used in one of the operands.
   Expected<uint64_t> eval() const override;
 
-  /// \returns the implicit format of this AST, if any, a format conflict if
-  /// the implicit formats of the AST's components conflict, or no format if
-  /// the AST has no implicit format (e.g. AST is made of a single literal).
-  ExpressionFormat getImplicitFormat() const override;
+  /// \returns the implicit format of this AST, if any, a diagnostic against
+  /// \p SM if the implicit formats of the AST's components conflict, or no
+  /// format if the AST has no implicit format (e.g. AST is made of a single
+  /// literal).
+  Expected<ExpressionFormat>
+  getImplicitFormat(const SourceMgr &SM) const override;
 };
 
 class FileCheckPatternContext;
 
 /// Class representing a substitution to perform in the RegExStr string.
 class Substitution {
 protected:
   /// Pointer to a class instance holding, among other things, the table with
private:
   /// \p Context points to the class instance holding the live string and
   /// numeric variables. \returns the class representing that operand in the
   /// AST of the expression or an error holding a diagnostic against \p SM
   /// otherwise.
   static Expected<std::unique_ptr<ExpressionAST>>
   parseNumericOperand(StringRef &Expr, AllowedOperand AO,
                       Optional<size_t> LineNumber,
                       FileCheckPatternContext *Context, const SourceMgr &SM);
-  /// Parses \p Expr for a binary operation at line \p LineNumber, or before
-  /// input is parsed if \p LineNumber is None. The left operand of this binary
-  /// operation is given in \p LeftOp and \p IsLegacyLineExpr indicates whether
-  /// we are parsing a legacy @LINE expression. Parameter \p Context points to
-  /// the class instance holding the live string and numeric variables.
-  /// \returns the class representing the binary operation in the AST of the
-  /// expression, or an error holding a diagnostic against \p SM otherwise.
+  /// Parses and updates \p RemainingExpr for a binary operation at line
+  /// \p LineNumber, or before input is parsed if \p LineNumber is None. The
+  /// left operand of this binary operation is given in \p LeftOp and \p Expr
+  /// holds the string for the full expression, including the left operand.
+  /// Parameter \p IsLegacyLineExpr indicates whether we are parsing a legacy
+  /// @LINE expression. Parameter \p Context points to the class instance
+  /// holding the live string and numeric variables. \returns the class
+  /// representing the binary operation in the AST of the expression, or an
+  /// error holding a diagnostic against \p SM otherwise.
   static Expected<std::unique_ptr<ExpressionAST>>
-  parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
-             bool IsLegacyLineExpr, Optional<size_t> LineNumber,
-             FileCheckPatternContext *Context, const SourceMgr &SM);
+  parseBinop(StringRef Expr, StringRef &RemainingExpr,
+             std::unique_ptr<ExpressionAST> LeftOp, bool IsLegacyLineExpr,
+             Optional<size_t> LineNumber, FileCheckPatternContext *Context,
+             const SourceMgr &SM);
 };
 
 //===----------------------------------------------------------------------===//
 // Check Strings.
 //===----------------------------------------------------------------------===//
 
 /// A check that we found in the input file.
 struct FileCheckString {

llvm/test/FileCheck/numeric-expression.txt

 CHECK-LABEL: USE CONV FMT IMPL MATCH
 CHECK-NEXT: [[# %x, VAR1]]
 CHECK-NEXT: [[# %X, VAR1]]
 CHECK-NEXT: [[# %u, VAR2]]
 CHECK-NEXT: [[# %u, VAR3]]
 
 ; Conflicting implicit format.
 RUN: %ProtectFileCheckOutput \
-RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT --input-file %s %s 2>&1 \
-RUN:   | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT-MSG %s
+RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT1 --input-file %s %s 2>&1 \
+RUN:   | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT1-MSG %s
+RUN: %ProtectFileCheckOutput \
+RUN: not FileCheck --check-prefixes CHECK,FMT-CONFLICT2 --input-file %s %s 2>&1 \
+RUN:   | FileCheck --strict-whitespace --check-prefix FMT-CONFLICT2-MSG %s
 
 VAR USE IMPL FMT CONFLICT
 23
-FMT-CONFLICT-LABEL: VAR USE IMPL FMT CONFLICT
-FMT-CONFLICT-NEXT: [[#VAR1 + VAR2]]
-FMT-CONFLICT-MSG: numeric-expression.txt:[[#@LINE-1]]:23: error: variables with conflicting format specifier: need an explicit one
-FMT-CONFLICT-MSG-NEXT: {{F}}MT-CONFLICT-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
-FMT-CONFLICT-MSG-NEXT: {{^                      \^$}}
+FMT-CONFLICT1-LABEL: VAR USE IMPL FMT CONFLICT
+FMT-CONFLICT1-NEXT: [[#VAR1 + VAR2]]
+FMT-CONFLICT1-MSG: numeric-expression.txt:[[#@LINE-1]]:24: error: implicit format conflict between 'VAR1' (%u) and 'VAR2' (%x), need an explicit format specifier
+FMT-CONFLICT1-MSG-NEXT: {{F}}MT-CONFLICT1-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
+FMT-CONFLICT1-MSG-NEXT: {{^                       \^$}}
+
+VAR USE IMPL FMT CONFLICT COMPLEX
+34
+FMT-CONFLICT2-LABEL: VAR USE IMPL FMT CONFLICT
+FMT-CONFLICT2-NEXT: [[#VAR1 + VAR1a + VAR2]]
+FMT-CONFLICT2-MSG: numeric-expression.txt:[[#@LINE-1]]:24: error: implicit format conflict between 'VAR1 + VAR1a' (%u) and 'VAR2' (%x), need an explicit format specifier
+FMT-CONFLICT2-MSG-NEXT: {{F}}MT-CONFLICT2-NEXT: {{\[\[#VAR1 \+ VAR1a \+ VAR2\]\]}}
+FMT-CONFLICT2-MSG-NEXT: {{^                       \^$}}
 
 ; Explicitly specified format can override conflicting implicit formats.
 VAR USE IMPL OVERRIDE FMT CONFLICT
 23
 CHECK-LABEL: VAR USE IMPL OVERRIDE FMT CONFLICT
 CHECK-NEXT: [[# %u, VAR1 + VAR2]]
 
 ; Numeric expressions using more than one variable defined on other lines.

llvm/unittests/Support/FileCheckTest.cpp

 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;
 }
 
+static std::string toString(const std::unordered_set<std::string> &Set) {
+  bool First = true;
+  std::string Str;
+  for (StringRef S : Set) {
+    Str += Twine(First ? "{" + S : ", " + S).str();
+    First = false;
+  }
+  Str += '}';
+  return Str;
+}
+
+template <typename ErrorT>
+static void expectSameErrors(std::unordered_set<std::string> ExpectedMsgs,
+                             Error Err) {
+  auto AnyErrorMsgMatch = [&ExpectedMsgs](std::string &&ErrorMsg) -> bool {
+    for (auto ExpectedMsgItr = ExpectedMsgs.begin(),
+              ExpectedMsgEnd = ExpectedMsgs.end();
+         ExpectedMsgItr != ExpectedMsgEnd; ++ExpectedMsgItr) {
+      if (ErrorMsg.find(*ExpectedMsgItr) != std::string::npos) {
+        ExpectedMsgs.erase(ExpectedMsgItr);
+        return true;
+      }
+    }
+    return false;
+  };
+
+  Error RemainingErrors = std::move(Err);
+  do {
+    RemainingErrors =
+        handleErrors(std::move(RemainingErrors), [&](const ErrorT &E) {
+          EXPECT_TRUE(AnyErrorMsgMatch(E.message()))
+              << "Unexpected error message:" << std::endl
+              << E.message();
+        });
+  } while (RemainingErrors && !ExpectedMsgs.empty());
+  EXPECT_THAT_ERROR(std::move(RemainingErrors), Succeeded());
+  EXPECT_TRUE(ExpectedMsgs.empty())
+      << "Error message(s) not found:" << std::endl
+      << toString(ExpectedMsgs);
+}
+
 template <typename ErrorT>
 static void expectError(StringRef ExpectedMsg, Error Err) {
-  bool ErrorHandled = false;
-  EXPECT_THAT_ERROR(handleErrors(std::move(Err),
-                                 [&](const ErrorT &E) {
-                                   EXPECT_NE(
-                                       E.message().find(ExpectedMsg.str()),
-                                       std::string::npos);
-                                   ErrorHandled = true;
-                                 }),
-                    Succeeded());
-  EXPECT_TRUE(ErrorHandled);
+  expectSameErrors<ErrorT>({ExpectedMsg.str()}, std::move(Err));
 }
 
 static void expectDiagnosticError(StringRef ExpectedMsg, Error Err) {
   expectError<ErrorDiagnostic>(ExpectedMsg, std::move(Err));
 }
 
 struct ExpressionFormatParameterisedFixture
     : public ::testing::TestWithParam<
TEST_F(FileCheckTest, NoFormatProperties) {
   ExpressionFormat NoFormat(ExpressionFormat::Kind::NoFormat);
   expectError<StringError>("trying to match value with invalid format",
                            NoFormat.getWildcardRegex().takeError());
   expectError<StringError>("trying to match value with invalid format",
                            NoFormat.getMatchingString(18).takeError());
   EXPECT_FALSE(bool(NoFormat));
 }
 
-TEST_F(FileCheckTest, ConflictFormatProperties) {
-  ExpressionFormat ConflictFormat(ExpressionFormat::Kind::Conflict);
-  expectError<StringError>("trying to match value with invalid format",
-                           ConflictFormat.getWildcardRegex().takeError());
-  expectError<StringError>("trying to match value with invalid format",
-                           ConflictFormat.getMatchingString(18).takeError());
-  EXPECT_FALSE(bool(ConflictFormat));
-}
-
 TEST_F(FileCheckTest, FormatEqualityOperators) {
   ExpressionFormat UnsignedFormat(ExpressionFormat::Kind::Unsigned);
   ExpressionFormat UnsignedFormat2(ExpressionFormat::Kind::Unsigned);
   EXPECT_TRUE(UnsignedFormat == UnsignedFormat2);
   EXPECT_FALSE(UnsignedFormat != UnsignedFormat2);
 
   ExpressionFormat HexLowerFormat(ExpressionFormat::Kind::HexLower);
   EXPECT_FALSE(UnsignedFormat == HexLowerFormat);
   EXPECT_TRUE(UnsignedFormat != HexLowerFormat);
 
   ExpressionFormat NoFormat(ExpressionFormat::Kind::NoFormat);
   ExpressionFormat NoFormat2(ExpressionFormat::Kind::NoFormat);
   EXPECT_FALSE(NoFormat == NoFormat2);
   EXPECT_TRUE(NoFormat != NoFormat2);
-
-  ExpressionFormat ConflictFormat(ExpressionFormat::Kind::Conflict);
-  ExpressionFormat ConflictFormat2(ExpressionFormat::Kind::Conflict);
-  EXPECT_TRUE(ConflictFormat == ConflictFormat2);
-  EXPECT_FALSE(ConflictFormat != ConflictFormat2);
 }
 
 TEST_F(FileCheckTest, FormatKindEqualityOperators) {
   ExpressionFormat UnsignedFormat(ExpressionFormat::Kind::Unsigned);
   EXPECT_TRUE(UnsignedFormat == ExpressionFormat::Kind::Unsigned);
   EXPECT_FALSE(UnsignedFormat != ExpressionFormat::Kind::Unsigned);
   EXPECT_FALSE(UnsignedFormat == ExpressionFormat::Kind::HexLower);
   EXPECT_TRUE(UnsignedFormat != ExpressionFormat::Kind::HexLower);
-  ExpressionFormat ConflictFormat(ExpressionFormat::Kind::Conflict);
-  EXPECT_TRUE(ConflictFormat == ExpressionFormat::Kind::Conflict);
-  EXPECT_FALSE(ConflictFormat != ExpressionFormat::Kind::Conflict);
   ExpressionFormat NoFormat(ExpressionFormat::Kind::NoFormat);
   EXPECT_TRUE(NoFormat == ExpressionFormat::Kind::NoFormat);
   EXPECT_FALSE(NoFormat != ExpressionFormat::Kind::NoFormat);
 }
 
 TEST_F(FileCheckTest, Literal) {
+  SourceMgr SM;
+
   // Eval returns the literal's value.
-  ExpressionLiteral Ten(10);
+  ExpressionLiteral Ten(bufferize(SM, "10"), 10);
   Expected<uint64_t> Value = Ten.eval();
   ASSERT_THAT_EXPECTED(Value, Succeeded());
   EXPECT_EQ(10U, *Value);
-  EXPECT_EQ(Ten.getImplicitFormat(), ExpressionFormat::Kind::NoFormat);
+  Expected<ExpressionFormat> ImplicitFormat = Ten.getImplicitFormat(SM);
+  ASSERT_THAT_EXPECTED(ImplicitFormat, Succeeded());
+  EXPECT_EQ(*ImplicitFormat, ExpressionFormat::Kind::NoFormat);
 
   // Max value can be correctly represented.
-  ExpressionLiteral Max(std::numeric_limits<uint64_t>::max());
+  uint64_t MaxUint64 = std::numeric_limits<uint64_t>::max();
+  ExpressionLiteral Max(bufferize(SM, std::to_string(MaxUint64)), MaxUint64);
   Value = Max.eval();
   ASSERT_THAT_EXPECTED(Value, Succeeded());
   EXPECT_EQ(std::numeric_limits<uint64_t>::max(), *Value);
 }
 
-static std::string toString(const std::unordered_set<std::string> &Set) {
-  bool First = true;
-  std::string Str;
-  for (StringRef S : Set) {
-    Str += Twine(First ? "{" + S : ", " + S).str();
-    First = false;
-  }
-  Str += '}';
-  return Str;
-}
-
 TEST_F(FileCheckTest, Expression) {
+  SourceMgr SM;
+
   std::unique_ptr<ExpressionLiteral> Ten =
-      std::make_unique<ExpressionLiteral>(10);
+      std::make_unique<ExpressionLiteral>(bufferize(SM, "10"), 10);
   ExpressionLiteral *TenPtr = Ten.get();
   Expression Expr(std::move(Ten),
                   ExpressionFormat(ExpressionFormat::Kind::HexLower));
   EXPECT_EQ(Expr.getAST(), TenPtr);
   EXPECT_EQ(Expr.getFormat(), ExpressionFormat::Kind::HexLower);
 }
 
 static void
 expectUndefErrors(std::unordered_set<std::string> ExpectedUndefVarNames,
                   Error Err) {
   EXPECT_THAT_ERROR(handleErrors(std::move(Err),
                                  [&](const UndefVarError &E) {
                                    EXPECT_EQ(ExpectedUndefVarNames.erase(
                                                  std::string(E.getVarName())),
                                              1U);
                                  }),
                     Succeeded());
   EXPECT_TRUE(ExpectedUndefVarNames.empty()) << toString(ExpectedUndefVarNames);
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
 TEST_F(FileCheckTest, NumericVariable) {
+  SourceMgr SM;
+
   // Undefined variable: getValue and eval fail, error returned by eval holds
   // the name of the undefined variable.
   NumericVariable FooVar("FOO",
                          ExpressionFormat(ExpressionFormat::Kind::Unsigned), 1);
   EXPECT_EQ("FOO", FooVar.getName());
   EXPECT_EQ(FooVar.getImplicitFormat(), ExpressionFormat::Kind::Unsigned);
   NumericVariableUse FooVarUse("FOO", &FooVar);
-  EXPECT_EQ(FooVarUse.getImplicitFormat(), ExpressionFormat::Kind::Unsigned);
+  Expected<ExpressionFormat> ImplicitFormat = FooVarUse.getImplicitFormat(SM);
+  ASSERT_THAT_EXPECTED(ImplicitFormat, Succeeded());
+  EXPECT_EQ(*ImplicitFormat, ExpressionFormat::Kind::Unsigned);
   EXPECT_FALSE(FooVar.getValue());
   Expected<uint64_t> EvalResult = FooVarUse.eval();
   expectUndefErrors({"FOO"}, EvalResult.takeError());
 
   FooVar.setValue(42);
 
   // Defined variable: getValue and eval return value set.
   Optional<uint64_t> Value = FooVar.getValue();
   ASSERT_TRUE(Value);
   EXPECT_EQ(42U, *Value);
   EvalResult = FooVarUse.eval();
   ASSERT_THAT_EXPECTED(EvalResult, Succeeded());
   EXPECT_EQ(42U, *EvalResult);
 
   // Clearing variable: getValue and eval fail. Error returned by eval holds
   // the name of the cleared variable.
   FooVar.clearValue();
   EXPECT_FALSE(FooVar.getValue());
   EvalResult = FooVarUse.eval();
   expectUndefErrors({"FOO"}, EvalResult.takeError());
 }
 
 TEST_F(FileCheckTest, Binop) {
-  NumericVariable FooVar("FOO",
+  SourceMgr SM;
+
+  StringRef ExprStr = bufferize(SM, "FOO+BAR");
+  StringRef FooStr = ExprStr.take_front(3);
+  NumericVariable FooVar(FooStr,
                          ExpressionFormat(ExpressionFormat::Kind::Unsigned), 1);
   FooVar.setValue(42);
   std::unique_ptr<NumericVariableUse> FooVarUse =
-      std::make_unique<NumericVariableUse>("FOO", &FooVar);
-  NumericVariable BarVar("BAR",
+      std::make_unique<NumericVariableUse>(FooStr, &FooVar);
+  StringRef BarStr = ExprStr.take_back(3);
+  NumericVariable BarVar(BarStr,
                          ExpressionFormat(ExpressionFormat::Kind::Unsigned), 2);
   BarVar.setValue(18);
   std::unique_ptr<NumericVariableUse> BarVarUse =
-      std::make_unique<NumericVariableUse>("BAR", &BarVar);
-  BinaryOperation Binop(doAdd, std::move(FooVarUse), std::move(BarVarUse));
+      std::make_unique<NumericVariableUse>(BarStr, &BarVar);
+  BinaryOperation Binop(ExprStr, doAdd, std::move(FooVarUse),
+                        std::move(BarVarUse));
 
   // Defined variables: eval returns right value; implicit format is as
   // expected.
   Expected<uint64_t> Value = Binop.eval();
   ASSERT_THAT_EXPECTED(Value, Succeeded());
   EXPECT_EQ(60U, *Value);
-  EXPECT_EQ(Binop.getImplicitFormat(), ExpressionFormat::Kind::Unsigned);
+  Expected<ExpressionFormat> ImplicitFormat = Binop.getImplicitFormat(SM);
+  ASSERT_THAT_EXPECTED(ImplicitFormat, Succeeded());
+  EXPECT_EQ(*ImplicitFormat, ExpressionFormat::Kind::Unsigned);
 
   // 1 undefined variable: eval fails, error contains name of undefined
   // variable.
   FooVar.clearValue();
   Value = Binop.eval();
   expectUndefErrors({"FOO"}, Value.takeError());
 
   // 2 undefined variables: eval fails, error contains names of all undefined
   // variables.
   BarVar.clearValue();
   Value = Binop.eval();
   expectUndefErrors({"FOO", "BAR"}, Value.takeError());
 
   // Literal + Variable has format of variable.
-  FooVarUse = std::make_unique<NumericVariableUse>("FOO", &FooVar);
+  ExprStr = bufferize(SM, "FOO+18");
+  FooStr = ExprStr.take_front(3);
+  StringRef EighteenStr = ExprStr.take_back(2);
+  FooVarUse = std::make_unique<NumericVariableUse>(FooStr, &FooVar);
   std::unique_ptr<ExpressionLiteral> Eighteen =
-      std::make_unique<ExpressionLiteral>(18);
-  Binop = BinaryOperation(doAdd, std::move(FooVarUse), std::move(Eighteen));
-  EXPECT_EQ(Binop.getImplicitFormat(), ExpressionFormat::Kind::Unsigned);
-  FooVarUse = std::make_unique<NumericVariableUse>("FOO", &FooVar);
-  Eighteen = std::make_unique<ExpressionLiteral>(18);
-  Binop = BinaryOperation(doAdd, std::move(Eighteen), std::move(FooVarUse));
-  EXPECT_EQ(Binop.getImplicitFormat(), ExpressionFormat::Kind::Unsigned);
+      std::make_unique<ExpressionLiteral>(EighteenStr, 18);
+  Binop = BinaryOperation(ExprStr, doAdd, std::move(FooVarUse),
+                          std::move(Eighteen));
+  ImplicitFormat = Binop.getImplicitFormat(SM);
+  ASSERT_THAT_EXPECTED(ImplicitFormat, Succeeded());
+  EXPECT_EQ(*ImplicitFormat, ExpressionFormat::Kind::Unsigned);
+  ExprStr = bufferize(SM, "18+FOO");
+  FooStr = ExprStr.take_back(3);
+  EighteenStr = ExprStr.take_front(2);
+  FooVarUse = std::make_unique<NumericVariableUse>(FooStr, &FooVar);
+  Eighteen = std::make_unique<ExpressionLiteral>(EighteenStr, 18);
+  Binop = BinaryOperation(ExprStr, doAdd, std::move(Eighteen),
+                          std::move(FooVarUse));
+  ImplicitFormat = Binop.getImplicitFormat(SM);
+  ASSERT_THAT_EXPECTED(ImplicitFormat, Succeeded());
+  EXPECT_EQ(*ImplicitFormat, ExpressionFormat::Kind::Unsigned);
 
   // Variables with different implicit format conflict.
-  NumericVariable BazVar("BAZ",
+  ExprStr = bufferize(SM, "FOO+BAZ");
+  FooStr = ExprStr.take_front(3);
+  StringRef BazStr = ExprStr.take_back(3);
+  NumericVariable BazVar(BazStr,
                          ExpressionFormat(ExpressionFormat::Kind::HexLower), 3);
-  FooVarUse = std::make_unique<NumericVariableUse>("BAZ", &FooVar);
+  FooVarUse = std::make_unique<NumericVariableUse>(FooStr, &FooVar);
   std::unique_ptr<NumericVariableUse> BazVarUse =
-      std::make_unique<NumericVariableUse>("BAZ", &BazVar);
-  Binop = BinaryOperation(doAdd, std::move(FooVarUse), std::move(BazVarUse));
-  EXPECT_EQ(Binop.getImplicitFormat(), ExpressionFormat::Kind::Conflict);
+      std::make_unique<NumericVariableUse>(BazStr, &BazVar);
+  Binop = BinaryOperation(ExprStr, doAdd, std::move(FooVarUse),
+                          std::move(BazVarUse));
+  ImplicitFormat = Binop.getImplicitFormat(SM);
+  expectDiagnosticError(
+      "implicit format conflict between 'FOO' (%u) and 'BAZ' (%x), "
+      "need an explicit format specifier",
+      ImplicitFormat.takeError());
+
+  // All variable conflicts are reported.
+  ExprStr = bufferize(SM, "(FOO+BAZ)+(FOO+QUUX)");
+  StringRef Paren1ExprStr = ExprStr.substr(1, 7);
+  FooStr = Paren1ExprStr.take_front(3);
+  BazStr = Paren1ExprStr.take_back(3);
+  StringRef Paren2ExprStr = ExprStr.substr(ExprStr.rfind('(') + 1, 8);
+  StringRef FooStr2 = Paren2ExprStr.take_front(3);
+  StringRef QuuxStr = Paren2ExprStr.take_back(4);
+  FooVarUse = std::make_unique<NumericVariableUse>(FooStr, &FooVar);
+  BazVarUse = std::make_unique<NumericVariableUse>(BazStr, &BazVar);
+  std::unique_ptr<NumericVariableUse> FooVarUse2 =
+      std::make_unique<NumericVariableUse>(FooStr2, &FooVar);
+  NumericVariable QuuxVar(
+      QuuxStr, ExpressionFormat(ExpressionFormat::Kind::HexLower), 4);
+  std::unique_ptr<NumericVariableUse> QuuxVarUse =
+      std::make_unique<NumericVariableUse>(QuuxStr, &QuuxVar);
+  std::unique_ptr<BinaryOperation> Binop1 = std::make_unique<BinaryOperation>(
+      ExprStr.take_front(9), doAdd, std::move(FooVarUse), std::move(BazVarUse));
+  std::unique_ptr<BinaryOperation> Binop2 = std::make_unique<BinaryOperation>(
+      ExprStr.take_back(10), doAdd, std::move(FooVarUse2),
+      std::move(QuuxVarUse));
+  std::unique_ptr<BinaryOperation> OuterBinop =
+      std::make_unique<BinaryOperation>(ExprStr, doAdd, std::move(Binop1),
+                                        std::move(Binop2));
+  ImplicitFormat = OuterBinop->getImplicitFormat(SM);
+  expectSameErrors<ErrorDiagnostic>(
+      {"implicit format conflict between 'FOO' (%u) and 'BAZ' (%x), "
+       "need an explicit format specifier",
+       "implicit format conflict between 'FOO' (%u) and 'QUUX' (%x), "
+       "need an explicit format specifier"},
+      ImplicitFormat.takeError());
 }
 
 TEST_F(FileCheckTest, ValidVarNameStart) {
   EXPECT_TRUE(Pattern::isValidVarNameStart('a'));
   EXPECT_TRUE(Pattern::isValidVarNameStart('G'));
   EXPECT_TRUE(Pattern::isValidVarNameStart('_'));
   EXPECT_FALSE(Pattern::isValidVarNameStart('2'));
   EXPECT_FALSE(Pattern::isValidVarNameStart('$'));
TEST_F(FileCheckTest, ParseNumericSubstitutionBlock) {
   // conflict.
   EXPECT_THAT_EXPECTED(Tester.parseSubst("FOO+VAR_UNSIGNED"), Succeeded());
 
   // Valid implicit format conflict in presence of explicit formats.
   EXPECT_THAT_EXPECTED(Tester.parseSubst("%X,FOO+VAR_LOWER_HEX"), Succeeded());
 
   // Implicit format conflict.
   expectDiagnosticError(
-      "variables with conflicting format specifier: need an explicit one",
+      "implicit format conflict between 'FOO' (%u) and "
+      "'VAR_LOWER_HEX' (%x), need an explicit format specifier",
       Tester.parseSubst("FOO+VAR_LOWER_HEX").takeError());
 }
 
 TEST_F(FileCheckTest, ParsePattern) {
   PatternTester Tester;
 
   // Invalid space in string substitution.
   EXPECT_TRUE(Tester.parsePattern("[[ BAR]]"));