I do not understand the need for this change.

dotnet/android#10472 is an exception from the use of [Export]. While the attribute is Java.Interop.ExportAttribute, that attribute does not live in this repo; it's in:

https://github.com/dotnet/android/blob/4a09dd44cfb2bae13b4ba771e4a32b26eeae90ea/src/Xamarin.Android.NamingCustomAttributes/Java.Interop/ExportAttribute.cs

The other question raised in dotnet/android#10472, but not explicitly asked or answered, is where does the generic delegate come from?

The answer to that unasked question is "from Mono.Android.Export.dll", e.g.

https://github.com/dotnet/android/blob/4a09dd44cfb2bae13b4ba771e4a32b26eeae90ea/src/Mono.Android.Export/CallbackCode.cs#L564-L601

Plus lots of other changes.

Aside: I like the idea, but this is without question both an API and an ABI break, because JniNativeMethodRegistration is a struct. Which means code like this is (was?) perfectly valid:

JniNativeMethodRegistration CreateRegistration()
{
    JniNativeMethodRegistration s;
    s.Name = "a";
    s.Signature = "()V";
    s.Marshaler = null;
    return s;
}

Turning the field Marshaler into a property is absolutely an API and ABI break. Any existing code like the above will break at runtime.

(Is there any such code in the wild? Probably not. Thus, this might be fine. Maybe.)

@jonpryor thanks for the feedback, I update the PR description to better state the motivation for this change and hopefully clarifies the points you raised.

Which means code like this is (was?) perfectly valid:

I added a setter to the Marshaler property, the code snippet that you added can now be compiled and will work the same way it did until now. I also reverted the changes to MarshalMemberBuilder to use the field and property setters instead of using the constructor:

As I wrote in the updated PR description, this is an API change, but I believe it is justified.

An alternative would be introducing a new struct JniNativeMethodRegistration2 (ideally with a more creative name), which would have the new shape. We would then introduce a new overload of the _RegisterNatives method that would accept an array of these new structs. The code for registering natives in dotnet/android will become a bit more complicated, but it would mean we don't need to break the ABI.

Closing this PR because a revisited solution dotnet/android#10503 does not require these changes.

@simonrozsival wrote:

I added a setter to the Marshaler property, the code snippet that you added can now be compiled and will work the same way it did until now.

That solves the API break, but leaves the ABI break. Any existing assemblies which use the (no longer existing!) Marshaler field will break:

Unhandled exception. System.MissingFieldException: Field not found: 'mylib.JniNativeMethodRegistration.Marshaler'.
   at Program.<<Main>$>g__CreateFoo|0_0()
   at Program.<Main>$(String[] args) in /Volumes/Xamarin-Work/tmp/abi-break/app/Program.cs:line 4

It might be justified, but it could break existing code. (This could be addressed via an IL rewriter step! But such a thing would also need to be written.)

I agree that making the JniNativeMethodRegistration members fields was a mistake. They should have been properties. (Yay versioning.)

However, what's done is done. There are only four paths forward:

1. Do nothing! (Always an option…)
2. Do the change, "ignore" the fallout (i.e. "tell people to recompile their code"). This may be justified, but not now; we could consider this for .NET 11, do the change in January, and hope (pray) that that's enough testing to see how many customers this will impact.
3. Do the change, provide build-system integrations to fixup existing assemblies. Slows down the build process. Also arguably justifiable, but increases maintenance costs.
4. Introduce a new struct (JniNativeMethodRegistration2?) and "overload" all places which currently use JniNativeMethodRegistration. This is A Lot™, but means existing code won't break.

Right now, (1) "do nothing" is the path forward, as this change isn't necessary to address dotnet/android#10472.

(I can foresee a future where the Marshaler member should be an IntPtr, as part of future NativeAOT plus (my favorite) jni-marshalmethodgen efforts, which is why I've previously pondered such changes. Then I hit the ABI break aspect, and kick the can down the road until "later"…)