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

Conversation

raulsntos
Copy link
Member

  • Implements ClassDB::get_method_list_with_compatibility to retrieve all methods from a class including compat methods.
  • C# bindings generator now also generates compat methods.
  • All generated C# methods now use ClassDB::get_method_with_compatibility.

With this we no longer need to add compat methods to C# manually in Compat.cs. The ones already added don't have a compat method registered in ClassDB1 so we need to keep them unless we decide to register them.

Notes about conflicting method signatures2:

Sometimes there can be compat methods with a method signature that conflicts with another method. This can happen when the only thing that changes in the method signature is the return type or the const qualifier. In these cases, the generated C# syntax is invalid and won't compile so we ignore these methods (they aren't generated).

When there's a conflict, the non-compat methods take precedence, then the compat methods in order from most recently added to least recent.

  • If the conflict is due to the const qualifier it won't break binary compatibility (although it may change the behavior).
  • If the conflict is due to the return type then it breaks binary compatibility but I don't think there's anything we can do here.

Footnotes

  1. These compat methods aren't registered in ClassDB because they break compat in the 4.1 release. Since GDExtension in 4.1 was already going to be incompatible with 4.0, it was decided that there was no point in adding compat methods.

  2. Read more about this in https://github.com/godotengine/godot/pull/77183#issuecomment-1555960767.

@raulsntos raulsntos added this to the 4.2 milestone Aug 11, 2023
@raulsntos raulsntos requested review from a team as code owners August 11, 2023 19:28
@@ -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.

// 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.

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.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Core additions seem good to me.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Some excessive whitespace

core/object/class_db.cpp Outdated Show resolved Hide resolved
core/object/class_db.cpp Outdated Show resolved Hide resolved
core/object/class_db.cpp Outdated Show resolved Hide resolved
core/object/class_db.cpp Outdated Show resolved Hide resolved
- Implements `ClassDB::get_method_list_with_compatibility` to retrieve all methods from a class including compat methods.
- C# bindings generator now also generates compat methods.
- All generated C# methods now use `ClassDB::get_method_with_compatibility`.
@raulsntos raulsntos force-pushed the dotnet/generate-compat-methods-from-classdb branch from eda1e06 to 5f6082a Compare September 19, 2023 18:35
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

Generated code diff: https://gist.githubusercontent.com/RedworkDE/30cacee4e1ea34757a08e02ba55c2462/raw/745b5bc1cff07955e52d37124b9e5389fc887b31/80527.diff (when rebased onto master; search for EditorBrowsable to find the new compat methods)

Haven't looked at everything in too much of a depth, but it generally looks good to me.

@akien-mga akien-mga merged commit 017541b into godotengine:master Sep 26, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/generate-compat-methods-from-classdb branch September 26, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants