Skip to content
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.

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

[Java.Interop] remove DynamicallyAccessedMemberTypes.Interfaces #1285

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

jonathanpeppers
Copy link
Member

Context: dotnet/android#9630

As #9630 required new declarations of
DynamicallyAccessedMemberTypes.Interfaces, we intestigated to see why these were needed in Java.Interop.

There are two cases:

var listIface   = typeof (IList<>);
var listType    =
    (from iface in type.GetInterfaces ().Concat (new[]{type})
        where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface)
        select iface)
    .FirstOrDefault ();

This first one, if IList<> were trimmed away. We don't actually care, as everything in this code path would still work. We can suppress the warning instead.

The second case:

JniValueMarshalerAttribute? ifaceAttribute = null;
foreach (var iface in type.GetInterfaces ()) {
    marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> ();
    if (marshalerAttr != null) {
        if (ifaceAttribute != null)
            throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}.");

        ifaceAttribute = marshalerAttr;
    }
}
if (ifaceAttribute != null)
    return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!;

Feels like we should be able to remove this code completely.

With these changes we can remove DynamicallyAccessedMemberTypes.Interfaces.

I also introduced build-tools/trim-analyzers/trim-analyzers.props that will setup the appropriate trimmer MSBuild properties to make trimmer warnings an error. This should keep us from accidentally creating warnings. I only use this setting in projects that were already using $(EnableAotAnalyzer).

Context: dotnet/android#9630

As #9630 required new declarations of
`DynamicallyAccessedMemberTypes.Interfaces`, we intestigated to see
*why* these were needed in Java.Interop.

There are two cases:

    var listIface   = typeof (IList<>);
    var listType    =
        (from iface in type.GetInterfaces ().Concat (new[]{type})
            where (listIface).IsAssignableFrom (iface.IsGenericType ? iface.GetGenericTypeDefinition () : iface)
            select iface)
        .FirstOrDefault ();

This first one, if `IList<>` were trimmed away. We don't actually
care, as everything in this code path would still work. We can
suppress the warning instead.

The second case:

    JniValueMarshalerAttribute? ifaceAttribute = null;
    foreach (var iface in type.GetInterfaces ()) {
        marshalerAttr = iface.GetCustomAttribute<JniValueMarshalerAttribute> ();
        if (marshalerAttr != null) {
            if (ifaceAttribute != null)
                throw new NotSupportedException ($"There is more than one interface with custom marshaler for type {type}.");

            ifaceAttribute = marshalerAttr;
        }
    }
    if (ifaceAttribute != null)
        return (JniValueMarshaler) Activator.CreateInstance (ifaceAttribute.MarshalerType)!;

Feels like we should be able to remove this code completely.

With these changes we can remove `DynamicallyAccessedMemberTypes.Interfaces`.

I also introduced `build-tools/trim-analyzers/trim-analyzers.props`
that will setup the appropriate trimmer MSBuild properties to make
trimmer warnings an error. This should keep us from accidentally
creating warnings. I only use this setting in projects that were
already using `$(EnableAotAnalyzer)`.

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (7)
  • build-tools/trim-analyzers/trim-analyzers.props: Language not supported
  • src/Java.Interop.Tools.TypeNameMappings/Java.Interop.Tools.TypeNameMappings.csproj: Language not supported
  • src/Java.Interop/Java.Interop.csproj: Language not supported
  • src/Java.Interop/Java.Interop/JniBuiltinMarshalers.cs: Evaluated as low risk
  • src/Java.Interop/Java.Interop/JniRuntime.JniMarshalMemberBuilder.cs: Evaluated as low risk
  • src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs: Evaluated as low risk
  • src/Java.Interop/Java.Interop/JniStringValueMarshaler.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/Java.Interop/Java.Interop/JniValueMarshaler.cs:123

  • The removal of DynamicallyAccessedMemberTypes.Interfaces might cause issues if any code relies on dynamically accessed interfaces. Ensure that all necessary interfaces are still accessible and properly handled.
internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

src/Java.Interop/Java.Interop/JniValueMarshaler.cs:135

  • Ensure that there are sufficient tests covering the behavior of CreateValue when only constructors are dynamically accessed. This is to ensure that no functionality relying on interfaces is broken.
public abstract object? CreateValue (ref JniObjectReference reference, JniObjectReferenceOptions options, [DynamicallyAccessedMembers (Constructors)] Type? targetType = null);
@jonathanpeppers
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

jonathanpeppers added a commit to dotnet/android that referenced this pull request Dec 18, 2024
@jonathanpeppers
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonathanpeppers
Copy link
Member Author

@jonpryor I think the changes here + dotnet/android#9630 cause a crash in Mono.Android-Tests:

