dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

.NET 9, ILC reports trimmer warnings for `k__BackingField` for auto-properties #108978

Open jonathanpeppers opened 1 month ago

jonathanpeppers commented 1 month ago

Description

For the property:

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Type? IHybridWebView.InvokeJavaScriptType { get; set; }

We got the trimmer warning:

ILC: IL2114: Microsoft.Maui.Controls.HybridWebView.<Microsoft.Maui.IHybridWebView.InvokeJavaScriptType>k__BackingField: 'DynamicallyAccessedMembersAttribute' on 'Microsoft.Maui.Controls.HybridWebView' or one of its base types

This was inside a .NET MAUI project on iOS.

We think something like this will workaround this case, but CI is ongoing:

Reproduction Steps

<ItemGroup>
    <PackageReference Include="Microsoft.Maui.Controls.Foldable" Version="$(MauiVersion)" />
    <PackageReference Include="Microsoft.Maui.Controls.Maps" Version="$(MauiVersion)" />
    <PackageReference Include="Microsoft.Maui.Graphics.Skia" Version="$(MauiVersion)" />
</ItemGroup>
<ItemGroup>
    <TrimmerRootAssembly Include="Microsoft.Maui" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Controls" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Controls.Foldable" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Controls.Maps" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Controls.Xaml" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Essentials" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Graphics" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Graphics.Skia" RootMode="All" />
    <TrimmerRootAssembly Include="Microsoft.Maui.Maps" RootMode="All" />
</ItemGroup>

Expected behavior

We would not get trimmer warnings for k__BackingField.

Actual behavior

We get trimmer warnings for k__BackingField.

Regression?

Not sure

Known Workarounds

I assume we can do:

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
private Type? _invokeJavaScriptType;

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Type? IHybridWebView.InvokeJavaScriptType
{
    get => _invokeJavaScriptType;
    set => _invokeJavaScriptType = value;
}

Configuration

> dotnet --version
9.0.100-rc.2.24426.11

Other information

No response

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

sbomer commented 1 month ago

The warning is due to DynamicallyAccessedMembers on this base class of the property containing type: https://github.com/dotnet/maui/blob/d8552206eb4a7f39bea3ac1c3d71ce9373e88338/src/Controls/src/Core/VisualElement/VisualElement_StyleSheet.cs#L12, which has NonPublicFields. This annotation says that non-public fields of any derived types might be reflected over, and the warning guards against violating the annotations on the field via reflection.

Here's an example of what the warning guards against:

using System.Reflection;
using System.Diagnostics.CodeAnalysis;

var t = new C().GetType();
AssignField(t, typeof(D));
if (C.P.GetMethods().Length == 0) {
    throw new Exception("No methods"); // Will throw in AOT only
}

static void AssignField([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicFields)] Type t, Type value) {
    foreach (var field in t.GetFields(BindingFlags.NonPublic | BindingFlags.Static)) {
        field.SetValue(null, value);
    }
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicFields)]
class C {
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
    public static Type? P { get; set; }
}

class D {
    public static void M() {}
}

Changing it to an explicitly implemented property won't help, it'll just move the warning to the explicit field.

A more precise analysis could potentially get rid of these warnings and instead warn at the GetType or AssignField callsite. But the current semantics basically make it illegal to put DynamicallyAccessedMembers annotations on fields when a base type has DynamicallyAccessedMembers.(Non)PublicFields. In practice it's probably OK to suppress this if you're reasonably sure that nobody will use reflection to violate the field annotation.

jonathanpeppers commented 1 month ago

So, I think the issue here is that the warning message could be improved. If the property was mentioned instead of k__BackingField, I wouldn't have thought it was a bug.