Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate remaining exits to FATAL_*_ERROR calls #704

Merged
merged 11 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions common/check.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,35 +9,37 @@
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"

namespace CheckInternal {
namespace Carbon {

// Wraps a stream and exiting for CHECK.
class ExitWrapper {
// Wraps a stream and exiting for fatal errors.
class ExitingStream {
public:
ExitWrapper() {
// Start by printing a stack trace.
llvm::sys::PrintStackTrace(llvm::errs());
}
~ExitWrapper() {
LLVM_ATTRIBUTE_NORETURN ~ExitingStream() {
// Finish with a newline.
llvm::errs() << "\n";
exit(-1);
}

// Indicates that initial input is in, so this is where a ": " should be added
// before user input.
ExitWrapper& add_separator() {
ExitingStream& add_separator() {
separator = true;
return *this;
}

// Prints a stack traces.
ExitingStream& print_stack() {
llvm::sys::PrintStackTrace(llvm::errs());
return *this;
}

// If the bool cast occurs, it's because the condition is false. This supports
// && short-circuiting the creation of ExitWrapper.
// && short-circuiting the creation of ExitingStream.
explicit operator bool() const { return true; }

// Forward output to llvm::errs.
template <typename T>
ExitWrapper& operator<<(const T& message) {
ExitingStream& operator<<(const T& message) {
if (separator) {
llvm::errs() << ": ";
separator = false;
Expand All @@ -51,17 +53,23 @@ class ExitWrapper {
bool separator = false;
};

} // namespace CheckInternal

// Checks the given condition, and if it's false, prints a stack, streams the
// error message, then exits. This should be used for unexpected errors, such as
// a bug in the application.
//
// For example:
// CHECK(is_valid) << "Data is not valid!";
#define CHECK(condition) \
(!(condition)) && \
(CheckInternal::ExitWrapper() << "CHECK failure: " #condition) \
#define CHECK(condition) \
(!(condition)) && \
(Carbon::ExitingStream().print_stack() << "CHECK failure: " #condition) \
.add_separator()

// This is similar to CHECK, but is unconditional.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth explicitly documenting why you'd want to use this instead of CHECK(false).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

//
// For example:
// FATAL() << "Unreachable!";
#define FATAL() Carbon::ExitingStream().print_stack() << "FATAL: "

} // namespace Carbon

#endif // COMMON_CHECK_H_
14 changes: 12 additions & 2 deletions common/check_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Carbon {
TEST(CheckTest, CheckTrue) { CHECK(true); }

TEST(CheckTest, CheckFalse) {
ASSERT_DEATH({ CHECK(false); }, "CHECK failure: false");
ASSERT_DEATH({ CHECK(false); }, "\nCHECK failure: false\n");
}

TEST(CheckTest, CheckTrueCallbackNotUsed) {
Expand All @@ -25,7 +25,7 @@ TEST(CheckTest, CheckTrueCallbackNotUsed) {
}

TEST(CheckTest, CheckFalseMessage) {
ASSERT_DEATH({ CHECK(false) << "msg"; }, "CHECK failure: false: msg");
ASSERT_DEATH({ CHECK(false) << "msg"; }, "\nCHECK failure: false: msg\n");
}

TEST(CheckTest, CheckOutputForms) {
Expand All @@ -35,4 +35,14 @@ TEST(CheckTest, CheckOutputForms) {
CHECK(true) << msg << str << i << 0;
}

TEST(CheckTest, Fatal) {
ASSERT_DEATH({ FATAL() << "msg"; }, "\nFATAL: msg\n");
}

auto FatalNoReturnRequired() -> int { FATAL() << "msg"; }

TEST(ErrorTest, FatalNoReturnRequired) {
ASSERT_DEATH({ FatalNoReturnRequired(); }, "\nFATAL: msg\n");
}

} // namespace Carbon
2 changes: 1 addition & 1 deletion executable_semantics/ast/pattern.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ AlternativePattern::AlternativePattern(int line_num,
const TuplePattern* arguments)
: Pattern(Kind::AlternativePattern, line_num), arguments(arguments) {
if (alternative->tag() != ExpressionKind::FieldAccessExpression) {
FATAL_USER_ERROR(alternative->line_num)
FATAL_PROGRAM_ERROR(alternative->line_num)
<< "Alternative pattern must have the form of a field access.";
}
const auto& field_access = alternative->GetFieldAccessExpression();
Expand Down
2 changes: 1 addition & 1 deletion executable_semantics/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ cc_library(
name = "error",
hdrs = ["error.h"],
deps = [
"@llvm-project//llvm:Support",
"//common:check",
],
)

Expand Down
45 changes: 12 additions & 33 deletions executable_semantics/common/error.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,56 +5,35 @@
#ifndef EXECUTABLE_SEMANTICS_COMMON_ERROR_H_
#define EXECUTABLE_SEMANTICS_COMMON_ERROR_H_

#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/Signals.h"
#include "llvm/Support/raw_ostream.h"
#include "common/check.h"

namespace Carbon {

namespace ErrorInternal {

// An error-printing stream that exits on destruction.
class ExitingStream {
public:
// Ends the error with a newline and exits.
LLVM_ATTRIBUTE_NORETURN virtual ~ExitingStream() {
llvm::errs() << "\n";
exit(-1);
}

// Forward output to llvm::errs.
template <typename T>
ExitingStream& operator<<(const T& message) {
llvm::errs() << message;
return *this;
}
};

} // namespace ErrorInternal

// Prints an error and exits. This should be used for non-recoverable errors
// with user input.
//
// For example:
// FATAL_USER_ERROR(line_num) << "Line is bad!";
// FATAL_USER_ERROR_NO_LINE() << "Application is bad!";
// FATAL_PROGRAM_ERROR(line_num) << "Line is bad!";
// FATAL_PROGRAM_ERROR_NO_LINE() << "Application is bad!";
//
// Where possible, try to identify the error as a compilation error or runtime
// error. The generic user error option is provided as a fallback for cases that
// don't fit either of those classifications.
// Where possible, try to identify the error as a compilation or
// runtime error. Use CHECK/FATAL for internal errors. The generic program error
// option is provided as a fallback for cases that don't fit those
// classifications.

#define FATAL_USER_ERROR_NO_LINE() ErrorInternal::ExitingStream() << "ERROR: "
#define FATAL_PROGRAM_ERROR_NO_LINE() \
Carbon::ExitingStream() << "PROGRAM ERROR: "

#define FATAL_USER_ERROR(line) FATAL_USER_ERROR_NO_LINE() << line << ": "
#define FATAL_PROGRAM_ERROR(line) FATAL_PROGRAM_ERROR_NO_LINE() << line << ": "

#define FATAL_COMPILATION_ERROR_NO_LINE() \
ErrorInternal::ExitingStream() << "COMPILATION ERROR: "
Carbon::ExitingStream() << "COMPILATION ERROR: "

#define FATAL_COMPILATION_ERROR(line) \
FATAL_COMPILATION_ERROR_NO_LINE() << line << ": "

#define FATAL_RUNTIME_ERROR_NO_LINE() \
ErrorInternal::ExitingStream() << "RUNTIME ERROR: "
Carbon::ExitingStream() << "RUNTIME ERROR: "

#define FATAL_RUNTIME_ERROR(line) FATAL_RUNTIME_ERROR_NO_LINE() << line << ": "

Expand Down
20 changes: 8 additions & 12 deletions executable_semantics/common/error_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,24 @@
namespace Carbon {
namespace {

TEST(ErrorTest, FatalUserError) {
ASSERT_DEATH({ FATAL_RUNTIME_ERROR_NO_LINE() << "test"; }, "ERROR: test\n");
TEST(ErrorTest, FatalProgramError) {
ASSERT_DEATH({ FATAL_PROGRAM_ERROR_NO_LINE() << "test"; },
"^PROGRAM ERROR: test\n");
}

TEST(ErrorTest, FatalRuntimeError) {
ASSERT_DEATH({ FATAL_RUNTIME_ERROR_NO_LINE() << "test"; },
"RUNTIME ERROR: test\n");
"^RUNTIME ERROR: test\n");
}

TEST(ErrorTest, FatalCompilationError) {
ASSERT_DEATH({ FATAL_COMPILATION_ERROR_NO_LINE() << "test"; },
"COMPILATION ERROR: test\n");
"^COMPILATION ERROR: test\n");
}

TEST(ErrorTest, FatalUserErrorLine) {
ASSERT_DEATH({ FATAL_USER_ERROR(1) << "test"; }, "ERROR: 1: test\n");
}

auto NoReturnRequired() -> int { FATAL_USER_ERROR_NO_LINE() << "test"; }

TEST(ErrorTest, NoReturnRequired) {
ASSERT_DEATH({ NoReturnRequired(); }, "ERROR: test\n");
TEST(ErrorTest, FatalProgramErrorLine) {
ASSERT_DEATH({ FATAL_PROGRAM_ERROR(1) << "test"; },
"^PROGRAM ERROR: 1: test\n");
}

} // namespace
Expand Down
1 change: 0 additions & 1 deletion executable_semantics/interpreter/dictionary.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#ifndef EXECUTABLE_SEMANTICS_INTERPRETER_DICTIONARY_H_
#define EXECUTABLE_SEMANTICS_INTERPRETER_DICTIONARY_H_

#include <iostream>
#include <list>
#include <optional>
#include <string>
Expand Down
5 changes: 2 additions & 3 deletions executable_semantics/interpreter/heap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@ void Heap::Write(const Address& a, const Value* v, int line_num) {

void Heap::CheckAlive(const Address& address, int line_num) {
if (!alive_[address.index]) {
FATAL_RUNTIME_ERROR(line_num)
<< ": undefined behavior: access to dead value "
<< *values_[address.index];
FATAL_RUNTIME_ERROR(line_num) << "undefined behavior: access to dead value "
<< *values_[address.index];
}
}

Expand Down
35 changes: 8 additions & 27 deletions executable_semantics/interpreter/interpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ auto EvalPrim(Operator op, const std::vector<const Value*>& args, int line_num)
case Operator::Ptr:
return global_arena->New<PointerType>(args[0]);
case Operator::Deref:
llvm::errs() << line_num << ": dereference not implemented yet\n";
exit(-1);
FATAL() << "dereference not implemented yet";
}
}

Expand Down Expand Up @@ -298,10 +297,7 @@ auto PatternMatch(const Value* p, const Value* v, Env values,
return values;
}
default:
llvm::errs()
<< "internal error, expected a tuple value in pattern, not " << *v
<< "\n";
exit(-1);
FATAL() << "expected a tuple value in pattern, not " << *v;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of why I want to encourage CHECK for internal errors: if we had CHECK(v->Tag() == Value::Kind::TupleValue); up on line 275, we wouldn't need the switch statement, which would simplify the code while making the intent clearer.

I'm not asking you to make this change; just noting it as an example of the motivation for my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, but I think often we do just want something more like FATAL(), e.g. the previous edit on line 102.

}
case Value::Kind::AlternativeValue:
switch (v->Tag()) {
Expand All @@ -320,11 +316,7 @@ auto PatternMatch(const Value* p, const Value* v, Env values,
return *matches;
}
default:
llvm::errs()
<< "internal error, expected a choice alternative in pattern, "
"not "
<< *v << "\n";
exit(-1);
FATAL() << "expected a choice alternative in pattern, not " << *v;
}
case Value::Kind::FunctionType:
switch (v->Tag()) {
Expand Down Expand Up @@ -377,11 +369,7 @@ void PatternAssignment(const Value* pat, const Value* val, int line_num) {
break;
}
default:
llvm::errs()
<< "internal error, expected a tuple value on right-hand-side, "
"not "
<< *val << "\n";
exit(-1);
FATAL() << "expected a tuple value on right-hand-side, not " << *val;
}
break;
}
Expand All @@ -397,11 +385,7 @@ void PatternAssignment(const Value* pat, const Value* val, int line_num) {
break;
}
default:
llvm::errs()
<< "internal error, expected an alternative in left-hand-side, "
"not "
<< *val << "\n";
exit(-1);
FATAL() << "expected an alternative in left-hand-side, not " << *val;
}
break;
}
Expand All @@ -428,8 +412,7 @@ void StepLvalue() {
CurrentEnv(state).Get(exp->GetIdentifierExpression().name);
if (!pointer) {
FATAL_RUNTIME_ERROR(exp->line_num)
<< ": could not find `" << exp->GetIdentifierExpression().name
<< "`";
<< "could not find `" << exp->GetIdentifierExpression().name << "`";
}
const Value* v = global_arena->New<PointerValue>(*pointer);
frame->todo.Pop();
Expand Down Expand Up @@ -610,8 +593,7 @@ void StepExp() {
CurrentEnv(state).Get(exp->GetIdentifierExpression().name);
if (!pointer) {
FATAL_RUNTIME_ERROR(exp->line_num)
<< ": could not find `" << exp->GetIdentifierExpression().name
<< "`";
<< "could not find `" << exp->GetIdentifierExpression().name << "`";
}
const Value* pointee = state->heap.Read(*pointer, exp->line_num);
frame->todo.Pop(1);
Expand Down Expand Up @@ -670,8 +652,7 @@ void StepExp() {
frame->todo.Pop(1);
CallFunction(exp->line_num, act->results, state);
} else {
llvm::errs() << "internal error in handle_value with Call\n";
exit(-1);
FATAL() << "in handle_value with Call pos " << act->pos;
}
break;
case ExpressionKind::IntTypeLiteral: {
Expand Down
Loading