From 18c3622becbd018208a0c4b53ca49528ce144e5d Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Fri, 1 Dec 2023 09:03:03 -0800 Subject: [PATCH] Have autoupdate discard conflict markers when possible. (#3440) This now puts file content into a string, allowing conflict markers to be elided from file content. When code executes, this means it executes without seeing conflict markers, without a temporary update to the file that would only remove conflict markers. Also refactors the main process flow, because it was getting a little too lengthy. This means passing a bunch of parameters passed around (partly because TestContext is private on the test class, and I don't want to change that). I'm hoping that overall it's easier to read the core loop now. Mostly tested with some manually added conflict markers, and that current tests don't change. --------- Co-authored-by: Richard Smith --- testing/file_test/file_test_base.cpp | 444 +++++++++++++++++++-------- testing/file_test/file_test_base.h | 10 +- 2 files changed, 318 insertions(+), 136 deletions(-) diff --git a/testing/file_test/file_test_base.cpp b/testing/file_test/file_test_base.cpp index ac7eca2072eb..446135932a6c 100644 --- a/testing/file_test/file_test_base.cpp +++ b/testing/file_test/file_test_base.cpp @@ -13,6 +13,7 @@ #include "absl/flags/flag.h" #include "absl/flags/parse.h" #include "common/check.h" +#include "common/error.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/Twine.h" #include "llvm/Support/FormatVariadic.h" @@ -265,151 +266,146 @@ auto FileTestBase::DoArgReplacements( return Success(); } -auto FileTestBase::ProcessTestFile(TestContext& context) -> ErrorOr { - // Original file content, and a cursor for walking through it. - llvm::StringRef file_content = context.input_content; - llvm::StringRef cursor = file_content; +// Processes conflict markers, including tracking of whether code is within a +// conflict marker. Returns true if the line is consumed. +static auto TryConsumeConflictMarker(llvm::StringRef line, + llvm::StringRef line_trimmed, + bool* inside_conflict_marker) + -> ErrorOr { + bool is_start = line.starts_with("<<<<<<<"); + bool is_middle = line.starts_with("======="); + bool is_end = line.starts_with(">>>>>>>"); - // Whether content has been found, only updated before a file split is found + // When running the test, any conflict marker is an error. + if (!absl::GetFlag(FLAGS_autoupdate) && (is_start || is_middle || is_end)) { + return ErrorBuilder() << "Conflict marker found:\n" << line; + } + + // Autoupdate tracks conflict markers for context, and will discard + // conflicting lines when it can autoupdate them. + if (*inside_conflict_marker) { + if (is_start) { + return ErrorBuilder() << "Unexpected conflict marker inside conflict:\n" + << line; + } + if (is_middle) { + return true; + } + if (is_end) { + *inside_conflict_marker = false; + return true; + } + + // Look for CHECK lines, which can be discarded. + if (line_trimmed.starts_with("// CHECK:STDOUT:") || + line_trimmed.starts_with("// CHECK:STDERR:")) { + return true; + } + + return ErrorBuilder() + << "Autoupdate can't discard non-CHECK lines inside conflicts:\n" + << line; + } else { + if (is_start) { + *inside_conflict_marker = true; + return true; + } + if (is_middle || is_end) { + return ErrorBuilder() << "Unexpected conflict marker outside conflict:\n" + << line; + } + return false; + } +} + +// State for file splitting logic: TryConsumeSplit and FinishSplit. +struct SplitState { + auto has_splits() const -> bool { return file_index > 0; } + + auto add_content(llvm::StringRef line) -> void { + content.append(line); + content.append("\n"); + } + + // Whether content has been found. Only updated before a file split is found // (which may be never). - bool found_content_pre_split = false; + bool found_code_pre_split = false; - // Whether either AUTOUDPATE or NOAUTOUPDATE was found. - bool found_autoupdate = false; + // The current file name, considering splits. Empty for the default file. + llvm::StringRef filename = ""; - // The index in the current test file. Will be reset on splits. - int line_index = 0; + // The accumulated content for the file being built. This may elide some of + // the original content, such as conflict markers. + std::string content; - // The current file name, considering splits. Not set for the default file. - llvm::StringRef current_file_name; + // The current file index. + int file_index = 0; +}; - // The current file's start. - const char* current_file_start = nullptr; +// Adds a file. Used for both split and unsplit test files. +static auto AddTestFile(llvm::StringRef filename, std::string* content, + llvm::SmallVector* test_files) + -> void { + test_files->push_back( + {.filename = filename.str(), .content = std::move(*content)}); + content->clear(); +} - int file_number = 0; - while (!cursor.empty()) { - auto [line, next_cursor] = cursor.split("\n"); - cursor = next_cursor; - auto line_trimmed = line.ltrim(); - - static constexpr llvm::StringLiteral SplitPrefix = "// ---"; - if (line_trimmed.consume_front(SplitPrefix)) { - if (!found_autoupdate) { - // If there's a split, all output is appended at the end of each file - // before AUTOUPDATE. We may want to change that, but it's not necessary - // to handle right now. - return ErrorBuilder() - << "AUTOUPDATE/NOAUTOUPDATE setting must be in the first file."; - } - - context.has_splits = true; - ++file_number; - context.non_check_lines.push_back(FileTestLine(file_number, 0, line)); - // On a file split, add the previous file, then start a new one. - if (current_file_start) { - context.test_files.push_back(TestFile( - current_file_name.str(), - llvm::StringRef(current_file_start, line_trimmed.begin() - - current_file_start - - SplitPrefix.size()))); - } else if (found_content_pre_split) { - // For the first split, we make sure there was no content prior. - return ErrorBuilder() - << "When using split files, there must be no content before the " - "first split file."; - } - current_file_name = line_trimmed.trim(); - current_file_start = cursor.begin(); - line_index = 0; - continue; - } else if (!current_file_start && !line_trimmed.starts_with("//") && - !line_trimmed.empty()) { - found_content_pre_split = true; +// Process file split ("---") lines when found. Returns true if the line is +// consumed. +static auto TryConsumeSplit( + llvm::StringRef line, llvm::StringRef line_trimmed, bool found_autoupdate, + int* line_index, SplitState* split, + llvm::SmallVector* test_files, + llvm::SmallVector* non_check_lines) -> ErrorOr { + if (!line_trimmed.consume_front("// ---")) { + if (!split->has_splits() && !line_trimmed.starts_with("//") && + !line_trimmed.empty()) { + split->found_code_pre_split = true; } - ++line_index; - // Process expectations when found. - if (line_trimmed.consume_front("// CHECK")) { - // Don't build expectations when doing an autoupdate. We don't want to - // break the autoupdate on an invalid CHECK line. - if (!absl::GetFlag(FLAGS_autoupdate)) { - llvm::SmallVector>* expected = nullptr; - if (line_trimmed.consume_front(":STDOUT:")) { - expected = &context.expected_stdout; - } else if (line_trimmed.consume_front(":STDERR:")) { - expected = &context.expected_stderr; - } else { - return ErrorBuilder() << "Unexpected CHECK in input: " << line.str(); - } - CARBON_ASSIGN_OR_RETURN(Matcher check_matcher, - TransformExpectation(line_index, line_trimmed)); - expected->push_back(check_matcher); - } - } else { - context.non_check_lines.push_back( - FileTestLine(file_number, line_index, line)); - if (line_trimmed.consume_front("// ARGS: ")) { - if (context.test_args.empty()) { - // Split the line into arguments. - std::pair cursor = - llvm::getToken(line_trimmed); - while (!cursor.first.empty()) { - context.test_args.push_back(std::string(cursor.first)); - cursor = llvm::getToken(cursor.second); - } - } else { - return ErrorBuilder() - << "ARGS was specified multiple times: " << line.str(); - } - } else if (line_trimmed == "// AUTOUPDATE" || - line_trimmed == "// NOAUTOUPDATE") { - if (found_autoupdate) { - return ErrorBuilder() - << "Multiple AUTOUPDATE/NOAUTOUPDATE settings found"; - } - found_autoupdate = true; - if (line_trimmed == "// AUTOUPDATE") { - context.autoupdate_line_number = line_index; - } - } else if (line_trimmed == "// SET-CHECK-SUBSET") { - if (!context.check_subset) { - context.check_subset = true; - } else { - return ErrorBuilder() - << "SET-CHECK-SUBSET was specified multiple times"; - } - } - } + // Add the line to the current file's content (which may not be a split + // file). + split->add_content(line); + return false; } if (!found_autoupdate) { - return ErrorBuilder() << "Missing AUTOUPDATE/NOAUTOUPDATE setting"; + // If there's a split, all output is appended at the end of each file + // before AUTOUPDATE. We may want to change that, but it's not + // necessary to handle right now. + return ErrorBuilder() << "AUTOUPDATE/NOAUTOUPDATE setting must be in " + "the first file."; } - if (current_file_start) { - context.test_files.push_back( - TestFile(current_file_name.str(), - llvm::StringRef(current_file_start, - file_content.end() - current_file_start))); + // On a file split, add the previous file, then start a new one. + if (split->has_splits()) { + AddTestFile(split->filename, &split->content, test_files); } else { - // If no file splitting happened, use the main file as the test file. - // There will always be a `/` unless tests are in the repo root. - context.test_files.push_back(TestFile( - test_name_.drop_front(test_name_.rfind("/") + 1).str(), file_content)); + split->content.clear(); + if (split->found_code_pre_split) { + // For the first split, we make sure there was no content prior. + return ErrorBuilder() << "When using split files, there must be no " + "content before the first split file."; + } } - // Assume there is always a suffix `\n` in output. - if (!context.expected_stdout.empty()) { - context.expected_stdout.push_back(StrEq("")); + ++split->file_index; + split->filename = line_trimmed.trim(); + if (split->filename.empty()) { + return ErrorBuilder() << "Missing filename for split."; } - if (!context.expected_stderr.empty()) { - context.expected_stderr.push_back(StrEq("")); - } - - return Success(); + // The split line is added to non_check_lines for retention in autoupdate, but + // is not added to the test file content. + *line_index = 0; + non_check_lines->push_back( + FileTestLine(split->file_index, *line_index, line)); + return true; } -auto FileTestBase::TransformExpectation(int line_index, llvm::StringRef in) +// Transforms an expectation on a given line from `FileCheck` syntax into a +// standard regex matcher. +static auto TransformExpectation(int line_index, llvm::StringRef in) -> ErrorOr> { if (in.empty()) { return Matcher{StrEq("")}; @@ -495,6 +491,197 @@ auto FileTestBase::TransformExpectation(int line_index, llvm::StringRef in) return Matcher{MatchesRegex(str)}; } +// Once all content is processed, do any remaining split processing. +static auto FinishSplit(llvm::StringRef test_name, SplitState* split, + llvm::SmallVector* test_files) + -> void { + if (split->has_splits()) { + AddTestFile(split->filename, &split->content, test_files); + } else { + // If no file splitting happened, use the main file as the test file. + // There will always be a `/` unless tests are in the repo root. + AddTestFile(test_name.drop_front(test_name.rfind("/") + 1), &split->content, + test_files); + } +} + +// Process CHECK lines when found. Returns true if the line is consumed. +static auto TryConsumeCheck( + int line_index, llvm::StringRef line, llvm::StringRef line_trimmed, + llvm::SmallVector>* expected_stdout, + llvm::SmallVector>* expected_stderr) + -> ErrorOr { + if (!line_trimmed.consume_front("// CHECK")) { + return false; + } + + // Don't build expectations when doing an autoupdate. We don't want to + // break the autoupdate on an invalid CHECK line. + if (!absl::GetFlag(FLAGS_autoupdate)) { + llvm::SmallVector>* expected; + if (line_trimmed.consume_front(":STDOUT:")) { + expected = expected_stdout; + } else if (line_trimmed.consume_front(":STDERR:")) { + expected = expected_stderr; + } else { + return ErrorBuilder() << "Unexpected CHECK in input: " << line.str(); + } + CARBON_ASSIGN_OR_RETURN(Matcher check_matcher, + TransformExpectation(line_index, line_trimmed)); + expected->push_back(check_matcher); + } + return true; +} + +// Processes ARGS lines when found. Returns true if the line is consumed. +static auto TryConsumeArgs(llvm::StringRef line, llvm::StringRef line_trimmed, + llvm::SmallVector* args) + -> ErrorOr { + if (!line_trimmed.consume_front("// ARGS: ")) { + return false; + } + + if (!args->empty()) { + return ErrorBuilder() << "ARGS was specified multiple times: " + << line.str(); + } + + // Split the line into arguments. + std::pair cursor = + llvm::getToken(line_trimmed); + while (!cursor.first.empty()) { + args->push_back(std::string(cursor.first)); + cursor = llvm::getToken(cursor.second); + } + + return true; +} + +// Processes AUTOUPDATE lines when found. Returns true if the line is consumed. +static auto TryConsumeAutoupdate(int line_index, llvm::StringRef line_trimmed, + bool* found_autoupdate, + std::optional* autoupdate_line_number) + -> ErrorOr { + static constexpr llvm::StringLiteral Autoupdate = "// AUTOUPDATE"; + static constexpr llvm::StringLiteral NoAutoupdate = "// NOAUTOUPDATE"; + if (line_trimmed != Autoupdate && line_trimmed != NoAutoupdate) { + return false; + } + if (*found_autoupdate) { + return ErrorBuilder() << "Multiple AUTOUPDATE/NOAUTOUPDATE settings found"; + } + *found_autoupdate = true; + if (line_trimmed == Autoupdate) { + *autoupdate_line_number = line_index; + } + return true; +} + +// Processes SET-CHECK-SUBSET lines when found. Returns true if the line is +// consumed. +static auto TryConsumeSetCheckSubset(llvm::StringRef line_trimmed, + bool* check_subset) -> ErrorOr { + if (line_trimmed != "// SET-CHECK-SUBSET") { + return false; + } + if (*check_subset) { + return ErrorBuilder() << "SET-CHECK-SUBSET was specified multiple times"; + } + *check_subset = true; + return true; +} + +auto FileTestBase::ProcessTestFile(TestContext& context) -> ErrorOr { + // Original file content, and a cursor for walking through it. + llvm::StringRef file_content = context.input_content; + llvm::StringRef cursor = file_content; + + // Whether either AUTOUDPATE or NOAUTOUPDATE was found. + bool found_autoupdate = false; + + // The index in the current test file. Will be reset on splits. + int line_index = 0; + + SplitState split; + + // When autoupdating, we track whether we're inside conflict markers. + // Otherwise conflict markers are errors. + bool inside_conflict_marker = false; + + while (!cursor.empty()) { + auto [line, next_cursor] = cursor.split("\n"); + cursor = next_cursor; + auto line_trimmed = line.ltrim(); + + bool is_consumed = false; + CARBON_ASSIGN_OR_RETURN( + is_consumed, + TryConsumeConflictMarker(line, line_trimmed, &inside_conflict_marker)); + if (is_consumed) { + continue; + } + + // At this point, remaining lines are part of the test input. + CARBON_ASSIGN_OR_RETURN( + is_consumed, + TryConsumeSplit(line, line_trimmed, found_autoupdate, &line_index, + &split, &context.test_files, &context.non_check_lines)); + if (is_consumed) { + continue; + } + + ++line_index; + + CARBON_ASSIGN_OR_RETURN( + is_consumed, + TryConsumeCheck(line_index, line, line_trimmed, + &context.expected_stdout, &context.expected_stderr)); + if (is_consumed) { + continue; + } + + // At this point, lines are retained as non-CHECK lines. + context.non_check_lines.push_back( + FileTestLine(split.file_index, line_index, line)); + + CARBON_ASSIGN_OR_RETURN( + is_consumed, TryConsumeArgs(line, line_trimmed, &context.test_args)); + if (is_consumed) { + continue; + } + CARBON_ASSIGN_OR_RETURN( + is_consumed, + TryConsumeAutoupdate(line_index, line_trimmed, &found_autoupdate, + &context.autoupdate_line_number)); + if (is_consumed) { + continue; + } + CARBON_ASSIGN_OR_RETURN( + is_consumed, + TryConsumeSetCheckSubset(line_trimmed, &context.check_subset)); + if (is_consumed) { + continue; + } + } + + if (!found_autoupdate) { + return ErrorBuilder() << "Missing AUTOUPDATE/NOAUTOUPDATE setting"; + } + + context.has_splits = split.has_splits(); + FinishSplit(test_name_, &split, &context.test_files); + + // Assume there is always a suffix `\n` in output. + if (!context.expected_stdout.empty()) { + context.expected_stdout.push_back(StrEq("")); + } + if (!context.expected_stderr.empty()) { + context.expected_stderr.push_back(StrEq("")); + } + + return Success(); +} + // Returns the tests to run. static auto GetTests() -> llvm::SmallVector { // Prefer a user-specified list if present. @@ -539,8 +726,11 @@ static auto Main(int argc, char** argv) -> int { for (const auto& test_name : tests) { std::unique_ptr test(test_factory.factory_fn(test_name)); auto result = test->Autoupdate(); - llvm::errs() << (result.ok() ? (*result ? "!" : ".") - : result.error().message()); + if (result.ok()) { + llvm::errs() << (*result ? "!" : "."); + } else { + llvm::errs() << result.error().message() << "\n"; + } } llvm::errs() << "\nDone!\n"; return EXIT_SUCCESS; diff --git a/testing/file_test/file_test_base.h b/testing/file_test/file_test_base.h index 9691c2c1ccee..fefbdbbc00e9 100644 --- a/testing/file_test/file_test_base.h +++ b/testing/file_test/file_test_base.h @@ -24,9 +24,6 @@ namespace Carbon::Testing { class FileTestBase : public testing::Test { public: struct TestFile { - explicit TestFile(std::string filename, llvm::StringRef content) - : filename(std::move(filename)), content(content) {} - friend void PrintTo(const TestFile& f, std::ostream* os) { // Print content escaped. llvm::raw_os_ostream os_wrap(*os); @@ -36,7 +33,7 @@ class FileTestBase : public testing::Test { } std::string filename; - llvm::StringRef content; + std::string content; }; // Provided for child class convenience. @@ -146,11 +143,6 @@ class FileTestBase : public testing::Test { // Processes the test input, producing test files and expected output. auto ProcessTestFile(TestContext& context) -> ErrorOr; - // Transforms an expectation on a given line from `FileCheck` syntax into a - // standard regex matcher. - static auto TransformExpectation(int line_index, llvm::StringRef in) - -> ErrorOr>; - // Runs the FileTestAutoupdater, returning the result. auto RunAutoupdater(const TestContext& context, bool dry_run) -> bool;