Error: track message and prefix/location separately. (#1529)

This allows us to combine multiple Errors together without repeating the prefix
information. Also fixes several cases where two "COMPILATION ERROR" prefixes
would be prepended to the same message when errors with prefixes and locations
were produced by the lexer and parser.

common/error.h

@@ -23,19 +23,48 @@ struct Success {};
 class [[nodiscard]] Error {
  public:
   // Represents an error state.
-  explicit Error(llvm::Twine message) : message_(message.str()) {
+  explicit Error(llvm::Twine prefix, llvm::Twine location, llvm::Twine message)
+      : prefix_(prefix.str()),
+        location_(location.str()),
+        message_(message.str()) {
     CARBON_CHECK(!message_.empty()) << "Errors must have a message.";
   }
 
-  Error(Error&& other) noexcept : message_(std::move(other.message_)) {}
+  // Represents an error with no associated prefix or location.
+  // TODO: Consider using two different types.
+  explicit Error(llvm::Twine message) : Error("", "", message) {}
+
+  Error(Error&& other) noexcept
+      : prefix_(std::move(other.prefix_)),
+        location_(std::move(other.location_)),
+        message_(std::move(other.message_)) {}
+
+  // Prints the error string.
+  void Print(llvm::raw_ostream& out) const {
+    if (!prefix().empty()) {
+      out << prefix() << ": ";
+    }
+    if (!location().empty()) {
+      out << location() << ": ";
+    }
+    out << message();
+  }
+
+  // Returns the prefix to prepend to the error, such as "ERROR".
+  auto prefix() const -> const std::string& { return prefix_; }
 
-  // Prints the error string. Note this marks as used.
-  void Print(llvm::raw_ostream& out) const { out << message(); }
+  // Returns a string describing the location of the error, such as
+  // "file.cc:123".
+  auto location() const -> const std::string& { return location_; }
 
   // Returns the error message.
   auto message() const -> const std::string& { return message_; }
 
  private:
+  // A prefix, indicating the kind of error.
+  std::string prefix_;
+  // The location associated with the error.
+  std::string location_;
   // The error message.
   std::string message_;
 };
@@ -109,7 +138,12 @@ class [[nodiscard]] ErrorOr {
 // `Error` and `ErrorOr<T>`.
 class ErrorBuilder {
  public:
-  ErrorBuilder() : out_(std::make_unique<llvm::raw_string_ostream>(message_)) {}
+  explicit ErrorBuilder(std::string prefix, std::string location)
+      : prefix_(std::move(prefix)),
+        location_(std::move(location)),
+        out_(std::make_unique<llvm::raw_string_ostream>(message_)) {}
+
+  explicit ErrorBuilder() : ErrorBuilder("", "") {}
 
   // Accumulates string message.
   template <typename T>
@@ -119,15 +153,17 @@ class ErrorBuilder {
   }
 
   // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns.
-  operator Error() { return Error(message_); }
+  operator Error() { return Error(prefix_, location_, message_); }
 
   template <typename T>
   // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns.
   operator ErrorOr<T>() {
-    return Error(message_);
+    return Error(prefix_, location_, message_);
   }
 
  private:
+  std::string prefix_;
+  std::string location_;
   std::string message_;
   // Use a pointer to allow move construction.
   std::unique_ptr<llvm::raw_string_ostream> out_;

explorer/common/error_builders.h

@@ -22,21 +22,15 @@ namespace Carbon {
 // provided as a fallback for cases that don't fit those classifications.
 
 inline auto CompilationError(SourceLocation loc) -> ErrorBuilder {
-  ErrorBuilder builder;
-  (void)(builder << "COMPILATION ERROR: " << loc << ": ");
-  return builder;
+  return ErrorBuilder("COMPILATION ERROR", loc.ToString());
 }
 
 inline auto ProgramError(SourceLocation loc) -> ErrorBuilder {
-  ErrorBuilder builder;
-  (void)(builder << "PROGRAM ERROR: " << loc << ": ");
-  return builder;
+  return ErrorBuilder("PROGRAM ERROR", loc.ToString());
 }
 
 inline auto RuntimeError(SourceLocation loc) -> ErrorBuilder {
-  ErrorBuilder builder;
-  (void)(builder << "RUNTIME ERROR: " << loc << ": ");
-  return builder;
+  return ErrorBuilder("RUNTIME ERROR", loc.ToString());
 }
 
 }  // namespace Carbon

explorer/common/error_builders_test.cpp

@@ -11,19 +11,35 @@
 namespace Carbon::Testing {
 namespace {
 
+auto ToString(const Error& err) -> std::string {
+  std::string result;
+  llvm::raw_string_ostream out(result);
+  err.Print(out);
+  return result;
+}
+
 TEST(ErrorBuildersTest, CompilationError) {
   Error err = CompilationError(SourceLocation("x", 1)) << "test";
-  EXPECT_EQ(err.message(), "COMPILATION ERROR: x:1: test");
+  EXPECT_EQ(err.prefix(), "COMPILATION ERROR");
+  EXPECT_EQ(err.location(), "x:1");
+  EXPECT_EQ(err.message(), "test");
+  EXPECT_EQ(ToString(err), "COMPILATION ERROR: x:1: test");
 }
 
 TEST(ErrorBuildersTest, ProgramError) {
   Error err = ProgramError(SourceLocation("x", 1)) << "test";
-  EXPECT_EQ(err.message(), "PROGRAM ERROR: x:1: test");
+  EXPECT_EQ(err.prefix(), "PROGRAM ERROR");
+  EXPECT_EQ(err.location(), "x:1");
+  EXPECT_EQ(err.message(), "test");
+  EXPECT_EQ(ToString(err), "PROGRAM ERROR: x:1: test");
 }
 
 TEST(ErrorBuildersTest, RuntimeError) {
   Error err = RuntimeError(SourceLocation("x", 1)) << "test";
-  EXPECT_EQ(err.message(), "RUNTIME ERROR: x:1: test");
+  EXPECT_EQ(err.prefix(), "RUNTIME ERROR");
+  EXPECT_EQ(err.location(), "x:1");
+  EXPECT_EQ(err.message(), "test");
+  EXPECT_EQ(ToString(err), "RUNTIME ERROR: x:1: test");
 }
 
 }  // namespace

explorer/common/source_location.h

@@ -33,6 +33,12 @@ class SourceLocation {
   void Print(llvm::raw_ostream& out) const {
     out << filename_ << ":" << line_num_;
   }
+  auto ToString() const -> std::string {
+    std::string result;
+    llvm::raw_string_ostream out(result);
+    Print(out);
+    return result;
+  }
   LLVM_DUMP_METHOD void Dump() const { Print(llvm::errs()); }
 
  private:

explorer/main.cpp

@@ -89,7 +89,7 @@ static auto Main(llvm::StringRef default_prelude_file, int argc, char* argv[])
 auto ExplorerMain(llvm::StringRef default_prelude_file, int argc, char** argv)
     -> int {
   if (auto result = Main(default_prelude_file, argc, argv); !result.ok()) {
-    llvm::errs() << result.error().message() << "\n";
+    llvm::errs() << result.error() << "\n";
     return EXIT_FAILURE;
   }
   return EXIT_SUCCESS;

explorer/syntax/lexer.lpp

@@ -249,7 +249,7 @@ operand_start         [(A-Za-z0-9_\"]
   if (intrinsic.ok()) {
     return CARBON_ARG_TOKEN(intrinsic_identifier, *intrinsic);
   } else {
-    return context.RecordSyntaxError(intrinsic.error().message());
+    return context.RecordSyntaxError(std::move(intrinsic).error());
   }
 }
 

explorer/syntax/parse.cpp

@@ -29,10 +29,11 @@ static auto ParseImpl(yyscan_t scanner, Nonnull<Arena*> arena,
   }
 
   if (auto syntax_error_code = parser(); syntax_error_code != 0) {
-    const std::string error_message = context.error_messages().empty()
-                                          ? "Unknown parser error"
-                                          : context.error_messages()[0];
-    return Error(error_message);
+    auto errors = context.take_errors();
+    if (errors.empty()) {
+      return Error("Unknown parser erroor");
+    }
+    return std::move(errors.front());
   }
 
   // Return parse results.

explorer/syntax/parse_and_lex_context.cpp

@@ -4,23 +4,20 @@
 
 #include "explorer/syntax/parse_and_lex_context.h"
 
+#include "explorer/common/error_builders.h"
+
 namespace Carbon {
 
-auto ParseAndLexContext::RecordSyntaxError(const std::string& message,
-                                           bool prefix_with_newline)
-    -> Parser::symbol_type {
-  // Optionally adds a newline in trace mode because trace prints an incomplete
-  // line "Reading a token: " which can prevent LIT from finding expected
-  // patterns.
-  // TODO: support formatting of `SourceLocation` instances with formatv().
-  std::string full_message;
-  llvm::raw_string_ostream(full_message)
-      << (prefix_with_newline && parser_debug() ? "\n" : "")
-      << "COMPILATION ERROR: " << source_loc() << ": " << message;
-  error_messages_.push_back(full_message);
+auto ParseAndLexContext::RecordSyntaxError(Error error) -> Parser::symbol_type {
+  errors_.push_back(std::move(error));
 
   // TODO: use `YYerror` token once bison is upgraded to at least 3.5.
   return Parser::make_END_OF_FILE(current_token_position);
 }
 
+auto ParseAndLexContext::RecordSyntaxError(const std::string& message)
+    -> Parser::symbol_type {
+  return RecordSyntaxError(CompilationError(source_loc()) << message);
+}
+
 }  // namespace Carbon

explorer/syntax/parse_and_lex_context.h

@@ -21,11 +21,10 @@ class ParseAndLexContext {
                      bool parser_debug)
       : input_file_name_(input_file_name), parser_debug_(parser_debug) {}
 
-  // Formats ands records a lexer error. Returns an error token as a
-  // convenience.
-  auto RecordSyntaxError(const std::string& message,
-                         bool prefix_with_newline = false)
-      -> Parser::symbol_type;
+  // Formats ands records a lexing oor parsing error. Returns an error token as
+  // a convenience.
+  auto RecordSyntaxError(Error error) -> Parser::symbol_type;
+  auto RecordSyntaxError(const std::string& message) -> Parser::symbol_type;
 
   auto source_loc() const -> SourceLocation {
     return SourceLocation(input_file_name_,
@@ -37,8 +36,10 @@ class ParseAndLexContext {
   // The source range of the token being (or just) lex'd.
   location current_token_position;
 
-  auto error_messages() const -> const std::vector<std::string> {
-    return error_messages_;
+  auto take_errors() -> std::vector<Error> {
+    std::vector<Error> errors = std::move(errors_);
+    errors_.clear();
+    return errors;
   }
 
  private:
@@ -48,7 +49,7 @@ class ParseAndLexContext {
 
   bool parser_debug_;
 
-  std::vector<std::string> error_messages_;
+  std::vector<Error> errors_;
 };
 
 }  // namespace Carbon

explorer/syntax/parser.ypp

@@ -689,7 +689,7 @@ non_expression_pattern:
       if (alternative_pattern.ok()) {
         $$ = *alternative_pattern;
       } else {
-        context.RecordSyntaxError(alternative_pattern.error().message());
+        context.RecordSyntaxError(std::move(alternative_pattern).error());
         YYERROR;
       }
     }
@@ -923,7 +923,7 @@ function_declaration:
       if (fn.ok()) {
         $$ = *fn;
       } else {
-        context.RecordSyntaxError(fn.error().message());
+        context.RecordSyntaxError(std::move(fn).error());
         YYERROR;
       }
     }
@@ -934,7 +934,7 @@ function_declaration:
       if (fn.ok()) {
         $$ = *fn;
       } else {
-        context.RecordSyntaxError(fn.error().message());
+        context.RecordSyntaxError(std::move(fn).error());
         YYERROR;
       }
     }
@@ -1025,7 +1025,7 @@ declaration:
       if (impl.ok()) {
         $$ = *impl;
       } else {
-        context.RecordSyntaxError(impl.error().message());
+        context.RecordSyntaxError(std::move(impl).error());
         YYERROR;
       }
     }

explorer/syntax/prelude.cpp

@@ -15,8 +15,8 @@ void AddPrelude(std::string_view prelude_file_name, Nonnull<Arena*> arena,
   if (!parse_result.ok()) {
     // Try again with tracing, to help diagnose the problem.
     ErrorOr<AST> trace_parse_result = Parse(arena, prelude_file_name, true);
-    CARBON_FATAL() << "Failed to parse prelude: "
-                   << trace_parse_result.error().message();
+    CARBON_FATAL() << "Failed to parse prelude:\n"
+                   << trace_parse_result.error();
   }
   const auto& prelude = *parse_result;
   declarations->insert(declarations->begin(), prelude.declarations.begin(),

explorer/testdata/addr/fail-method-me-misspelled.carbon

@@ -0,0 +1,18 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+class C {
+  // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/addr/fail-method-me-misspelled.carbon:[[@LINE+1]]: illegal binding pattern in implicit parameter list
+  fn F[addr mew: Self*]() {}
+}
+
+fn Main() -> i32 { return 0; }

explorer/testdata/as/fail_no_conversion.carbon

@@ -15,6 +15,6 @@ class A { var n: i32; }
 fn Main() -> i32 {
   var a: A = {.n = 5};
   // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/as/fail_no_conversion.carbon:[[@LINE+2]]: type error in `as`: `class A` is not explicitly convertible to `i32`:
-  // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/as/fail_no_conversion.carbon:[[@LINE+1]]: could not find implementation of interface As(T = i32) for class A
+  // CHECK: could not find implementation of interface As(T = i32) for class A
   return a as i32;
 }

explorer/testdata/basic_syntax/fail_unknown_intrinsic.carbon

@@ -0,0 +1,16 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+fn Main() -> i32 {
+  // CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_unknown_intrinsic.carbon:[[@LINE+1]]: Unknown intrinsic 'nonexistent'
+  return __intrinsic_nonexistent();
+}

explorer/testdata/generic_function/fail_missing_exclam.carbon

@@ -0,0 +1,18 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+
+package ExplorerTest api;
+
+// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/generic_function/fail_missing_exclam.carbon:[[@LINE+1]]: illegal binding pattern in implicit parameter list
+fn F[T: Type]();
+
+fn Main() -> i32 {
+  return 0;
+}

explorer/testdata/match/fail_not_alternative.carbon

@@ -0,0 +1,22 @@
+// 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
+//
+// RUN: %{not} %{explorer} %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
+// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
+// RUN:   %{FileCheck} --match-full-lines --allow-unused-prefixes %s
+// AUTOUPDATE: %{explorer} %s
+// CHECK: PROGRAM ERROR: {{.*}}/explorer/testdata/match/fail_not_alternative.carbon:17: Alternative pattern must have the form of a field access.
+
+package ExplorerTest api;
+
+fn Main() -> i32 {
+  var x: i32 = 0;
+  match (x) {
+    case i32(n: i32) => {
+      return 1;
+    }
+  }
+  return 0;
+}