10
\$\begingroup\$

I've been looking into and experimenting with various paradigms for error handling in C++.

My goals for a robust error handling mechanism would be:

  • Enforce handling the error cases (in other words, make it more difficult to NOT handle the error case).
    • i.e. don't let things like result.value()->do_stuff() slide without proper checks that *result.value() actually exists and is valid. (my issue with things like std::expected btw, nothing is really stopping you from not calling result.has_value() and just directly proceeding to result.value())
    • regarding exceptions, nothing is stopping you from just not writing try and catch. plus you have no idea what exceptions could be thrown without spelunking into the library's source code.
      • maybe Java was right all along with checked exceptions!
    • i personally like golang style with result, err != nil; if err != nil {...}. though unlike golang, C++ does not enforce variables to be used (in golang, if you forgot to deal with err, you'd get a compiler error). I suppose you could turn on -Wunused-variable and treat warnings as errors. still, C++ doesn't actually have multiple return values so you could just pack the result into a single variable and skip dealing with the error.
  • Try to leverage the compiler as much as possible, instead of trusting things to go right at runtime.
  • Accept that user/client error cases are valid states of the program and encode that into the program, instead of trying to bury them under the rug or treat them as footnotes
  • Be able to support complex error data types generically, to support rich and customized error data collection and output.

Currently I'm limited to C++17, so I'll be using that in this demo. Here is a Result class I came up with to support the above paradigm:

namespace util { template<typename Data, typename Error> class Result { public: Result(Data&& data) : _result(std::move(data)) {} Result(Error&& error) : _result(std::move(error)) {} using DataFunction = std::function<void(Data&&)>; using ErrorFunction = std::function<void(Error&&)>; [[nodiscard]] bool handle(DataFunction&& data_func, ErrorFunction&& error_func) { if (std::holds_alternative<Error>(_result)) { error_func(std::move(std::get<Error>(_result))); return false; } data_func(std::move(std::get<Data>(_result))); return true; } private: std::variant<Data, Error> _result; }; } 

It's essentially a wrapper around std::variant.

You would call handle() and provide two lambda functions to "unwrap" the variant in order to actually get the real data (or the error). In data_func and error_func, you deal with the Data case and Error cases separately.

handle returns a bool that's false if the Error state was returned (with a [[nodiscard]] to discourage ignoring of the bool). The purpose of the bool is to allow for writing early return logic (since you wouldn't be able to do that inside of error_func).


To test out the ergonomics of this Result class, I made a small sample program that parses a "fake" configuration file format. The format is just a bunch of key=value pairs separated by newlines. I'll use snippets from there to explain.

Here is an example of Result class usage:

 static util::Result<FakeConfig, InitError> init(const std::string& filename) { using Reason = InitError::Reason; std::ifstream file(filename); if (!file.is_open()) { return InitError(Reason::INVALID_FILE); } std::unordered_map<std::string, std::string> dictionary; int line_num = 1; std::string line; while (std::getline(file, line)) { if (line.size() > 0) { std::string key, value; std::unique_ptr<InitError> error; if (!split(line, "=").handle( [&key, &value](std::pair<std::string, std::string>&& tokens) { std::tie(key, value) = tokens; }, [](SplitError&& split_error) { } )) { return InitError::from_line(filename, line_num, Reason::SYNTAX_ERROR); } if (dictionary.find(key) != dictionary.end()) { return InitError::from_line(filename, line_num, Reason::KEY_ALREADY_DEFINED); } dictionary[key] = value; } ++line_num; } return FakeConfig(std::move(dictionary)); } 

Since Result defines Result(Data&&) and Result(Error&&) constructors, we can just return whichever type we want directly (either the FakeConfig object or InitError in this case).


