[FileCheck] Improve printing variables with escapes

llvm/lib/FileCheck/FileCheck.cpp

@@ -264,7 +264,7 @@ BinaryOperation::getImplicitFormat(const SourceMgr &SM) const {
                                                          : *RightFormat;
 }
 
-Expected<std::string> NumericSubstitution::getResult() const {
+Expected<std::string> NumericSubstitution::getResultRegex() const {
   assert(ExpressionPointer->getAST() != nullptr &&
          "Substituting empty expression");
   Expected<APInt> EvaluatedValue = ExpressionPointer->getAST()->eval();
@@ -274,14 +274,55 @@ Expected<std::string> NumericSubstitution::getResult() const {
   return Format.getMatchingString(*EvaluatedValue);
 }
 
-Expected<std::string> StringSubstitution::getResult() const {
+Expected<std::string> NumericSubstitution::getResultForDiagnostics() const {
+  // The "regex" returned by getResultRegex() is just a numeric value
+  // like '42', '0x2A', '-17', 'DEADBEEF' etc. This is already suitable for use
+  // in diagnostics.
+  Expected<std::string> Literal = getResultRegex();
+  if (!Literal)
+    return Literal;
+
+  return "\"" + std::move(*Literal) + "\"";
+}
+
+Expected<std::string> StringSubstitution::getResultRegex() const {
   // Look up the value and escape it so that we can put it into the regex.
   Expected<StringRef> VarVal = Context->getPatternVarValue(FromStr);
   if (!VarVal)
     return VarVal.takeError();
   return Regex::escape(*VarVal);
 }
 
+Expected<std::string> StringSubstitution::getResultForDiagnostics() const {
+  Expected<StringRef> VarVal = Context->getPatternVarValue(FromStr);
+  if (!VarVal)
+    return VarVal.takeError();
+
+  std::string Result;
+  Result.reserve(VarVal->size() + 2);
+  raw_string_ostream OS(Result);
+
+  OS << '"';
+  // Escape the string if it contains any characters that
+  // make it hard to read, such as non-printable characters (including all
+  // whitespace except space) and double quotes. These are the characters that
+  // are escaped by write_escaped(), except we do not include backslashes,
+  // because they are common in Windows paths and escaping them would make the
+  // output harder to read. However, when we do escape, backslashes are escaped
+  // as well, otherwise the output would be ambiguous.
+  const bool NeedsEscaping =
+      llvm::any_of(*VarVal, [](char C) { return !isPrint(C) || C == '"'; });
+  if (NeedsEscaping)
+    OS.write_escaped(*VarVal);
+  else
+    OS << *VarVal;
+  OS << '"';
+  if (NeedsEscaping)
+    OS << " (escaped value)";
+
+  return Result;
+}
+
 bool Pattern::isValidVarNameStart(char C) { return C == '_' || isAlpha(C); }
 
 Expected<Pattern::VariableProperties>
@@ -1106,7 +1147,7 @@ Pattern::MatchResult Pattern::match(StringRef Buffer,
     Error Errs = Error::success();
     for (const auto &Substitution : Substitutions) {
       // Substitute and check for failure (e.g. use of undefined variable).
-      Expected<std::string> Value = Substitution->getResult();
+      Expected<std::string> Value = Substitution->getResultRegex();
       if (!Value) {
         // Convert to an ErrorDiagnostic to get location information. This is
         // done here rather than printMatch/printNoMatch since now we know which
@@ -1210,16 +1251,17 @@ void Pattern::printSubstitutions(const SourceMgr &SM, StringRef Buffer,
       SmallString<256> Msg;
       raw_svector_ostream OS(Msg);
 
-      Expected<std::string> MatchedValue = Substitution->getResult();
+      Expected<std::string> MatchedValue =
+          Substitution->getResultForDiagnostics();
       // Substitution failures are handled in printNoMatch().
       if (!MatchedValue) {
         consumeError(MatchedValue.takeError());
         continue;
       }
 
       OS << "with \"";
-      OS.write_escaped(Substitution->getFromString()) << "\" equal to \"";
-      OS.write_escaped(*MatchedValue) << "\"";
+      OS.write_escaped(Substitution->getFromString()) << "\" equal to ";
+      OS << *MatchedValue;
 
       // We report only the start of the match/search range to suggest we are
       // reporting the substitutions as set at the start of the match/search.

llvm/lib/FileCheck/FileCheckImpl.h

@@ -366,9 +366,15 @@ class Substitution {
   /// \returns the index where the substitution is to be performed in RegExStr.
   size_t getIndex() const { return InsertIdx; }
 
+  /// \returns a regular expression string that matches the result of the
+  /// substitution represented by this class instance or an error if
+  /// substitution failed.
+  virtual Expected<std::string> getResultRegex() const = 0;
+
   /// \returns a string containing the result of the substitution represented
-  /// by this class instance or an error if substitution failed.
-  virtual Expected<std::string> getResult() const = 0;
+  /// by this class instance in a form suitable for diagnostics, or an error if
+  /// substitution failed.
+  virtual Expected<std::string> getResultForDiagnostics() const = 0;
 };
 
 class StringSubstitution : public Substitution {
@@ -379,7 +385,12 @@ class StringSubstitution : public Substitution {
 
   /// \returns the text that the string variable in this substitution matched
   /// when defined, or an error if the variable is undefined.
-  Expected<std::string> getResult() const override;
+  Expected<std::string> getResultRegex() const override;
+
+  /// \returns the text that the string variable in this substitution matched
+  /// when defined, in a form suitable for diagnostics, or an error if the
+  /// variable is undefined.
+  Expected<std::string> getResultForDiagnostics() const override;
 };
 
 class NumericSubstitution : public Substitution {
@@ -397,7 +408,12 @@ class NumericSubstitution : public Substitution {
 
   /// \returns a string containing the result of evaluating the expression in
   /// this substitution, or an error if evaluation failed.
-  Expected<std::string> getResult() const override;
+  Expected<std::string> getResultRegex() const override;
+
+  /// \returns a string containing the result of evaluating the expression in
+  /// this substitution, in a form suitable for diagnostics, or an error if
+  /// evaluation failed.
+  Expected<std::string> getResultForDiagnostics() const override;
 };
 
 //===----------------------------------------------------------------------===//

llvm/test/FileCheck/var-escape.txt

@@ -14,9 +14,9 @@ VARS-NEXT: [[WINPATH]] [[NOT_ESCAPED]] [[ESCAPED]] [[#$NUMERIC + 0]]
 ; RUN:   -dump-input=never --strict-whitespace --check-prefix=VARS --input-file=%t.1 %s 2>&1 \
 ; RUN: | FileCheck %s
 
-CHECK: with "WINPATH" equal to "A:\\\\windows\\\\style\\\\path"
-CHECK: with "NOT_ESCAPED" equal to "shouldn't be escaped \\[a-Z\\]\\\\\\+\\$"
-CHECK: with "ESCAPED" equal to "\\\\ \014\013 needs\to \"be\" escaped\\\000"
+CHECK: with "WINPATH" equal to "A:\windows\style\path"
+CHECK: with "NOT_ESCAPED" equal to "shouldn't be escaped [a-Z]\+$"
+CHECK: with "ESCAPED" equal to "\\ \014\013 needs\to \"be\" escaped\000" (escaped value)
 CHECK: with "$NUMERIC + 0" equal to "DEADBEEF"
 
 ; Test escaping of the name of a numeric substitution, which might contain

llvm/unittests/FileCheck/FileCheckTest.cpp

@@ -1439,7 +1439,7 @@ TEST_F(FileCheckTest, Substitution) {
   // Substitution of an undefined string variable fails and error holds that
   // variable's name.
   StringSubstitution StringSubstitution(&Context, "VAR404", 42);
-  Expected<std::string> SubstValue = StringSubstitution.getResult();
+  Expected<std::string> SubstValue = StringSubstitution.getResultRegex();
   expectUndefErrors({"VAR404"}, SubstValue.takeError());
 
   // Numeric substitution blocks constituted of defined numeric variables are
@@ -1452,20 +1452,20 @@ TEST_F(FileCheckTest, Substitution) {
       std::move(NVarUse), ExpressionFormat(ExpressionFormat::Kind::HexUpper));
   NumericSubstitution SubstitutionN(&Context, "N", std::move(ExpressionN),
                                     /*InsertIdx=*/30);
-  SubstValue = SubstitutionN.getResult();
+  SubstValue = SubstitutionN.getResultRegex();
   ASSERT_THAT_EXPECTED(SubstValue, Succeeded());
   EXPECT_EQ("A", *SubstValue);
 
   // Substitution of an undefined numeric variable fails, error holds name of
   // undefined variable.
   NVar.clearValue();
-  SubstValue = SubstitutionN.getResult();
+  SubstValue = SubstitutionN.getResultRegex();
   expectUndefErrors({"N"}, SubstValue.takeError());
 
   // Substitution of a defined string variable returns the right value.
   Pattern P(Check::CheckPlain, &Context, 1);
   StringSubstitution = llvm::StringSubstitution(&Context, "FOO", 42);
-  SubstValue = StringSubstitution.getResult();
+  SubstValue = StringSubstitution.getResultRegex();
   ASSERT_THAT_EXPECTED(SubstValue, Succeeded());
   EXPECT_EQ("BAR", *SubstValue);
 }