GDExtension: Add compatibility system for virtual methods

[dsnopek]

This is the start of a compatibility system that will allow us to change the signature of virtual methods, and not crash existing GDExtensions that implemented the old version of the virtual methods.

It allows registering compatibility virtual methods, like:

#ifndef DISABLE_DEPRECATED
	// The "_save_compat_100447" is an alias that allows us to refer to this older version of the virtual method.
	GDVIRTUAL3R_COMPAT(_save_compat_100447, Error, _save, Ref<Resource>, String, uint32_t)
#endif

	// This is the current version that has the new signature with the new argument.
	GDVIRTUAL4R(Error, _save, Ref<Resource>, String, uint32_t, FileAccess::SaveIntegrityLevel)

Then when we're calling the virtual method, we can do something like this:

	Error err = ERR_METHOD_NOT_FOUND;

	if (GDVIRTUAL_CALL(_save, p_resource, p_path, p_flags, p_integrity_level, err)) {
		// We return here if the GDExtension implements the current version of the virtual method.
		return err;
	}

#ifndef DISABLE_DEPRECATED
	// If not, we fall back on the old one. Notice the "_save_compat_100447" alias we're using here instead of "_save".
	GDVIRTUAL_CALL(_save_compat_100447, p_resource, p_path, p_flags, err);
#endif // DISABLE_DEPRECATED

	return err;
}

This also changes the extension_api.json file to include a hash for the signature of each virtual method, and updates the GDExtension interface to include a hash argument when Godot is asking the GDExtension for a particular virtual method.

The idea is: the bindings will hang on to the hashes for all the virtual methods they support, and when Godot asks for a particular virtual method, if the hash doesn't match what the GDExtension has, then return nullptr as if the GDExtension didn't implement the virtual method at all. So, if a GDExtension was compiled with an older hash, then the first GDVIRTUAL_CALL() will return false (indicating that the virtual method isn't implemented), but then the second one will succeed because the hash will match what the GDExtension has.

PR godotengine/godot-cpp#1676 are the companion changes for godot-cpp

Marking as DRAFT because there's still a couple things to do:

• Make a godot-cpp PR that implements this
• Do lots and lots more testing :-)
• Store the hashes for the compatibility methods, so we can include them in the JSON
• Compare the hashes for virtual methods when comparing JSON files, so that we can detected missing compatibility methods via CI

NOTE: This presently includes the changes from PR #100447 so that I'd have a change to test, since there is a virtual method signature that we want to change there.

UPDATE: I've moved the changes for PR #100447 to PR #101154, which gives a concrete use-case to test.

[Zylann]

GDVIRTUAL also includes calls to scripts, so is this going to benefit scripts as well?
Also, will extensions be able to define compatibility virtual methods too?

[dsnopek]

GDVIRTUAL also includes calls to scripts, so is this going to benefit scripts as well?

No, the goal here is entirely about GDExtension binary compatibility. Although, your comment makes me realize that the GDVIRTUAL*_COMPAT() macros shouldn't include the bits that call script functions, because if the script method isn't there, we're just checking for it over and over (first when trying to call the current version of the method, and then for each compat version).

Also, will extensions be able to define compatibility virtual methods too?

Eventually. Right now, one GDExtension can't extend a class from another GDExtension, so the problem doesn't exist there yet. But it's a good question: I'll try to think through if the API added here will work for that case too

[Bromeon]

So, I implemented this in godot-rust/gdext#991. Was much more work than I anticipated 😁

