dotnet / runtime

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

Support `Type.GetMember` usage analysis properly with IL3050 warning (NAOT) #94745

Open hamarb123 opened 1 year ago

hamarb123 commented 1 year ago

Description

Using the Type.GetMember function gives warnings that are not necessary when AOT analysis is enabled.

Reproduction Steps

Enable AOT compatible analysis: <IsAotCompatible>true</IsAotCompatible>. Write code like so:

void X(Type t)
{
    t.GetMember("SomeMethod", MemberTypes.Method, BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic);
}

Expected behavior

All that should be necessary to appease the analyser is:

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]

Actual behavior

Instead I get the following:

image

Regression?

No response

Known Workarounds

Ignore the warning.

Configuration

.NET SDK 8.0.0 VS 17.8.0 Windows 10.0.19045 x64 I don't see why it would be specific to this configuration

Other information

This was likely just overlooked when implementing the analyser. I've only discovered it now since I'm only now adding AOT support.

ghost commented 1 year ago

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

Issue Details
### Description Using the `Type.GetMember` function gives warnings that are not necessary when AOT analysis is enabled. ### Reproduction Steps Enable AOT compatible analysis: `true`. Write code like so: ```csharp type.GetMember("SomeMethod", MemberTypes.Method, BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic); ``` ### Expected behavior All that should be necessary to appease the analyser is: ```csharp [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)] ``` ### Actual behavior Instead I get the following: image ### Regression? _No response_ ### Known Workarounds Ignore the warning. ### Configuration .NET SDK 8.0.0 VS 17.8.0 Windows 10.0.19045 x64 I don't see why it would be specific to this configuration ### Other information This was likely just looked over when implementing the analyser. I've only discovered it now since I'm only now adding AOT support.
Author: hamarb123
Assignees: -
Labels: `untriaged`, `area-NativeAOT-coreclr`
Milestone: -
hamarb123 commented 1 year ago

I'd be happy to make a PR to fix this btw - if someone could point me to the code for this analyser, and people are happy for me to do it.

agocke commented 1 year ago

I think you're confusing GetMethod with GetMember. You're correct that GetMethod only requires PublicMethods and PrivateMethods, but GetMember requires DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors | DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods | DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields | DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties | DynamicallyAccessedMemberTypes.PublicEvents | DynamicallyAccessedMemberTypes.NonPublicEvents

hamarb123 commented 1 year ago

@agocke no I'm not, I specified MemberTypes.Method in the code, therefore I don't see why I'd need anything other than methods.

MichalStrehovsky commented 1 year ago

We'd need to extend intrinsic handling of this method in the Roslyn analyzer and in ILLinker/NativeAOT compiler. Searching the codebase for Type_GetMember should get you close enough. Looks like we already look at BindingFlags but that doesn't help much.

Known Workarounds

Ignore the warning.

The warning should be suppressible and this might be safe to suppress (unless there's some obscure corner case behavior difference between GetMember(MemberTypes.Method) and GetMethod, which we'd need to investigate when the intrinsic is introduced).

ghost commented 1 year ago

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

Issue Details
### Description Using the `Type.GetMember` function gives warnings that are not necessary when AOT analysis is enabled. ### Reproduction Steps Enable AOT compatible analysis: `true`. Write code like so: ```csharp void X(Type t) { t.GetMember("SomeMethod", MemberTypes.Method, BindingFlags.Public | BindingFlags.Static | BindingFlags.NonPublic); } ``` ### Expected behavior All that should be necessary to appease the analyser is: ```csharp [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)] ``` ### Actual behavior Instead I get the following: image ### Regression? _No response_ ### Known Workarounds Ignore the warning. ### Configuration .NET SDK 8.0.0 VS 17.8.0 Windows 10.0.19045 x64 I don't see why it would be specific to this configuration ### Other information This was likely just overlooked when implementing the analyser. I've only discovered it now since I'm only now adding AOT support.
Author: hamarb123
Assignees: -
Labels: `untriaged`, `area-Tools-ILLink`
Milestone: -
vitek-karas commented 1 year ago

The fix would be here: https://github.com/dotnet/runtime/blob/18cac43e79503cb2d9424dd19822e7ee8444db26/src/tools/illink/src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs#L382

The value of the new enum should already be tracked as an integer by dataflow, so I don't think it would need changes to the dataflow itself.

Note that this code is shared by all 3 tools (ILLink, ILCompiler, the analyzer). Would need some more logic to figure out the correct member type masks but probably not too difficult.

Technically we can do even better and basically map this onto the handling of GetMethod - which has the ability to preserve only a single method, if the name of the method is a constant. But that's maybe a next level optimization on top.

I agree with Michal that we should investigate if there are behavioral differences between GetMember(.. Methods ...) and GetMethod().

agocke commented 1 year ago

Yup, my mistake, missed the binding flags. It would be cool to support this properly

hamarb123 commented 1 year ago

I agree with Michal that we should investigate if there are behavioral differences between GetMember(.. Methods ...) and GetMethod().

Both GetMember and GetMethod begin to share the same implementation by this point.

https://github.com/dotnet/runtime/blob/2e912a327d1836c328258ccb656c6d7cde6cdbce/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.BindingFlags.cs#L67

https://github.com/dotnet/runtime/blob/2e912a327d1836c328258ccb656c6d7cde6cdbce/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Runtime/TypeInfos/RuntimeTypeInfo.GetMember.cs#L106

The only differences I see up to this point is that GetMember allows a pattern with * at the end to specify anything starting with the prior characters (e.g., abc* means anything starting with abc). That seems to be the only difference to me.