[FileCheck] Better diagnostic for format conflict

Thomas Preud'homme · memfrob · commit 31c81d7f0e83 · 2022-10-04T16:48:36.000-04:00
Summary: Improve error message in case of conflict between several implicit format to mention the operand that conflict. Reviewers: jhenderson, jdenny, probinson, grimar, arichardson, rnk Reviewed By: jdenny Subscribers: hiraditya, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D77741
diff --git a/llvm/lib/Support/FileCheck.cpp b/llvm/lib/Support/FileCheck.cpp
@@ -25,6 +25,20 @@
 
 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:
@@ -71,7 +85,7 @@ Expected<uint64_t> NumericVariableUse::eval() const {
   if (Value)
     return *Value;
 
-  return make_error<UndefVarError>(Name);
+  return make_error<UndefVarError>(getExpressionStr());
 }
 
 Expected<uint64_t> BinaryOperation::eval() const {
@@ -92,18 +106,31 @@ Expected<uint64_t> BinaryOperation::eval() const {
   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);
+  }
 
-  return Format;
+  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 *LeftFormat != ExpressionFormat::Kind::NoFormat ? *LeftFormat
+                                                         : *RightFormat;
 }
 
 Expected<std::string> NumericSubstitution::getResult() const {
@@ -262,9 +289,12 @@ Expected<std::unique_ptr<ExpressionAST>> Pattern::parseNumericOperand(
 
   // 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 + "'");
@@ -279,17 +309,18 @@ static uint64_t sub(uint64_t LeftOp, uint64_t 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 '+':
@@ -304,19 +335,20 @@ Pattern::parseBinop(StringRef &Expr, std::unique_ptr<ExpressionAST> LeftOp,
   }
 
   // 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));
 }
 
@@ -370,17 +402,18 @@ Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(
 
   // 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(
@@ -396,19 +429,17 @@ Expected<std::unique_ptr<Expression>> Pattern::parseNumericSubstitutionBlock(
   // 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 =
diff --git a/llvm/lib/Support/FileCheckImpl.h b/llvm/lib/Support/FileCheckImpl.h
@@ -39,9 +39,6 @@ struct ExpressionFormat {
     /// 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.
@@ -55,9 +52,7 @@ struct ExpressionFormat {
 
 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.
@@ -73,16 +68,19 @@ struct ExpressionFormat {
 
   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
@@ -95,17 +93,26 @@ struct ExpressionFormat {
 
 /// 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();
   }
 };
@@ -117,8 +124,10 @@ class ExpressionLiteral : public ExpressionAST {
   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; }
@@ -222,21 +231,18 @@ class NumericVariable {
 /// 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();
   }
 };
@@ -257,9 +263,10 @@ class BinaryOperation : public ExpressionAST {
   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);
   }
@@ -270,10 +277,12 @@ class BinaryOperation : public ExpressionAST {
   /// 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;
@@ -663,17 +672,20 @@ class Pattern {
   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);
 };
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/test/FileCheck/numeric-expression.txt b/llvm/test/FileCheck/numeric-expression.txt
@@ -185,16 +185,27 @@ 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
diff --git a/llvm/unittests/Support/FileCheckTest.cpp b/llvm/unittests/Support/FileCheckTest.cpp
