Replaced std::exit() with return Carbon::ErrorOr for expected errors like invalid syntax

[pk19604014]

The trigger for this change is the work to build a structured fuzzer for Carbon, as the fuzzing framework doesn’t like std::exit() calls. However, the change should also allow for using executable semantics as a library, and for easier testing.

Most of the change is pretty mechanical: replacing void with llvm::Error and T with llvm::Expected for fallible functions + updating the code to handle/propagate failures. Internal logic checks continue to use RAW_EXITING_STREAM which will std::abort() if triggered.

Some highlights:

• executable_semantics/common/error.h - added an ErrorBuilder class for streaming error messages to and then converting to either llvm::Error or llvm::Expected. Updated FATAL_COMPILATION_ERROR macro family to use ErrorBuilder, the code now looks like return FATAL_COMPILATION_ERROR() << “message”. Added RETURN_IF_ERROR and ASSING_OR_RETURN macros to help reduce some boilerplate code.

• common/ostream.h - adjusted operator<< injected into the llvm namespace to use a new HasLlvmRawOstreamOp predicate, otherwise the operator was trying to define itself for Carbon::AST type and breaking unimplemented_example_test.

• executable_semantics/syntax/lexer.lpp - updated to return END_OF_FILE on syntax errors. Newer bison versions (3.5+) have a special YYerror token for this, but https://github.com/jmillikin/rules_bison that Carbon is using only supports bison versions up to 3.3.2, and my quick attempt to hack it locally to support 3.5 was not successful (failed to find textstyle.h header, etc.).

• Added fallible AST node constructors for IntrinsicExpression, ​​AlternativePattern, and FunctionDeclaration, defined as factory functions taking an arena as a template parameter, so that the logic can still live in the same file as the main class while not depending on the actual arena implementation.

• executable_semantics/syntax/parser.ypp - updated to use the new fallible constructor functions for IntrinsicExpression, ​​AlternativePattern, and FunctionDeclaration.

• Some fun with the PatternMatch() function which changed to return llvm::Expected, while callers continued to do things like if (PatternMatch(xx)) which return true for any successful call, regardless of whether the actual bool result is true or false ;) Still not sure what the best solution is to effectively prevent such misuse, but for now just updated to use ASSIGN_OR_RETURN.

[jonmeow]

The essence of the RETURN_IF_ERROR and ASSIGN_OR_RETURN approach seems good, although I'm more apprehensive about the specific use of llvm::Error/Expected due to the issues I've encountered with their bool cast behaviors. Rolling our own types for this, without the bool casts, may yield better results long-term.

(note, trying to keep comments light since the PR is still in draft)

[jonmeow]

TBH, if it were up to me, I'd just do something naive like:

struct Error {
 public:
  static Error Success() { return Error(); }
  explicit Error(std::string message) : message_(message) {}
  ~Error() { CHECK(used_); }
  bool ok() { return !message.has_value(); }
  const std::string& message() { used_ = true; return *message_; }

 private:
  bool used_ = false; // Suggesting this mainly because I think it's in LLVM.
  std::optional<std::string> message_;
};

template<typename T>
struct ErrorOr {
 public:
  // Should be implicit
  ErrorOr(std::variant<Error, T> val) : val_(val);
  ~ErrorOr() { CHECK(used_); }

  bool ok() { return std::holds_alternative<T>(val_); }
  Error error() { CHECK(!ok()); used_ = true; return std::get<Error>(val_); }
  const T& value() { CHECK(ok()); used_ = true; return std::get<T>(val_); }

 private:
  bool used_ = false;
  std::variant<Error, T> val_;
};

Or even just copy llvm::Expected/Error and rip out a few pieces, like the implicit cast to bool, and start from there -- with the hope of tearing out a few pieces quickly rather than letting llvm::Expected/Error sink into the codebase. I feel I can't say enough how difficult I find those classes to understand, and I'd much rather not introduce them.

But I think this is up to @geoffromer if he strongly prefers going ahead with llvm::Error/Expected, rather than delaying a little more time to avoid stepping more in that direction.

[geoffromer]

But I think this is up to @geoffromer if he strongly prefers going ahead with llvm::Error/Expected, rather than delaying a little more time to avoid stepping more in that direction.

I was previously pretty neutral between the two options in the short term, but I'm starting to agree more with you that we should start out with our own types, after learning that the bool conversion is the only way of querying the llvm types, and after seeing the const_cast nastiness that's caused by the lack of read-only access to the error.

[pk19604014]

Thanks for the reviews!

And sorry for some single-comment spam, still figuring out how to use github ;)

[pk19604014]

Thanks! I'll be on vacation all of next week, will get back to the PR when I return.

[jonmeow]

Out of side-discussion, there's now Carbon::Error and Carbon::ErrorOr per #1137. Can you please switch from llvm::Error/Expected to that?

[jonmeow]

Thanks, this looks great!

My sense is if there are still issues which we need to fix, this is big enough that it may be easiest to just have this merged and fix forward for the rest. But it may be best overall to just start playing with this and see how it goes.