[AOT] Enable analysis and annotate Http.Results

[JamesNK]

Fixes #46075

PR enables trimming and AOT analysis on Microsoft.AspNetCore.Http.Results and fixes warnings.

• Adds trim warnings to methods that take object routeValues. Reflection is used to get property values.
• Adds safe overloads of warning methods that take RouteValueDictionary. This is the same pattern we used for LinkGenerator extension methods. Will need API review. Follow up task [AOT] Add safe route value overloads to Results and TypeResults #46229
• Adds a reflection fallback for calling static PopulateMetadata method.
• Adds temporary suppressions around Response.WriteAsJsonAsync and ProblemDetails.Extensions calls. I'm guessing the warnings will be removed from these with Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch #45886

[eerhardt]

We should write tests for this.... As of now it is untested code, right?

[JamesNK]

Yes. Has the IsDynamicCodeSupported arrived in aspnetcore?

#45860

[eerhardt]

Yes, that one is in the current runtime we are using in aspnetcore. It's the DI switch that isn't here yet - blocked on a runtime bug in #46055.

Note even if the IsDynamicCodeSupported wasn't in, we could still write tests by making targeted console apps that are published for AOT and executed.

[davidfowl]

Seeing the changes to the results that generate links by using reflection on an object makes me think we should have generic overloads where we can mark them with DAM attributes.

[JamesNK]

That would solve trimming. However, without dynamic code generation, the properties would still be accessed using slow reflection in AOT. I don't think we should encourage that.

aspnetcore/src/Shared/PropertyHelper/PropertyHelper.cs

Lines 214 to 243 in 4535ea1

	// MakeGenericMethod + value type requires IsDynamicCodeSupported to be true.
	if (RuntimeFeature.IsDynamicCodeSupported)
	{
	// TODO: Remove disable when https://github.com/dotnet/linker/issues/2715 is complete.
	#pragma warning disable IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
	// Instance methods in the CLR can be turned into static methods where the first parameter
	// is open over "target". This parameter is always passed by reference, so we have a code
	// path for value types and a code path for reference types.
	if (getMethod.DeclaringType!.IsValueType)
	{
	// Create a delegate (ref TDeclaringType) -> TValue
	return MakeFastPropertyGetter(
	typeof(ByRefFunc<,>),
	getMethod,
	propertyGetterByRefWrapperMethod);
	}
	else
	{
	// Create a delegate TDeclaringType -> TValue
	return MakeFastPropertyGetter(
	typeof(Func<,>),
	getMethod,
	propertyGetterWrapperMethod);
	}
	#pragma warning restore IL3050 // Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.
	}
	else
	{
	return propertyInfo.GetValue;
	}

[davidfowl]

Reflection isn’t slow with AOT. Now that we’re doing the work I think it’s time to quantify this on NativeAOT.

cc @jkotas

[jkotas]

Reflection isn’t slow with AOT. Now that we’re doing the work I think it’s time to quantify this on NativeAOT.

I am not sure how fast it is today.

We have work planned for ,NET 8 to make reflection fast (on all runtimes). dotnet/designs#286 is the design document.

cc @steveharter

[brunolins16]

• Adds temporary suppressions around Response.WriteAsJsonAsync and ProblemDetails.Extensions calls. I'm guessing the warnings will be removed from these with Making JsonOptions AOT/Trimmer-safe with EnsureJsonTrimmability switch #45886

This PR should remove all Json related wanings #46008

[JamesNK]

Reflection isn’t slow with AOT. Now that we’re doing the work I think it’s time to quantify this on NativeAOT.

cc @jkotas

I wrote some benchmarks:

public class JitBenchmarks
{
    [Benchmark]
    public RouteValueDictionary RawDictionary() => new RouteValueDictionary { ["foo"] = "One", ["bar"] = "Two", ["baz"] = "Three" };

    [Benchmark]
    public RouteValueDictionary PropertyReflection() => new RouteValueDictionary(new { foo = "One", bar = "Two", baz = "Three" });
}

[SimpleJob(RuntimeMoniker.NativeAot80)]
public class AotBenchmarks
{
    [Benchmark]
    public RouteValueDictionary RawDictionary() => new RouteValueDictionary { ["foo"] = "One", ["bar"] = "Two", ["baz"] = "Three" };

