Error handling in SFML 3

[Bromeon]

Back in 2015, I created several forum threads around the topic of error handling/reporting:

• to promote the use of assertions
• to discuss logging with sf::err()

Some discussions around the topic date back to 2011 and 2013:

• why SFML didn't use exceptions
• Exceptions in SFML 3?

These are quite interesting, already to understand historic context and how SFML 1 and 2 evolved to be without exceptions.

---

Error handling in SFML 3

Now the year is 2022, a lot has changed, and at last we have the option to introduce breaking changes and redesign the API for better userfriendlyness. Handling errors is an important topic, and there are different options to tackle them in SFML 3. I'll jump right into them:

1. Boolean return types
   E.g. SFML 2 sf::Image::loadFromFile()

   • ✔️ Simple.
   • ✔️ Easy to port to bindings.
   • ❌ Can be forgotten ([[nodiscard]] is needed on every single method).
   • ❌ Needs separate output parameters.
   • ❌ No way to transport error messages, needs sf::err() side channel.

2. Optionals
   Basically all the disadvantages of bool except no need for output parameters.

   • ✔️ Simple to understand and document.
   • ✔️ Easy to port to bindings.
   • ❌ Can be forgotten ([[nodiscard]] is needed on every single method).
   • ❌ No way to transport error messages.

3. Assertions
   Listed for completeness, but they are not really a solution for runtime errors. But we might want to use them more often to complement other error mechanisms in the case of logic errors.

   • ✔️ Immediate breakpoint, must be fixed.
   • ✔️ No runtime overhead in release.
   • ❌ Only useful for errors that are bugs (not loading files etc).
   • ❌ Do not allow the user to be lenient about certain errors.

4. Exceptions

   • ✔️ User cannot ignore them.
   • ✔️ Automatically propagate upward.
   • ✔️ Can be polymorphic, allowing automatic fallbacks and higher-level handling.
   • ✔️ Can theoretically transport extra data beyond message, although rarely done in practice.
   • ✔️ Can be used in constructors.
   • ❌ Hard to document and follow, especially once multiple layers/callbacks/virtual functions are involved.
   • ❌ Exception safety can be hard to achieve, and we need to enter the whole noexcept rabbit hole.
   • ❌ Need verbose try/catch block even in situations where the user explicitly wants to ignore them.
   • ❌ Not easy to map to bindings, especially due to polymorphism.
   • ❌ Small runtime overhead (more or less depending on exception mechanism).

5. Result type
   Inspired by std::expected proposal or Rust's Result type.
   Basically std::variant<T, E> but with a highly specialized API.

   • ✔️ User cannot forget them ([[nodiscard]] can be declared directly on the type).
   • ✔️ User can however explicitly and concisely ignore them (e.g. with (void) or an ignore() method).
   • ✔️ Allow to transport arbitrary data as part of the error (e.g. an enum, message, context info).
   • ✔️ Function signature makes immediately obvious which methods can fail, and how they can do so -- users are not surprised by unexpected errors and cannot miss any documentation.
   • ❌ No automatic propagation (could probably be made somewhat ergonomic as a library solution).
   • ❌ Require static factory functions such as Image::fromFile(), no constructor support (if this is truly a disadvantage).
   • ❌ Non-polymorphic unless explicitly designed.
   • ❌ Requires either custom-built type for SFML or use of an established library.

[vittorioromeo]

Good writeup. I think that the

❌ Can be forgotten ([[nodiscard]] is needed on every single method).

point in (1) is a bit overstated, as adding [[nodiscard]] to these functions is trivial and it completely solves the problem. Similarly, I also think it is overstated in (2), as returning std::optional does force the user to at least manually dereference the optional or check its state, which prevents it from "being forgotten".

[Bromeon]

Definitely, some points weigh heavier than others.

[...] adding [[nodiscard]] to these functions is trivial and it completely solves the problem.

Still, I don't think that fully accounts for reality. We see how often it happens that developers forget things like explicit for constructors, despite "trivial".

My main point however is that [[nodiscard]] can not only be annotated to functions, but also to classes. A class sf::Result could be [[nodiscard]] on a type level, meaning that returning this type would always mean it has to be handled. This not only solves the "forgetting" issue, but makes the method signature expressive without the use of any attributes.

[Chnossos]

