Switch custom error stream output to diagnostic (#4846)

This switches most error printing to use diagnostics instead of direct
stream writes, even when not a specific file diagnostic. I'm allowing
empty filenames for this use-case.

This allows a little more specific testing to validate coverage of
output using the diagnostic coverage test. I'm adding a few tests to
cover things that weren't previously tested.

Separately, this also forces a little more standardization in format...
considering how changes like #4568 show effort being spent to _mirror_
diagnostic style, my thought is now to just use diagnostic code where
possible.

Note this also allows incrementally better testing of the language
server; I'm changing the crash fix from #4847 in favor of diagnostic
testing.

---------

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>

toolchain/diagnostics/BUILD

@@ -81,6 +81,15 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "file_diagnostics",
+    hdrs = ["file_diagnostics.h"],
+    deps = [
+        ":diagnostic_emitter",
+        "@llvm-project//llvm:Support",
+    ],
+)
+
 cc_library(
     name = "format_providers",
     srcs = ["format_providers.cpp"],

toolchain/diagnostics/coverage_test.cpp

@@ -24,8 +24,10 @@ constexpr DiagnosticKind UntestedDiagnosticKinds[] = {
     DiagnosticKind::TestDiagnostic,
     DiagnosticKind::TestDiagnosticNote,
 
-    // Driver specific.
-    DiagnosticKind::CppInteropFuzzing,
+    // Diagnosing erroneous install conditions, but test environments are
+    // typically correct.
+    DiagnosticKind::CompilePreludeManifestError,
+    DiagnosticKind::DriverInstallInvalid,
 
     // These diagnose filesystem issues that are hard to unit test.
     DiagnosticKind::ErrorReadingFile,

toolchain/diagnostics/diagnostic.cpp

@@ -10,6 +10,9 @@
 namespace Carbon {
 
 auto DiagnosticLoc::FormatLocation(llvm::raw_ostream& out) const -> void {
+  if (filename.empty()) {
+    return;
+  }
   out << filename;
   if (line_number > 0) {
     out << ":" << line_number;
@@ -17,6 +20,7 @@ auto DiagnosticLoc::FormatLocation(llvm::raw_ostream& out) const -> void {
       out << ":" << column_number;
     }
   }
+  out << ": ";
 }
 
 auto DiagnosticLoc::FormatSnippet(llvm::raw_ostream& out, int indent) const

toolchain/diagnostics/diagnostic.h

@@ -50,8 +50,9 @@ enum class DiagnosticLevel : int8_t {
 // is required to be less than SourceBuffer that it refers to due to the
 // contained filename and line references.
 struct DiagnosticLoc {
-  // Write the filename, line number, and column number corresponding to this
-  // location to the given stream.
+  // Writes the location to the given stream. It will be formatted as
+  // `<filename>:<line_number>:<column_number>: ` with parts dropped when
+  // unknown.
   auto FormatLocation(llvm::raw_ostream& out) const -> void;
 
   // Write the source snippet corresponding to this location to the given

toolchain/diagnostics/diagnostic_consumer.cpp

@@ -20,18 +20,18 @@ auto StreamDiagnosticConsumer::HandleDiagnostic(Diagnostic diagnostic) -> void {
     message.loc.FormatLocation(*stream_);
     switch (message.level) {
       case DiagnosticLevel::Error:
-        *stream_ << ": error";
+        *stream_ << "error: ";
         break;
       case DiagnosticLevel::Warning:
-        *stream_ << ": warning";
+        *stream_ << "warning: ";
         break;
       case DiagnosticLevel::Note:
-        *stream_ << ": note";
+        *stream_ << "note: ";
         break;
       case DiagnosticLevel::LocationInfo:
         break;
     }
-    *stream_ << ": " << message.format_fn(message);
+    *stream_ << message.format_fn(message);
     if (include_diagnostic_kind_) {
       *stream_ << " [" << message.kind << "]";
     }
@@ -46,8 +46,7 @@ auto StreamDiagnosticConsumer::HandleDiagnostic(Diagnostic diagnostic) -> void {
 }
 
 auto ConsoleDiagnosticConsumer() -> DiagnosticConsumer& {
-  static auto* consumer = new StreamDiagnosticConsumer(
-      llvm::errs(), /*include_diagnostic_kind=*/false);
+  static auto* consumer = new StreamDiagnosticConsumer(&llvm::errs());
   return *consumer;
 }
 

toolchain/diagnostics/diagnostic_consumer.h

@@ -39,14 +39,16 @@ class DiagnosticConsumer {
 // A diagnostic consumer that prints to a stream.
 class StreamDiagnosticConsumer : public DiagnosticConsumer {
  public:
-  explicit StreamDiagnosticConsumer(llvm::raw_ostream& stream,
-                                    bool include_diagnostic_kind)
-      : stream_(&stream), include_diagnostic_kind_(include_diagnostic_kind) {}
+  explicit StreamDiagnosticConsumer(llvm::raw_ostream* stream)
+      : stream_(stream) {}
 
   auto HandleDiagnostic(Diagnostic diagnostic) -> void override;
   auto Flush() -> void override { stream_->flush(); }
 
   auto set_stream(llvm::raw_ostream* stream) -> void { stream_ = stream; }
+  auto set_include_diagnostic_kind(bool value) -> void {
+    include_diagnostic_kind_ = value;
+  }
 
  private:
   auto Print(const DiagnosticMessage& message, llvm::StringRef prefix) -> void;
@@ -54,7 +56,7 @@ class StreamDiagnosticConsumer : public DiagnosticConsumer {
   llvm::raw_ostream* stream_;
 
   // Whether to include the diagnostic kind when printing.
-  bool include_diagnostic_kind_;
+  bool include_diagnostic_kind_ = false;
 
   // Whethere we've printed a diagnostic. Used for printing separators.
   bool printed_diagnostic_ = false;

toolchain/diagnostics/diagnostic_emitter.h

@@ -173,9 +173,12 @@ class DiagnosticEmitter {
 
   // The `converter` and `consumer` are required to outlive the diagnostic
   // emitter.
+  // TODO: Adjust construction to take stored arguments as pointers, and
+  // consider taking `converter` by value (move/copy) instead of reference.
   explicit DiagnosticEmitter(DiagnosticConverter<LocT>& converter,
                              DiagnosticConsumer& consumer)
       : converter_(&converter), consumer_(&consumer) {}
+
   ~DiagnosticEmitter() = default;
 
   // Emits an error.
@@ -229,6 +232,37 @@ class DiagnosticEmitter {
       annotate_fns_;
 };
 
+// This relies on `void*` location handling on `DiagnosticEmitter`.
+//
+// TODO: Based on how this ends up used or if we get more distinct emitters, it
+// might be worth considering having diagnostics specify that they don't apply
+// to source-location carrying emitters. For example, this might look like a
+// `CARBON_NO_LOC_DIAGNOSTIC` macro, or some other factoring. But it might end
+// up being more noise than it is worth.
+class NoLocDiagnosticEmitter : public DiagnosticEmitter<void*> {
+ public:
+  // This constructor only applies to NoLocDiagnosticEmitter.
+  explicit NoLocDiagnosticEmitter(DiagnosticConsumer* consumer)
+      : DiagnosticEmitter(converter_, *consumer) {}
+
+  // Emits an error. This specialization only applies to
+  // `NoLocDiagnosticEmitter`.
+  template <typename... Args>
+  auto Emit(const DiagnosticBase<Args...>& diagnostic_base,
+            Internal::NoTypeDeduction<Args>... args) -> void {
+    DiagnosticEmitter::Emit(nullptr, diagnostic_base, args...);
+  }
+
+ private:
+  struct Converter : DiagnosticConverter<void*> {
+    auto ConvertLoc(void* /*loc*/, ContextFnT /*context_fn*/) const
+        -> ConvertedDiagnosticLoc override {
+      return {.loc = {.filename = ""}, .last_byte_offset = -1};
+    }
+  };
+  Converter converter_;
+};
+
 // An RAII object that denotes a scope in which any diagnostic produced should
 // be annotated in some way.
 //

toolchain/diagnostics/diagnostic_kind.def

@@ -16,6 +16,21 @@
 #error "Must define the x-macro to use this file."
 #endif
 
+// ============================================================================
+// Driver and subcommand diagnostics
+// ============================================================================
+
+CARBON_DIAGNOSTIC_KIND(DriverInstallInvalid)
+CARBON_DIAGNOSTIC_KIND(DriverCommandLineParseFailed)
+CARBON_DIAGNOSTIC_KIND(ClangFuzzingDisallowed)
+CARBON_DIAGNOSTIC_KIND(CompilePhaseFlagConflict)
+CARBON_DIAGNOSTIC_KIND(CompilePreludeManifestError)
+CARBON_DIAGNOSTIC_KIND(CompileInputNotRegularFile)
+CARBON_DIAGNOSTIC_KIND(CompileOutputFileOpenError)
+CARBON_DIAGNOSTIC_KIND(FormatMultipleFilesToOneOutput)
+CARBON_DIAGNOSTIC_KIND(LanguageServerMissingInputStream)
+CARBON_DIAGNOSTIC_KIND(LanguageServerTransportError)
+
 // ============================================================================
 // SourceBuffer diagnostics
 // ============================================================================

toolchain/diagnostics/file_diagnostics.h

@@ -0,0 +1,37 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_TOOLCHAIN_DIAGNOSTICS_FILE_DIAGNOSTICS_H_
+#define CARBON_TOOLCHAIN_DIAGNOSTICS_FILE_DIAGNOSTICS_H_
+
+#include "toolchain/diagnostics/diagnostic_emitter.h"
+
+namespace Carbon {
+
+// We frequently want a `DiagnosticEmitter` that directly uses a filename. Note
+// that an empty string can be used for a diagnostic that has no particular
+// location.
+//
+// Note this provides no way to set a line or column on diagnostics. More
+// specific emitters must be used for that.
+class FileDiagnosticEmitter : public DiagnosticEmitter<llvm::StringRef> {
+ public:
+  explicit FileDiagnosticEmitter(DiagnosticConsumer* consumer)
+      : DiagnosticEmitter<llvm::StringRef>(converter_, *consumer) {}
+
+ private:
+  // Converts a filename directly to the diagnostic location.
+  struct FileDiagnosticConverter : DiagnosticConverter<llvm::StringRef> {
+    auto ConvertLoc(llvm::StringRef filename, ContextFnT /*context_fn*/) const
+        -> ConvertedDiagnosticLoc override {
+      return {.loc = {.filename = filename}, .last_byte_offset = -1};
+    }
+  };
+
+  FileDiagnosticConverter converter_;
+};
+
+}  // namespace Carbon
+
+#endif  // CARBON_TOOLCHAIN_DIAGNOSTICS_FILE_DIAGNOSTICS_H_

toolchain/diagnostics/testdata/fail_multiline_token.carbon

@@ -2,7 +2,7 @@
 // Exceptions. See /LICENSE for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
-// ARGS: compile --phase=parse %s
+// ARGS: --include-diagnostic-kind compile --phase=parse %s
 //
 // AUTOUPDATE
 // TIP: To test this file alone, run:
@@ -10,7 +10,7 @@
 // TIP: To dump output, run:
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/diagnostics/testdata/fail_multiline_token.carbon
 
-// CHECK:STDERR: fail_multiline_token.carbon:[[@LINE+4]]:1: error: unrecognized declaration introducer
+// CHECK:STDERR: fail_multiline_token.carbon:[[@LINE+4]]:1: error: unrecognized declaration introducer [UnrecognizedDecl]
 // CHECK:STDERR: '''
 // CHECK:STDERR: ^~~
 // CHECK:STDERR: