[RFC] Rename ErrorOr and Expected

As mentioned in issue 68828, the names of some of our error-handling classes are unintuitive and even misleading. I propose to change them as follows.

  • ErrorOr<T> (contains std::error_code and maybe T) becomes ErrorCodeOr<T>
  • Expected<T> (contains llvm::Error and maybe T) becomes ErrorOr<T>

Obviously all of the old ErrorOr instances would have to be changed first, before starting on the Expected instances.

1 Like

Renaming something into a name we used to have but changed in the meantime is generally not a good idea. I am 100% sure I will be very confused by this at some point, at the latest, when I look at new and old code at the same time, e.g., compare code and an old PR, or do git blame.

Can we come up with a fresh name?

I had the same thought as @jdoerfert reading the proposal, what about ErrOr<T>? :wink:

(note that “expected” is also consistent with the C++23 std::expected: std::expected - cppreference.com )

I’m very sure that the existing ErrorOr is a bad name choice and should be changed.
I’m willing to say Expected is what it is and leave it alone.

3 Likes

Why do we need both of them? Structure-wise they are essentially the same, i.e. “T or error information”.

Edit: I’m suggesting we convert uses of ErrorOr to Expected.

1 Like

Note that:

template< class T, class E >
class expected;

It is up to you what you put in E . The LLVM expected seems to be missing the E.

ErrorOr<T> is a simple enum or T, and doesn’t insist that you check whether it worked.
Expected<T> uses llvm::Error which is more heavyweight, and aborts if you don’t check it.

If someone doesn’t care what the error was, they can use std::optional. If they want to have error information, then they can use a type where the error check is mandatory. I understand that they are not exactly equivalent, but I think that we should have a canonical way of handling errors, and that Expected fits the bill a lot better than ErrorOr.

llvm::Expected<T> is roughly std::expected<T, llvm::Error> probably with some extra methods (I haven’t looked). I’m not proposing to redefine it that way, although someone could.

In fact, Expected is used far more often than ErrorOr. But as I skim through the uses of ErrorOr, I see it used quite often in file operations, where std::error_code (basically, errno values) is way more appropriate than llvm::Error. I’m reluctant to say those are all wrong.

My point is more about having a single mechanism for error handling, than the details of individual classes. If the current implementation of Expected is deficient, we should consider improving it. Maybe Expected should take that extra E, which can be defaulted to llvm:Error for compatibility?

On that note, I’m wondering if there are any uses of ErrorOr, where the “error” is really a “failure”…

4 Likes

Aligning expected with the standard is the right direction. I prefer partial specialisation over hidden defaults.

template <class T> using ExpectedError = expected<T, llvm::Error>;

The standard does not have a default and leaves it fully open, it seems appropriate to me that the default Error in llvm would be llvm::Error, so I would think that this would be a good setup for LLVM:

template <class T, class E = llvm::Error>
class Expected;

Can the ECError (it is a llvm::Error that wraps a std::error_code) be enough to obsolete the ErrorOr path?

I think this would be an ideal outcome. I think the only trouble is figuring out how to handle known error codes in a reasonably ergonomic way, in code like this and this looking for llvm::errc::no_such_file_or_directory. Can we make some utility that handles specific error codes better than we do currently?

@lhames FYI

As for the renaming dance - if we waited a release or so before reusing the name it might not be the worst thing. But if the standard (albeit more generic) version is called expected, maybe it’s OK to just leave llvm::Expected as-is even if the ErrorOr name is freed up.

I don’t think we need to generalize llvm::Expected to allow for other error entities if we don’t have a compelling use case (if we really want to keep error_code handling around, then maybe that’s sufficient use case, but then we still need a name for it - and yeah, maybe that name is Expected<T, error_code> or something)

But would be great to just standardize on llvm::Error (& consequently just llvm::Expected without being able to configure the error object)

When I get a chance, I will look at replacing ErrorOr with ECError and see how that goes, with particular attention to the point @rnk raised. This would be more than a simple rename, I think, so might require a bit more effort than I was expecting.

Re. std::expected we can look ahead to that, but note that LLVM’s current dialect is C++17 so we can’t assume std::expected is available.

There was llvm::Optional for the same reasons. The std version is only available in later dialects. llvm::Optional is gone now. Adding llvm::E/eexpected with no default until we move to C++23 ought to be an improvement.

llvm::Expected has the additional behavior that it will abort if the error isn’t checked; so its behavior does not align as cleanly with std::expected as in the Optional/optional case. I think the intention to eliminate llvm::Expected would have to be a separate discussion related to the value of that additional behavior.

ATM I am not intending to do anything wrt llvm::Expected.

2 Likes

Aligning the llvm Expected with the standard makes the transition smoother and leaves you the option what to put in E : llvm::Error, std::error_code, …

As @pogo59 mentioned, Expected has extra checking (it checks that you checked the error, and assert-fails if you don’t) std::expected doesn’t have and won’t have - so it’s not as obvious that we’ll want to transition to the standard one (if I had to bet right now - we won’t ever trasnsition llvm::Expected to std::expected because the extra error checking checking is of significant value) - so I wouldn’t be inclined to over-generalize it now/with that direction in mind.

5 Likes