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.

Sign up for GitHub

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

Jump to bottom

[Mono.Android] is now "trimming safe" #8778

Merged
merged 6 commits into from
Mar 13, 2024
Merged

[Mono.Android] is now "trimming safe" #8778

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6399051
[Mono.Android] is now "trimming safe"
jonathanpeppers Feb 29, 2024
e4bbe8c
Changes to make diff in `AndroidEnvironment` smaller
jonathanpeppers Mar 11, 2024
3e34f0f
Set `$(IlcTreatWarningsAsErrors)`
jonathanpeppers Mar 11, 2024
ffd33ed
One less IL2068 suppression
jonathanpeppers Mar 11, 2024
db695b4
[DAM] not needed on JavaCollection<T>, JavaList<T>, JavaDictionary<K,V>
jonathanpeppers Mar 11, 2024
79b9908
Update justification message
jonathanpeppers Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions build-tools/trim-analyzers/trim-analyzers.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<!-- Import this to enable trim warnings of all kinds -->
<Project>
<PropertyGroup>
<!-- Sets assembly metadata, enable analyzers -->
<IsTrimmable>true</IsTrimmable>
<EnableSingleFileAnalyzer>true</EnableSingleFileAnalyzer>
<EnableAotAnalyzer>true</EnableAotAnalyzer>
<!-- In app projects, tells ILLink to emit warnings as errors -->
<ILLinkTreatWarningsAsErrors>true</ILLinkTreatWarningsAsErrors>
<!--
Trim warnings, codes listed here:
* https://github.com/dotnet/runtime/blob/7403a062960d092c73ce3f07d3ff323ffdf7de43/src/tools/illink/src/linker/Resources/Strings.resx
* https://github.com/dotnet/docs/tree/9cb45cf9cd34f3b7259a023c3d92a124a87090d5/docs/core/deploying/trimming/trim-warnings
-->
<WarningsAsErrors>
$(WarningsAsErrors);
IL2000;IL2001;IL2002;IL2003;IL2004;
Comment on lines +15 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Is this to get the analyzer warnings to be treated as errors?
For trimmer the ILLinkTreatWarningsAsErrors should be enough to do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found it didn't work in a class library, unless I added these.

Let me look at an MSBuild log to see why.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find anything in the ILLink analyzer that would look for $(ILLinkTreatWarningsAsErrors) and promote the warnings to errors.

I was looking here:

Copy link
Member

Choose a reason for hiding this comment

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

Analyzer doesn't do this, if you need this for the analyzer you do need to use the Roslyn's properties - so WarnAsError or TreatWarningsAsErrors (if you can enable it for all warnings). The ILLinkTreatWarningsAsErrors is only for ILLink.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what we have here is OK for now. We could consider $(TreatWarningsAsErrors) as some point, but we have ~31 long-standing warnings from binding the Android API surface. There are a lot fewer now than there used to be, so it might be reasonable to fix these in .NET 9.

