Xamarin-macios: [rfc] NativeObjectAttribute: The class attribute to generate INativeObjects classes

[dalexsoto]

Description

Currently we do not provide a mechanism in the generator to generate bindings for Core Foundation objects or objects like VTSession.

The idea is to teach the generator how to create the boilerplate code for this kind of objects and be used in conjunction of [ExternizerAttribute] to simplify the binding process for this kind of objects.

Introducing [NativeObject]:

[AttributeUsage (AttributeTargets.Interface, AllowMultiple = false)]
public class NativeObjectAttribute : Attribute {
    public Visibility OwnsCtorVisibility { get; set; } = Visibility.Internal;
    public NativeObjectAttribute () { }
    public NativeObjectAttribute (Visibility ownsCtorVisibility) => OwnsCtorVisibility = ownsCtorVisibility;
}

This would be used as follows:

[NativeObject]
interface NFoo {
}

This will instruct the generator to create a partial subclass of the following new abstract class named NativeObject that would live inside ObjCRuntime namespace:

public abstract class NativeObject : INativeObject, IDisposable {

    IntPtr handle;
    public IntPtr Handle {
        get => handle; 
        protected set => InitializeHandle (value); 
    }

    protected NativeObject (IntPtr handle, bool owns)
    {
        Handle = handle;
        if (!owns)
            DangerousRetain ();
    }

    ~NativeObject ()
    {
        Dispose (false);
    }

    public void Dispose ()
    {
        Dispose (true);
        GC.SuppressFinalize (this);
    }

    protected virtual void Dispose (bool disposing)
    {
        if (Handle != IntPtr.Zero) {
            DangerousRelease ();
            handle = IntPtr.Zero;
        }
    }

    protected virtual void DangerousRetain () => CFObject.CFRetain (Handle);
    protected virtual void DangerousRelease () => CFObject.CFRelease (Handle);

    protected virtual void InitializeHandle (IntPtr handle)
    {
        if (Handle == IntPtr.Zero && Class.ThrowOnInitFailure) {
            throw new Exception ($"Could not initialize an instance of the type '{GetType ().FullName}': handle is null.\n" +
                "It is possible to ignore this condition by setting ObjCRuntime.Class.ThrowOnInitFailure to false.");
        }
        this.handle = handle;
    }
}

The generated code would look like this:

public partial class NFoo : NativeObject {

    internal NFoo (IntPtr handle, bool owns) : base (handle, owns)
    {
    }
}

Most of the time these objects have factory methods, this is the reason why the .ctor (IntPtr,bool) is internal and can be implemented either manually or by using the [ExternizerAttribute].

[rolfbjarne]

I like the idea :smile:

A few points:

• I think it should be ObjCRuntime.NativeObject to match ObjCRuntime.INativeObject.
• We use constructor chaining usually:

    protected internal NSNativeObject (IntPtr handle)
        : this (handle, false)
    {
    }

• You don't need to generate the destructor, the base destructor will be called anyway.

[dalexsoto]

Thanks for the feedback @rolfbjarne, I have edited the proposal with your suggestions ❤️ 👍

[migueldeicaza]

I do not think that we want to use constructor chaining for these ones, what is the scenario you have in mind?

[migueldeicaza]

I love the idea.

I wonder if we can extend this to also generate some of the methods, for certain idioms. Most APIs for example take the first parameter as the handle, and we could handle a few operations there ourselves.

[spouliot]

About the idea: I ❤️ it :)

About NativeObject

• please check our internal trello board - adding a base native type was discussed before (many times) and there might be useful notes there;
• nothing should be internal - it needs to be usable from 3rd party bindings;
• NativeObject (IntPtr handle) is not needed, we should always use the one with bool (and mention the parameter name) since it's a frequent source of mistakes/bugs in memory management. It also remove the need to [Preserve] the 2nd .ctor;
• not everything use CFRetain and CFRelease (some use specialized versions) so those should be moved to virtual protected methods that we can override (even if manually);

e.g.

protected virtual IntPtr Retain () { return CFObject.Retain (Handle); }
protected virtual void Release () { CFObject.Release (Handle); }

Maybe they should be prefixed Dangerous* too... not sure - it's less an issue if we have them protected (and not public)

[rolfbjarne]

I vote for a Dangerous* prefix, since it matches the NSObject API.

[dalexsoto]

Ok guys I have updated the [rfc] with the provided feedback also my vote is on the Dangerous prefix :)

[spouliot]

@dalexsoto I don't see the updates

[dalexsoto]

@spouliot sorry, seems I forgot to save heh, now the code is updated 👍

[rolfbjarne]

A few more comments:

• NativeObject's constructor can be protected. Since it's an abstract class, it doesn't really matter, but it might if we ever make the class non-abstract. And in general I think it makes sense to use the most restricted visibility that makes sense.
• Should DangerousRetain's return type be IntPtr or void? There's no guarantee that overriding methods can return something useful (the P/Invokes they invoke might have a different signature). The return value from CFRetain is documented to be the input value, so there's no information lost if we don't return it to the caller. An alternative is to return this (NativeObject), like we do for NSObject.DangerousRetain:

public NativeObject DangerousRetain ()
{
    CFObject.CFRetain (Handle);
    return this;
}

