dotnet / linker

388 stars 127 forks source link

Address-taken methods should be considered reflection visible #3061

Closed eerhardt closed 1 year ago

eerhardt commented 1 year ago

ASP.NET Minimal APIs use the parameter names of methods in order to match them with route parameters.

When trying to run an ASP.NET Minimal API app with TrimMode=full, you get an exception at runtime:

System.InvalidOperationException: Encountered a parameter of type 'System.String' without a name. Parameters must have a name.
  at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArgument(ParameterInfo, FactoryContext)
  at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateArguments(ParameterInfo[], FactoryContext)
  at Microsoft.AspNetCore.Http.RequestDelegateFactory.CreateTargetableRequestDelegate(MethodInfo, Expression, FactoryContext, Expression`1 )
  at Microsoft.AspNetCore.Http.RequestDelegateFactory.Create(Delegate, RequestDelegateFactoryOptions )
  at Microsoft.AspNetCore.Routing.RouteEndpointDataSource.CreateRouteEndpointBuilder(RouteEntry, RoutePattern , IReadOnlyList`1 , IReadOnlyList`1 )
  at Microsoft.AspNetCore.Routing.RouteEndpointDataSource.get_Endpoints()

ASP.NET should set the switch --keep-metadata parametername in its SDK to prevent parameter names from being trimmed. However, there is no official supported way of doing it.

We should add a way to allow ASP.NET apps to tell the trimmer to not trim parameter names.

cc @sbomer

MichalStrehovsky commented 1 year ago

Illink will only trim parameter names that are not obvious targets of reflection (and reserves the right to do more with such methods in the future - NativeAOT will in fact completely remove metadata from such method.)

How is ASP.NET obtaining the MethodInfo in the first place? It should be the case that one can only obtain such crippled MethodInfo though code that is generating a warning.

eerhardt commented 1 year ago

How is ASP.NET obtaining the MethodInfo in the first place?

By calling Delegate.Method:

https://github.com/dotnet/aspnetcore/blob/ce60f0d10c0adb79b2b824f5da05e41e1199f6cf/src/Http/Http.Extensions/src/RequestDelegateFactory.cs#L176

The Delegates are passed in to the Minimal API APIs: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis?view=aspnetcore-6.0#route-parameters

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/users/{userId}/books/{bookId}", 
    (int userId, int bookId) => $"The user id is {userId} and book id is {bookId}");

app.Run();
MichalStrehovsky commented 1 year ago

By calling Delegate.Method:

It's this IL Linker bug then: https://github.com/dotnet/runtime/pull/70198#issuecomment-1145755049

We can use this issue to track fixing that. Exposing --keep-metadata parametername as a public switch should not be needed.

eerhardt commented 1 year ago

FYI @agocke - If we care about fully trimming ASP.NET Minimal APIs applications (but not NativeAOT'ing them), then this will need to get fixed.

@davidfowl @DamianEdwards - do you think that is a scenario we want to enable in .NET 8?

agocke commented 1 year ago

Happy to prioritize this if it's blocking. As Michal mentioned, this is already a trimming difference with NativeAOT, so it's definitely a bug we want to fix.

DamianEdwards commented 1 year ago

@eerhardt yes for sure, our goals include enabling customers who can't use NativeAOT to still get the benfits of full trimming.

vitek-karas commented 1 year ago

This has been fixed by https://github.com/dotnet/runtime/pull/85831.