Start using [[nodiscard]] in Core to remind callers to handle errors

[Ivorforce]

Describe the project you are working on

Godot engine reliability.

Describe the problem or limitation you are having in your project

Some Godot types can error out of functions early. In this case, often the expected invariants of the called function are broken.

To make a clear example:

Vector<int> vector;
vector.resize(1024);  // This can silently fail and leave vector with size 0.
for (int i = 0; i < vector.size(); i++) {
    print_line(i);  // This may silently not be called.
}

There are many other instances where this can lead to a crash:

Vector<int> vector;
vector.resize(1024);  // This can silently fail and leave vector with size 0.
vector.ptr()[500];  // Potential segfault

Just as an example, there have recently been some 'mysterious' crashes around indexing errors this may help make less mysterious, e.g.:

• Godot 4.4beta3 crashes on Chrome OS' Linux subsystem (virgl driver) godot#102657
• [4.3] Crash when exporting in headless mode on linux with godot 4.3 godot#99284
• Random crashes occur during runtime, Program crashed with signal 4 godot#99064

It should be noted that in most cases where Error is produced, it is also immediately logged and should be part of the error log. However, even in that case, forcing proper error handling may help debug the issues and eliminate possible paths of failure.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

A feature exists in C++ to handle this kind of situation: [[nodiscard]] (C++17). Godot should use [[nodiscard]] internally where appropriate, to remind callers to handle errors explicitly.

In some cases, it is appropriate to simply log an error and exit the function early and gracefully:

Error error = function();
ERR_FAIL_COND(error);

In other cases, a crash is warranted because it's impossible to handle the situation gracefully. In this case, the program should exit with an appropriate, easy to understand error message.

Error error = function();
ERR_CRASH_COND(error);

(more appropriate macros should be chosen, depending on the situation)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Define functions with [[nodiscard]] like so:

[[nodiscard]] Error _realloc(Size p_alloc_size);

If the function is called without handling a potential error (or explicitly ignoring it), the compiler will complain:

warning: ignoring return value of '_realloc', declared with attribute warn_unused_result [-Wunused-result]

If this enhancement will not be used often, can it be worked around with a few lines of script?

It's a systematic issue.

Is there a reason why this should be core and not an add-on in the asset library?

It's core.

[AThousandShips]

Vector::resize would normally only fail under otherwise unrecoverable situations, though cases where the provided argument is too large due to invalid data would be a case

[Ivorforce]

Yes, most of CowData / Vector error cases are rare, but should still be handled for the case of invalid input data (or the actual lack of RAM edge case).

[AThousandShips]

I think that it should be part of more of a holistic approach to dealing with various problems, [[nodiscard]] is useful but it also risks creating situations were we just discard it by other means because we don't need it, creating a code smell, it's a hard to strike balance depending on context

[Ivorforce]

Fully agreed that we'll need other techniques to have a holistic approach for problems!

For [[nodiscard]], specifically, it forces the caller to think about what to do with the return value / potential error:
If it's ignored, it's explicit.
If it crashes, it's explicit.
If it early returns, it's explicit.

Especially, it will be useful in code review because it exposes the chosen strategy. I think that makes it very suitable for this problem specifically.
But yes, other kinds of problems definitely need other kinds of solutions.