I think this is the best solution, since it matches what NSObject does.

• What should happen if the constructor is called with handle == IntPtr.Zero? Currently it will crash, because calling CFRetain (NULL) or CFRelease (NULL) crashes. I think we should do like we do for NSObjects: throw an exception unless Class.ThrowOnInitFailure is false (but the downside here is that it might be a breaking change).

@spouliot what do you think?

[dalexsoto]

I am more inclined to use void as return for DangerousRetain since it needs to be virtual because as @spouliot pointed out not everything uses the CF[Retatin|Release] and returning this is not quite obvious, it can lead to our customers incorrectly doing return base.DangerousRetain () which is not really what they want and as @rolfbjarne mentioned it adds very little value to return something. As for the parity with what NSObject does it is not very useful either in this case since you won't really override Dangerous[Retain|Release] behaviors on NSObject derived classes or at least it's super unlikely OTOTH is a more common thing to do with CF special types.

As for the IntPtr.Zero +1 on throwing the exception if Class.ThrowOnInitFailure is false, a managed exception is thousand of times better describing where the error is than a crashlog (well most of the time :P).

[spouliot]

Yes, the .ctor should be protected since it's in an abstract type. It won't be a breaking change to remove abstract (on the type) or make it (the .ctor) public in the future. I can't see why but it won't break 😄

IntPtr XRetain (IntPtr) is the common pattern - but I can't remember a rule (from Apple) without it's exception(s) so we better not make this a requirement since I've never seen it used. Since it's protected it's less likely to be mis-used - but, without some need, I'd say return void to keep this simpler (and the caller to a protected method does not need this to be returned).

Throwing on IntPtr.Zero is a good idea (some Retain/Release overrides are just to avoid crashes from CFRetain/CFRelease). However it might limit us from transitioning some existing types to use this new code (and I would not be surprise if there's a case where it's actually allowed). What about adding:

protected IntPtr ValidateHandle (IntPtr handle)
{
    if (handle == IntPtr.Zero)
        throw new ArgumentException ("Uninitialized handle"); 
    return handle;
}

and either in .ctor use

Handle = ValidateHandle (handle);

or call ValidateHandle in the Handle setter.

If we hit a type where IntPtr.Zero is valid - or where additional checks should be made then it can be overridden.

[rolfbjarne]

I like the ValidateHandle idea, but I'd make it closer to how NSObject implements it (just because I think that code that does similar things should look similar too):

protected virtual void InitializeHandle (IntPtr handle)
{
    if (handle == IntPtr.Zero && Class.ThrowOnInitFailure) {
        throw new Exception (string.Format ("Could not initialize an instance of the type '{0}': handle is null.\n" +
            "It is possible to ignore this condition by setting ObjCRuntime.Class.ThrowOnInitFailure to false.",
            GetType ().FullName));
    }
    this.Handle = handle;
}

[dalexsoto]

I have updated the [rfc] with the provided feedback 👍 Thanks!

[migueldeicaza]

While not everything uses CFObject.Retain, CFObject.Release, I do not think that the way to solve this should be to provide virtual methods that immediately need to be overwritten.

If we need to configure those, they should be parameters to the attribute:

[Native (Retain="MyRetainer", Release="MyReleaser")]

By extension the currently named DangerouRelease and DangerousRetain should not be virtual (and I might be ok not even surfacing those at all).

[migueldeicaza]

We were having a discussion about a more fundamental change, it seems like we could introduce a CFObject base class (we use it today with two static methods) that would serve as a base class for all of the CF-family of objects and implements both interfaces out of the box and is already hardwired to CFRetain/CFRelease, this would cover likely 95% of all cases.

It is also potentially possible to change our existing implementations to call into this implementation

[migueldeicaza]

I revisit my opinion on one of my comments above, I had not noticed that this was generator support for a NativeObject class that would be baked in.

If this would be baked in as a base class:

• there would be minimal use for the generator
• there is value on the virtual methods for Retain/Release.

Since I am landing a large set of native bindings like this for Network and soon security, I will add this NativeObject as presented in the PR.

We should then discuss whether there is much value in the generator support for providing the two overrides, I suspect not.

[dalexsoto]

I do not have any objections 👍

[migueldeicaza]

One additional bit, I think this should go into CoreFoundation instead. While generally it should work with any toll-free bridged objects, this would convey the wrong idea as to what this is.

[migueldeicaza]

Given that these are protected methods, I do not think that the "Dangerous" prefix is required.

[migueldeicaza]

These are now part of my Network PR

[migueldeicaza]

Heads up: as I am working on this, I figured that NativeObject might benefit once and for all for tracking the queue on which the object was created and to perform the dispose on that queue.

While for NSObjects we do this on the main thread, I suspect that NativeObjects really should track this on their own, at least as an option.

[rolfbjarne]

Heads up: as I am working on this, I figured that NativeObject might benefit once and for all for tracking the queue on which the object was created and to perform the dispose on that queue.

While for NSObjects we do this on the main thread, I suspect that NativeObjects really should track this on their own, at least as an option.

This is discussed somewhat here:
https://github.com/xamarin/xamarin-macios/issues/4130#issuecomment-398702385

TL;DR: there are many pitfalls when capturing the current queue and trying to schedule something on it at a later point, so this is not trivial.