The idea is: the bindings will hang on to the hashes for all the virtual methods they support, and when Godot asks for a particular virtual method, if the hash doesn't match what the GDExtension has, then return nullptr as if the GDExtension didn't implement the virtual method at all. So, if a GDExtension was compiled with an older hash, then the first GDVIRTUAL_CALL() will return false (indicating that the virtual method isn't implemented), but then the second one will succeed because the hash will match what the GDExtension has.

So, the way I did this was:

• Memorize all the virtual function hashes from extension_api.json.
• In the get_virtual callback, I get virtual_method_name + hash parameters. Check whether the user overrides the method and the hash matches the memorized one. Otherwise return nullptr.

That's it, right? Binding maintainers always check against the single hash from the JSON?

Is there an easy way to test/simulate whether this works? I assume we can't unit-test this, so maybe compile against one Godot version and run in another? ResourceFormatSaver::_save is a bit difficult to test as well 🤔

Otherwise I wonder if we should merge this soon and get user feedback to iron out things during remaining 4.4 cycle...

[Bromeon]

As long as we are reasonably certain that the initial hashes won't change, this looks good to me!

[Bromeon]

Can you imagine that there's any way how the hash is going to change in the future?

Just because I remember the big issue we had with re-hashing methods in 4.0.x, and also a few other cases where it wasn't clear how const-ness and some other properties would affect hashes.

[dsnopek]

The hash is computed here the same way as with non-virtual methods.

Hopefully, we won't have to mass change how the hashes are done, but there's always the possibility of fixing bugs in the hashing process, which is why we had those issues previously. If that comes up for virtual methods, I imagine we'll handle it in the same way?

a few other cases where it wasn't clear how const-ness and some other properties would affect hashes.

Well, const-ness does affect the hashes. I think the discussion earlier was about whether it should or not? Since const-ness doesn't actually affect binary compatibility, it's a judgement call on our part of whether that should change the hash, but if remember right a number of people (including @vnen?) were in favor of const-ness being part of the hash.

Anyway, in this PR it's calculating the hash the same way as with non-virtual methods, which I personally think is fine.

[dsnopek]

So, the way I did this was:

• Memorize all the virtual function hashes from extension_api.json.
• In the get_virtual callback, I get virtual_method_name + hash parameters. Check whether the user overrides the method and the hash matches the memorized one. Otherwise return nullptr.

That's it, right? Binding maintainers always check against the single hash from the JSON?

Yep, that is correct! That's the same thing as the godot-cpp PR.

Is there an easy way to test/simulate whether this works? I assume we can't unit-test this, so maybe compile against one Godot version and run in another? ResourceFormatSaver::_save is a bit difficult to test as well 🤔

I still need to do a full proper test as well, but yes, that's basically how you'd do it. I've got a small personal project that has a ResourceFormatSaver in it, which is what I was planning to use. I'll try to do that soon!

Otherwise I wonder if we should merge this soon and get user feedback to iron out things during remaining 4.4 cycle...

I'd be in favor of that! Lemme see how quickly I can get this out of a draft state.

[dsnopek]

Alright, I've done a bunch of testing over the last several days and this seems to be working nicely for the 3 cases:

• An extension built for Godot 4.3 that doesn't know about the virtual method compatibility system
• An extension built for Godot 4.4 (so, knows about the compatibility system) but implements the older version of the virtual method
• An extension built for Godot 4.4 that implements the latest version of the virtual method

So, I've moved the bits that add a real compatibility virtual method for ResourceFormatSaver::_save() over to PR #101154

There's only one thing in this PR that I'm not crazy about, but I think it's acceptable and better than the alternative, and that's in this bit of code:

#define _GDVIRTUAL_GET_DEPRECATED(m_virtual, m_name_sn, m_compat)\\
	else if (m_compat || ClassDB::get_virtual_method_compatibility_hashes(get_class_static(), m_name_sn).size() == 0) {\\
		if (_get_extension()->get_virtual_call_data && _get_extension()->call_virtual_with_data) {\\
			m_virtual = _get_extension()->get_virtual_call_data(_get_extension()->class_userdata, &m_name_sn);\\
		} else if (_get_extension()->get_virtual) {\\
			m_virtual = (void *)_get_extension()->get_virtual(_get_extension()->class_userdata, &m_name_sn);\\
		}\\
	}

The call to ClassDB::get_virtual_method_compatibility_hashes() isn't ideal. It's needed to account for GDExtensions built for Godot 4.3 or earlier, which aren't aware of the compatibility system, and this whole chunk of code only happens in that case (with a GDExtension built for Godot 4.4 or later, this we'll never get to this point). It will also only happen once, when the virtual method is called for the first time - all subsequent times it's called won't do this. So, I personally think this is acceptable.

However, it isn't ideal, because it's doing a runtime check for something that we know ahead of time when doing the GDVIRTUAL_CALL(). The alternative is that we add a new GDVIRTUAL_CALL_NO_DEPRECATED() macro (or some better name?) and then developers need to know to use this other macro when calling the latest version of a virtual method in the case that that virtual method has some compatibility methods. Then we don't need the runtime check, BUT now developers need to know to use this special macro in this particular circumstance.

In my opinion, the non-ideal runtime check in this PR is the way to go, because it only happens in very specific circumstances, and the alternative (while theoretically better) is a messy thing for Godot contributors to think about, and if we ever remove deprecated code from before Godot 4.4, it'd be totally unnecessary, since this whole thing is just to account for GDExtensions built for Godot 4.3 and earlier.

Anyway, taking this out of DRAFT now!