Do note that using exceptions, it is possible to build a backtrace to add information at each level using std::nested_exception. I have built such a library myself for work which provides lots of assertion styles to make the user code readable while retaining a high quality of debug information. Sample code would look like this:

#include <exception/Exception.hpp>

void level2()
{
    B_ASSERT(someFuncThatReturnsFalse(), "Error during {}", __FUNCTION__);
}

void level1() try
{
    level2()
}
catch (...)
{
    THROW_NESTED("Error during {}", __FUNCTION__);
}

int main() try
{
    level1();
    return EXIT_SUCCESS;
}
catch (const std::exception & e)
{
    std::cerr << "Fatal error:\n";
    Exception::printExceptionStack(e, std::cerr);
    return EXIT_FAILURE;
}

Which prints (in debug mode):

Fatal error:
 * Error during level1() [main.cpp:level1():14]
  * Error during level2() [main.cpp:level2():5]

It is easy to add new assertions for third-party libraries, whether they use error codes or exception themselves, and use them internally. Main disadvantage is the use of the heavy try-catch notation but that's a given when using exceptions.

On the pros side, it makes the end code really easy to write and read, as well as making errors easy to locate without needing any non-standard libraries (except for fmt for much-needed error string formatting). You can let the error go up without having to write any line of code, compared to error-by-return methods that need to be handled at every level to propagate it back to the right level (pseudo-code):

#include <expected/Expected.hpp>

[[nodiscard]] Expected<int> level2()
{
    if (someFuncThatReturnsFalse())
        return 42;
    else
        return fail("Error during level2()"); // Are file name, function name and line number added?
}

[[nodiscard]] Expected<int> level1()
{
    if (auto const & [value, error] = level2(); error.empty())
        return value;
    else
    {
        // Repackage to add level info + propagate, but what about file name, func name, etc?
        return fail("Error during level1(): {}", error);
    }
}

int main()
{
    if (auto const & [value, error] = level1(); error.empty())
    {
        std::cout << value << std::endl;
        return EXIT_SUCCESS;
    }
    else
    {
        std::cerr << "Fatal error: " << error << std::endl;
        return EXIT_FAILURE;
    }
}

[ChrisThrasher]

TL;DR I vote for Option 2.

---

Option 1 is the same as Option 2 given the limitations of C++03. Option 2 is the natural modernization of what we're already doing. We can transition to this pattern easily and the library won't feel much different from how errors are treated in SFML 2. We're still returning truth-y values to indicate success or failure but we do so in a more expressive and idiomatic way that allows for shorter user code and better const-ness.

Assertions seem somewhat orthogonal to me. They're mostly a developer tool to catch invariant violations. Sure, users with debug builds of SFML can benefit from them but we cannot rely on users having access to debug builds so we cannot craft APIs around this assumption.

Exceptions are my preference in the general case. They're well understood and impossible to accidentally ignore. It's worth mentioning that I seldom see anyone handle sf::Font::loadFromFile's failure other than returning from main or throwing. If the API already threw upon failure, users would still get the behavior they seemingly want. However I understand its shortcomings and how it may do a poor job serving the last ~10% of users who do want to take action if a resource fails to initialize.

std::expected is maybe the ideal solution. I've used tl::expected (a clone of C++23's std::expected) and really like it. It gives me a lot of what I like about exceptions. The implementation is ~3,000 lines long so it's not reasonable to implement this ourselves. If we are to depend on tl::expected then that's a discussion unto itself. In the end I think that this is just out of our reach and not worth the marginal improvement over std::optional.

[JonnyPtn]

Id say option 5 is best - although we can't use the STL one the arguments for it still stand and for SFML's purposes it wouldn't be too hard to implement

[eXpl0it3r]

For me std::optional doesn't really solve the main issue of moving the error information into the user code. Redirecting sf:err() is a really poor solution (see this ugly code for spdlog) and std::optional doesn't address this in anyway.
std::optional is an improvement in the way it ensures non-default constructability and invalid resource states, but in my opinion it makes the API much harder to use.

---

std::exception on the other hand solves the issue of passing error information, but it's not available in C++17. As this will be a fundamental "type" in C++23, I don't want to polyfill this, nor use tl::expected. General reasons against basic type polyfills:

