[RFC] Provide a better API to convert NSObject to a protocol interface

[rolfbjarne]

Exhibit A

INSUrlProtocolClient client;
public CustomNSUrlConnectionDataDelegate (CustomNSUrlProtocol handler)
{
    client = (INSUrlProtocolClient)handler.WeakClient;
}

this pattern is broken, because handler.WeakClient is typed as an NSObject, and you may not get an instance of a NSUrlProtocol subclass back, in which case you end up with an InvalidCastException.

Unfortunately it's very easy to do this mistake in C#, and the correct code is not at all obvious:

INSUrlProtocolClient client;
public CustomNSUrlConnectionDataDelegate (CustomNSUrlProtocol handler)
{
    client = Runtime.GetINativeObject<INSUrlProtocolClient> (handler.WeakClient.Handle, false);
}

so I suggest something else:

class NSObject {
    public T AsProtocol<T> () where T: class, INativeObject
    {
        if (!typeof (T).IsInterface)
            throw new ArgumentException ($"{typeof (T)}.FullName is not an interface");
        // ...
        // verify that T is an interface representing a protocol

        var rv = this as T;
        if (rv != null)
            return rv;
        return Runtime.GetINativeObject<T> (Handle, false);
    }
}

INSUrlProtocolClient client;
public CustomNSUrlConnectionDataDelegate (CustomNSUrlProtocol handler)
{
    client = handler?.WeakClient.AsProtocol<INSUrlProtocolClient> ();
}

this should be much more discoverable.

[spouliot]

Q: in exhibit A why is the MAC code different ? It this because of OS/API difference ? or some other difference we could hide ?

AsProtocol<T> never verify if T is a protocol, what it does seems more line an UnsafeNativeCast<T>. I think the former is more practical but it should validate its input (or else it's gonna be abused into the later and the name will only add confusion to such code).

It's not obvious that the API, by itself, will lead developers to use it, like in exhibit A. Adding documentation to WeakClient can help but, if reading documentation is required, we can already provide the current correct (if not obvious) code. It does not diminish much the need for a better API, but it makes documenting the right solution a much quicker solution.

[rolfbjarne]

Q: in exhibit A why is the MAC code different ? It this because of OS/API difference ? or some other difference we could hide ?

That's a temporary workaround that became permanent (https://github.com/xamarin/maccore/commit/89e981147d47833e6e8eeaeda8a9635044f10e2c).

It's just sample code though, there have been other cases too (basically every time we've told people to call Runtime.GetINativeObject themselves).

AsProtocol never verify if T is a protocol, what it does seems more line an UnsafeNativeCast.

It will throw an exception if given a T that's not a protocol, although the exception might not be as good as it could be (it will complain that it can't find the wrapper class to instantiate).

And in any case T is constrained to INativeObject, which should cut down on the possible interfaces that are not protocols a lot.

It's not obvious that the API, by itself, will lead developers to use it, like in exhibit A.

As a developer, if I was told the correct code would be Runtime.GetINativeObject (...) my reaction would probably be something like "how was I supposed to know that?" [1]. If the correct code is .AsProtocol<T> (), I think I'd feel a bit better about it.

So not obvious, but still (slightly) more obvious.

Also, could this be something for Xamarin.Analysis? Detect casts from NSObject to IProtocol and warn about it. @VincentDondain?

[1] Yes, I realize the answer is RT*M, but not everybody does that :smile:

[dalexsoto]

Android has JavaCast, I am not entirely sure this is just a Protocols problem, take for example going from NSMutableArray<T> to NSArray<T> or when the native API returns InternalFoo instead of Foo or when you get ParentFoo out of a more generic API but the API takes ChildFoo instead.

Maybe we can introduce NativeCast<T>? I agree that Runtime.GetINativeObject<T> is not very easy to discover nor understand how to use it.

[1] Yes, I realize the answer is RT*M, but not everybody does that 😄

+1

[spouliot]

I'm not opposed to the proposal: adding an API or even its name :-) but I have two issues.

1. the API should do what its name suggest

It will throw an exception if given a T that's not a protocol,

Only if it reach Runtime.GetINativeObject. Something like x.AsProtocol<NSObject> (); or x.AsProtocol<IDisposable> (); would not throw. It will also cast to any protocol, conforming or not, which is not quite what I would expect.

Our low-level API are unsafe. We have no choice (e.g. there's no type safety in ObjC) and we do not promote them for general usage (the main reason for this RFC).

However our higher-level API should be safe to use and without surprises (even if the documentation is not looked up).

2. The root issue is not addressed since jumping from the original source (above) to AsProtocol is not the most natural step.

As a developer, if I was told the correct code

by whom ? developers have more chances reading the docs than being told by someone who knows the internal well enough to suggest any workaround (even simple looking like AsProtocol).

If/when we discover cases like this one (where our API force this) then we must update documentation.

Side notes:

Also, could this be something for Xamarin.Analysis?

No, the Xamarin.Analysis is only done on the project/msbuild options, not on the code but Roslyn analyzers (more like fxcop/gendarme) could do so.

Maybe we can introduce NativeCast?

@dalexsoto let's not :) but if we ever do (and we should know what it fix and make sure no better alternative exists) then let's call it UnsafeNativeCast<T> since the call will succeed at casting anything.

[rolfbjarne]

Something like x.AsProtocol (); or x.AsProtocol (); would not throw.

x.AsProtocol<IDisposable> () wouldn't compile, since IDisposable isn't an INativeObject (the generic constraint on T).

x.AsProtocol<NSObject> () would compile and run fine though, I hadn't considered non-interface types.

I can verify T easily enough, that's not a problem (except that it'll be slower in the best case scenario when the managed instance already implements the managed interface).

I've modified the proposal accordingly.

2. The root issue is not addressed since jumping from the original source (above) to AsProtocol is not the most natural step.

The most natural step is to use a C#-style cast (like the sample code is doing).

Unfortunately I don't see how we can make that work, so I'm trying to come up with a more natural step. I've always considered GetINativeObject (and GetNSObject for that matter) internal API app developers should never have to use.

by whom ? developers have more chances reading the docs than being told by someone who knows the internal well enough to suggest any workaround (even simple looking like AsProtocol).

StackOverflow, Forums, support, etc. Basically everywhere people (who don't look at the documentation) look for help when they're stuck.

If/when we discover cases like this one (where our API force this) then we must update documentation.

Agreed.