[.NET 6 linker] Keep wrapper types referenced from ProtocolAttribute

[sbomer]

@rolfbjarne in dotnet/linker#1570 (comment):

What other meaning does the attribute have if it's not supposed to survive linking?
Or rather - what other tool uses the attribute (before linker)?

The attribute is used to mark that a managed interface represents an Objective-C protocol.

Example:

[Protocol (Name = "MyProtocol", WrapperType = typeof (MyProtocolWrapper)]
interface IMyProtocol {
}
class MyProtocolWrapper : IMyProtocol {
    MyProtocolWrapper (IntPtr handle, bool owns) { ... }
}

When we build a Xamarin.iOS app, we process all the assemblies and create a table of all such interfaces, and their corresponding wrapper types. The resulting table is stored in native code (it's a simple lookup table: metadata token for interface => metadata token for wrapper class). At runtime, when we need to figure out which managed type to instantiate given a native object that implements a particular Objective-C protocol, we check the table and create an instance of the corresponding wrapper class.

This means that:

We don't need the Protocol attribute at runtime (we already have all the information it represents in other ways), so we'd like to link it away.
We need the MyProtocolWrapper class.

This could be solved with a custom IMarkHandler that is triggered for marked types. Whenever an interface is marked, it should look for ProtocolAttribute and keep whatever is needed on the referenced wrapper type.

See more discussion in dotnet/linker#1570.

[spouliot]

yes, I started look at this after your PR (it mentioned it when approving the PR) but I'm not sure I'll complete it before vacations.

The existing code is part of CoreMarkStep.cs and needs to be ported. It's a bit more complex that just marking all protocols and their members.

[spouliot]

I'm a bit surprised all our all tests are working without this, even when I re-enabled test that used to fail a few months back.

Even the test case I made... something is marking the *Wrapper when the interface is used. E.g. adding

var da = new UIDynamicAnimator (button);
var di = da.GetDynamicItems (CoreGraphics.CGRect.Empty);

means IUIDynamicItem is marked (since it's the return type for GetDynamicItems).

[Export("itemsInRect:")]
public IUIDynamicItem[] GetDynamicItems(CGRect P_0)
{
	return NSArray.ArrayFromHandle<IUIDynamicItem>(Messaging.IntPtr_objc_msgSend_CGRect(base.Handle, Selector.GetHandle("itemsInRect:"), P_0));
}

and this works because UIDynamicItemWrapper is part of the final, linked application - as it should be, but without any fix I recall ?!?

There's no managed reference to that type so it's being preserved at build time. Time to read the code again I guess :)

[spouliot]

So, with ILLink (dotnet) the [Protocol] attribute (even if removed) is processed and marks the type of the WrapperType (Y) (or any other Type property of the attribute). This is what brings wrapper needed for a protocol (X) inside the app.

[Protocol (Name = "X", WrapperType = typeof (Y))]

That actually gets better results (a few types) than our legacy logic. At least I have not been able to create a test case that would fail. Whatever is needed to create the test is actually bringing the extra (I.e. not missing anymore) code that is required.

Incidentally it also explains #11280 (where the same feature is less than optimal when applied to a non-required protocol member parameter).

@rolfbjarne I don't think we need extra logic anymore, unless

• we disable this to fix [dotnet] LinkAll 'LinkedAwayGenericTypeAsOptionalMemberInProtocol' fails #11280 (not sure we can) or
• you can think of a test case that would not work ?

[rolfbjarne]

We remove the Protocol attribute post-sweep, because I ran into problems at the time:

https://github.com/xamarin/xamarin-macios/blob/18a5351987cc52e9c79a3d57d65d2aed9267e72c/tools/dotnet-linker/Steps/RemoveAttributesStep.cs#L6-L23

This might be reworked to use an IMarkHandler, and when it encounters a Protocol attribute, also mark the WrapperType, and then remove the attribute. That sounds like it should work like it used to (and fix #11280 as well).

[spouliot]

We remove the Protocol attribute post-sweep, because I ran into problems at the time:

Yes, that's fine :-)

This might be reworked to use an IMarkHandler, and when it encounters a Protocol attribute, also mark the WrapperType, and then remove the attribute.

Why ?

My point was that I do not think we need to change (or add) anything :-) at least not for [Protocol].

ATAICT the required *Wrapper types are always preserved correctly, unless

you can think of a test case that would not work ?

???

If not then I think we can close this issue.

Now [ProtocolMember] is different (since the non-required members can bring stuff we might not need) but I rather cover it inside #11280 as it's an optimization (for size) and not a functional issue.

[rolfbjarne]

This might be reworked to use an IMarkHandler, and when it encounters a Protocol attribute, also mark the WrapperType, and then remove the attribute.

Why ?

The point being that if we let the linker remove the Protocol[Member] attributes (or do it earlier ourselves), then presumably it won't mark the additional types these attributes reference (i.e. what #11280 is about).

If not then I think we can close this issue.

Now [ProtocolMember] is different (since the non-required members can bring stuff we might not need) but I rather cover it inside #11280 as it's an optimization (for size) and not a functional issue.

Agreed, let's close this issue, and continue in #11280.