mgravell / Pipelines.Sockets.Unofficial

.NET managed sockets wrapper using the new "Pipelines" API
Other
412 stars 54 forks source link

Trim and AOT warnings in Delegates and PerTypeHelpers #72

Open eerhardt opened 1 year ago

eerhardt commented 1 year ago

In trying to use Microsoft.Extensions.Caching.StackExchangeRedis in a Native AOT app (see https://github.com/dotnet/aspnetcore/issues/45910), I'm getting the following warnings from these two places:

https://github.com/mgravell/Pipelines.Sockets.Unofficial/blob/f044f671282603407b0ec306feb8e11556717492/src/Pipelines.Sockets.Unofficial/Delegates.cs#L37-L58

/_/src/Pipelines.Sockets.Unofficial/Delegates.cs(52): AOT analysis warning IL3050: Pipelines.Sockets.Unofficial.Delegates.GetGetter<T>(String): Using member 'System.Reflection.Emit.DynamicMethod.DynamicMethod(String,Type,Type[],Type,Boolean)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. Creating a DynamicMethod requires dynamic code. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]

This looks simply like we can just check for RuntimeFeature.IsDynamicCodeSupported and fallback to reflection when it isn't supported. But the bigger issue is that _invocationList and _invocationCount don't appear to be fields of MulticastDelegate in Native AOT: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/System/MulticastDelegate.cs. So I'm not sure how to fix this.

https://github.com/mgravell/Pipelines.Sockets.Unofficial/blob/f044f671282603407b0ec306feb8e11556717492/src/Pipelines.Sockets.Unofficial/Arenas/PerTypeHelpers.cs#L13-L47

/_/src/Pipelines.Sockets.Unofficial/Arenas/PerTypeHelpers.cs(39): Trim analysis warning IL2090: Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1.<PreferPinned>g__Calculate|3_0(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Type.MakeGenericType(Type[])'. The generic parameter 'T' of 'Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1' 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. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/Pipelines.Sockets.Unofficial/Arenas/PerTypeHelpers.cs(39): AOT analysis warning IL3050: Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1.<PreferPinned>g__Calculate|3_0(): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/Pipelines.Sockets.Unofficial/Arenas/PerTypeHelpers.cs(19): Trim analysis warning IL2090: Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1.<PreferUnmanaged>g__Calculate|2_0(): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'System.Type.MakeGenericType(Type[])'. The generic parameter 'T' of 'Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1' 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. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]
/_/src/Pipelines.Sockets.Unofficial/Arenas/PerTypeHelpers.cs(19): AOT analysis warning IL3050: Pipelines.Sockets.Unofficial.Arenas.PerTypeHelpers`1.<PreferUnmanaged>g__Calculate|2_0(): Using member 'System.Type.MakeGenericType(Type[])' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [C:\git\azure-activedirectory-identitymodel-extensions-for-dotnet\test\Microsoft.IdentityModel.AotCompatibility.TestApp\Microsoft.IdentityModel.AotCompatibility.TestApp.csproj]

Maybe these can be wrapped in RuntimeFeature.IsDynamicCodeSupported? I'm a bit confused by this code because it isn't actually returning the result of invoking those properties through reflection.

cc @mgravell

mgravell commented 1 year ago

So; you're absolutely right about the allocater thing; that was... just bad code, very embarrassing. I'm torn now; do I fix it, or do I deprecate the scenario on the basis that it has literally never worked, so... does it matter? but now I need to go check "are we relying on that pin", because if we are: GODAMMIT

on the delegates thing: no additional change should be required - it already handles the "no field" scenario

I've added a work-in-progress tidy up that also adds an AOT smoke test project: https://github.com/mgravell/Pipelines.Sockets.Unofficial/pull/74

I'm going to see whether SE.Redis relies on that flag before deciding how to proceed on the allocator thing

mgravell commented 1 year ago

reopening to continue discussion; initial fixes: look good and merged

mgravell commented 1 year ago

SE.Redis doesn't use the Delegates helper or the flags, so: no urgency on resolving those I'm going to merge my fixes "as is" for now, and see if we can get a step further (we seem to be actively discussing it, let's finish that conversation first)