• It takes a lot more effort to onboard people on a new language feature, being a front-runner with a polyfill would make this even harder, as there aren't even enough teaching resources around
• It partially satisfies people with "old" compilers, as they have to learn this pattern, that seems SFML specific, but actually isn't
• It doesn't satisfy people with "latest" compilers, as they really want to use std::expected and not sf::Expected
• Implementing and maintaining a polyfill can be a project on its own

---

Exceptions seem like the obvious choice to me (in absence of std::expected). They can carry any kind of error information and be as granular as we want them to be. They keep the API very simple. It's a concept widely taught and easily understood. You can handle them at the layer where you want to. IDEs support them with stacktrace displaying and more.

One point that seems a bit misunderstood even in the draft PR #2237 is that exception handling has to be done in a similar fashion as you would with a return error codes or error types (i.e. ADT which means for all the non fancy C++ terms speakers Abstract Data Type). But a feature of exceptions is that you can handle exceptions separately from your implementation code. You don't have to have a try-catch block around single function calls that can throw, instead you can pick the size of the wanted scope. For example having a top-level exception handler or handling load exceptions around multiple loading statements, sharing the error handling code for all of them and more. The reasons to have a minimal scope as touched on in #2237 seem to be to have better clarity on the origin of the error on a code level, i.e. what part of my code can throw an error. To me this seems to fall in the categories of arguments like having to know a given type of a variable at all times, sure it could sometimes be useful to know, but:

