[Error] add expectSuccess()

[zturner]

Summary

Often times you expect an operation to succeed, and it's cumbersome and impacts readability to constantly be checking errors which you assume are going to succeed. It's even worse with Expected<T>, because you have to write something like:

auto ExpectedFoo = getFoo();
if (!ExpectedFoo)
  return ExpectedFoo.takeError();
auto &Foo = *ExpectedFoo;

which is 4 lines of code for something that should take 1 (assuming, again, that you have special knowledge which would allow you to know in advance it will succeed). You can reduce it to 3 lines by doing this:

auto ExpectedFoo = getFoo();
consumeError(ExpectedFoo.takeError());
auto &Foo = *ExpectedFoo;

But now if there ever actually *is* an error, it's dropped on the floor. So we could add an assert, but now we're back to 4 lines.

It would be nice to have a function which asserts if there is an error, so we get notified in debug builds, but continues on as if nothing ever happened in release. We already have a similar construct which is ExitOnErr, but we want a similar construct that doesn't exit and still allows us to extract the value and consume the error in a single line. With this patch we can write:

expectSuccess(doSomething());    // returns an Error
auto Value = expectSuccess(getSomething());    // returns an Expected<T>

and it just works, with 1 line.

[chandlerc]

I would suggest "check" instead of "expect" to make it more clear that this will assert.

Also, why assert? I don't really expect (ba-dum) Expected<T> and this part of the error handling APIs to happen in hot paths; would it be better to *always* report_fatal_error?

Would it be better to allow either with different names? checkSuccess for always checking, assertSuccess for debug-only checking?

[zturner]

I think we should minimize use of report_fatal_error in library builds. This seems like a textbook assert use case to me. No different than assert(foo != nullptr); The only objection I have to having both a check and an assert method (and it is a minor objection, mind you) is YAGNI.

Naming wise, what about assertOnError() and/or failOnError? This has slightly more symmetry with the name of the existing ExitOnError class .

[lhames]

I believe this is the same as the current 'cantFail' function?

[chandlerc]

FWIW, I'm happy w/ all of the alternatives (and names) listed by Zach. Not adding a function is always good too.

I will point out that failOnError or assertOnError would be much more discoverable for me than cantFail.

I wonder if there is a better name for Expected<T> to give a hint that it consumes the expected and produces the T? Maybe assertAndGet? assertOrGet?

[lhames]

cantFail always felt like a quirky name though - what about 'assertSuccess'?

This seems like a textbook assert use case to me. No different than assert(foo != nullptr);

Yep. The only reason to use cantFail/expectSuccess is if you *know* the callee can't return an error via this callsite. If you're wrong about that it's a programmatic error like any other.

[chandlerc]

In D34600#796166, @lhames wrote:

cantFail always felt like a quirky name though - what about 'assertSuccess'?

This seems like a textbook assert use case to me. No different than assert(foo != nullptr);

Yep. The only reason to use cantFail/expectSuccess is if you *know* the callee can't return an error via this callsite. If you're wrong about that it's a programmatic error like any other.

Sorry, total agreement. I was only aiming for one that wasn't even behind NDEBUG as it seems pointless to "optimize" this code path rather than crashing quickly. But no big deal anyways. I think the interesting thing is finding a good name. I'll let you two figure this out, i"ve given my meager ideas.

[lhames]

FWIW, I'm happy w/ all of the alternatives (and names) listed by Zach.

Ditto. I'm happy for this to be renamed to something more obvious. Ideally not too wordy though - there are some situations where this API might be used a bit. (E.g.: I'm about to errorize the ORC APIs, so clients building local JITs will have to wrap calls with cantFail).

I wonder if there is a better name for Expected<T> to give a hint that it consumes the expected and produces the T? Maybe assertAndGet? assertOrGet?

FWIW I found that in practice it obvious from context that cantFail would return the wrapped result:

auto X = cantFail(foo(...));

Once people know to look for an 'assertSuccess' type function, the lack of an 'AndGet' suffix seems unlikely to confuse them.

[chandlerc]

In D34600#796195, @lhames wrote:

FWIW I found that in practice it obvious from context that cantFail would return the wrapped result:

auto X = cantFail(foo(...));

Once people know to look for an 'assertSuccess' type function, the lack of an 'AndGet' suffix seems unlikely to confuse them.

I think it will be a bit more confusing in other contexts:

return assertSuccess(foo(...)); // void or T? Both compile...

[lhames]

I was only aiming for one that wasn't even behind NDEBUG as it seems pointless to "optimize" this code path rather than crashing quickly.

Yep. We tossed this question around a few times during the development of Error. It fell in the same basket as "should we track error propagation even in optimized builds" (which I would expect to add more overhead than this). In the end we settled on minimizing the performance overhead for now. It's a discussion I'm happy to revisit though: practice production code is likely to be exposed to environments we haven't tested, and having it fail loudly and clearly (rather than invoking UB) might be worth a little extra overhead? (But I guess you could say that about any assertion).

As a half-way point we could consider putting it under its own flag, something like CHECK_ERRORS_IN_RELEASE.

But no big deal anyways. I think the interesting thing is finding a good name. I'll let you two figure this out...

Sure. Zachary - do you have a preference?

[lhames]

I think it will be a bit more confusing in other contexts:

return assertSuccess(foo(...)); // void or T? Both compile...

Good point. I withdraw my objection to the suffix. :)

[lhames]

Ping. Zachery - do you have a preference on the names?

assertSuccess and assertSuccessAndGet?
expectSuccess and expectSuccessAndGet?

[lhames]

I added an optional error message to cantFail in 312066 to make it easier to recognize which call failed, so the renamed API would be:

Error assertSuccess(Error Err, const char *AssertMsg = nullptr)

template <typename T>
T assertSuccess(Expected<T>, const char *AssertMsg = nullptr)

Sound good?

Cheers,
Lang.

llvm/include/llvm/Support/Error.h

union {
     AlignedCharArrayUnion<error_type> ErrorStorage;
   };
   bool HasError : 1;
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   bool Unchecked : 1;
 #endif
 };
 
+/// assert if an Error condition is present, but check it regardless.  This
+/// method should be used when an error condition is not expected and you want
+/// to forego checking the value.  For example, you may wish to write the code:
+///
+/// if (willOperationSucceed()) {
+///   expectSuccess(doOperation());
+/// }
+///
+/// and save the extra line(s) that would normally be needed to check the return
+/// value.
+inline void expectSuccess(Error Err) {
+  assert(!Err);
+  consumeError(std::move(Err));
+}
+
+/// Similar to expectSuccess(Error), but for Expected<T>
+template <typename T> T expectSuccess(Expected<T> &&Value) {
+  expectSuccess(Value.takeError());
+  return *Value;
+}
+
 /// Helper for Expected<T>s used as out-parameters.
 ///
 /// See ErrorAsOutParameter.
 template <typename T>
 class ExpectedAsOutParameter {
 public:
   ExpectedAsOutParameter(Expected<T> *ValOrErr)
     : ValOrErr(ValOrErr) {