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

[Mono.Android] fix a set of the "easiest" trimmer warnings #8731

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

jonathanpeppers
Copy link
Member

Context: #5652

Introduce a new trim-analyzers.targets file to be used in this repo:

  1. Enables appropriate analyzers.

  2. $(ILLinkTreatWarningsAsErrors) only does anything in builds that would run ILLink, such as an application build. Useful to be here.

  3. List all the explicit ILLink and AOT warnings, so that they trigger build errors.

Eventually we should be able to import this file for Mono.Android.csproj.

Fixing the following so far:

TypeConverter

Context: https://source.dot.net/#System.ComponentModel.TypeConverter/System/ComponentModel/TypeConverter.cs,226

src\Mono.Android\System.Drawing\SizeFConverter.cs(121,49):
error IL2046: Base member 'System.ComponentModel.TypeConverter.GetProperties(ITypeDescriptorContext, Object, Attribute[])' with 'RequiresUnreferencedCodeAttribute' has a derived member 'System.Drawing.SizeFConverter.GetProperties(ITypeDescriptorContext, Object, Attribute[])' without 'RequiresUnreferencedCodeAttribute'. 'RequiresUnreferencedCodeAttribute' annotations must match across all interface implementations or overrides.

Various TypeConverter implementations need to specify [RequiresUnreferencedCode] to match the base type.

ColorValueMarshaler & IJavaObjectValueMarshaler

Context: dotnet/java-interop@7d1e705

From the trimmer warnings solved in Java.Interop.dll, we need to bring these changes forward to any *Marshaler types.

AndroidClientHandler

'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicFields' in call to 'System.Type.GetField(String, BindingFlags)'. The return value of method 'System.Collections.IEnumerator.Current.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

This class had a loop that was not trimming safe:

for (var type = GetType (); type != null; type = type.BaseType) {
    field = type.GetField (fieldName, BindingFlags.Instance | BindingFlags.NonPublic);
    if (field != null)
        break;
}

If we look at the actual base types of AndroidClientHandler, we can simplify this loop to something that is trimming safe:

const BindingFlags flags = BindingFlags.Instance | BindingFlags.NonPublic;
FieldInfo? field = typeof (HttpClientHandler).GetField (fieldName, flags) ??
    typeof (HttpMessageHandler).GetField (fieldName, flags);

There should be no need to iterate beyond HttpMessageHandler, the code is looking for this field:

https://github.com/dotnet/runtime/blob/135fec006e727a31763271984cd712f1659ccbd3/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L25

Context: dotnet#5652

Introduce a new `trim-analyzers.targets` file to be used in this repo:

1. Enables appropriate analyzers.

2. `$(ILLinkTreatWarningsAsErrors)` only does anything in builds that
   would run `ILLink`, such as an application build. Useful to be here.

3. List all the explicit ILLink and AOT warnings, so that they trigger
   build errors.

Eventually we should be able to import this file for
`Mono.Android.csproj`.

Fixing the following so far:

~~ TypeConverter ~~

Context: https://source.dot.net/#System.ComponentModel.TypeConverter/System/ComponentModel/TypeConverter.cs,226

    src\Mono.Android\System.Drawing\SizeFConverter.cs(121,49):
    error IL2046: Base member 'System.ComponentModel.TypeConverter.GetProperties(ITypeDescriptorContext, Object, Attribute[])' with 'RequiresUnreferencedCodeAttribute' has a derived member 'System.Drawing.SizeFConverter.GetProperties(ITypeDescriptorContext, Object, Attribute[])' without 'RequiresUnreferencedCodeAttribute'. 'RequiresUnreferencedCodeAttribute' annotations must match across all interface implementations or overrides.

Various `TypeConverter` implementations need to specify
`[RequiresUnreferencedCode]` to match the base type.

~~ ColorValueMarshaler & IJavaObjectValueMarshaler ~~

Context: dotnet/java-interop@7d1e705

From the trimmer warnings solved in `Java.Interop.dll`, we need to
bring these changes forward to any `*Marshaler` types.