IL2005;IL2006;IL2007;IL2008;IL2009;
IL2010;IL2011;IL2012;IL2013;IL2014;
IL2015;IL2016;IL2017;IL2018;IL2019;
IL2020;IL2021;IL2022;IL2023;IL2024;
IL2025;IL2026;IL2027;IL2028;IL2029;
IL2030;IL2031;IL2032;IL2033;IL2034;
IL2035;IL2036;IL2037;IL2038;IL2039;
IL2040;IL2041;IL2042;IL2043;IL2044;
IL2045;IL2046;IL2047;IL2048;IL2049;
IL2050;IL2051;IL2052;IL2053;IL2054;
IL2055;IL2056;IL2057;IL2058;IL2059;
IL2060;IL2061;IL2062;IL2063;IL2064;
IL2065;IL2066;IL2067;IL2068;IL2069;
IL2070;IL2071;IL2072;IL2073;IL2074;
IL2075;IL2076;IL2077;IL2078;IL2079;
IL2080;IL2081;IL2082;IL2083;IL2084;
IL2085;IL2086;IL2087;IL2088;IL2089;
IL2090;IL2091;IL2092;IL2093;IL2094;
IL2095;IL2096;IL2097;IL2098;IL2099;
IL2100;IL2101;IL2102;IL2103;IL2104;
IL2105;IL2106;IL2107;IL2108;IL2109;
IL2110;IL2111;IL2112;IL2113;IL2114;
IL2115;IL2116;IL2117;IL2118;IL2119;
IL2120;IL2121;IL2122;IL2123;IL2124;
IL2125;IL2126;IL2127;IL2128;IL2129;
</WarningsAsErrors>
<!-- In NativeAOT app projects, tells Ilc to emit warnings as errors -->
<IlcTreatWarningsAsErrors>true</IlcTreatWarningsAsErrors>
<!--
NativeAOT warnings, codes listed here:
* https://github.com/dotnet/docs/tree/9cb45cf9cd34f3b7259a023c3d92a124a87090d5/docs/core/deploying/native-aot/warnings
-->
<WarningsAsErrors>
jonathanpeppers marked this conversation as resolved.
Show resolved Hide resolved
$(WarningsAsErrors);
IL3050;IL3051;IL3052;IL3053;IL3054;IL3055;IL3056;
</WarningsAsErrors>
</PropertyGroup>
</Project>
11 changes: 10 additions & 1 deletion src/Mono.Android/Android.OS/AsyncTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,16 @@ namespace Android.OS {

[global::System.Runtime.Versioning.ObsoletedOSPlatform ("android30.0")]
[Register ("android/os/AsyncTask", DoNotGenerateAcw=true)]
public abstract class AsyncTask<TParams, TProgress, TResult> : AsyncTask {
public abstract class AsyncTask<
[DynamicallyAccessedMembers (Constructors)]
TParams,
[DynamicallyAccessedMembers (Constructors)]
TProgress,
[DynamicallyAccessedMembers (Constructors)]
TResult
> : AsyncTask {

const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

static IntPtr java_class_handle;
internal static IntPtr class_ref {
Expand Down
20 changes: 17 additions & 3 deletions src/Mono.Android/Android.Runtime/AndroidEnvironment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public static class AndroidEnvironment {
static IX509TrustManager? sslTrustManager;
static KeyStore? certStore;
static object lock_ = new object ();
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
static Type? httpMessageHandlerType;

static void SetupTrustManager ()
Expand Down Expand Up @@ -335,11 +336,18 @@ static IWebProxy GetDefaultProxy ()
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor, typeof (Xamarin.Android.Net.AndroidMessageHandler))]
static object GetHttpMessageHandler ()
{
// FIXME: https://github.com/xamarin/xamarin-android/issues/8797
// Note that this is a problem for custom $(AndroidHttpClientHandlerType) or $XA_HTTP_CLIENT_HANDLER_TYPE
[UnconditionalSuppressMessage ("Trimming", "IL2057", Justification = "DynamicDependency should preserve AndroidMessageHandler.")]
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
static Type TypeGetType (string typeName) =>
Type.GetType (typeName, throwOnError: false);

if (httpMessageHandlerType is null) {
var handlerTypeName = Environment.GetEnvironmentVariable ("XA_HTTP_CLIENT_HANDLER_TYPE")?.Trim ();
Type? handlerType = null;
if (!String.IsNullOrEmpty (handlerTypeName))
handlerType = Type.GetType (handlerTypeName, throwOnError: false);
handlerType = TypeGetType (handlerTypeName);

// We don't do any type checking or casting here to avoid dependency on System.Net.Http in Mono.Android.dll
if (handlerType is null || !IsAcceptableHttpMessageHandlerType (handlerType)) {
Expand Down Expand Up @@ -369,13 +377,19 @@ static bool IsAcceptableHttpMessageHandlerType (Type handlerType)
return true;
}

static bool Extends (Type handlerType, string baseTypeName) {
static bool Extends (
Type handlerType,
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
string baseTypeName)
{
var baseType = Type.GetType (baseTypeName, throwOnError: false);
return baseType?.IsAssignableFrom (handlerType) ?? false;
}

static Type GetFallbackHttpMessageHandlerType (string typeName = "Xamarin.Android.Net.AndroidMessageHandler, Mono.Android")
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
static Type GetFallbackHttpMessageHandlerType ()
{
const string typeName = "Xamarin.Android.Net.AndroidMessageHandler, Mono.Android";
var handlerType = Type.GetType (typeName, throwOnError: false)
?? throw new InvalidOperationException ($"The {typeName} was not found. The type was probably linked away.");

Expand Down
36 changes: 29 additions & 7 deletions src/Mono.Android/Android.Runtime/JNIEnv.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,27 @@

namespace Android.Runtime {
public static partial class JNIEnv {
const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

[ThreadStatic]
static byte[]? mvid_bytes;

public static IntPtr Handle => JniEnvironment.EnvironmentPointer;

static Array ArrayCreateInstance (Type elementType, int length) =>
// FIXME: https://github.com/xamarin/xamarin-android/issues/8724
// IL3050 disabled in source: if someone uses NativeAOT, they will get the warning.
#pragma warning disable IL3050
Array.CreateInstance (elementType, length);
#pragma warning restore IL3050

static Type MakeArrayType (Type type) =>
// FIXME: https://github.com/xamarin/xamarin-android/issues/8724
// IL3050 disabled in source: if someone uses NativeAOT, they will get the warning.
#pragma warning disable IL3050
type.MakeArrayType ();
#pragma warning restore IL3050

internal static IntPtr IdentityHash (IntPtr v)
{
return JNIEnvInit.LocalRefsAreIndirect ? RuntimeNativeMethods._monodroid_get_identity_hash_code (Handle, v) : v;
Expand Down Expand Up @@ -807,7 +823,7 @@ public static void CopyArray (IntPtr src, Array dest, Type? elementType = null)
throw new ArgumentNullException ("dest");

if (elementType != null && elementType.IsValueType)
AssertCompatibleArrayTypes (src, elementType.MakeArrayType ());
AssertCompatibleArrayTypes (src, MakeArrayType (elementType));

if (elementType != null && elementType.IsArray) {
for (int i = 0; i < dest.Length; ++i) {
Expand Down Expand Up @@ -950,7 +966,7 @@ public static void CopyArray (Array source, Type elementType, IntPtr dest)
throw new ArgumentNullException ("elementType");

if (elementType.IsValueType)
AssertCompatibleArrayTypes (elementType.MakeArrayType (), dest);
AssertCompatibleArrayTypes (MakeArrayType (elementType), dest);

Action<Array, IntPtr> converter = GetConverter (CopyManagedToNativeArray, elementType, dest);

Expand Down Expand Up @@ -1057,12 +1073,12 @@ public static void CopyArray<T> (T[] src, IntPtr dest)
}
} },
{ typeof (IJavaObject), (type, source, len) => {
var r = Array.CreateInstance (type!, len);
var r = ArrayCreateInstance (type!, len);
CopyArray (source, r, type);
return r;
} },
{ typeof (Array), (type, source, len) => {
var r = Array.CreateInstance (type!, len);
var r = ArrayCreateInstance (type!, len);
CopyArray (source, r, type);
return r;
} },
Expand All @@ -1075,7 +1091,7 @@ public static void CopyArray<T> (T[] src, IntPtr dest)
return null;

if (element_type != null && element_type.IsValueType)
AssertCompatibleArrayTypes (array_ptr, element_type.MakeArrayType ());
AssertCompatibleArrayTypes (array_ptr, MakeArrayType (element_type));

int cnt = _GetArrayLength (array_ptr);

Expand Down Expand Up @@ -1130,7 +1146,10 @@ static int _GetArrayLength (IntPtr array_ptr)
return ret;
}

public static T[]? GetArray<T> (Java.Lang.Object[] array)
public static T[]? GetArray<
[DynamicallyAccessedMembers (Constructors)]
T
> (Java.Lang.Object[] array)
{
if (array == null)
return null;
Expand Down Expand Up @@ -1244,7 +1263,10 @@ static IntPtr GetArrayElementClass<T>(T[] values)
return FindClass (elementType);
}

public static void CopyObjectArray<T>(IntPtr source, T[] destination)
public static void CopyObjectArray<
[DynamicallyAccessedMembers (Constructors)]
T
>(IntPtr source, T[] destination)
{
if (source == IntPtr.Zero)
return;
Expand Down
16 changes: 14 additions & 2 deletions src/Mono.Android/Android.Runtime/JavaCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ namespace Android.Runtime {
// java.util.Collection allows null values
public class JavaCollection : Java.Lang.Object, System.Collections.ICollection {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

internal static IntPtr collection_class = JNIEnv.FindClass ("java/util/Collection");

internal static IntPtr id_add;
Expand Down Expand Up @@ -148,6 +150,11 @@ internal Java.Lang.Object[] ToArray ()
//
public void CopyTo (Array array, int array_index)
{
[UnconditionalSuppressMessage ("Trimming", "IL2073", Justification = "JavaCollection<T> constructors are preserved by the MarkJavaObjects trimmer step.")]
[return: DynamicallyAccessedMembers (Constructors)]
static Type GetElementType (Array array) =>
array.GetType ().GetElementType ();

if (array == null)
throw new ArgumentNullException ("array");
if (array_index < 0)
Expand All @@ -164,7 +171,7 @@ public void CopyTo (Array array, int array_index)
JavaConvert.FromJniHandle (
JNIEnv.GetObjectArrayElement (lrefArray, i),
JniHandleOwnership.TransferLocalRef,
array.GetType ().GetElementType ()),
GetElementType (array)),
array_index + i);
JNIEnv.DeleteLocalRef (lrefArray);
}
Expand Down Expand Up @@ -202,8 +209,13 @@ public static IntPtr ToLocalJniHandle (ICollection? items)
}
}

// Preserve FromJniHandle
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
[Register ("java/util/Collection", DoNotGenerateAcw=true)]
public sealed class JavaCollection<T> : JavaCollection, ICollection<T> {
public sealed class JavaCollection<
[DynamicallyAccessedMembers (Constructors)]
T
> : JavaCollection, ICollection<T> {

public JavaCollection (IntPtr handle, JniHandleOwnership transfer)
: base (handle, transfer)
Expand Down
11 changes: 10 additions & 1 deletion src/Mono.Android/Android.Runtime/JavaDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ namespace Android.Runtime {
// java.util.HashMap allows null keys and values
public class JavaDictionary : Java.Lang.Object, System.Collections.IDictionary {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;

class DictionaryEnumerator : IDictionaryEnumerator {

IEnumerator simple_enumerator;
Expand Down Expand Up @@ -395,8 +397,15 @@ public static IntPtr ToLocalJniHandle (IDictionary? dictionary)
// instantiates a type we don't know at this time, so we have no information about the exceptions
// it may throw.
//
// Preserve FromJniHandle
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
[Register ("java/util/HashMap", DoNotGenerateAcw=true)]
public class JavaDictionary<K, V> : JavaDictionary, IDictionary<K, V> {
public class JavaDictionary<
[DynamicallyAccessedMembers (Constructors)]
K,
[DynamicallyAccessedMembers (Constructors)]
V
> : JavaDictionary, IDictionary<K, V> {

[Register (".ctor", "()V", "")]
public JavaDictionary ()
Expand Down
20 changes: 17 additions & 3 deletions src/Mono.Android/Android.Runtime/JavaList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Android.Runtime {
// java.util.ArrayList allows null values
public partial class JavaList : Java.Lang.Object, System.Collections.IList {

internal const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors;
internal static readonly JniPeerMembers list_members = new XAPeerMembers ("java/util/List", typeof (JavaList), isInterface: true);

//
Expand All @@ -23,7 +24,10 @@ public partial class JavaList : Java.Lang.Object, System.Collections.IList {
//
// https://developer.android.com/reference/java/util/List.html?hl=en#get(int)
//
internal unsafe object? InternalGet (int location, Type? targetType = null)
internal unsafe object? InternalGet (
int location,
[DynamicallyAccessedMembers (Constructors)]
Type? targetType = null)
{
const string id = "get.(I)Ljava/lang/Object;";
JniObjectReference obj;
Expand Down Expand Up @@ -266,14 +270,19 @@ public unsafe bool Contains (object? item)

public void CopyTo (Array array, int array_index)
{
[UnconditionalSuppressMessage ("Trimming", "IL2073", Justification = "JavaList<T> constructors are preserved by the MarkJavaObjects trimmer step.")]
[return: DynamicallyAccessedMembers (Constructors)]
static Type GetElementType (Array array) =>
array.GetType ().GetElementType ();

if (array == null)
throw new ArgumentNullException ("array");
if (array_index < 0)
throw new ArgumentOutOfRangeException ("array_index");
if (array.Length < array_index + Count)
throw new ArgumentException ("array");

var targetType = array.GetType ().GetElementType ();
var targetType = GetElementType (array);
int c = Count;
for (int i = 0; i < c; i++)
array.SetValue (InternalGet (i, targetType), array_index + i);
Expand Down Expand Up @@ -672,8 +681,13 @@ public virtual Java.Lang.Object [] ToArray ()
#endregion
}

// Preserve FromJniHandle
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
[Register ("java/util/ArrayList", DoNotGenerateAcw=true)]
public class JavaList<T> : JavaList, IList<T> {
public class JavaList<
[DynamicallyAccessedMembers (Constructors)]
T
> : JavaList, IList<T> {

//
// Exception audit:
Expand Down
5 changes: 4 additions & 1 deletion src/Mono.Android/Android.Runtime/JavaSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,10 @@ public static IntPtr ToLocalJniHandle (ICollection? items)

[Register ("java/util/HashSet", DoNotGenerateAcw=true)]
// java.util.HashSet allows null
public class JavaSet<T> : JavaSet, ICollection<T> {
public class JavaSet<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
T
> : JavaSet, ICollection<T> {

//
// Exception audit:
Expand Down
6 changes: 5 additions & 1 deletion src/Mono.Android/Android.Util/SparseArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics.CodeAnalysis;

using Android.Runtime;

Expand All @@ -7,7 +8,10 @@
namespace Android.Util
{
[Register ("android/util/SparseArray", DoNotGenerateAcw=true)]
public partial class SparseArray<E> : SparseArray
public partial class SparseArray<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
E
> : SparseArray
{
public SparseArray ()
{
Expand Down
6 changes: 5 additions & 1 deletion src/Mono.Android/Android.Widget/AdapterView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using Android.Views;
using JLO = Java.Lang.Object;

Expand Down Expand Up @@ -49,7 +50,10 @@ public event EventHandler ItemSelectionCleared {
}

[Register ("android/widget/AdapterView", DoNotGenerateAcw=true)]
public abstract class AdapterView<T> : AdapterView where T : IAdapter {
public abstract class AdapterView<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
T
> : AdapterView where T : IAdapter {

public AdapterView (IntPtr handle, JniHandleOwnership transfer)
: base (handle, transfer)
Expand Down
Loading