Fix expected argument count for Callable call errors

[dalexeev]

• Closes Strange error message: "Method expected 0 arguments, but called with 0" #80826.

godot/core/variant/callable.h

Lines 57 to 70 in 970be7a

	struct CallError {
	enum Error {
	CALL_OK,
	CALL_ERROR_INVALID_METHOD,
	CALL_ERROR_INVALID_ARGUMENT, // expected is variant type
	CALL_ERROR_TOO_MANY_ARGUMENTS, // expected is number of arguments
	CALL_ERROR_TOO_FEW_ARGUMENTS, // expected is number of arguments
	CALL_ERROR_INSTANCE_IS_NULL,
	CALL_ERROR_METHOD_NOT_CONST,
	};
	Error error = Error::CALL_OK;
	int argument = 0;
	int expected = 0;
	};

[akien-mga]

From a cursory look this seems sensible, but I'm surprised that it was wrong in so many places in the codebase. Is that due to an API design change that wasn't properly propagated?

[dalexeev]

Is that due to an API design change that wasn't properly propagated?

I think argument was the right property before. Then, after the refactoring, the code was not updated in some places, and perhaps bad cases increased due to copy-paste without looking into the header. See diff.

Error error;
int argument;
Type expected;

See also #76259. The problem is that #36368 declared expected as a property responsible for this (diff), but until #76259, argument was actually responsible for this (diff).

[dalexeev]

Perhaps the first mistake was in the comment due to bad copy-paste and the following was intended:

 	enum Error {
 		CALL_OK,
 		CALL_ERROR_INVALID_METHOD,
 		CALL_ERROR_INVALID_ARGUMENT, // expected is variant type
- 		CALL_ERROR_TOO_MANY_ARGUMENTS, // expected is number of arguments
- 		CALL_ERROR_TOO_FEW_ARGUMENTS, // expected is number of arguments
+ 		CALL_ERROR_TOO_MANY_ARGUMENTS, // argument is number of arguments 
+ 		CALL_ERROR_TOO_FEW_ARGUMENTS, // argument is number of arguments
 		CALL_ERROR_INSTANCE_IS_NULL,
 		CALL_ERROR_METHOD_NOT_CONST,
 	};

 	Error error = Error::CALL_OK;
 	int argument = 0;
- 	int expected = 0;
+ 	Variant::Type expected = Variant::NIL;

And then we started changing argument to expected in the code to match the comment.

In my opinion, the original version (like in 3.x) makes more sense, since the argument property is always responsible for the argument count/index. Now in #81605 we have to use switch:

switch (r_call_error.error) {
    case Callable::CallError::CALL_ERROR_INVALID_ARGUMENT:
        r_call_error.argument -= captures_amount;
        // ...
        break;
    case Callable::CallError::CALL_ERROR_TOO_MANY_ARGUMENTS:
    case Callable::CallError::CALL_ERROR_TOO_FEW_ARGUMENTS:
        r_call_error.expected -= captures_amount;
        // ...
        break;
    default:
        break;
}

I'm not sure if CallError is an internal API or if it's also exposed to GDExtension.

[akien-mga]

I'm not sure if CallError is an internal API or if it's also exposed to GDExtension.

GDExtension has a similar struct (and the comments are even more wrong as they were capitalized :)).

typedef enum {
	GDEXTENSION_CALL_OK,
	GDEXTENSION_CALL_ERROR_INVALID_METHOD,
	GDEXTENSION_CALL_ERROR_INVALID_ARGUMENT, // Expected a different variant type.
	GDEXTENSION_CALL_ERROR_TOO_MANY_ARGUMENTS, // Expected lower number of arguments.
	GDEXTENSION_CALL_ERROR_TOO_FEW_ARGUMENTS, // Expected higher number of arguments.
	GDEXTENSION_CALL_ERROR_INSTANCE_IS_NULL,
	GDEXTENSION_CALL_ERROR_METHOD_NOT_CONST, // Used for const call.
} GDExtensionCallErrorType;

typedef struct {
	GDExtensionCallErrorType error;
	int32_t argument;
	int32_t expected;
} GDExtensionCallError;

If we need to change the meaning of argument/expected, maybe this might need to be done via a new GDExtensionCallError2 struct, preserving the compatibility for GDExtensionCallError uses somewhat. CC @dsnopek

[dsnopek]

If we need to change the meaning of argument/expected, maybe this might need to be done via a new GDExtensionCallError2 struct, preserving the compatibility for GDExtensionCallError uses somewhat

Hrm. If we wanted to make a new GDExtensionCallError2, then we'd need to also make new versions of every function that deals with GDExtensionCallError, which includes some function pointers that are used on other structs, which would need new versions along with all the functions that accept those too. This quickly becomes quite a few changes.

Am I correct in understanding that this just affects the accuracy of some error messages? If so, the worst that could happen, is that a GDExtension compiled for Godot 4.1 but opened in Godot 4.2 could generate some incorrect error messages in some cases. This could maybe just be an acceptable weirdness with using older GDExtensions? It's not a crash, and it's not an error interpreted as a success or vice-versa.

We should also probably make changes similar to those in this PR to godot-cpp, so that it's using argument and expected correctly too.

[ParadoxV5]

If we need to change the meaning of argument/expected, maybe this might need to be done via a new GDExtensionCallError2 struct, preserving the compatibility for GDExtensionCallError uses somewhat.

Am I correct in understanding that this just affects the accuracy of some error messages? […] This could maybe just be an acceptable weirdness with using older GDExtensions? It's not a crash, and it's not an error interpreted as a success or vice-versa.

We should also probably make changes similar to those in this PR to godot-cpp, so that it's using argument and expected correctly too.

Here’s what this GDExtension user thinks. I say no needed, because

• This is a bug (within Godot Engine), not a feature (something intended according to documentation and/or intuition). See @dalexeev’s comment
  • My code actually has error.expected ? error.expected : error.argument to work this mistake around.
• You folks haven’t promised API stability yet. (e.g., 4.0.2 patch release breaks GDExtension API #75779)

[akien-mga]

The PR has been updated to fix the bug without breaking compat, so I think it's good to go.
Similar fixes might be needed in godot-cpp.

[akien-mga]

Thanks!