    [Benchmark]
    public RouteValueDictionary PropertyReflection() => new RouteValueDictionary(new { foo = "One", bar = "Two", baz = "Three" });
}

public class Program
{
    public static void Main(string[] args)
    {
        _ = BenchmarkRunner.Run(new[] { typeof (AotBenchmarks), typeof(JitBenchmarks) } );
    }
}

JIT:

BenchmarkDotNet=v0.13.4, OS=Windows 11 (10.0.22621.1105)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.100-alpha.1.23060.5
  [Host]     : .NET 8.0.0 (8.0.23.5802), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.0 (8.0.23.5802), X64 RyuJIT AVX2


|             Method |     Mean |    Error |   StdDev |
|------------------- |---------:|---------:|---------:|
|      RawDictionary | 18.09 ns | 0.337 ns | 0.581 ns |
| PropertyReflection | 28.57 ns | 0.381 ns | 0.318 ns |

AOT:

BenchmarkDotNet=v0.13.4, OS=Windows 11 (10.0.22621.1105)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK=8.0.100-alpha.1.23060.5
  [Host]        : .NET 8.0.0 (8.0.23.5802), X64 RyuJIT AVX2
  NativeAOT 8.0 : .NET 8.0.0-alpha.1.23058.2, X64 NativeAOT AVX2

Job=NativeAOT 8.0  Runtime=NativeAOT 8.0

|             Method |     Mean |    Error |   StdDev |
|------------------- |---------:|---------:|---------:|
|      RawDictionary | 24.91 ns | 0.531 ns | 0.443 ns |
| PropertyReflection | 41.66 ns | 0.865 ns | 1.124 ns |

AOT seems generally slower, but native AOT reflection isn't that far off compiled expressions.

My other concern is the number of overloads. We have Redirect(object values) today. Choices to add:

• Add Redirect(RouteValueDictionary values). This is consistent with what was added to link generation for trimming.
• Add Redirect<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] TRouteValues>(TRouteValues values). Would also catch method calls with RouteValueDictionary. The downside is it would include all public properties from RouteValueDictionary, but that isn't important.
• Add both.

[JamesNK]

I thought about this more, and generics + DynamicallyAccessedMembers doesn't work with Nullable<T>.

If the value is Nullable<T>, then DynamicallyAccessedMembers on the method instructs Nullable<T> to preserve its properties, not the actual value of T.

[davidfowl]

If the value is Nullable, then DynamicallyAccessedMembers on the method instructs Nullable to preserve its properties, not the actual value of T.

Sounds like something we can ask @vitek-karas about? Seems like it could be fixable no?

[vitek-karas]

If the value is Nullable, then DynamicallyAccessedMembers on the method instructs Nullable to preserve its properties, not the actual value of T.

Sounds like something we can ask @vitek-karas about? Seems like it could be fixable no?

That should not be the case. Trimmers (both AOT and linker) have a special case behavior for Nullable<T>. Applying annotation to Nullable<T> should apply the annotation to both the Nullable<> as well as the specific T. This is a special case, it won't work for any other generics. The main driver for this decision was that C# kind of hides the Nullable<>. Consider:

    void PrintProperties<[DynamicallyAccessedMembers(Properties)] T>()
    {
         foreach (var p in Nullable.GetUnderlyingType(typeof(T)).GetProperties()) { ... }
    }

    record A(int One, int Two);
    struct record B(int One, int Two);

    PrintProperties<A?>();
    PrintProperties<B?>();

Without the special case there would be a very distinct behavioral difference between A? and B? but there's basically no visible difference in the syntax. In once case the ? has no impact on the generated type system, in the other case the ? actually changes the semantic type of the value.

Do you have a specific example where this maybe doesn't work?

/cc @jtschuster as the author of most of the relevant changes in the linker

[JamesNK]

Ok! I didn't check this, I just guessed behavior. I'm sure I've seen linker warnings coming from doing type = Nullable.GetUnderlyingType(type), and the new type loses all the annotated linker data. Was it my imagination? Or is this a recent change?

