Add RETURN_IF_ERR macro, propagating an error upwards

[Ivorforce]

Godot does not use exceptions. This often results in patterns where errors are propagated upwards:

Error error = expression();
if (error) {
	return error;
}

I propose to support this paradigm with a dedicated macro:

RETURN_IF_ERROR(expression());

The benefit of this is threefold:

• Error propagation is encouraged (through the 'syntactic sugar').
• Existing code is (arguably) easier to read than before.
• The code is standardized, and if desired, could be improved (e.g. by adding more printing).

An arguable downside is that it's 'another godot-thing' to remember about the code base. I personally think the naming makes it clear enough what the macro does, though.

Relations

The idea of this macro may be familiar to Rust programmers. Rust embraces exception-free coding so much it made this concept into a core part of its grammar, with the question mark ? operator.
It has been proposed to add this operator to C++ too, for exception-free projects like Godot, but it's unlikely it will make it into the standard anytime soon.

[AThousandShips]

There should probably be an EDMSG version for completeness

[AThousandShips]

My concern with this is that it's unintuitive, RETURN_IF_ERR doesn't print any message, which you'd expect from the rest of the error macros

I'd say RETURN_IF_ERR_MSG should be named differently to clarify this

[AThousandShips]

Now these messages might not be helpful, but they:

• Provide a code location for failure, which is important for testing
• Was meant to trigger an error originally, otherwise the code would have been the if (err != OK) format

So should either be replaced by a new macro with a message, or restored

We could go over some of these discussing if they are worth changing away from errors, but we should assume the original macro was used intentionally

I'm also not sure about the readability in some of these cases

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[AThousandShips]

This is incorrect, this removes an error print

[Ivorforce]

Now these messages might not be helpful, but they:

• Provide a code location for failure, which is important for testing
• Was meant to trigger an error originally, otherwise the code would have been the if (err != OK) format

Agreed. I'd actually argue that all RETURN_IF_ERR calls should print an error and the propagation site. This should make it a lot easier to debug user submitted problems, for all the cases that did not print before.
What do you think?

[AThousandShips]

I'd avoid replacing non-printing cases, again, we should assume the choice was conscious. It would at least require a more detailed review to assess what and why to change, most of these are near other code that does use error printing.

Some of these are cases where the source code prints an error, and there's little reason to have a print on the caller, so I'd say:

• It should print where it is the most informative
• It shouldn't print on multiple levels except when relevant
• It shouldn't print for cases where the error condition is "normal"

[Ivorforce]

I'd avoid replacing non-printing cases, again, we should assume the choice was conscious. It would at least require a more detailed review to assess what and why to change, most of these are near other code that does use error printing.

Some of these are cases where the source code prints an error, and there's little reason to have a print on the caller, so I'd say:

• It should print where it is the most informative
• It shouldn't print on multiple levels except when relevant
• It shouldn't print for cases where the error condition is "normal"

I anticipated there might be resistance to the ERR_FAIL_COND_V(err, err) change, so it's isolated in a separate commit :)
So yea, I'm open to just using the RETURN_IF_RR part of this PR for now.
(Although your concern above with the expectation to print would be resolved if we added printing in all cases).

Before any action though, I'd suggest we wait for more people to weigh in on this question.

[AThousandShips]

Incorrect as well, this mutates a class variable not a local one, should be:

Suggested change

	RETURN_IF_ERR(parse_variant(value));
	error = parse_variant(value);
	RETURN_IF_ERR(error);

[Ivorforce]

Oh man, good catch! I definitely should have noticed that.

[AThousandShips]

This changes the behavior of the loop, which should be investigated, but this changes the behavior, so should be excluded

[AThousandShips]

Also, as a tangent but relevant due to the introduction of a new macro, I have some concerns about further encouraging moving functional code into checking macros. This isn't a problem in these cases, and it's not a hill I'm dying on, but it's a thing I've been thinking about and growing concerned about in some cases that I think is worth mentioning.

Since we have some macros that are stripped out on certain builds, like DEV_ASSERT it's critical to ensure intentionality with macro use, and while I don't foresee any case where our current ERR_FAIL base macros becoming conditional, we do have those dev only cases, and might want to expand their use, or add further granularity to that

And for that I feel it would be good to reduce the amount of functional code that is put inside these macros in some cases, because:

Err err = foo();
DEV_ASSERT(err == OK);

Is not the same as:

DEV_ASSERT(foo() == OK);

And it might, in the future, be a good idea to fully enforce that these kind of macros should never have functional code in them except when that's explicitly meant to be error only, or even straight up forbid mutating code there at all to prevent some strange edge cases

Just a thought, but relevant to the changes away from having separate calls

Edit:
I also feel that this new macro fails a measure I like to use for macro usage: does the improvement outweigh the cost of losing readability by obscuring code

To me this removes a few lines of code and reduces the number of local declarations, but adds a layer of obscurity

However one thing I think would be valuable would be a set of ERR_FAIL_NOT_OK macros, which do the same but with a good descriptive default message, with the added bonus of normalizing the checks (we currently mix err and err != OK, which makes for confusing error messages), but those could be confusing because they return explicitly, so could be named like ERR_FAIL_NOT_OK_RET for the version that returns the error itself