12-18 18:50:09.648  6790  6790 I DOTNET  : JavaProxyThrowable: obtaining Java stack trace threw an exception: System.NotImplementedException: Do not know how to dispose: Invalid.
12-18 18:50:09.648  6790  6790 I DOTNET  :    at Java.Interop.JniObjectReference.Dispose(JniObjectReference& reference)
12-18 18:50:09.648  6790  6790 I DOTNET  :    at Java.Interop.JniObjectReference.Dispose(JniObjectReference& reference, JniObjectReferenceOptions options)
12-18 18:50:09.648  6790  6790 I DOTNET  :    at Android.Runtime.AndroidValueManager.CreatePeer(JniObjectReference& reference, JniObjectReferenceOptions options, Type targetType)
12-18 18:50:09.650  6790  6790 I DOTNET  :    at Java.Interop.JavaPeerableValueMarshaler.CreateGenericValue(JniObjectReference& reference, JniObjectReferenceOptions options, Type targetType)
12-18 18:50:09.650  6790  6790 I DOTNET  :    at Java.Interop.JniRuntime.JniValueManager.GetValue(JniObjectReference& reference, JniObjectReferenceOptions options, Type targetType)
12-18 18:50:09.652  6790  6790 I DOTNET  :    at Java.Lang.Object.GetObject(IntPtr handle, JniHandleOwnership transfer, Type type)
12-18 18:50:09.652  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv.<CreateNativeArrayElementToManaged>g__GetObject|73_11(IntPtr e, Type t)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayElementToManaged>b__73_9(Type type, IntPtr source, Int32 index)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv.CopyArray(IntPtr src, Array dest, Type elementType)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv.<>c.<CreateNativeArrayToManaged>b__99_12(Type type, IntPtr source, Int32 len)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv._GetArray(IntPtr array_ptr, Type element_type)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JNIEnv.GetArray(IntPtr array_ptr, JniHandleOwnership transfer, Type element_type)
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Java.Lang.Throwable.GetStackTrace()
12-18 18:50:09.653  6790  6790 I DOTNET  :    at Android.Runtime.JavaProxyThrowable.TranslateStackTrace()

run-Mono.Android.NET_Tests-Debug-1285.binlog.zip

@jonpryor
Copy link
Member

@jonathanpeppers wrote:

I think the changes here + dotnet/android#9630 cause a crash in Mono.Android-Tests

The cause of the crash is here:

JniObjectReference reference = new (handle);

The default type in the JniObjectReference constructor is .Invalid:

public JniObjectReference (IntPtr handle, JniObjectReferenceType type = JniObjectReferenceType.Invalid)

…which we don't know how to dispose, because we don't know what it is.

The fix is…another utility function!

partial class JNIEnv {
    internal static JniObjectReference CreateJniObjectReference (IntPtr handle, JniHandleOwnership transfer)
    {
        if (transfer.HasFlag (JniHandleOwnership.TransferLocalRef))
            return new JniObjectReference (handle, JniObjectReferenceType.Local);
        if (transfer.HasFlag (JniHandleOwnership.TransferGlobalRef))
            return new JniObjectReference (handle, JniObjectReferenceType.Global);
         return new JniObjectReference (handle); // there be possible dragons here!
    }
}

Then Object.GetObject<T>() is updated to use:

var reference = JNIEnv.CreateJniObjectReference (handle, transfer);

@jonpryor jonpryor merged commit 2c06b3c into main Dec 18, 2024
4 checks passed
@jonpryor jonpryor deleted the dev/peppers/trimmer/interfaces branch December 18, 2024 21:24
jonpryor added a commit to dotnet/android that referenced this pull request Dec 19, 2024
Changes: dotnet/java-interop@f800ea5...2c06b3c

  * dotnet/java-interop@2c06b3c2: [Java.Interop] remove `DynamicallyAccessedMemberTypes.Interfaces` (dotnet/java-interop#1285)
jonpryor added a commit to dotnet/android that referenced this pull request Dec 19, 2024
Changes: dotnet/java-interop@f800ea5...2c06b3c

  * dotnet/java-interop@2c06b3c2: [Java.Interop] remove `DynamicallyAccessedMemberTypes.Interfaces` (dotnet/java-interop#1285)
jonpryor added a commit to dotnet/android that referenced this pull request Dec 19, 2024
Changes: dotnet/java-interop@f800ea5...2c06b3c

  * dotnet/java-interop@2c06b3c2: [Java.Interop] remove `DynamicallyAccessedMemberTypes.Interfaces` (dotnet/java-interop#1285)
jonpryor added a commit to dotnet/android that referenced this pull request Dec 19, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants