Skip to content

Commit

Permalink
Add a backwards-compatibility system for GDExtension method
Browse files Browse the repository at this point in the history
This adds a way to ensure that methods that were modified in the Godot API will continue working in older builds of GDExtension even if the new signature is different.

```C++
// New version (changed)
ClassDB::bind_method(D_METHOD("add_sphere","radius","position"),&MyShapes::add_sphere);
// Compatibility version (still available to extensions).
ClassDB::bind_compatibility_method(D_METHOD("add_sphere","radius"),&MyShapes::_compat_add_sphere);
```

**Q**: If I add an extra argument and provide a default value (hence can still be called the same), do I still have to provide the compatibility version?
**A**: Yes, you must still provide a compatibility method. Most language bindings use the raw method pointer to do the call and process the default parameters in the binding language, hence if the actual method signature changes it will no longer work.

**Q**: If I removed a method, can I still bind a compatibility version even though the main method no longer exists?
**A**: Yes, for methods that were removed or renamed, compatibility versions can still be provided.

**Q**: Would it be possible to automate checking that methods were removed by mistake?
**A**: Yes, as part of a future PR, the idea is to add a a command line option to Godot that can be run like : `$ godot --test-api-compatibility older_api_dump.json`, which will also be integrated to the CI runs.
  • Loading branch information
reduz committed Apr 25, 2023
1 parent 45cd5dc commit 6dbede1
Show file tree
Hide file tree
Showing 3 changed files with 150 additions and 31 deletions.
7 changes: 6 additions & 1 deletion core/extension/gdextension_interface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,12 @@ static GDExtensionScriptInstancePtr gdextension_script_instance_create(const GDE
static GDExtensionMethodBindPtr gdextension_classdb_get_method_bind(GDExtensionConstStringNamePtr p_classname, GDExtensionConstStringNamePtr p_methodname, GDExtensionInt p_hash) {
const StringName classname = *reinterpret_cast<const StringName *>(p_classname);
const StringName methodname = *reinterpret_cast<const StringName *>(p_methodname);
MethodBind *mb = ClassDB::get_method(classname, methodname);
bool exists;
MethodBind *mb = ClassDB::get_method_with_compatibility(classname, methodname, p_hash, &exists);
if (!mb && exists) {
ERR_PRINT("Method '" + classname + "." + methodname + "' has changed and no compatibility fallback has been provided. Please open an issue.");
return nullptr;
}
ERR_FAIL_COND_V(!mb, nullptr);
if (mb->get_hash() != p_hash) {
ERR_PRINT("Hash mismatch for method '" + classname + "." + methodname + "'.");
Expand Down
102 changes: 96 additions & 6 deletions core/object/class_db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,38 @@ MethodBind *ClassDB::get_method(const StringName &p_class, const StringName &p_n
return nullptr;
}

MethodBind *ClassDB::get_method_with_compatibility(const StringName &p_class, const StringName &p_name, uint64_t p_hash, bool *r_method_exists) {
OBJTYPE_RLOCK;

ClassInfo *type = classes.getptr(p_class);

while (type) {
MethodBind **method = type->method_map.getptr(p_name);
if (method && *method) {
if (r_method_exists) {
*r_method_exists = true;
}
if ((*method)->get_hash() == p_hash) {
return *method;
}
}

LocalVector<MethodBind *> *compat = type->method_map_compatibility.getptr(p_name);
if (compat) {
if (r_method_exists) {
*r_method_exists = true;
}
for (uint32_t i = 0; i < compat->size(); i++) {
if ((*compat)[i]->get_hash() == p_hash) {
return (*compat)[i];
}
}
}
type = type->inherits_ptr;
}
return nullptr;
}

void ClassDB::bind_integer_constant(const StringName &p_class, const StringName &p_enum, const StringName &p_name, int64_t p_constant, bool p_is_bitfield) {
OBJTYPE_WLOCK;

Expand Down Expand Up @@ -1268,11 +1300,30 @@ bool ClassDB::has_method(const StringName &p_class, const StringName &p_method,
}

void ClassDB::bind_method_custom(const StringName &p_class, MethodBind *p_method) {
_bind_method_custom(p_class, p_method, false);
}
void ClassDB::bind_compatibility_method_custom(const StringName &p_class, MethodBind *p_method) {
_bind_method_custom(p_class, p_method, true);
}

void ClassDB::_bind_compatibility(ClassInfo *type, MethodBind *p_method) {
if (!type->method_map_compatibility.has(p_method->get_name())) {
type->method_map_compatibility.insert(p_method->get_name(), LocalVector<MethodBind *>());
}
type->method_map_compatibility[p_method->get_name()].push_back(p_method);
}

void ClassDB::_bind_method_custom(const StringName &p_class, MethodBind *p_method, bool p_compatibility) {
ClassInfo *type = classes.getptr(p_class);
if (!type) {
ERR_FAIL_MSG("Couldn't bind custom method '" + p_method->get_name() + "' for instance '" + p_class + "'.");
}

if (p_compatibility) {
_bind_compatibility(type, p_method);
return;
}

if (type->method_map.has(p_method->get_name())) {
// overloading not supported
ERR_FAIL_MSG("Method already bound '" + p_class + "::" + p_method->get_name() + "'.");
Expand All @@ -1285,11 +1336,44 @@ void ClassDB::bind_method_custom(const StringName &p_class, MethodBind *p_method
type->method_map[p_method->get_name()] = p_method;
}

MethodBind *ClassDB::_bind_vararg_method(MethodBind *p_bind, const StringName &p_name, const Vector<Variant> &p_default_args, bool p_compatibility) {
MethodBind *bind = p_bind;
bind->set_name(p_name);
bind->set_default_arguments(p_default_args);

String instance_type = bind->get_instance_class();

ClassInfo *type = classes.getptr(instance_type);
if (!type) {
memdelete(bind);
ERR_FAIL_COND_V(!type, nullptr);
}

if (p_compatibility) {
_bind_compatibility(type, bind);
return bind;
}

if (type->method_map.has(p_name)) {
memdelete(bind);
// Overloading not supported
ERR_FAIL_V_MSG(nullptr, "Method already bound: " + instance_type + "::" + p_name + ".");
}
type->method_map[p_name] = bind;
#ifdef DEBUG_METHODS_ENABLED
MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const MethodDefinition &method_name, const Variant **p_defs, int p_defcount) {
// FIXME: <reduz> set_return_type is no longer in MethodBind, so I guess it should be moved to vararg method bind
//bind->set_return_type("Variant");
type->method_order.push_back(p_name);
#endif

return bind;
}

#ifdef DEBUG_METHODS_ENABLED
MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, bool p_compatibility, const MethodDefinition &method_name, const Variant **p_defs, int p_defcount) {
StringName mdname = method_name.name;
#else
MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const char *method_name, const Variant **p_defs, int p_defcount) {
MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, bool p_compatibility, const char *method_name, const Variant **p_defs, int p_defcount) {
StringName mdname = StaticCString::create(method_name);
#endif

Expand All @@ -1301,7 +1385,7 @@ MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const c

#ifdef DEBUG_ENABLED

ERR_FAIL_COND_V_MSG(has_method(instance_type, mdname), nullptr, "Class " + String(instance_type) + " already has a method " + String(mdname) + ".");
ERR_FAIL_COND_V_MSG(!p_compatibility && has_method(instance_type, mdname), nullptr, "Class " + String(instance_type) + " already has a method " + String(mdname) + ".");
#endif

ClassInfo *type = classes.getptr(instance_type);
Expand All @@ -1310,7 +1394,7 @@ MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const c
ERR_FAIL_V_MSG(nullptr, "Couldn't bind method '" + mdname + "' for instance '" + instance_type + "'.");
}

if (type->method_map.has(mdname)) {
if (!p_compatibility && type->method_map.has(mdname)) {
memdelete(p_bind);
// overloading not supported
ERR_FAIL_V_MSG(nullptr, "Method already bound '" + instance_type + "::" + mdname + "'.");
Expand All @@ -1325,10 +1409,16 @@ MethodBind *ClassDB::bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const c

p_bind->set_argument_names(method_name.args);

type->method_order.push_back(mdname);
if (!p_compatibility) {
type->method_order.push_back(mdname);
}
#endif

type->method_map[mdname] = p_bind;
if (p_compatibility) {
_bind_compatibility(type, p_bind);
} else {
type->method_map[mdname] = p_bind;
}

Vector<Variant> defvals;

Expand Down
72 changes: 48 additions & 24 deletions core/object/class_db.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ class ClassDB {
ObjectGDExtension *gdextension = nullptr;

HashMap<StringName, MethodBind *> method_map;
HashMap<StringName, LocalVector<MethodBind *>> method_map_compatibility;
HashMap<StringName, int64_t> constant_map;
struct EnumInfo {
List<StringName> constants;
Expand Down Expand Up @@ -148,9 +149,9 @@ class ClassDB {
static HashMap<StringName, StringName> compat_classes;

#ifdef DEBUG_METHODS_ENABLED
static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const MethodDefinition &method_name, const Variant **p_defs, int p_defcount);
static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, bool p_compatibility, const MethodDefinition &method_name, const Variant **p_defs, int p_defcount);
#else
static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, const char *method_name, const Variant **p_defs, int p_defcount);
static MethodBind *bind_methodfi(uint32_t p_flags, MethodBind *p_bind, bool p_compatibility, const char *method_name, const Variant **p_defs, int p_defcount);
#endif

static APIType current_api;
Expand All @@ -172,6 +173,9 @@ class ClassDB {
// Non-locking variants of get_parent_class and is_parent_class.
static StringName _get_parent_class(const StringName &p_class);
static bool _is_parent_class(const StringName &p_class, const StringName &p_inherits);
static void _bind_compatibility(ClassInfo *type, MethodBind *p_method);
static MethodBind *_bind_vararg_method(MethodBind *p_bind, const StringName &p_name, const Vector<Variant> &p_default_args, bool p_compatibility);
static void _bind_method_custom(const StringName &p_class, MethodBind *p_method, bool p_compatibility);

public:
// DO NOT USE THIS!!!!!! NEEDS TO BE PUBLIC BUT DO NOT USE NO MATTER WHAT!!!
Expand Down Expand Up @@ -273,7 +277,7 @@ class ClassDB {
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, false, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
}

template <class N, class M, typename... VarArgs>
Expand All @@ -288,46 +292,65 @@ class ClassDB {
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, false, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
}

template <class N, class M, typename... VarArgs>
static MethodBind *bind_compatibility_method(N p_method_name, M p_method, VarArgs... p_args) {
Variant args[sizeof...(p_args) + 1] = { p_args..., Variant() }; // +1 makes sure zero sized arrays are also supported.
const Variant *argptrs[sizeof...(p_args) + 1];
for (uint32_t i = 0; i < sizeof...(p_args); i++) {
argptrs[i] = &args[i];
}
MethodBind *bind = create_method_bind(p_method);
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, true, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
}

template <class N, class M, typename... VarArgs>
static MethodBind *bind_compatibility_static_method(const StringName &p_class, N p_method_name, M p_method, VarArgs... p_args) {
Variant args[sizeof...(p_args) + 1] = { p_args..., Variant() }; // +1 makes sure zero sized arrays are also supported.
const Variant *argptrs[sizeof...(p_args) + 1];
for (uint32_t i = 0; i < sizeof...(p_args); i++) {
argptrs[i] = &args[i];
}
MethodBind *bind = create_static_method_bind(p_method);
bind->set_instance_class(p_class);
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, true, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
}
template <class M>
static MethodBind *bind_vararg_method(uint32_t p_flags, const StringName &p_name, M p_method, const MethodInfo &p_info = MethodInfo(), const Vector<Variant> &p_default_args = Vector<Variant>(), bool p_return_nil_is_variant = true) {
GLOBAL_LOCK_FUNCTION;

MethodBind *bind = create_vararg_method_bind(p_method, p_info, p_return_nil_is_variant);
ERR_FAIL_COND_V(!bind, nullptr);

bind->set_name(p_name);
bind->set_default_arguments(p_default_args);
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return _bind_vararg_method(bind, p_name, p_default_args, false);
}

String instance_type = bind->get_instance_class();
template <class M>
static MethodBind *bind_compatibility_vararg_method(uint32_t p_flags, const StringName &p_name, M p_method, const MethodInfo &p_info = MethodInfo(), const Vector<Variant> &p_default_args = Vector<Variant>(), bool p_return_nil_is_variant = true) {
GLOBAL_LOCK_FUNCTION;

ClassInfo *type = classes.getptr(instance_type);
if (!type) {
memdelete(bind);
ERR_FAIL_COND_V(!type, nullptr);
}
MethodBind *bind = create_vararg_method_bind(p_method, p_info, p_return_nil_is_variant);
ERR_FAIL_COND_V(!bind, nullptr);

if (type->method_map.has(p_name)) {
memdelete(bind);
// Overloading not supported
ERR_FAIL_V_MSG(nullptr, "Method already bound: " + instance_type + "::" + p_name + ".");
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
type->method_map[p_name] = bind;
#ifdef DEBUG_METHODS_ENABLED
// FIXME: <reduz> set_return_type is no longer in MethodBind, so I guess it should be moved to vararg method bind
//bind->set_return_type("Variant");
type->method_order.push_back(p_name);
#endif

return bind;
return _bind_vararg_method(bind, p_name, p_default_args, true);
}

static void bind_method_custom(const StringName &p_class, MethodBind *p_method);
static void bind_compatibility_method_custom(const StringName &p_class, MethodBind *p_method);

static void add_signal(const StringName &p_class, const MethodInfo &p_signal);
static bool has_signal(const StringName &p_class, const StringName &p_signal, bool p_no_inheritance = false);
Expand Down Expand Up @@ -358,6 +381,7 @@ class ClassDB {
static void get_method_list(const StringName &p_class, List<MethodInfo> *p_methods, bool p_no_inheritance = false, bool p_exclude_from_properties = false);
static bool get_method_info(const StringName &p_class, const StringName &p_method, MethodInfo *r_info, bool p_no_inheritance = false, bool p_exclude_from_properties = false);
static MethodBind *get_method(const StringName &p_class, const StringName &p_name);
static MethodBind *get_method_with_compatibility(const StringName &p_class, const StringName &p_name, uint64_t p_hash, bool *r_method_exists = nullptr);

static void add_virtual_method(const StringName &p_class, const MethodInfo &p_method, bool p_virtual = true, const Vector<String> &p_arg_names = Vector<String>(), bool p_object_core = false);
static void get_virtual_methods(const StringName &p_class, List<MethodInfo> *p_methods, bool p_no_inheritance = false);
Expand Down

0 comments on commit 6dbede1

Please sign in to comment.