The special handling of Nullable<T> should solve the problem I mentioned.

[JamesNK]

Seeing the changes to the results that generate links by using reflection on an object makes me think we should have generic overloads where we can mark them with DAM attributes.

Damn, I found a scenario where we can't be trim-safe with DAM attribute. GetProperties on an interface doesn't return its inherited properties. To get all the properties for the type that implements an interface, you must get its properties, then get all the interfaces that it implements, then get those properties, then merge them together. This is what RouteValueDictionary does.

aspnetcore/src/Shared/PropertyHelper/PropertyHelper.cs

Lines 510 to 521 in e523876

	var propertyHelpers = new List<PropertyHelper>();
	// We avoid loading indexed properties using the Where statement.
	AddInterestingProperties(propertyHelpers, type);
	if (type.IsInterface)
	{
	// Reflection does not return information about inherited properties on the interface itself.
	foreach (var @interface in type.GetInterfaces())
	{
	AddInterestingProperties(propertyHelpers, @interface);
	}
	}

Unfortunately, that loses DAM information because you're calling GetProperties on interface types returned by GetInterfaces. These types no longer have the DAM information, and GetProperties on them isn't safe.

For example:

public interface IRoleUser : IUser
{
    public string Role { get; set; }
}

public interface IUser
{
    public string UserName { get; set; }
}

public RouteValueDictionary FromObject<TRouteValues>([DynamicallyAccessedMembers(Properties | Interfaces)] TRouteValues values)
{
    return new RouteValueDictioanry(values);
}

IRoleUser user = new RoleUserImpl();
var routeValueDictionary = FromObject(user);

Only the Role property is safely included.

[vitek-karas]

The All annotation would make this work - at least in terms of what is removed. All keeps all members on the type, regardless of if they're public/private and it propagates to base class and all interfaces. Note that it also applies to all nested types - hopefully that will not cause size problems.

It might still produce warnings though because trimming analysis doesn't see through collections, so GetInterfaces returns unannotated values. So it would require a suppression unfortunately.

[JamesNK]

Good to know. Thanks.

---

Unfortunately, another challenge. Adding a generic argument for route values to TypeResults creates ambiguous method calls.

  public static IResult CreatedAtRoute(string? routeName = null, object? routeValues = null, object? value = null)
+ public static IResult CreatedAtRoute<TRouteValues>(string? routeName, TRouteValues routeValues, object? value = null)
  public static IResult CreatedAtRoute<TValue>(string? routeName = null, object? routeValues = null, TValue? value = default)
+ public static IResult CreatedAtRoute<TValue, TRouteValues>(string? routeName, TRouteValues routeValues, TValue? value = default)

CreatedAtRoute<TRouteValues> and CreatedAtRoute<TValue> creates a conflict if one generic argument is specified and parameters that match both overloads.

For example, this is ambigious:

TypedResults.CreatedAtRoute<object>("RouteName", new { param1 = "param1Value" }, new User());

It's a source-breaking change, and the method call needs to be modified to one of:

// both generic arguments
TypedResults.CreatedAtRoute<object, object>("RouteName", new { param1 = "param1Value" }, new User());

// or remove generic argument
TypedResults.CreatedAtRoute("RouteName", new { param1 = "param1Value" }, new User());

@davidfowl What do you think? I'm concerned about how many overloads there are with optional parameters. And generic arguments. I worry that there are undiscovered ambiguous method call situations today, and adding future overloads will be an ambiguous method call minefield.

[davidfowl]

I think we can mark the existing methods as unsafe and discuss a way forward based on what you've discovered.

@jaredpar this is a ref count for the API that lets us set an attribute to influence overload resolution for preferred methods.

[JamesNK]

All feedback applied. Please take a look.

[JamesNK]

All outstanding feedback applied. Please review 🙏

[halter73]

LGTM! I'm glad that Results<...> continues to work even if we're over-keeping members. I agree that adding a RemoteExecutor test for the non-IsDynamicCodeSupported-case makes sense.

[halter73]

Why were we able to remove these suppressions? Can we do the same in Results.cs?

[eerhardt]

LGTM.
Thanks for the great work here, @JamesNK! I had some minor comments.