Here is an example of how a caller would use a function that returns a Result type:

 std::unique_ptr<FakeConfig> config; if (!FakeConfig::init(filename).handle( [&config] (FakeConfig&& fc) { config = std::make_unique<FakeConfig>(std::move(fc)); }, [&filename] (FakeConfig::InitError&& error) { std::cerr << "Error initializing from file: " << filename << std::endl; using Reason = FakeConfig::InitError::Reason; switch (error.reason) { case Reason::INVALID_FILE: { std::cerr << "Invalid file" << std::endl; break; } case Reason::SYNTAX_ERROR: { std::cerr << "Syntax error on line " << error.line_num << std::endl; std::cerr << "\t\t" << error.line << std::endl; break; } case Reason::KEY_ALREADY_DEFINED: { std::cerr << "Key already defined on line " << error.line_num << std::endl; std::cerr << "\t\t" << error.line << std::endl; break; } } } )) { // early return logic return -1; } // continue main program logic with *config ... 

We must handle() the return type of FakeConfig::init(). We do this by providing data_func for the success case (here, we move the FakeConfig return type into the config pointer), and providing error_func (here we just print out an actual error message based on the contents of the InitError return type).

If handle() is false, then we just return early. Otherwise, we can continue with *config.


Here is the full code. Compiled with g++ and -std=c++17 on macOS.

#include <cctype> #include <fstream> #include <functional> #include <iostream> #include <string> #include <unordered_map> #include <variant> namespace util { template<typename Data, typename Error> class Result { public: Result(Data&& data) : _result(std::move(data)) {} Result(Error&& error) : _result(std::move(error)) {} using DataFunction = std::function<void(Data&&)>; using ErrorFunction = std::function<void(Error&&)>; [[nodiscard]] bool handle(DataFunction&& data_func, ErrorFunction&& error_func) { if (std::holds_alternative<Error>(_result)) { error_func(std::move(std::get<Error>(_result))); return false; } data_func(std::move(std::get<Data>(_result))); return true; } private: std::variant<Data, Error> _result; }; } // // simple example to demonstrate use case: // parse a text file containing "key=value" pairs, newline separated. // // the file could look like the below: // // dog=max // cat=tom // tiger=tigger // donkey=eeyore // // this is less trivial than something like division (and checking division by zero or not) // but otherwise a fairly easy example to understand for demonstrating the Result class // namespace { std::string fetch_line(const std::string& filename, int line_num) { std::ifstream file(filename); int l = 1; std::string line; while (std::getline(file, line)) { if (l == line_num) { return line; } ++l; } return ""; } struct SplitError {}; util::Result<std::pair<std::string, std::string>, SplitError> split( const std::string& str, const std::string& delim ) { size_t delim_location = str.find(delim); if (delim_location == std::string::npos) { return SplitError(); } std::string key = str.substr(0, delim_location); std::string value = str.substr(delim_location + delim.size()); if (key.size() == 0 || value.size() == 0) { return SplitError(); } return std::make_pair(key, value); } bool is_all_alphanum(const std::string& str, size_t start) { if (start >= str.size()) return false; for (size_t i = start; i < str.size(); ++i) { if (!std::isalnum(str[i])) { return false; } } return true; } } class FakeConfig { public: FakeConfig(FakeConfig&&) = default; struct InitError { enum class Reason { INVALID_FILE, SYNTAX_ERROR, KEY_ALREADY_DEFINED // disallow re-assignment of keys } reason; int line_num; std::string line; InitError(InitError&&) = default; InitError(InitError::Reason r) : reason(r) {} static InitError from_line(const std::string& filename, int line_num, Reason reason) { std::string line = fetch_line(filename, line_num); InitError error(reason); error.line_num = line_num; error.line = line; return error; } }; static util::Result<FakeConfig, InitError> init(const std::string& filename) { using Reason = InitError::Reason; std::ifstream file(filename); if (!file.is_open()) { return InitError(Reason::INVALID_FILE); } std::unordered_map<std::string, std::string> dictionary; int line_num = 1; std::string line; while (std::getline(file, line)) { if (line.size() > 0) { std::string key, value; std::unique_ptr<InitError> error; if (!split(line, "=").handle( [&key, &value](std::pair<std::string, std::string>&& tokens) { std::tie(key, value) = tokens; }, [](SplitError&& split_error) { } )) { return InitError::from_line(filename, line_num, Reason::SYNTAX_ERROR); } if (dictionary.find(key) != dictionary.end()) { return InitError::from_line(filename, line_num, Reason::KEY_ALREADY_DEFINED); } dictionary[key] = value; } ++line_num; } return FakeConfig(std::move(dictionary)); } struct GetError { enum class Reason { ILLEGAL_KEY, KEY_NOT_FOUND, } reason; std::string key; GetError(Reason r, const std::string& k) : reason(r), key(k) {} }; util::Result<std::string, GetError> get(const std::string& key) { using Reason = GetError::Reason; bool is_legal_key = key.size() >= 1 && std::isalpha(key[0]) && is_all_alphanum(key, 1) ; if (!is_legal_key) { return GetError(Reason::ILLEGAL_KEY, key); } if (_values.find(key) == _values.end()) { return GetError(Reason::KEY_NOT_FOUND, key); } return std::string(_values[key]); } private: FakeConfig(std::unordered_map<std::string, std::string>&& values) : _values(values) {} std::unordered_map<std::string, std::string> _values; }; int main(int argc, char** argv) { if (argc < 2) { std::cerr << "usage: ./parser filename" << std::endl; return -1; } std::string filename = argv[1]; std::unique_ptr<FakeConfig> config; if (!FakeConfig::init(filename).handle( [&config] (FakeConfig&& fc) { config = std::make_unique<FakeConfig>(std::move(fc)); }, [&filename] (FakeConfig::InitError&& error) { std::cerr << "Error initializing from file: " << filename << std::endl; using Reason = FakeConfig::InitError::Reason; switch (error.reason) { case Reason::INVALID_FILE: { std::cerr << "Invalid file" << std::endl; break; } case Reason::SYNTAX_ERROR: { std::cerr << "Syntax error on line " << error.line_num << std::endl; std::cerr << "\t\t" << error.line << std::endl; break; } case Reason::KEY_ALREADY_DEFINED: { std::cerr << "Key already defined on line " << error.line_num << std::endl; std::cerr << "\t\t" << error.line << std::endl; break; } } } )) { return -1; } std::string input; do { std::cout << "Enter a key (or ctrl-C to quit): "; std::cin >> input; std::cout << "\t"; if (!config->get(input).handle( [&input] (std::string&& value) { std::cout << input << " -> " << value << std::endl; }, [&input] (FakeConfig::GetError&& error) { std::cerr << "Could not get key: " << input << std::endl << "\t"; using Reason = FakeConfig::GetError::Reason; switch (error.reason) { case Reason::ILLEGAL_KEY: { std::cerr << "\"" << input << "\" is not a legal key" << std::endl; break; } case Reason::KEY_NOT_FOUND: { std::cerr << "key \"" << input << "\" was not found" << std::endl; break; } } } )) { // no early return, just continue with the next input } std::cout << std::endl; } while (true); return 0; } 

And some sample output

./parser animals.txt Enter a key (or ctrl-C to quit): tiger tiger -> tigger Enter a key (or ctrl-C to quit): donkey donkey -> eeyore Enter a key (or ctrl-C to quit): asdfqwef Could not get key: asdfqwef key "asdfqwef" was not found Enter a key (or ctrl-C to quit): asdf@@ Could not get key: asdf@@ "asdf@@" is not a legal key Enter a key (or ctrl-C to quit): ^C 
./parser animals Error initializing from file: animals Invalid file 
Error initializing from file: animals_error.txt Syntax error on line 4 tigertigger= 

I think this error handling paradigm may be in other languages, but I couldn't find anything similar in C++. This might be essentially makeshift pattern matching?

Would appreciate feedback and please point out any potential pitfalls. Particularly feedback focused on the Result class itself and the usage examples. The FakeConfig code itself is just a silly demo program to actually test out this idea.

\$\endgroup\$

    2 Answers 2

    14
    \$\begingroup\$

    Design review

    Your design description has a number of factual errors and misunderstandings in it, and more than a little questionable reasoning, all of which undermines the design of Result. The net result is that Result is inferior to both exceptions and expected in just about every way.

    Misconceptions

    Let’s start with clearing up some of the misconceptions.

    Misconception 1: with std::expected “nothing is stopping you” from calling .value() without checking

    This is a pernicious misconception because it is “technically true”, while still being factually wrong.

    There are, basically, two ways to get the value out of a std::expected:

    • operator* (or operator->); or
    • .value().

    In the first case, the thing stopping you from doing operator* without checking—either with .has_value() or with operator bool—is the fact that you may be triggering UB. If you do operator* without checking, your program is simply flat-out wrong.

    While it is certainly possible to just… not do it… it is not easy to forget to check. std::expected does not automatically convert to the value type, so you can’t accidentally access it. You have to explicitly do operator*, and if you have to take that extra, explicit step, it is pretty hard to forget to check first. I can’t even imagine how that might happen. You have to pretty consciously choose to not do the check before the access, which really makes it no different from checking the return value of .handle().

    As for .value(), you just try not checking before (when there is an error), and see what happens. I guarantee you, you will not miss that there was an error, and that you did not check.

    Put another way, yes, it is “technically true” that you can call .value() without checking. But it’s “technically true” bullshit, because .value()does the checking.

    So basically, if you don’t want your program to have UB, or to crash, then that is what is stopping you from accessing the value in an expected without checking. And you can’t forget to check, because the access is explicit, so the moment you remember (or are reminded by a compile error) to do the extra step for access, you are automatically reminded to check. (Or choose not to check, and just let the exception happen.)

    Misconception 2: with exceptions “nothing is stopping you” from not writing try / catch

    This is another “technically true” pernicious misconception.

    There are two situations to consider:

    1. you want to handle some kind of error (or all errors); or
    2. you do not want to handle any errors.

    In case 1… yes, there is absolutely something stopping you from not writing the try block: you can’t not write it if you want to handle the error. I mean, obviously, right? If you write any code to handle the error, where the hell are you going to put it, if not in the try block?

    In case 2… sure, you can get away with not writing the try block… but that’s a good thing. I fail to see how your system improves things by forcing a coder to write superfluous, pointless error handling code when they don’t want to handle an error.

    So the thing stopping you from not writing the try block if you want to handle the error is that… you need to write the try block to handle the error. And if you don’t want to handle the error… why is not writing the try block a problem?

    Misconception 3: you have no idea what exceptions could be thrown without reading a library’s source code

    Have you never heard of documentation?

    The logic of exceptions is that you don’t need to… and shouldn’t… handle exceptions that you don’t know about.

    Imagine a system that reads temperature values from sensors, does some calculation with them, and produces some computed data. Suppose that, when first set up, the sensors are simple, directly-connected devices. These can fail, and when they do, the get_sensor_value() function throws a sensor_failure exception.

    Now the compute_data() function has no way to deal with a sensor failure. Yet, by your logic, it still needs to be coded to explicitly deal with sensor_failure exceptions. At the very least, you seem to think it should announce the possibility that it might be propagating sensor_failure exceptions, if not actually have a try block right there in the function to handle it (how? ¯\_(ツ)_/¯ you tell me; you seem to think all errors must be handled, and must be handled right where they happen).

    But okay, somehow we figure it all out, and write compute_data() the way you think it should be written… meaning it explicitly handles and propagates sensor_errors.

    And then the system is modified, so that the sensors are now queried over a Wi-fi or I²C interface or something. Now the get_sensor_value() function might throw a new error: connection_error. This is a distinct error, because if the connection is bad, we can try reconnecting, and because if the connection is good but the sensor is bad, we need a different kind of alert: “replace the sensor” versus “check the wi-fi”.

    But now we have to completely rewrite compute_data(), because now it has a whole new class of error to handle and propagate. That could mean a whole new battery expensive code review and testing, if this is a safety-critical system.

    If compute_data() had been written correctly, using proper exception reasoning, then there would be no need for any of that. It would never have handled sensor_error in the first place, and it doesn’t need to know anything about connection_error. It remains exactly the same, and performs the same, even as the underlying system changes.

    If compute_data()could handle sensor_error, or connection_error, then it would. If it can’t, then it shouldn’t. And it can’t handle them, there is no point littering the code with references to things that have no relevance at that layer.

    This is just one example of why checked exceptions are a terrible idea. But I would have thought it obvious that they just can’t work in general. It is literally impossible to write a stable, robust library with checked exceptions, because you have no knowledge about or control over what the sub-libraries might do, exception-wise. This is why even when Java checked exceptions were a thing, everybody just did throws Exception (or other such tricks).

    Misconceptions 4 & 5: unlike golang, C++ does not enforce variables to be used AND C++ doesn't actually have multiple return values

    False.

    Problems with Result

    Confusion between Data and Error types

    What happens if I do: Result<int, int>?

    And why not? Sometimes (often with OS functions) the only error you get from a function is an int error code. And if the actual value you want is an int….

    Okay, you say, just disallow using the same type for both. It may introduce massive headaches if you have to make a bespoke enumeration for the error codes returned by the OS, or if you wrap them in a superfluous class, but that’s a sacrifice you’re willing to make.

    Except… no, still won’t work.

    To see why, imagine you have a Result<std::string, std::runtime_error>… which seems like a perfectly reasonable thing to want… and then you do this:

    auto result = Result<std::string, std::runtime_error>{"..."}; 

    Even though std::string and std::runtime_error are completely distinct types, and even though this would work just fine if you replaced either type with, say int, this code won’t work.

    This is why std::unexpected exists.

    std::unexpected not only makes it impossible to create ambiguous code like the above, it clearly and explicitly signals that you are creating an error state. Consider:

    // assume `using result_t = Result<X, Y>;` is defined somewhere far away. auto f() -> result_t { return 42; // error or not? } 

    With std::expected:

    // assume `using result_t = std::expected<X, Y>;` auto f() -> result_t { return 42; // cannot be an error } auto g() -> result_t { return std::unexpected{42}; // must be an error } 

    Getting the actual result is terrible ergonomics

    For a type named Result, getting the actual result out of it turns out to be a nightmare.

    I’m just going to quote your own code, stripped down for simplicity (not a good sign, since the code I’m quoting is already supposed to be a simple, stripped down example):

    std::unique_ptr<FakeConfig> config; (void)FakeConfig::init(filename).handle( [&config] (FakeConfig&& fc) { config = std::make_unique<FakeConfig>(std::move(fc)); }, [] (auto&&) {} ); 

    Basically, to get a result out of Result, I have to:

    1. create a placeholder variable… so the result type either better have a cheap “default” state, or I’d have to use an optional
    2. pass a reference to that placeholder via the capture of the “success” lambda; then
    3. assign to that reference in the lambda.

    And of course, with all that, good luck getting return value optimization to kick in.

    And the worse part is, for all the talk about making sure errors have to be handled, all these gymnastics means it is easy to use an (effectively) uninitialized result value. Not just possibleeasy. In fact, you have to be very careful not to do it:

    auto f() -> Result<std::string, int> { return -1; } auto result = "i have not been initialized!!!"s; (void)f().handle( [&result](auto&& s) { result = s; }, [](auto&&) {} ); std::println("what do we have here: {}", result); 

    In-place error handling makes code ugly, if not unreadable

    Okay, so the thing with your example code is that whenever any error handling has to be done, it is always done right there, at the point of call. That is not realistic.

    Take the split() function for example. It takes a string and a delimiter, and splits the string into two by the first appearance of that delimiter. It can fail in a couple ways, so it returns a Result<string, SplitError>.

    Now where it is used is in FakeConfig::init(), it looks like this:

    if (!split(line, "=").handle( [&key, &value](std::pair<std::string, std::string>&& tokens) { std::tie(key, value) = tokens; }, [](SplitError&& split_error) { } )) { return InitError::from_line(filename, line_num, Reason::SYNTAX_ERROR); } 

    As you can see, if there is an error, it is handled right there, right at the point of call.

    In real code, this rarely happens. Much more likely, split() will be buried in some kind of line parsing function that doesn’t have the file name or line number. That function will do the split and then either use the result, or let an error bubble up just like the above code does. Which means that function will also need a chunky if block like the above. And so will any deeper functions. And in fact, the way the code is written, it is cheating, because split() should not know that it is an error for the before or after parts to be empty. Lumping it all together hides how ghastly the code would look if the logic was split up, and the error-handling had to be repeated over and over.

    This is what just two levels of depth look like with Result:

    auto split(std::string_view s, std::string_view delim) -> Result<std::pair<std::string_view, std::string_view>, parse_error> { if (auto const pos = s.find(delim); pos != std::string_view::npos) return std::pair{s.substr(0, pos), s.substr(pos + delim.size())}; else return parse_error::missing_delimiter; } auto parse_setting(std::string_view line) -> Result<std::pair<std::string_view, std::string_view>, parse_error> { auto k = std::string_view{}; auto v = std::string_view{}; auto err = parse_error{}; if (not split(line, "=").handle( [&k, &v](auto&& p) { std::tie(k, v) = p; }, [&err](auto&& e) { err = e; } )) { return err; } if (k.empty()) return parse_error::empty_key; if (v.empty()) return parse_error::empty_value; return std::pair{k, v}; } auto parse_config(std::istream& in) -> Result<std::unordered_map<std::string, std::string>, parse_error> { auto settings = std::unordered_map<std::string, std::string>{}; for (auto line = std::string{}; std::getline(in, line); /*nothing*/) { auto k = std::string_view{}; auto v = std::string_view{}; auto err = parse_error{}; if (not parse_setting(line).handle( [&k, &v](auto&& p) { std::tie(v, v) = p; }, [&err](auto&& e) { err = e; } )) { return err; } settings.emplace(k, v); } return settings; } 

    (I deliberately included a typo above. Did you spot it?)

    Look at what this kind of code would look like with std::expected:

    auto split(std::string_view s, std::string_view delim) -> std::expected<std::pair<std::string_view, std::string_view>, parse_error> { if (auto const pos = s.find(delim); pos != std::string_view::npos) return std::pair{s.substr(0, pos), s.substr(pos + delim.size())}; else return std::unexpected{parse_error::missing_delimiter}; } auto parse_setting(std::string_view line) -> std::expected<std::pair<std::string_view, std::string_view>, parse_error> { return split(line, "=") .and_then([](auto&& p) -> std::expected<std::pair<std::string_view, std::string_view>, parse_error> { if (std::get<0>(p).empty()) return std::unexpected{return parse_error::empty_key}; if (std::get<1>(p).empty()) return std::unexpected{return parse_error::empty_value}; return p; } ; } auto parse_config(std::istream& in) -> std::expected<std::unordered_map<std::string, std::string>, parse_error> { auto settings = std::unordered_map<std::string, std::string>{}; for (auto line = std::string{}; std::getline(in, line); /*nothing*/) { if (auto r = parse_setting(line); r) settings.emplace(std::get<0>(*r), std::get<1>(*r)); else return std::unexpected{r.error()}; } return settings; } 

    Yes, the lowest-level function is basically identical… but look at the higher level functions that want to pass through errors. They are almost half as long, and unlike the Result versions, much harder to screw up. (Did you notice I left the key uninitialized in the parse_setting() of the Result example?) Almost all of the code in the functions is actual business logic, not error-handling. Well, some of it is generating errors when problems are detected, but almost none of it is merely propagating errors. Literally 1–2 lines—the else and the return in parse_config() is all of the error propagating code. By contrast, around a third of the code in the Result code is just for passing errors up the chain.

    Now with exceptions (assuming the use of <system_error>):

    auto split(std::string_view s, std::string_view delim) -> std::pair<std::string_view, std::string_view> { if (auto const pos = s.find(delim); pos != std::string_view::npos) return std::pair{s.substr(0, pos), s.substr(pos + delim.size())}; else throw std::system_error{std::error_code{parse_error::missing_delimiter}}; } auto parse_setting(std::string_view line) -> std::pair<std::string_view, std::string_view> { auto const [k, v] = split(line, "="); if (k.empty()) throw std::system_error{std::error_code{return parse_error::empty_key}}; if (v.empty()) throw std::system_error{std::error_code{return parse_error::empty_value}}; return {k, v}; } auto parse_config(std::istream& in) -> std::unordered_map<std::string, std::string> { auto settings = std::unordered_map<std::string, std::string>{}; for (auto line = std::string{}; std::getline(in, line); /*nothing*/) { auto const [k, v] = parse_setting(line); settings.emplace(k, v); } return settings; } 

    Not a single bit of error-propagation code in sight.

    (Note that I don’t suggest using exceptions for parsing code, because parse errors are hardly exceptional. But for code where errors are exceptional, this is the way.)

    Code review

     Result(Data&& data) : _result(std::move(data)) {} Result(Error&& error) : _result(std::move(error)) {} 

    Why are you taking the arguments by rvalue reference? What if I want to copy some data into the result, and not move it? That’s hardly a strange thing to want to do.

     using DataFunction = std::function<void(Data&&)>; using ErrorFunction = std::function<void(Error&&)>; [[nodiscard]] bool handle(DataFunction&& data_func, ErrorFunction&& error_func) { if (std::holds_alternative<Error>(_result)) { error_func(std::move(std::get<Error>(_result))); return false; } data_func(std::move(std::get<Data>(_result))); return true; } 

    Why are you using std::function, and not just taking arbitrary callables? And again, why force rvalue arguments everywhere?

     template<DataFunction, ErrorFunction> [[nodiscard]] bool handle(DataFunction&& data_func, ErrorFunction&& error_func) { if (std::holds_alternative<Error>(_result)) { std::forward<ErrorFunction>(error_func)(std::get<Error>(_result)); return false; } std::forward<DataFunction>(data_func)(std::get<Data>(_result)); return true; } 

    Note the use of std::forward() when making the calls.

    In C++20 and beyond you could, of course, constrain the template parameters to actually be callables. But for C++17, this would have to do.

    Given that Data and Error might be the same type, std::holds_alternative<Error> is a bad idea. You should use .index() or .get_if() with an index, to be safe.

    Also, don’t move the arguments. If you do, then you introduce the possibility of use-after-move if you call .handle() twice on the same result. What you could do is use an rvalue reference qualifier on .handle(), and move the arguments if .handle() is called on an rvalue. But generally, never move something unless you are about to destroy or re-assign it (or it’s already an rvalue, so someone else is planning to destroy or re-assign it). You are doing neither of those things with _result, so moving it here is unsafe.

    Extension

    Handle errors where it makes sense

    Errors shouldn’t always be handled at the point where they happen. Sometimes it makes sense to let the errors bubble up the stack to be handled at a higher level.

    This is true no matter what error handling mechanism you use, and when you are not handling an error, but rather just letting it bubble up—which is by far the most common situation—it is better to have as lightweight syntax as possible. You simply can’t beat exceptions for this; std::expected is a bit noisier.

    Imagine a somewhat simple task: you want to get a file from an HTTP server. So you have a simple function auto get(url) -> std::vector<std::byte>. Now, this can fail in dozens of ways:

    • the server may not support the method you’re using
    • the server may be misconfigured
    • the connection may be spotty, and may drop
    • you may not have the right authorization
    • the file may have been moved
    • the file simply may not exist
    • and many, many more possibilities.

    Now, get() will probably not have all the logic within its braces. More likely it calls several subroutines that do more specific work. Let’s imagine the call stack might look something like:

    get (downloads a file) ↓ connect (connects to an http server) ↓ request (actually requests file from server) 

    Let’s say we’re at the bottom of that stack, and the server reports “the file has moved”. Where is the best place to handle that? Do we need to go back up the chain? No, request() can just do exactly what it was already doing, just with the new address. Higher level functions don’t even need to know the error exists.

    Let’s say now that request() starts to download the file, but there is something wrong. The server is misconfigured, and is not encrypting the data properly, or maybe it said it supports HTTP/2, but it’s not working. request() doesn’t need to know that, and doesn’t need to know anything about encryption at all. All it has to do is throw an exception reporting that it’s getting back gibberish. At the next level up connect() can catch that, and decide to retry with a different encryption algorithm or HTTP version. Not only does request() not need to handle the error, nothing further up the call stack needs to know anything about the gory details of HTTP versions or encryption algorithms.

    Now let’s say the server reports back to request() “file not found”, or “unauthorized”. What can request() do with this? Nothing. Let it pass up the chain. What can connect() do about it? Nothing. Pass it up the chain. What can get() do about it? Nothing. Pass it up to the caller, and let them decide what to do.

    Forcing every operation at every level to handle—or even be aware of—every possible error that might bubble up from below is absurd. This is why checked exceptions were a terrible idea, and this is why requiring noisy explicit error handling like your Go example (I am aware Go has other options, but none are great) is a poor design. Nothing beats exceptions for the ergonomics of this. Whatever other flaws exceptions may have (and they are wildly exaggerated, or sometimes flat-out misrepresented), the way they let errors transparently propagate unless you actually want to handle them is pure perfection. And even when you do handle them, you can push the error handling as far away from the business logic as you want, because the catch blocks don’t need to be right there (unlike, say, an else block). You can’t make a more ergonomic error handling system.

    std::expected is far, far less ergonomic. It’s better suited for cases where errors are “normal”, not strange or rare, and dealing with errors is what you should expect to do. Parsing is a good example: parse errors are just a fact of life; you can’t seriously write a parser and not expect that the data you’re parsing might be malformed. Even so, it is still fairly easy to just let errors bubble up the call stack. Especially if you use the monadic ops.

    But whatever the system, errors should be handled where it makes the most sense. Not (necessarily) immediately, and not (necessarily) in some centralized place far away from where they happen. A good error handling system makes either choice easy, ergonomic, and not bogged down with syntax. For this, exceptions are perfect. std::expected is… okay. Go’s error handling is just atrocious; even Go fanatics will admit that.

    (It is possible to perfectly replicate Go’s error handling in C++, though not with the language alone; you would need to use the right compiler flags. However, I can’t imagine anyone would ever actually do that, because it would be silly. C++ has better options.)

    \$\endgroup\$
    7
    • \$\begingroup\$I'm going to mark this as the answer since it directly answers many of my worries, comprehensively points out pitfalls, and provides detailed examples for std::expected usage. Will try out using std::variant or a third-party expected class in my C++17 code for now, thanks! Comments have a character limit so I'll write some more in reply to this.\$\endgroup\$CommentedNov 7, 2024 at 0:12
    • 3
      \$\begingroup\$"You have to explicitly do operator*, and if you have to take that extra, explicit step, it is pretty hard to forget to check first." => I'll disagree, hard, on that one. Maybe at the time you write you'll think about it, though honestly I wouldn't count on it. Refactoring is even trickier, between changing types and moving code around. Unless you know good linters which would flag such uses, the fact of the matter is that it's very easy to trigger UB without meaning to.\$\endgroup\$CommentedNov 7, 2024 at 12:55
    • 2
      \$\begingroup\$If you want compiler-enforced checking that there was no error before using a value, then you would use std::expected’s .value(), or monadic chaining (or exceptions). This is far superior to Go’s system, in many ways. And yes, it is compiler-enforced… unless it cannot be, such as if the error depends on run-time behaviour. As for exceptions requiring (or preferring) centralized error handling: no. Errors should be handled wherever it makes sense. I’ll extend the answer with an example.\$\endgroup\$
      – indi
      CommentedNov 7, 2024 at 17:27
    • 1
      \$\begingroup\$@MatthieuM. "Unless you know good linters which would flag such uses [...]"clang-tidy has bugprone-unchecked-optional-access which is likely to either be ported for std::expected in the near future, or even work as-is (I didn't test it for use with expected). Not that I disagree with your arguments, however.\$\endgroup\$
      – Erel
      CommentedNov 8, 2024 at 1:57
    • 2
      \$\begingroup\$@indi In the section Getting the actual result [...], it may also be worth to note that this makes the assumption that everything has a operator= that can be used in the first place. It may not always be the case, and that stresses your point on "better have a cheap default state [...]".\$\endgroup\$
      – Erel
      CommentedNov 8, 2024 at 2:21
    8
    \$\begingroup\$

    Your class does make sure the DataFunction is only called if there is actually Data stored in the variant. However, it suffers from a lot of drawbacks:

    About the robustness

    My goals for a robust error handling mechanism would be:

    • Enforce handling the error cases

    Sadly, often we don't write error handling not because we forgot to do it, but rather because we don't want to spend any effort doing it, for whatever reason (including valid ones). So if you want to enforce handling errors, the only way this is going to happen is if writing a proper error handler is less effort than not writing a proper error handler.

    The easy way to defeat your attempt at enforcement is to write an empty lambda:

    util::Result<SomeData, SomeError> result = some_function(…); (void)handle([](SomeData&& data){…}, [](auto){}); 

    Your Result class is also cumbersome to use; you have to pass in a function object just to handle the valid state, and even if you pass in a real error handler, you still have to handle the return value of handle(). It's more lines of code, with more chances of making mistakes.

    • regarding exceptions, nothing is stopping you from just not writing try and catch.

    True, but if you don't catch an exception, it will ensure the program is terminated instead of ignoring the issue. Depending on the situation, that behavior might actually be more robust. std::expected will throw an exception if you try to access the data using .value() if there is none, so incorrect handling of it will not be ignored.

    Don't unconditionally move data

    The constructors of Result taking r-value references and unconditionally std::move()ing them is a bad idea. It's possible to forget that this move happens, and you could then end up writing this kind of code:

    auto some_function() { SomeData data = …; Result<SomeData, SomeError> result = std::move(data); … // lots of other code, we forgot that we moved already … do_something_else(data); // UB? return result; } 

    Accessing moved-from objects is potentially undefined behavior. Instead of forcing the data to be moved into a Result, let the caller decide if it wants to copy or move it. The best way is to use forwarding references. I recommend you look at std::expected's constructors to see how it handles this.

    Using std::move() inside handle() is also problematic. What if this function is called twice? Again, only move is this is explicitly requested. Have a look at std::expected's way again. Also:

    Make handle() work for const objects

    One safety feature of the language is that you can declare something const, so you can't accidentally modify it. You'd want to do the same with result objects. However, since you don't have a const qualified overload of handle(), you can't use that safety feature with your class.

    Look at std::expected's other helpful features

    If you want this to be used instead of std::expected, it would be best if it provided all the features that that type does, like value_or(), error_or(), and_then() and so on. There is also a bunch of other basics found in most other types that you are missing, like an assignment operator, comparison operator, a swap(), and so on.

    Another feature of std::expected is that you can set the non-error type to void.

    \$\endgroup\$
    2
    • 2
      \$\begingroup\$Constructors (or any function) taking rvalue references will not be called on lvalues, so data wont be moved from. Instead, the compiler will error because it can't find a contructor taking a lvalue reference: still a serious limitation, but not UB.\$\endgroup\$
      – OxTaz
      CommentedNov 8, 2024 at 10:12
    • \$\begingroup\$@OxTaz Thanks for pointing that out. It's still a bit problematic though.\$\endgroup\$CommentedNov 8, 2024 at 20:07

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.