From 7a67715ac52bbb8c0dcd579da93fcc4165e5fbbc Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 26 Jul 2022 18:08:19 -0400 Subject: [PATCH] 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 | 50 ++++++++++++++++--- explorer/common/error_builders.h | 12 ++--- explorer/common/error_builders_test.cpp | 22 ++++++-- explorer/common/source_location.h | 6 +++ explorer/main.cpp | 2 +- explorer/syntax/lexer.lpp | 2 +- explorer/syntax/parse.cpp | 9 ++-- explorer/syntax/parse_and_lex_context.cpp | 21 ++++---- explorer/syntax/parse_and_lex_context.h | 17 ++++--- explorer/syntax/parser.ypp | 8 +-- explorer/syntax/prelude.cpp | 4 +- .../addr/fail-method-me-misspelled.carbon | 18 +++++++ .../testdata/as/fail_no_conversion.carbon | 2 +- .../fail_unknown_intrinsic.carbon | 16 ++++++ .../fail_missing_exclam.carbon | 18 +++++++ .../match/fail_not_alternative.carbon | 22 ++++++++ 16 files changed, 177 insertions(+), 52 deletions(-) create mode 100644 explorer/testdata/addr/fail-method-me-misspelled.carbon create mode 100644 explorer/testdata/basic_syntax/fail_unknown_intrinsic.carbon create mode 100644 explorer/testdata/generic_function/fail_missing_exclam.carbon create mode 100644 explorer/testdata/match/fail_not_alternative.carbon diff --git a/common/error.h b/common/error.h index 403befc461dd..1752c242408e 100644 --- a/common/error.h +++ b/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) {} - // Prints the error string. Note this marks as used. - void Print(llvm::raw_ostream& out) const { out << 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_; } + + // 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`. class ErrorBuilder { public: - ErrorBuilder() : out_(std::make_unique(message_)) {} + explicit ErrorBuilder(std::string prefix, std::string location) + : prefix_(std::move(prefix)), + location_(std::move(location)), + out_(std::make_unique(message_)) {} + + explicit ErrorBuilder() : ErrorBuilder("", "") {} // Accumulates string message. template @@ -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 // NOLINTNEXTLINE(google-explicit-constructor): Implicit cast for returns. operator ErrorOr() { - 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 out_; diff --git a/explorer/common/error_builders.h b/explorer/common/error_builders.h index 17ad3c243c24..9a31f098ee82 100644 --- a/explorer/common/error_builders.h +++ b/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 diff --git a/explorer/common/error_builders_test.cpp b/explorer/common/error_builders_test.cpp index f4ac9fd50dc7..a404f65248b1 100644 --- a/explorer/common/error_builders_test.cpp +++ b/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 diff --git a/explorer/common/source_location.h b/explorer/common/source_location.h index ec9fc26b7c9a..2e11e67dcfd1 100644 --- a/explorer/common/source_location.h +++ b/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: diff --git a/explorer/main.cpp b/explorer/main.cpp index 745aefa6e781..b5d0dd37975a 100644 --- a/explorer/main.cpp +++ b/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; diff --git a/explorer/syntax/lexer.lpp b/explorer/syntax/lexer.lpp index e18e040f9886..0b5b334d7662 100644 --- a/explorer/syntax/lexer.lpp +++ b/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()); } } diff --git a/explorer/syntax/parse.cpp b/explorer/syntax/parse.cpp index ef065afa19f9..81cdc9939c7f 100644 --- a/explorer/syntax/parse.cpp +++ b/explorer/syntax/parse.cpp @@ -29,10 +29,11 @@ static auto ParseImpl(yyscan_t scanner, Nonnull 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. diff --git a/explorer/syntax/parse_and_lex_context.cpp b/explorer/syntax/parse_and_lex_context.cpp index d2ec69766e6a..a75b16eb89f9 100644 --- a/explorer/syntax/parse_and_lex_context.cpp +++ b/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 diff --git a/explorer/syntax/parse_and_lex_context.h b/explorer/syntax/parse_and_lex_context.h index 358a492672f6..446a6a0affbd 100644 --- a/explorer/syntax/parse_and_lex_context.h +++ b/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 { - return error_messages_; + auto take_errors() -> std::vector { + std::vector errors = std::move(errors_); + errors_.clear(); + return errors; } private: @@ -48,7 +49,7 @@ class ParseAndLexContext { bool parser_debug_; - std::vector error_messages_; + std::vector errors_; }; } // namespace Carbon diff --git a/explorer/syntax/parser.ypp b/explorer/syntax/parser.ypp index 79e9357baee9..5ef6940a4c55 100644 --- a/explorer/syntax/parser.ypp +++ b/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; } } diff --git a/explorer/syntax/prelude.cpp b/explorer/syntax/prelude.cpp index c0c943079b8e..8e62c215d289 100644 --- a/explorer/syntax/prelude.cpp +++ b/explorer/syntax/prelude.cpp @@ -15,8 +15,8 @@ void AddPrelude(std::string_view prelude_file_name, Nonnull arena, if (!parse_result.ok()) { // Try again with tracing, to help diagnose the problem. ErrorOr 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(), diff --git a/explorer/testdata/addr/fail-method-me-misspelled.carbon b/explorer/testdata/addr/fail-method-me-misspelled.carbon new file mode 100644 index 000000000000..eb6f0a563e5b --- /dev/null +++ b/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; } diff --git a/explorer/testdata/as/fail_no_conversion.carbon b/explorer/testdata/as/fail_no_conversion.carbon index 3a4ad3318e5e..6121e783f7de 100644 --- a/explorer/testdata/as/fail_no_conversion.carbon +++ b/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; } diff --git a/explorer/testdata/basic_syntax/fail_unknown_intrinsic.carbon b/explorer/testdata/basic_syntax/fail_unknown_intrinsic.carbon new file mode 100644 index 000000000000..c5724dab6c6f --- /dev/null +++ b/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(); +} diff --git a/explorer/testdata/generic_function/fail_missing_exclam.carbon b/explorer/testdata/generic_function/fail_missing_exclam.carbon new file mode 100644 index 000000000000..6e08c8bd963a --- /dev/null +++ b/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; +} diff --git a/explorer/testdata/match/fail_not_alternative.carbon b/explorer/testdata/match/fail_not_alternative.carbon new file mode 100644 index 000000000000..36b7536d8433 --- /dev/null +++ b/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; +}