• You should handle specific exceptions (e.g. not std::exception but sf::LoadException), so the scope of what could throw this exception is drastically reduced
• The scope can most of the time be selected in a sensible way, which sometimes fails in a short code example (practically speaking one wouldn't handle exceptions in tiny examples anyways), as such it's usually clear what function will throw
• Knowing what specific function in the try block throws is more often than not information you don't even need to know, instead you want to log / retry / exit that whole code block up on failure, so it doesn't matter if it was line 15 or 16

[vittorioromeo]

Firstly, see my comments and @Bromeon's comments on the issue.

I strongly dislike and discourage the use of exceptions for situations where the user wants to react to an error relatively quickly rather than having it bubble up a lot, and my claim is that the vast majority of functions fit in this description.

---

If a font (or any other resource, really) fails to be loaded, you want to immediately either:

1. Log the issue and continue with a fallback resource.

2. Exit the program.

3. Interactively ask the user for the location (e.g. the resource is being loaded from a user-provided script rather than from the game engine itself).

All of these actions are to be taken immediately. Attempting to load a font in the middle of the game loop, as part of the game logic, and handling a possible failure in -- say -- main is just not good design and not something I'd like to promote.

Here's an analysis of some approaches when loading a single resources and trying to provide a fallback (real use case -- I used this "fallback" approach in real projects):

//
//
// WITHOUT ANY ERROR HANDLING MECHANISM
sf::Font font = sf::Font::loadFromFile("abcd.ttf");
sf::Text text(font, "hello"); // Possible run-time issue (!)

//
//
// EXCEPTIONS, NO ERROR HANDLING
sf::Font font = sf::Font::loadFromFile("abcd.ttf");
sf::Text text(font, "hello"); // No run-time issue -- good.
                              // But the code looks exactly the same as before!
                              // The user forgot to handle the error, because
                              // it's not obvious that there is one.
                              
//
//
// EXCEPTIONS + FALLBACK
sf::Font font = [&]
{
    try 
    {
        return sf::Font::loadFromFile("abcd.ttf");
    }
    catch (const sf::LoadException& e)
    {   
        return globalDefaultFont;
    }
}(); // Verbose, poor code, requires IIFE or default-constructible font (bad).
     // Discourages people from handling errors where they should be.
     // Encourages unnecessary bubbling-up of errors.

sf::Text text(font, "hello"); 

//
//
// OPTIONAL, NO ERROR HANDLING
sf::Font font = sf::Font::loadFromFile("abcd.ttf").value();
sf::Text text(font, "hello"); // No run-time issue -- good.
                              // The code has a clear indication that something
                              // can go wrong -- the `.value()` call above.

//
//
// OPTIONAL + FALLBACK
sf::Font font = sf::Font::loadFromFile("abcd.ttf").value_or(globalDefaultFont);
sf::Text text(font, "hello"); // No run-time issue -- good.
                              // The code has a clear indication that something
                              // can go wrong, and visibly provides a fallback.

---

Note that the example above is realistic for smaller games, but in larger games you usually have some sort of resource manager, or load resources on demand. Let's see a basic example of a resource manager:

class ResourceManager
{
private:
    std::unordered_map<std::string, sf::Font> m_fonts;
    // ...other resources...

public:
    void addFont(std::string_view key, sf::Font&& font);
    sf::Font const* getFont(std::string_view key) const; 
    // ...other resources...
};

Now, let's imagine we load all fonts at the beginning of the application, through a loadFonts function. How does the choice of error handling mechanism affect the correctness and verbosity of the code?

//
//
// EXCEPTIONS, NO ERROR HANDLING
void loadAllFonts()
{
    for (const auto& f : std::filesystem::recursive_directory_iterator(fontFolder))
    {
        if (!f.is_file() || !f.path().has_extension("ttf")) { continue; }
        
        resourceManager.addFont(f.path().filename(), 
                                sf::Font::loadFromFile(f.path()));
            // Whoops, the author completely forgot to address the error case,
            // yet the code compiles!
            //
            // In this case, the first failure will result in all the remaining
            // fonts *NOT* being loaded. 
            //
            // Is this what the author really intended...?
    }
}

//
//
// EXCEPTIONS, LOG AND CONTINUE, EVERYTHING IN THE TRY BLOCK
void loadAllFonts()
{
    for (const auto& f : std::filesystem::recursive_directory_iterator(fontFolder))
    {
        if (!f.is_file() || !f.path().has_extension("ttf")) { continue; }

        try // First issue: what error are we trying to catch? Could be anything
            // in the block below, including something thrown by `resourceManager`
            // or by `std::filesystem`. 
        {
            resourceManager.addFont(f.path().filename(), 
                                    sf::Font::loadFromFile(f.path()));
        }
        catch(const sf::LoadException& e) // Now we clarify what error we care 
                                          // about, but we still MANUALLY need
                                          // to figure out what part of the code
                                          // above throws this error.
        {
            errorLog() << "Couldn't load font from path " << f.path() 
                       << ", skipping it...\n";
        }
    }
}

//
//
// EXCEPTIONS, LOG AND CONTINUE, ISOLATE ERROR IN THE TRY BLOCK
void loadAllFonts()
{
    for (const auto& f : std::filesystem::recursive_directory_iterator(fontFolder))
    {
        if (!f.is_file() || !f.path().has_extension("ttf")) { continue; }

        sf::Font font = [&]
        {
            try // Now it is clear what can throw. But we had to use IIFE.
            {
                return sf::Font::loadFromFile(f.path())
            }
            catch(const sf::LoadException& e) 
            {
                errorLog() << "Couldn't load font from path " << f.path() 
                           << ", skipping it...\n";
            }
        }();

        resourceManager.addFont(f.path().filename(), std::move(font));
    }
}

//
//
// OPTIONAL, LOG AND CONTINUE
void loadAllFonts()
{
    for (const auto& f : std::filesystem::recursive_directory_iterator(fontFolder))
    {
        if (!f.is_file() || !f.path().has_extension("ttf")) { continue; }

        std::optional<sf::Font> maybeFont = sf::Font::loadFromFile(f.path());
            // Clearly forces the author of the code to THINK what should be
            // done in case of error, at THIS point in time.
        
        if (!maybeFont.has_value()) // Usual control flow.
        {
            errorLog() << "Couldn't load font from path " << f.path() 
                       << ", skipping it...\n";

            continue;
        }

        resourceManager.addFont(f.path().filename(), std::move(*maybeFont));
    }
}

//
//
// THEORETICAL SFML EXPECTED, LOG AND CONTINUE, WITH SOME SUGAR
void loadAllFonts()
{
    for (const auto& f : std::filesystem::recursive_directory_iterator(fontFolder))
    {
        if (!f.is_file() || !f.path().has_extension("ttf")) { continue; }

        sf::Font::loadFromFile(f.path())
            .onSuccess([](sf::Font& font)
            {
                resourceManager.addFont(f.path().filename(), std::move(font));
            })
            .onError([](const std::string& context)
            {
                errorLog() << "Couldn't load font from path " << f.path() 
                           << ", skipping it... context: " << context << '\n';
            });
    }
}

My personal preference and what I'd like everyone to use is either std::optional or sf::Expected. They force the author of the code to at least think about the error locally, and always give the option to propagate it upwards as an exception if they want to.

[ChrisThrasher]

We're solving a problem of a similar nature to the commitee when standardizing <chrono> or <random>. Those are both powerful, complicated APIs. For the vast majority of users, they could get by with a simpler and easier API. However, in the name of being inclusive to many use cases, the committee standardized two very verbose APIs that are incredibly flexible to a myriad of use cases.

On a scale of

srand(time(NULL));
return rand();

to

auto seed_data = std::array<int, std::mt19937::state_size>();
auto random_device = std::random_device();
std::generate_n(std::data(seed_data), std::size(seed_data), std::ref(random_device));
auto sequence = std::seed_seq(std::begin(seed_data), std::end(seed_data));
auto rng = std::mt19937(sequence);
return std::uniform_int_distribution(0, std::numeric_limits<std::uint16_t>::max())(rng);

where does SFML land?

(Yes, this is what it looks like if you want to very correctly initialize a Mersenne twister random number generator and get a single integer out of it).

Using exceptions+constructors puts us very close to the "keep simple things simple" side of the continuum while the use of factory functions and algebraic data types is a big step towards the complexity (and power!) of something like the <random> API. There is no way to quantity our position on this continuum but hopefully it provides a nice mental model for reasoning about API design.

I think the question of error handling boils down to how common is it for users to want to write custom fallback code in the event of resource loading failing. I'd wager that the majority of users won't handle such errors and just want to exit the program. Exceptions+constructors serve that group perfectly. They get to write idiomatic C++ code exactly how they'd expect and so long as their program keeps running they can be certain that certain runtime bugs are not present.

It's the minority of users who want custom fallback behavior who benefit from a more complicated API involving algebraic data types and factory functions. In my analogy, these are the people who want industrial-grade randomness without the quirks of rand(). I'm leaning towards the more complicated API so that we can better support those users but I acknowledge that we're adding complexity that most users will not benefit from.

[therocode]

I don't agree that expected type VS exceptions is a matter of complexity VS simplicity where exceptions is the simple one. Both can have simple APIs and both are equally powerful IMO.

If you just wanna exit the program and you don't care about handling the error recovery, with and expected type you'd just do result.unwrap() or whatever the method would be.

So in my mind, the "expected is for the minority of people who want power, flexibility and complexity, whereas exceptions is for people who want simple newbie-friendly APIs" is a false dichotomy.

The difference IMO is more just a difference in style, with concrete differences in how you use things. The initial post of this thread outlines it very well 👍. Both ways can also be idiomatic, depending on what flavour of C++ you subscribe to.

[therocode]

Some thoughts on the path to whatever ends up the desired goal.

Atm SFML is at "run t.load on uninitialised T, and return bool" which we all seem to agree is not ideal. Changing this to std::optional<T> loadT(); is a modernisation of the former, but has no extra error information. I think it's also easy to make the argument that we should have error information but that we don't know which style of this that we want and the question seems to mostly be around expected vs exceptions.

IMO we should focus on deciding which of these we want, thinking long-term future. I don't think we should take exceptions over expected becuase std::expected isn't available until C++23, as that's only a couple of years away but SFML is supposedly going to try and stay relevant for the foreseeable future, so that seems rather short-sighted IMO. Even if a polyfill is not ideal, a few years with a non-ideal polyfill (or even postponing this error discussion for post-C++23) is much better than ending up picking the suboptimal error handling style and living with that for the rest of the library's lifetime (assuming we would end up thinking that expected is preferred for SFML).

Also, going "return bool" -> "use exceptions" -> "use expected" is a quite jerky road, which fundamentally changes the error handling twice. To me anyway, "return bool" -> "return optional" -> "return expected" is a cleaner road, since the pattern is the same after the initial switch, we're just adding more information. Ideally personally for me, I'd want to see "return bool" -> ("return expected polyfill") -> "return expected" since that'd mean we get useful error info from the first change, and the second change becomes trivial. I think we should not go down the route of exceptions at all, unless we decide that expections is where SFML wants to be long term, and in that case we should go "return bool" -> "use exceptions" since that is possible to implement today, and has fewest steps.

And if it wasn't clear, my personal vote is expected and not exceptions with no extra arguments, it's all outlined above - and I do consider this more or less purely a choice of style/preference with no real critical dealmakers/dealbreakers on either side.