MethodBind::get_hash() fails to hash default arguments

[pkdawson]

Godot version

v4.2.dev.custom_build [8449592]

System information

Godot v4.2.dev (8449592) - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 4070 Ti (NVIDIA; 31.0.15.3713) - 13th Gen Intel(R) Core(TM) i5-13600K (20 Threads)

Issue description

The GDExtension method hash is supposed to include the values of its default arguments. This works for methods where every argument has a default, but fails when there are some arguments without a default.

This is because MethodBind::get_hash() starts counting at 0:

godot/core/object/method_bind.cpp

Lines 49 to 53 in 8449592

	hash = hash_murmur3_one_32(get_default_argument_count(), hash);
	for (int i = 0; i < get_default_argument_count(); i++) {
	Variant v = get_default_argument(i);
	hash = hash_murmur3_one_32(v.hash(), hash);
	}

But get_default_argument expects the absolute index of the argument in the function signature:

godot/core/object/method_bind.h

Lines 82 to 90 in 8449592

	_FORCE_INLINE_ Variant get_default_argument(int p_arg) const {
	int idx = p_arg - (argument_count - default_arguments.size());
	if (idx < 0 || idx >= default_arguments.size()) {
	return Variant();
	} else {
	return default_arguments[idx];
	}
	}

So instead of hashing the actual default value, we end up hashing a nil Variant.

Fixing this would be a major compatibility break because the method hashes will change, so it probably requires some discussion.

Steps to reproduce

In the godot-cpp test project, add call to one of the affected methods. For example: add_child(memnew(Node));

Set a debugger breakpoint inside MethodBind::get_default_argument with a condition of idx < 0.

Minimal reproduction project

N/A

[akien-mga]

CC @godotengine/gdextension

[pkdawson]

Looking at other callers of get_default_argument, this would also need to be fixed:

godot/core/object/class_db.cpp

Lines 224 to 228 in 8449592

	for (int i = 0; i < mb->get_default_argument_count(); i++) {
	//hash should not change, i hope for tis
	Variant da = mb->get_default_argument(i);
	hash = hash_murmur3_one_64(da.hash(), hash);
	}

And for reference, this looks correct:

godot/core/object/class_db.cpp

Lines 441 to 445 in 8449592

	for (int i = 0; i < p_method->get_argument_count(); i++) {
	if (p_method->has_default_argument(i)) {
	minfo.default_arguments.push_back(p_method->get_default_argument(i));
	}
	}

[dsnopek]

Eep! Thanks for catching this

Have you tried fixing the bug and seeing how many hashes change? If it's a manageable number, we could possibly add a version of ClassDB::bind_compatibility_method() that takes an explicit hash value and re-register each affected method with the old hash as well.

We had a similar issue in Godot 4.0.2 when a hashing bug was fixed - see #75779

For that issue, we also discussed registering compatibility methods with an explicit hash, but that time we decided it wasn't worth it. But since this has now come up twice, maybe this is a tool we need?

Personally, I'd really love not to break compatibility in Godot 4.2, if possible.

[pkdawson]

An explicit hash sounds like a reasonable approach.

Fixing the bug and diffing the extension_api.json shows 817 changed hashes (out of 13042). Wouldn't want to write all that manually, but should be doable with a script.

EDIT: Corrected the number of hashes

[dsnopek]

Oof, 817 is quite a few, but relatively speaking only ~6%.

Wouldn't want to write all that manually, but should be doable with a script.

Yeah, I think with a script we could generate the *.compat.inc files, and then manually add the static void _bind_compatibility_methods() to the headers. It'll only need more manual intervention for situations where a *.compat.inc file already exists, but we've only got 5 of those currently.

It'll just be a question of whether or not the other contributors/maintainers are OK with adding all of that.

[akien-mga]

Sounds like we'd be generating compat.inc files for a very significant portion of the codebase, which kind of defeats the original purpose of keep rare compat breakages to a dedicated file :/

[dsnopek]

Sounds like we'd be generating compat.inc files for a very significant portion of the codebase, which kind of defeats the original purpose of keep rare compat breakages to a dedicated file :/

Well, if this turns out to be unacceptable, we could try to consolidate all these particular fixes into a single file? Or, we could add something in ClassDB that automatically adds a compatibility method when registering a normal method that would be affected by this hash difference (and keep around the code for incorrect hash generation in order to automatically get the old hash)?

Personally, I'm open to any option that'll be acceptable to everyone else and means we don't have to break compatibility again for Godot 4.2 :-)

[akien-mga]

Or, we could add something in ClassDB that automatically adds a compatibility method when registering a normal method that would be affected by this hash difference (and keep around the code for incorrect hash generation in order to automatically get the old hash)?

That sounds like an interesting approach.

[dsnopek]

The primary downside of that approach is that it would add the compatibility registration for any method that could generate an incorrect hash (meaning, any method with default arguments where not all arguments have a default) regardless of whether it had a bad hash in Godot 4.1 or not.

So, like, even in Godot 4.9, if we add a method that has two arguments and only one default, this approach would lead to registering the compatibility method with a Godot 4.1 hash, even though we don't really need it, since the method didn't even exist in Godot 4.1. Whereas any approach where we specifically register compatibility hashes for the affected methods would target this only where needed.

That said, this may be an acceptable downside if bloating the code with all the specific instances is a blocker :-)

[pkdawson]

The other option I thought of would be to modify ClassDB::get_method_with_compatibility so that if it fails to find a match, it tries again with the old hash function.

This avoids adding hundreds of compatibility methods, but --validate-extension-api would complain about the missing compatibility. I'm not sure if anything else would break.

[dsnopek]

Yeah, that's also a possibility. Although, I think --validate-extension-api working is probably important, otherwise we'll need to keep the exceptions for these 817 methods in each of the *.expected files forever.

[akien-mga]

Can we make a single hashmap with the precomputed old and new hashes, kept in a single file, to use in get_method_with_compatibility? And then the validate check would also use that file to find false positives to ignore.

[dsnopek]

Yeah, I think there's a couple of ways we could keep all the ones from this issue in a single file. I may not have a chance to for a couple days, but there's a few ideas I want to experiment with.

[dsnopek]

Discussed at the GDExtension meeting, and we agree that some approach that isolates the changes to a single place and specifically addresses this problem is the way to go.