[FileCheck] Improve printing variables with escapes #145865
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-testing-tools Author: Mészáros Gergely (Maetveis) ChangesFirstly fix FileCheck printing string variables This is confusing because it is not clear if the printed Secondly, only escape when doing so makes it easier to read the value The common case that is motivating this change is variables that contain Full diff: https://github.com/llvm/llvm-project/pull/145865.diff 3 Files Affected:
diff --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index bcca499322aee..5ddb7420820dc 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/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,7 +274,17 @@ 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)
@@ -282,6 +292,37 @@ Expected<std::string> StringSubstitution::getResult() const {
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 tabs, newlines, quotes, and non-printable characters.
+ // Note that we do not include backslashes in this set, 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 C == '\t' || C == '\n' || C == '"' || !isPrint(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,7 +1251,7 @@ 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());
@@ -1218,8 +1259,8 @@ void Pattern::printSubstitutions(const SourceMgr &SM, StringRef Buffer,
}
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.
diff --git a/llvm/lib/FileCheck/FileCheckImpl.h b/llvm/lib/FileCheck/FileCheckImpl.h
index b3cd2af5d57e3..176668a3e5154 100644
--- a/llvm/lib/FileCheck/FileCheckImpl.h
+++ b/llvm/lib/FileCheck/FileCheckImpl.h
@@ -366,9 +366,14 @@ class Substitution {
/// \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
+ /// \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> getResult() const = 0;
+ virtual Expected<std::string> getResultRegex() const = 0;
+
+ /// \returns a string containing the result of the substitution represented
+ /// 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 +384,11 @@ 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 +406,11 @@ 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;
};
//===----------------------------------------------------------------------===//
diff --git a/llvm/test/FileCheck/var-escape.txt b/llvm/test/FileCheck/var-escape.txt
new file mode 100644
index 0000000000000..b18caf701ceac
--- /dev/null
+++ b/llvm/test/FileCheck/var-escape.txt
@@ -0,0 +1,17 @@
+; RUN: echo -e "WINPATH=A:\windows\style\path" > %t
+; RUN: echo -e "NOT_ESCAPED=shouldn't be escaped [a-Z]\+$" >> %t
+; RUN: echo -e 'ESCAPED=\\ needs\to "be" escaped\000' >> %t
+
+VARS: WINPATH=[[WINPATH:.*]]
+VARS: NOT_ESCAPED=[[NOT_ESCAPED:.*]]
+VARS-NEXT: ESCAPED=[[ESCAPED:.*]]
+; Trigger a failed match, to show variables values
+VARS-NEXT: [[WINPATH]] [[NOT_ESCAPED]] [[ESCAPED]]
+
+; RUN: %ProtectFileCheckOutput not FileCheck \
+; RUN: -dump-input=never --strict-whitespace --check-prefix=VARS --input-file=%t %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 "\\ needs\to \"be\" escaped\000" (escaped value)
|
Contributor
Author
|
Note for reviewers: I've split this to two commits, the first only adding the test, to show the effect it will have. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
RoboTux
reviewed
Jun 26, 2025
Contributor
RoboTux
left a comment
RoboTux
left a comment
There was a problem hiding this comment.
Note that although the commit are split when landing they are squashed into one. Might be worth creating two pull request.
Maetveis
force-pushed
the
filecheck_var_escapes
branch
from
15a5395 to
7a8de3c
Compare
Maetveis
changed the base branch from
main
to
users/Maetveis/filecheck_var_escapes_precommit
Contributor
Author
|
I separated the pre-commit test to #145906 |
RoboTux
reviewed
Jun 26, 2025
Contributor
RoboTux
left a comment
RoboTux
left a comment
There was a problem hiding this comment.
LGTM, happy to approve once rebased on the PR145906
Maetveis
added a commit
that referenced
this pull request
Jun 26, 2025
Maetveis
changed the base branch from
users/Maetveis/filecheck_var_escapes_precommit
to
main
Firstly fix FileCheck printing string variables double-escaped (first regex, then C-style). This is confusing because it is not clear if the printed value is the literal value or exactly how it is escaped, without looking at FileCheck's source code. Secondly, only escape when doing so makes it easier to read the value (when the string contains tabs, newlines or non-printable characters). When the variable value is escaped, make a note of it in the output too, in order to avoid confusion. The common case that is motivating this change is variables that contain windows style paths with backslashes. These were printed as `"C:\\\\Program Files\\\\MyApp\\\\file.txt"`. Now prefer to print them as `"C:\Program Files\MyApp\file.txt"`. Printing the value literally also makes it easier to search for variables in the output, since the user can just copy-paste it.
Maetveis
force-pushed
the
filecheck_var_escapes
branch
from
e2320aa to
937b192
Compare
anthonyhatran
pushed a commit
to anthonyhatran/llvm-project
that referenced
this pull request
Jun 26, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/25545 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Firstly fix FileCheck printing string variables
double-escaped (first regex, then C-style).
This is confusing because it is not clear if the printed
value is the literal value or exactly how it is escaped, without
looking at FileCheck's source code.
Secondly, only escape when doing so makes it easier to read the value
(when the string contains tabs, newlines or non-printable characters).
When the variable value is escaped, make a note of it in the output too,
in order to avoid confusion.
The common case that is motivating this change is variables that contain
windows style paths with backslashes. These were printed as
"C:\\\\Program Files\\\\MyApp\\\\file.txt".Now prefer to print them as
"C:\Program Files\MyApp\file.txt".Printing the value literally also makes it easier to search for
variables in the output, since the user can just copy-paste it.