Skip to content

[Bgen] Ensure that GC.KeepAlive is called in extension methods. #22433

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.

Sign up for GitHub

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

Merged
mandel-macaque merged 5 commits into from
Mar 23, 2025
Merged

[Bgen] Ensure that GC.KeepAlive is called in extension methods. #22433

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
360f46b
[Bgen] Ensure that GC.KeepAlive is called in extension methods.
mandel-macaque Mar 20, 2025
c5afe1e
Apply suggestions from code review
mandel-macaque Mar 20, 2025
c1426d5
Address code reviews.
mandel-macaque Mar 20, 2025
ca0e9b0
Auto-format source code
Mar 20, 2025
cddc790
Apply suggestions from code review
mandel-macaque Mar 21, 2025
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
30 changes: 27 additions & 3 deletions src/bgen/Generator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2958,7 +2958,7 @@ void GenerateInvoke (bool stret, bool supercall, MethodInfo mi, MemberInformatio
print ($"IntPtr {handleName};");
print ($"{handleName} = global::{NamespaceCache.Messaging}.IntPtr_objc_msgSend (Class.GetHandle (typeof (T)), Selector.GetHandle (\"alloc\"));");
print ($"{handleName} = {sig} ({handleName}, {selector_field}{args});");
print ($"{(assign_to_temp ? "ret = " : "return ")} global::ObjCRuntime.Runtime.GetINativeObject<T> ({handleName}, true);");
print ($"ret = global::ObjCRuntime.Runtime.GetINativeObject<T> ({handleName}, true);");
Copy link
Member

@rolfbjarne rolfbjarne Mar 20, 2025

Choose a reason for hiding this comment

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

Looks like we don't need the assign_to_temp variable/parameter, so it can be removed.

} else {
bool returns = mi.ReturnType != TypeCache.System_Void && mi.Name != "Constructor";
string cast_a = "", cast_b = "";
Expand All @@ -2973,20 +2973,26 @@ void GenerateInvoke (bool stret, bool supercall, MethodInfo mi, MemberInformatio

if (minfo.is_static)
print ("{0}{1}{2} (class_ptr, {5}{6}){7};",
returns ? (assign_to_temp ? "ret = " : "return ") : "",
returns ? "ret = " : "",
cast_a, sig, target_name,
"/*unusued3*/", //supercall ? "Super" : "",
selector_field, args, cast_b);
else
print ("{0}{1}{2} ({3}{4}, {5}{6}){7};",
returns ? (assign_to_temp ? "ret = " : "return ") : "",
returns ? "ret = " : "",
cast_a, sig, target_name,
handle,
selector_field, args, cast_b);

if (postproc.Length > 0)
print (postproc.ToString ());
}

if (target_name == "This") {
Copy link
Member

@rolfbjarne rolfbjarne Mar 20, 2025

Choose a reason for hiding this comment

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

IMHO this should not be condition on "This", but instead of whether the method is a an actual extension method. So further above:

var useExtensionMethod = (category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method);
string target_name = useExtensionMethod ? "this" : "This";

and then here:

Suggested change
if (target_name == "This") {
if (useExtensionMethod) {

Copy link
Contributor Author

@mandel-macaque mandel-macaque Mar 20, 2025

Choose a reason for hiding this comment

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

Sure, but will be the opposite, useInstanceMethod:

var useExtensionMethod = (category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method);

Is ensuring that we are not a category etc.. We could move it to an or, but I don't like to change bool operations just because.

Copy link
Member

@rolfbjarne rolfbjarne Mar 20, 2025

Choose a reason for hiding this comment

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

ah, yes, flip the logic:

var useExtensionMethod = !(category_type is null && !minfo.is_extension_method && !minfo.is_protocol_implementation_method);
string target_name = useExtensionMethod ? "This" : "this";

boolean logic is hard!

// if this is a extension of any kind, ensure that we keep alive the this parameter
// so that it is not collected before the msg send call has completed.
print ("GC.KeepAlive (This);");
}
}

void GenerateNewStyleInvoke (bool supercall, MethodInfo mi, MemberInformation minfo, string selector, string args, bool assign_to_temp, Type category_type)
Expand Down Expand Up @@ -3554,6 +3560,18 @@ public void GenerateMethodBody (MemberInformation minfo, MethodInfo mi, string s
var nullableReturn = isClassType ? "?" : string.Empty;
print ("{0}{1} ret;", TypeManager.FormatType (minfo.type, mi.ReturnType), nullableReturn);
}
} else if (mi.ReturnType != TypeCache.System_Void && mi.Name != "Constructor") {
if (minfo.is_bindAs) {
var bindAsAttrib = GetBindAsAttribute (minfo.mi);
// tricky, e.g. when an nullable `NSNumber[]` is bound as a `float[]`, since FormatType and bindAsAttrib have not clue about the original nullability
print ("{0} ret;", TypeManager.FormatType (bindAsAttrib.Type.DeclaringType, bindAsAttrib.Type));
} else {
print ("{0} ret;", TypeManager.FormatType (minfo.type, mi.ReturnType));
}
} else if (minfo.is_ctor && minfo.is_protocol_member) {
print ("// {0} is protocol {1} ", mi.Name, minfo.is_protocol_member);
Copy link
Member

@rolfbjarne rolfbjarne Mar 20, 2025

Choose a reason for hiding this comment

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

Debug statement?

// special case be cause constructors in protocol memners will be converted to factory methods
print ($"T? ret;");
}

bool needs_temp = use_temp_return || disposes.Length > 0;
Expand Down Expand Up @@ -3666,6 +3684,12 @@ public void GenerateMethodBody (MemberInformation minfo, MethodInfo mi, string s
// we can't be 100% confident that the ObjC API annotations are correct so we always null check inside generated code
print ("return ret!;");
}
} else if (minfo.is_ctor && minfo.is_protocol_member) {
// special case since ctrs in protocol members become create methods
print ("return ret;");
} else if (mi.ReturnType != TypeCache.System_Void && mi.Name != "Constructor") {
// general case if we do return and we are not a construc ctor.
print ("return ret;");
}
if (minfo.is_ctor)
WriteMarkDirtyIfDerived (sw, mi.DeclaringType);
Expand Down
Loading