Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Generate and use compat methods #80527

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 59 additions & 3 deletions core/object/class_db.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ void ClassDB::get_method_list(const StringName &p_class, List<MethodInfo> *p_met
}

#ifdef DEBUG_METHODS_ENABLED

for (const MethodInfo &E : type->virtual_methods) {
p_methods->push_back(E);
}
Expand All @@ -478,17 +477,74 @@ void ClassDB::get_method_list(const StringName &p_class, List<MethodInfo> *p_met

p_methods->push_back(minfo);
}

#else

for (KeyValue<StringName, MethodBind *> &E : type->method_map) {
MethodBind *m = E.value;
MethodInfo minfo = info_from_bind(m);
p_methods->push_back(minfo);
}
#endif

if (p_no_inheritance) {
break;
}

type = type->inherits_ptr;
}
}

void ClassDB::get_method_list_with_compatibility(const StringName &p_class, List<Pair<MethodInfo, uint32_t>> *p_methods, bool p_no_inheritance, bool p_exclude_from_properties) {
OBJTYPE_RLOCK;

ClassInfo *type = classes.getptr(p_class);

while (type) {
if (type->disabled) {
if (p_no_inheritance) {
break;
}

type = type->inherits_ptr;
continue;
}

#ifdef DEBUG_METHODS_ENABLED
for (const MethodInfo &E : type->virtual_methods) {
Pair<MethodInfo, uint32_t> pair(E, 0);
p_methods->push_back(pair);
}

for (const StringName &E : type->method_order) {
if (p_exclude_from_properties && type->methods_in_properties.has(E)) {
continue;
}

MethodBind *method = type->method_map.get(E);
MethodInfo minfo = info_from_bind(method);

Pair<MethodInfo, uint32_t> pair(minfo, method->get_hash());
p_methods->push_back(pair);
}
#else
for (KeyValue<StringName, MethodBind *> &E : type->method_map) {
MethodBind *method = E.value;
MethodInfo minfo = info_from_bind(method);

Pair<MethodInfo, uint32_t> pair(minfo, method->get_hash());
p_methods->push_back(pair);
}
#endif

for (const KeyValue<StringName, LocalVector<MethodBind *, unsigned int, false, false>> &E : type->method_map_compatibility) {
LocalVector<MethodBind *> compat = E.value;
for (MethodBind *method : compat) {
MethodInfo minfo = info_from_bind(method);

Pair<MethodInfo, uint32_t> pair(minfo, method->get_hash());
p_methods->push_back(pair);
}
}

if (p_no_inheritance) {
break;
}
Expand Down
1 change: 1 addition & 0 deletions core/object/class_db.h
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ class ClassDB {
static void set_method_flags(const StringName &p_class, const StringName &p_method, int p_flags);

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 void get_method_list_with_compatibility(const StringName &p_class, List<Pair<MethodInfo, uint32_t>> *p_methods_with_hash, 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, bool *r_is_deprecated = nullptr);
Expand Down
110 changes: 102 additions & 8 deletions modules/mono/editor/bindings_generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ StringBuilder &operator<<(StringBuilder &r_sb, const char *p_cstring) {

#define ICALL_PREFIX "godot_icall_"
#define ICALL_CLASSDB_GET_METHOD "ClassDB_get_method"
#define ICALL_CLASSDB_GET_METHOD_WITH_COMPATIBILITY "ClassDB_get_method_with_compatibility"
#define ICALL_CLASSDB_GET_CONSTRUCTOR "ClassDB_get_constructor"

#define C_LOCAL_RET "ret"
Expand Down Expand Up @@ -1354,6 +1355,7 @@ Error BindingsGenerator::_generate_cs_type(const TypeInterface &itype, const Str
output.append("namespace " BINDINGS_NAMESPACE ";\n\n");

output.append("using System;\n"); // IntPtr
output.append("using System.ComponentModel;\n"); // EditorBrowsable
output.append("using System.Diagnostics;\n"); // DebuggerBrowsable
output.append("using Godot.NativeInterop;\n");

Expand Down Expand Up @@ -1827,7 +1829,13 @@ Error BindingsGenerator::_generate_cs_type(const TypeInterface &itype, const Str
}
output << "\n"
<< INDENT1 "{\n";
HashMap<String, StringName> method_names;
for (const MethodInterface &imethod : itype.methods) {
if (method_names.has(imethod.proxy_name)) {
ERR_FAIL_COND_V_MSG(method_names[imethod.proxy_name] != imethod.cname, ERR_BUG, "Method name '" + imethod.proxy_name + "' already exists with a different value.");
continue;
}
method_names[imethod.proxy_name] = imethod.cname;
output << INDENT2 "/// <summary>\n"
<< INDENT2 "/// Cached name for the '" << imethod.cname << "' method.\n"
<< INDENT2 "/// </summary>\n"
Expand Down Expand Up @@ -2080,7 +2088,7 @@ Error BindingsGenerator::_generate_cs_method(const BindingsGenerator::TypeInterf

arguments_sig += iarg.name;

if (iarg.default_argument.size()) {
if (!p_imethod.is_compat && iarg.default_argument.size()) {
if (iarg.def_param_mode != ArgumentInterface::CONSTANT) {
arguments_sig += " = null";
} else {
Expand Down Expand Up @@ -2168,8 +2176,8 @@ Error BindingsGenerator::_generate_cs_method(const BindingsGenerator::TypeInterf
p_output << "GodotObject.";
}

p_output << ICALL_CLASSDB_GET_METHOD "(" BINDINGS_NATIVE_NAME_FIELD ", MethodName."
<< p_imethod.proxy_name
p_output << ICALL_CLASSDB_GET_METHOD_WITH_COMPATIBILITY "(" BINDINGS_NATIVE_NAME_FIELD ", MethodName."
<< p_imethod.proxy_name << ", " << itos(p_imethod.hash) << "ul"
<< ");\n";
}

Expand Down Expand Up @@ -2208,6 +2216,10 @@ Error BindingsGenerator::_generate_cs_method(const BindingsGenerator::TypeInterf
p_output.append("\")]");
}

if (p_imethod.is_compat) {
p_output.append(MEMBER_BEGIN "[EditorBrowsable(EditorBrowsableState.Never)]");
}

p_output.append(MEMBER_BEGIN);
p_output.append(p_imethod.is_internal ? "internal " : "public ");

Expand Down Expand Up @@ -2924,6 +2936,12 @@ bool method_has_ptr_parameter(MethodInfo p_method_info) {
return false;
}

struct SortMethodWithHashes {
_FORCE_INLINE_ bool operator()(const Pair<MethodInfo, uint32_t> &p_a, const Pair<MethodInfo, uint32_t> &p_b) const {
return p_a.first < p_b.first;
}
};

bool BindingsGenerator::_populate_object_type_interfaces() {
obj_types.clear();

Expand Down Expand Up @@ -3051,11 +3069,15 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
List<MethodInfo> virtual_method_list;
ClassDB::get_virtual_methods(type_cname, &virtual_method_list, true);

List<MethodInfo> method_list;
ClassDB::get_method_list(type_cname, &method_list, true);
method_list.sort();
List<Pair<MethodInfo, uint32_t>> method_list_with_hashes;
ClassDB::get_method_list_with_compatibility(type_cname, &method_list_with_hashes, true);
method_list_with_hashes.sort_custom_inplace<SortMethodWithHashes>();

List<MethodInterface> compat_methods;
for (const Pair<MethodInfo, uint32_t> &E : method_list_with_hashes) {
const MethodInfo &method_info = E.first;
const uint32_t hash = E.second;

for (const MethodInfo &method_info : method_list) {
int argc = method_info.arguments.size();

if (method_info.name.is_empty()) {
Expand All @@ -3077,6 +3099,7 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
MethodInterface imethod;
imethod.name = method_info.name;
imethod.cname = cname;
imethod.hash = hash;

if (method_info.flags & METHOD_FLAG_STATIC) {
imethod.is_static = true;
Expand All @@ -3089,7 +3112,17 @@ bool BindingsGenerator::_populate_object_type_interfaces() {

PropertyInfo return_info = method_info.return_val;

MethodBind *m = imethod.is_virtual ? nullptr : ClassDB::get_method(type_cname, method_info.name);
MethodBind *m = nullptr;

if (!imethod.is_virtual) {
bool method_exists = false;
m = ClassDB::get_method_with_compatibility(type_cname, method_info.name, hash, &method_exists, &imethod.is_compat);

if (unlikely(!method_exists)) {
ERR_FAIL_COND_V_MSG(!virtual_method_list.find(method_info), false,
"Missing MethodBind for non-virtual method: '" + itype.name + "." + imethod.name + "'.");
}
}

imethod.is_vararg = m && m->is_vararg();

Expand Down Expand Up @@ -3210,6 +3243,14 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
ERR_FAIL_COND_V_MSG(itype.find_property_by_name(imethod.cname), false,
"Method name conflicts with property: '" + itype.name + "." + imethod.name + "'.");

// Compat methods aren't added to the type yet, they need to be checked for conflicts
// after all the non-compat methods have been added. The compat methods are added in
// reverse so the most recently added ones take precedence over older compat methods.
if (imethod.is_compat) {
compat_methods.push_front(imethod);
continue;
}

// Methods starting with an underscore are ignored unless they're used as a property setter or getter
if (!imethod.is_virtual && imethod.name[0] == '_') {
for (const PropertyInterface &iprop : itype.properties) {
Expand All @@ -3224,6 +3265,15 @@ bool BindingsGenerator::_populate_object_type_interfaces() {
}
}

// Add compat methods that don't conflict with other methods in the type.
for (const MethodInterface &imethod : compat_methods) {
if (_method_has_conflicting_signature(imethod, itype)) {
WARN_PRINT("Method '" + imethod.name + "' conflicts with an already existing method in type '" + itype.name + "' and has been ignored.");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is noisy and there's nothing we can do about the warning anyway, so maybe it's better to remove it, or at least only print it with --verbose. I just wanted to make sure we were aware of ignored methods, but maybe it's not that important.

continue;
}
itype.methods.push_back(imethod);
}

// Populate signals

const HashMap<StringName, MethodInfo> &signal_map = class_info->signal_map;
Expand Down Expand Up @@ -4007,6 +4057,50 @@ void BindingsGenerator::_populate_global_constants() {
}
}

bool BindingsGenerator::_method_has_conflicting_signature(const MethodInterface &p_imethod, const TypeInterface &p_itype) {
// Compare p_imethod with all the methods already registered in p_itype.
for (const MethodInterface &method : p_itype.methods) {
if (method.proxy_name == p_imethod.proxy_name) {
if (_method_has_conflicting_signature(p_imethod, method)) {
return true;
}
}
}

return false;
}

bool BindingsGenerator::_method_has_conflicting_signature(const MethodInterface &p_imethod_left, const MethodInterface &p_imethod_right) {
// Check if a method already exists in p_itype with a method signature that would conflict with p_imethod.
// The return type is ignored because only changing the return type is not enough to avoid conflicts.
// The const keyword is also ignored since it doesn't generate different C# code.

if (p_imethod_left.arguments.size() != p_imethod_right.arguments.size()) {
// Different argument count, so no conflict.
return false;
}

for (int i = 0; i < p_imethod_left.arguments.size(); i++) {
const ArgumentInterface &iarg_left = p_imethod_left.arguments[i];
const ArgumentInterface &iarg_right = p_imethod_right.arguments[i];

if (iarg_left.type.cname != iarg_right.type.cname) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparing the cname may not be enough. There may be some types that are different in C++ but end up using the same type in C# (I think Vector<T> and TypedArray<T> both generate Godot.Collections.Generic<T>).

Unfortunately, we don't have access to the proxy_name here, and this method is called while the types are still being populated, so we may not be able to get it at this point.

// Different types for arguments in the same position, so no conflict.
return false;
}

if (iarg_left.def_param_mode != iarg_right.def_param_mode) {
// If the argument is a value type and nullable, it will be 'Nullable<T>' instead of 'T'
// and will not create a conflict.
if (iarg_left.def_param_mode == ArgumentInterface::NULLABLE_VAL || iarg_right.def_param_mode == ArgumentInterface::NULLABLE_VAL) {
return false;
}
}
}

return true;
}

void BindingsGenerator::_initialize_blacklisted_methods() {
blacklisted_methods["Object"].push_back("to_string"); // there is already ToString
blacklisted_methods["Object"].push_back("_to_string"); // override ToString instead
Expand Down
14 changes: 14 additions & 0 deletions modules/mono/editor/bindings_generator.h
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ class BindingsGenerator {
*/
String proxy_name;

/**
* Hash of the ClassDB method
*/
uint64_t hash = 0;

/**
* [TypeInterface::name] of the return type
*/
Expand Down Expand Up @@ -165,6 +170,12 @@ class BindingsGenerator {
*/
bool is_internal = false;

/**
* Determines if the method is a compatibility method added to avoid breaking binary compatibility.
* These methods will be generated but hidden and are considered deprecated.
*/
bool is_compat = false;

List<ArgumentInterface> arguments;

const DocData::MethodDoc *method_doc = nullptr;
Expand Down Expand Up @@ -783,6 +794,9 @@ class BindingsGenerator {

void _populate_global_constants();

bool _method_has_conflicting_signature(const MethodInterface &p_imethod, const TypeInterface &p_itype);
bool _method_has_conflicting_signature(const MethodInterface &p_imethod_left, const MethodInterface &p_imethod_right);

Error _generate_cs_type(const TypeInterface &itype, const String &p_output_file);

Error _generate_cs_property(const TypeInterface &p_itype, const PropertyInterface &p_iprop, StringBuilder &p_output);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,18 @@ internal static IntPtr ClassDB_get_method(StringName type, StringName method)
return methodBind;
}

internal static IntPtr ClassDB_get_method_with_compatibility(StringName type, StringName method, ulong hash)
{
var typeSelf = (godot_string_name)type.NativeValue;
var methodSelf = (godot_string_name)method.NativeValue;
IntPtr methodBind = NativeFuncs.godotsharp_method_bind_get_method_with_compatibility(typeSelf, methodSelf, hash);

if (methodBind == IntPtr.Zero)
throw new NativeMethodBindNotFoundException(type + "." + method);

return methodBind;
}

internal static unsafe delegate* unmanaged<IntPtr> ClassDB_get_constructor(StringName type)
{
// for some reason the '??' operator doesn't support 'delegate*'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ private partial struct UnmanagedCallbacks
public static partial IntPtr godotsharp_method_bind_get_method(in godot_string_name p_classname,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think godotsharp_method_bind_get_method is used anymore and could probably be removed.

in godot_string_name p_methodname);

public static partial IntPtr godotsharp_method_bind_get_method_with_compatibility(
in godot_string_name p_classname, in godot_string_name p_methodname, ulong p_hash);

public static partial delegate* unmanaged<IntPtr> godotsharp_get_class_constructor(
in godot_string_name p_classname);

Expand Down
5 changes: 5 additions & 0 deletions modules/mono/glue/runtime_interop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ MethodBind *godotsharp_method_bind_get_method(const StringName *p_classname, con
return ClassDB::get_method(*p_classname, *p_methodname);
}

MethodBind *godotsharp_method_bind_get_method_with_compatibility(const StringName *p_classname, const StringName *p_methodname, uint64_t p_hash) {
return ClassDB::get_method_with_compatibility(*p_classname, *p_methodname, p_hash);
}

godotsharp_class_creation_func godotsharp_get_class_constructor(const StringName *p_classname) {
ClassDB::ClassInfo *class_info = ClassDB::classes.getptr(*p_classname);
if (class_info) {
Expand Down Expand Up @@ -1416,6 +1420,7 @@ void godotsharp_object_to_string(Object *p_ptr, godot_string *r_str) {
static const void *unmanaged_callbacks[]{
(void *)godotsharp_dotnet_module_is_initialized,
(void *)godotsharp_method_bind_get_method,
(void *)godotsharp_method_bind_get_method_with_compatibility,
(void *)godotsharp_get_class_constructor,
(void *)godotsharp_engine_get_singleton,
(void *)godotsharp_stack_info_vector_resize,
Expand Down
Loading