Improve autoupdate diagnostic CHECK line positioning. (#5942)

When an error diagnostic has an unattached location, for example because
the diagnostic points into a file that's in the prelude, use the next
attached location to position the error diagnostic's CHECK line. In
particular, if the error is followed by a note, use the position of the
note to determine where to place the error.

This exposes a general mechanism to do final fixups of the CHECK lines
to individual file_test binaries, which the toolchain's binary uses to
special-case error / warning CHECK lines.

Author: zygoloid

testing/file_test/autoupdate.cpp

@@ -150,7 +150,7 @@ auto FileTestAutoupdater::GetFileAndLineNumber(
 }
 
 auto FileTestAutoupdater::BuildCheckLines(llvm::StringRef output,
-                                          const char* label) -> CheckLines {
+                                          bool is_stderr) -> CheckLines {
   if (output.empty()) {
     return CheckLines({});
   }
@@ -166,16 +166,19 @@ auto FileTestAutoupdater::BuildCheckLines(llvm::StringRef output,
     lines.pop_back();
   }
 
+  auto label =
+      is_stderr ? llvm::StringLiteral("STDERR") : llvm::StringLiteral("STDOUT");
+
   // The default file number for when no specific file is found.
   int default_file_number = 0;
 
-  llvm::SmallVector<CheckLine> check_lines;
+  CheckLineArray check_lines;
   for (const auto& line : lines) {
     // This code is relatively hot in our testing, and because when testing it
     // isn't run with an optimizer we benefit from making it use simple
     // constructs. For this reason, we avoid `llvm::formatv` and similar tools.
     std::string check_line;
-    check_line.reserve(line.size() + strlen(label) + strlen("// CHECK:: "));
+    check_line.reserve(line.size() + label.size() + strlen("// CHECK:: "));
     check_line.append("// CHECK:");
     check_line.append(label);
     check_line.append(":");
@@ -219,6 +222,7 @@ auto FileTestAutoupdater::BuildCheckLines(llvm::StringRef output,
                                               default_file_number, check_line);
     check_lines.push_back(CheckLine(file_and_line, check_line));
   }
+  finalize_check_lines_(check_lines, is_stderr);
   return CheckLines(check_lines);
 }
 

testing/file_test/autoupdate.h

@@ -33,6 +33,64 @@ class FileTestAutoupdater {
     std::string line_formatv;
   };
 
+  // The file and line number that a CHECK line refers to, and the
+  // replacement from which they were determined, if any.
+  struct FileAndLineNumber {
+    explicit FileAndLineNumber(int file_number) : file_number(file_number) {}
+
+    explicit FileAndLineNumber(const LineNumberReplacement* replacement,
+                               int file_number, absl::string_view line_number);
+
+    const LineNumberReplacement* replacement = nullptr;
+    int file_number;
+    int line_number = -1;
+  };
+
+  // A CHECK line which is integrated into autoupdate output.
+  //
+  // `final` because we use pointer arithmetic on this type.
+  class CheckLine final : public FileTestLineBase {
+   public:
+    explicit CheckLine(FileAndLineNumber file_and_line_number, std::string line)
+        : FileTestLineBase(file_and_line_number.file_number,
+                           file_and_line_number.line_number),
+          replacement_(file_and_line_number.replacement),
+          line_(std::move(line)) {}
+
+    auto Print(llvm::raw_ostream& out) const -> void override {
+      out << indent_ << line_;
+    }
+
+    // When the location of the CHECK in output is known, we can set the indent
+    // and its line.
+    auto SetOutputLine(llvm::StringRef indent, int output_file_number,
+                       int output_line_number) -> void {
+      indent_ = indent;
+      output_file_number_ = output_file_number;
+      output_line_number_ = output_line_number;
+    }
+
+    // When the location of all lines in a file are known, we can set the line
+    // offset based on the target line.
+    auto RemapLineNumbers(
+        const llvm::DenseMap<llvm::StringRef, int>& file_to_number_map,
+        const llvm::DenseMap<std::pair<int, int>, int>& output_line_remap,
+        const llvm::SmallVector<int>& new_last_line_numbers) -> void;
+
+    auto line() const -> llvm::StringRef { return line_; }
+
+    auto is_blank() const -> bool override { return false; }
+
+   private:
+    const LineNumberReplacement* replacement_;
+    std::string line_;
+    llvm::StringRef indent_;
+    int output_file_number_ = -1;
+    int output_line_number_ = -1;
+  };
+
+  using CheckLineArray = llvm::SmallVector<FileTestAutoupdater::CheckLine>;
+
   explicit FileTestAutoupdater(
       const std::filesystem::path& file_test_path, std::string test_command,
       std::string dump_command, llvm::StringRef input_content,
@@ -42,7 +100,8 @@ class FileTestAutoupdater {
       llvm::StringRef actual_stdout, llvm::StringRef actual_stderr,
       const std::optional<RE2>& default_file_re,
       const llvm::SmallVector<LineNumberReplacement>& line_number_replacements,
-      std::function<auto(std::string&)->void> do_extra_check_replacements)
+      std::function<auto(std::string&)->void> do_extra_check_replacements,
+      std::function<auto(CheckLineArray&, bool)->void> finalize_check_lines)
       : file_test_path_(file_test_path),
         test_command_(std::move(test_command)),
         dump_command_(std::move(dump_command)),
@@ -52,13 +111,14 @@ class FileTestAutoupdater {
         default_file_re_(default_file_re),
         line_number_replacements_(line_number_replacements),
         do_extra_check_replacements_(std::move(do_extra_check_replacements)),
+        finalize_check_lines_(std::move(finalize_check_lines)),
         autoupdate_split_file_(
             autoupdate_split ? std::optional(filenames.size()) : std::nullopt),
         file_to_number_map_(BuildFileToNumberMap(filenames)),
         // BuildCheckLines should only be called after other member
         // initialization.
-        stdout_(BuildCheckLines(actual_stdout, "STDOUT")),
-        stderr_(BuildCheckLines(actual_stderr, "STDERR")),
+        stdout_(BuildCheckLines(actual_stdout, /*is_stderr=*/false)),
+        stderr_(BuildCheckLines(actual_stderr, /*is_stderr=*/true)),
         any_attached_stdout_lines_(llvm::any_of(
             stdout_.lines,
             [&](const CheckLine& line) { return line.line_number() != -1; })),
@@ -78,19 +138,6 @@ class FileTestAutoupdater {
   auto Run(bool dry_run) -> bool;
 
  private:
-  // The file and line number that a CHECK line refers to, and the
-  // replacement from which they were determined, if any.
-  struct FileAndLineNumber {
-    explicit FileAndLineNumber(int file_number) : file_number(file_number) {}
-
-    explicit FileAndLineNumber(const LineNumberReplacement* replacement,
-                               int file_number, absl::string_view line_number);
-
-    const LineNumberReplacement* replacement = nullptr;
-    int file_number;
-    int line_number = -1;
-  };
-
   // A TIP line added by autoupdate. Not associated with any line in output.
   class TipLine : public FileTestLineBase {
    public:
@@ -105,55 +152,13 @@ class FileTestAutoupdater {
     std::string line_;
   };
 
-  // A CHECK line which is integrated into autoupdate output.
-  //
-  // `final` because we use pointer arithmetic on this type.
-  class CheckLine final : public FileTestLineBase {
-   public:
-    // RE2 is passed by a pointer because it doesn't support std::optional.
-    explicit CheckLine(FileAndLineNumber file_and_line_number, std::string line)
-        : FileTestLineBase(file_and_line_number.file_number,
-                           file_and_line_number.line_number),
-          replacement_(file_and_line_number.replacement),
-          line_(std::move(line)) {}
-
-    auto Print(llvm::raw_ostream& out) const -> void override {
-      out << indent_ << line_;
-    }
-
-    // When the location of the CHECK in output is known, we can set the indent
-    // and its line.
-    auto SetOutputLine(llvm::StringRef indent, int output_file_number,
-                       int output_line_number) -> void {
-      indent_ = indent;
-      output_file_number_ = output_file_number;
-      output_line_number_ = output_line_number;
-    }
-
-    // When the location of all lines in a file are known, we can set the line
-    // offset based on the target line.
-    auto RemapLineNumbers(
-        const llvm::DenseMap<llvm::StringRef, int>& file_to_number_map,
-        const llvm::DenseMap<std::pair<int, int>, int>& output_line_remap,
-        const llvm::SmallVector<int>& new_last_line_numbers) -> void;
-
-    auto is_blank() const -> bool override { return false; }
-
-   private:
-    const LineNumberReplacement* replacement_;
-    std::string line_;
-    llvm::StringRef indent_;
-    int output_file_number_ = -1;
-    int output_line_number_ = -1;
-  };
-
   // Clusters information for stdout and stderr.
   struct CheckLines {
-    explicit CheckLines(llvm::SmallVector<CheckLine> lines)
+    explicit CheckLines(CheckLineArray lines)
         : lines(std::move(lines)), cursor(this->lines.begin()) {}
 
     // The full list of check lines.
-    llvm::SmallVector<CheckLine> lines;
+    CheckLineArray lines;
     // An iterator into check_lines.
     CheckLine* cursor;
   };
@@ -176,7 +181,7 @@ class FileTestAutoupdater {
   }
 
   // Builds CheckLine lists for autoupdate.
-  auto BuildCheckLines(llvm::StringRef output, const char* label) -> CheckLines;
+  auto BuildCheckLines(llvm::StringRef output, bool is_stderr) -> CheckLines;
 
   // Adds a non-check line to the new_lines and output_line_remap. The caller
   // still needs to advance the cursor when ready.
@@ -214,6 +219,7 @@ class FileTestAutoupdater {
   const std::optional<RE2>& default_file_re_;
   const llvm::SmallVector<LineNumberReplacement>& line_number_replacements_;
   std::function<auto(std::string&)->void> do_extra_check_replacements_;
+  std::function<auto(CheckLineArray&, bool)->void> finalize_check_lines_;
 
   // If we have an autoupdate split that still needs to be processed, the file
   // number of the autoupdate split. Otherwise, this is nullopt.

testing/file_test/file_test_base.cpp

@@ -194,6 +194,9 @@ static auto RunAutoupdater(FileTestBase* test_base, const TestFile& test_file,
              test_base->GetLineNumberReplacements(expected_filenames),
              [&](std::string& line) {
                test_base->DoExtraCheckReplacements(line);
+             },
+             [&](FileTestBase::CheckLineArray& lines, bool is_stderr) {
+               test_base->FinalizeCheckLines(lines, is_stderr);
              })
       .Run(dry_run);
 }

testing/file_test/file_test_base.h

@@ -28,6 +28,7 @@ class FileTestBase {
  public:
   // Provided for child class convenience.
   using LineNumberReplacement = FileTestAutoupdater::LineNumberReplacement;
+  using CheckLineArray = FileTestAutoupdater::CheckLineArray;
 
   // The result of Run(), used to detect errors. Failing test files should be
   // named with a `fail_` prefix to indicate an expectation of failure.
@@ -101,6 +102,11 @@ class FileTestBase {
   virtual auto DoExtraCheckReplacements(std::string& /*check_line*/) const
       -> void {}
 
+  // Optionally allows children to perform tweaks on check lines before
+  // they are merged into the output file.
+  virtual auto FinalizeCheckLines(CheckLineArray& /*check_lines*/,
+                                  bool /*is_stderr*/) const -> void {}
+
   // Whether to allow running the test in parallel, particularly for autoupdate.
   // This can be overridden to force some tests to be run serially. At any given
   // time, all parallel tests and a single non-parallel test will be allowed to

testing/file_test/line.h

@@ -25,6 +25,11 @@ class FileTestLineBase : public Printable<FileTestLineBase> {
   auto file_number() const -> int { return file_number_; }
   auto line_number() const -> int { return line_number_; }
 
+  void set_location(int file_number, int line_number) {
+    file_number_ = file_number;
+    line_number_ = line_number;
+  }
+
  private:
   int file_number_;
   int line_number_;

toolchain/check/testdata/basics/type_literals.carbon

@@ -78,14 +78,14 @@ var test_i0: i0;
 // CHECK:STDERR:              ^~
 // CHECK:STDERR:
 var test_i1: i1;
-// CHECK:STDERR: fail_iN_bad_width.carbon:[[@LINE+7]]:15: error: bit width of integer type literal must be a multiple of 8; use `Core.Int(15)` instead [IntWidthNotMultipleOf8]
+// CHECK:STDERR: fail_iN_bad_width.carbon:[[@LINE+4]]:15: error: bit width of integer type literal must be a multiple of 8; use `Core.Int(15)` instead [IntWidthNotMultipleOf8]
 // CHECK:STDERR: var test_i15: i15;
 // CHECK:STDERR:               ^~~
 // CHECK:STDERR:
+var test_i15: i15;
 // CHECK:STDERR: min_prelude/parts/int.carbon:10:9: error: integer type width of 1000000000 is greater than the maximum supported width of 8388608 [IntWidthTooLarge]
 // CHECK:STDERR:   adapt MakeInt(N);
 // CHECK:STDERR:         ^~~~~~~~~~
-var test_i15: i15;
 // CHECK:STDERR: fail_iN_bad_width.carbon:[[@LINE+4]]:23: note: in `i1000000000` used here [ResolvingSpecificHere]
 // CHECK:STDERR: var test_i1000000000: i1000000000;
 // CHECK:STDERR:                       ^~~~~~~~~~~
@@ -115,14 +115,14 @@ var test_u0: u0;
 // CHECK:STDERR:              ^~
 // CHECK:STDERR:
 var test_u1: u1;
-// CHECK:STDERR: fail_uN_bad_width.carbon:[[@LINE+7]]:15: error: bit width of integer type literal must be a multiple of 8; use `Core.UInt(15)` instead [IntWidthNotMultipleOf8]
+// CHECK:STDERR: fail_uN_bad_width.carbon:[[@LINE+4]]:15: error: bit width of integer type literal must be a multiple of 8; use `Core.UInt(15)` instead [IntWidthNotMultipleOf8]
 // CHECK:STDERR: var test_u15: u15;
 // CHECK:STDERR:               ^~~
 // CHECK:STDERR:
+var test_u15: u15;
 // CHECK:STDERR: min_prelude/parts/uint.carbon:10:9: error: integer type width of 1000000000 is greater than the maximum supported width of 8388608 [IntWidthTooLarge]
 // CHECK:STDERR:   adapt MakeUInt(N);
 // CHECK:STDERR:         ^~~~~~~~~~~
-var test_u15: u15;
 // CHECK:STDERR: fail_uN_bad_width.carbon:[[@LINE+4]]:23: note: in `u1000000000` used here [ResolvingSpecificHere]
 // CHECK:STDERR: var test_u1000000000: u1000000000;
 // CHECK:STDERR:                       ^~~~~~~~~~~