~~ AndroidClientHandler ~~

    'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.NonPublicFields' in call to 'System.Type.GetField(String, BindingFlags)'. The return value of method 'System.Collections.IEnumerator.Current.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

This class had a loop that was not trimming safe:

    for (var type = GetType (); type != null; type = type.BaseType) {
        field = type.GetField (fieldName, BindingFlags.Instance | BindingFlags.NonPublic);
        if (field != null)
            break;
    }

If we *look* at the actual base types of `AndroidClientHandler`, we
can simplify this loop to something that *is* trimming safe:

    const BindingFlags flags = BindingFlags.Instance | BindingFlags.NonPublic;
    FieldInfo? field = typeof (HttpClientHandler).GetField (fieldName, flags) ??
        typeof (HttpMessageHandler).GetField (fieldName, flags);

There should be no need to iterate *beyond* `HttpMessageHandler`, the
code is looking for this field:

https://github.com/dotnet/runtime/blob/135fec006e727a31763271984cd712f1659ccbd3/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L25
@jonathanpeppers
Copy link
Member Author

/cc @simonrozsival @vitek-karas

@jonathanpeppers jonathanpeppers marked this pull request as ready for review February 16, 2024 18:27
@jonpryor jonpryor merged commit a82ac4e into dotnet:main Feb 23, 2024
18 of 29 checks passed
@jonathanpeppers jonathanpeppers deleted the EasyTrimmingWarnings branch February 23, 2024 19:48
grendello added a commit that referenced this pull request Feb 28, 2024
* main:
  Bump to xamarin/xamarin-android-tools/main@37d79c9 (#8752)
  Bump to dotnet/installer@d070660282 9.0.100-preview.3.24126.2 (#8763)
  Bump to xamarin/java.interop/main@14a9470 (#8766)
  $(AndroidPackVersionSuffix)=preview.3; net9 is 34.99.0.preview.3 (#8765)
  [Mono.Android] Do not dispose request content stream in AndroidMessageHandler (#8764)
  Bump com.android.tools:r8 from 8.2.42 to 8.2.47 (#8761)
  [Mono.Android] fix a set of the "easiest" trimmer warnings (#8731)
  Bump to dotnet/installer@0a73f814e1 9.0.100-preview.2.24122.3 (#8716)
  [ci] Always run the MAUI test job (#8750)
  Add a property required by #8478 (#8749)
  [xamarin-android-tools] import $(LibZipSharpVersion) value (#8738)
  Bump to xamarin/Java.Interop/main@c825dcad (#8701)
  Bump to xamarin/monodroid@cb01503327 (#8742)
  Bump to xamarin/Java.Interop/main@ae65609 (#8744)
  Bring in changes from PR #8478 (#8727)
  [xaprepare] Make 7zip work with "dangerous" symlinks in ZIPs (#8737)
  Bump NDK to r26c (#8732)
  Debugging MSBuild Tasks (#8730)
grendello added a commit that referenced this pull request Feb 28, 2024
* main:
  [Mono.Android] fix a set of the "easiest" trimmer warnings (#8731)
  Bump to dotnet/installer@0a73f814e1 9.0.100-preview.2.24122.3 (#8716)
grendello added a commit that referenced this pull request Mar 1, 2024
* main:
  [tests] fix duplicate sources in `NuGet.config` (#8772)
  Bump to xamarin/monodroid@e13723e701 (#8771)
  Bump to xamarin/xamarin-android-tools/main@37d79c9 (#8752)
  Bump to dotnet/installer@d070660282 9.0.100-preview.3.24126.2 (#8763)
  Bump to xamarin/java.interop/main@14a9470 (#8766)
  $(AndroidPackVersionSuffix)=preview.3; net9 is 34.99.0.preview.3 (#8765)
  [Mono.Android] Do not dispose request content stream in AndroidMessageHandler (#8764)
  Bump com.android.tools:r8 from 8.2.42 to 8.2.47 (#8761)
  [Mono.Android] fix a set of the "easiest" trimmer warnings (#8731)
  Bump to dotnet/installer@0a73f814e1 9.0.100-preview.2.24122.3 (#8716)
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2024
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