Exception unsupported_runtime_value could be more descriptive

[pauleonix]

unsupported_runtime_value is only used by expand. For usability it would be nice if the exception would include the unsupported value itself and ideally also the list of supported values.

[mbeutel]

Short answer: I disagree because reasons, change my mind :)

---

Long answer: I disagree, for the following reasons.

First, unsupported_runtime_value is a direct subclass of std::exception. I guess you are aware of the semantic exception hierarchy in the standard library (outlined here). Its two branches are:

• std::logic_error: "It reports errors that are a consequence of faulty logic within the program such as violating logical preconditions or class invariants and may be preventable."
• std::runtime_error: "It reports errors that are due to events beyond the scope of the program and can not be easily predicted."

In other words: a std::runtime_error represents something beyond the programmer's reach, like "the file you selected isn't writable", and must be reported to the user in some way. Conversely, a std::logic_error occurs when the program has a bug, the details of which are usually not reported to the user. (In my book, std::logic_error shouldn't need to exist at all, and all corresponding error conditions should be caught by some flavor of assert(). Throwing an exception implies that you can somehow recover from this error and keep going; but a logic error is, by definition, unrecoverable, it may at most be circumventable if you're lucky.)

There are several other exception types in the standard library which are not part of this hierarchy, for example std::bad_cast, std::bad_optional_access (the moral equivalent of NullReferenceException), std::bad_alloc or std::bad_variant_access. I don't know why their creators didn't bother to derive them from one of the two base classes of the semantic exception hierarchy, but I think we can agree that all of them, with the possible exception of std::bad_alloc which is an issue of its own, constitute programming errors and should definitely not be user-facing. What's a user to do upon being told that a bad cast has occurred?

expand() converts a runtime value to a variant of type-encoded values. Supplying an unsupported argument to expand() is conceptually similar to an out-of-range variant access, which is probably why I modeled unsupported_runtime_value after std::bad_variant_access. As you'll note, std::bad_variant_access, like the other exceptions not part of the semantic hierarchy, has a zero-argument constructor. The class does not record the minutiae of the bad variant access that occurred, and its what() member function just returns some generic message. I guess the thinking here is: the user won't see the exception message anyway, and the programmer diagnosing the bug can pry the details from the core dump. unsupported_runtime_value simply behaves consistently with the other exceptions.

One could argue that unsupported_runtime_value should be a subclass of std::invalid_argument, which does store a string member. I won't disagree, but in my opinion the whole exception subhierarchy of std::logic_error is superfluous. Generally you need a very good reason to write a catch statements for std::exception rather than std::runtime_error. I have so far seen only three legitimate reasons for that: unit test machinery, REPL environments, and processes which must be kept running at all costs. All of these reasons are disputed; but even so, writing catch statements that differentiate between subtypes of std::logic_error strikes me as plainly wrong and pointless.

But let's say we derive unsupported_runtime_value from std::invalid_argument, and we want to include a legible error message containing the unsupported value and the list of supported value. How would we do it? How to we obtain a string representation of some generic argument passed to expand()? We may sort of get away with built-in numeric types. But expand() is especially useful with enumeration types; how do we handle these? Of course, makeshift comes with machinery to convert enum values to string; but this relies on the user having provided enum string representations, which are entirely optional. Also, should the makeshift/variant.hpp header pull in all the reflection and metadata machinery just for the purpose of maybe generating a more legible non-user-facing error message if the user happened to have defined string representations?

Now, you may have a situation in which the value passed to expand() is provided directly by the user, and you may be tempted to argue that the error message should therefore be user-facing. I would disagree, arguing out that expand() is not designed for input validation, which must always be done explicitly by the programmer. expand() exists only for the sake of consistency with the standard library and the GSL (specifically, gsl::narrow()), which like to throw exceptions for conditions which, in my book, should be caught by assertions. I generally advise to avoid expand() and use expand_failfast() instead (and, likewise, use narrow_failfast() instead of narrow()). But regardless of which flavor you end up using, the validation of user input is your job.

---

Edit: Having noted that, however, I do agree that makeshift could make it easier to generate user-facing error messages. Specifically, there could be a more generic string conversion function (perhaps called makeshift::to_string()?) which also supports arrays. During user input validation, you could then generate the desired error message as follows:

std::println(stderr,
    "Error in parameter '{}': unsupported value '{}' (value needs to be one of {})",
    paramName,
    makeshift::to_string(value),
    makeshift::to_string(makeshift::metadata::values<T>());

[pauleonix]

Yeah, the "situation in which the value passed to expand() is provided directly by the user" is what I was thinking about given that expand is used a lot in existent code. I respect your decision that input validation is out of scope for expand and would welcome the addition of makeshift::to_string() to make input validation more straightforward.

I thought the implementation were more straightforward when I wrote the issue and forked the repo to do it myself only to then realize that it isn't and I should wait for your response.

[pauleonix]

Should I write a new issue for makeshift::to_string() or do you want to track it here?

[mbeutel]

Thanks. We can use this issue to track the suggestion.