Fix NativeAOT with minimal actions (#35167)

davidfowl · web-flow · commit 83927d9c988b · 2021-08-09T12:57:00.000-07:00
* Fix NativeAOT with minimal actions
- Generic methods for value types need to be visible at compile time so that the AOT compiler can generate code for the right instantiations (closed generic types). Since Enum.TryParse&lt;T&gt; is used purely via reflection is gets removed from the final AOT binary. This causes our logic that looks for Enum.TryParse&lt;T&gt; to fail. Instead we call back to the non-generic overload which doesn't get trimmed and can at runtime lookup the metadata for the enum type in order to allow parsing.
- Made TryParseMethodCache creatable so it could be tested more easily.
- Added tests
diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs
@@ -17,7 +17,8 @@ namespace Microsoft.AspNetCore.Http
     /// </summary>
     public static partial class RequestDelegateFactory
     {
-        private static readonly NullabilityInfoContext NullabilityContext = new NullabilityInfoContext();
+        private static readonly NullabilityInfoContext NullabilityContext = new();
+        private static readonly TryParseMethodCache TryParseMethodCache = new();
 
         private static readonly MethodInfo ExecuteTaskOfTMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTask), BindingFlags.NonPublic | BindingFlags.Static)!;
         private static readonly MethodInfo ExecuteTaskOfStringMethod = typeof(RequestDelegateFactory).GetMethod(nameof(ExecuteTaskOfString), BindingFlags.NonPublic | BindingFlags.Static)!;
@@ -42,7 +43,6 @@ public static partial class RequestDelegateFactory
         private static readonly ParameterExpression HttpContextExpr = Expression.Parameter(typeof(HttpContext), "httpContext");
         private static readonly ParameterExpression BodyValueExpr = Expression.Parameter(typeof(object), "bodyValue");
         private static readonly ParameterExpression WasParamCheckFailureExpr = Expression.Variable(typeof(bool), "wasParamCheckFailure");
-        private static readonly ParameterExpression TempSourceStringExpr = TryParseMethodCache.TempSourceStringExpr;
 
         private static readonly MemberExpression RequestServicesExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.RequestServices));
         private static readonly MemberExpression HttpRequestExpr = Expression.Property(HttpContextExpr, nameof(HttpContext.Request));
@@ -55,8 +55,8 @@ public static partial class RequestDelegateFactory
         private static readonly MemberExpression StatusCodeExpr = Expression.Property(HttpResponseExpr, nameof(HttpResponse.StatusCode));
         private static readonly MemberExpression CompletedTaskExpr = Expression.Property(null, (PropertyInfo)GetMemberInfo<Func<Task>>(() => Task.CompletedTask));
 
-        private static readonly BinaryExpression TempSourceStringNotNullExpr = Expression.NotEqual(TempSourceStringExpr, Expression.Constant(null));
-        private static readonly BinaryExpression TempSourceStringNullExpr = Expression.Equal(TempSourceStringExpr, Expression.Constant(null));
+        private static readonly BinaryExpression TempSourceStringNotNullExpr = Expression.NotEqual(TryParseMethodCache.TempSourceStringExpr, Expression.Constant(null));
+        private static readonly BinaryExpression TempSourceStringNullExpr = Expression.Equal(TryParseMethodCache.TempSourceStringExpr, Expression.Constant(null));
 
         /// <summary>
         /// Creates a <see cref="RequestDelegate"/> implementation for <paramref name="action"/>.
@@ -168,7 +168,7 @@ public static RequestDelegate Create(MethodInfo methodInfo, Func<HttpContext, ob
 
             if (factoryContext.UsingTempSourceString)
             {
-                responseWritingMethodCall = Expression.Block(new[] { TempSourceStringExpr }, responseWritingMethodCall);
+                responseWritingMethodCall = Expression.Block(new[] { TryParseMethodCache.TempSourceStringExpr }, responseWritingMethodCall);
             }
 
             return HandleRequestBodyAndCompileRequestDelegate(responseWritingMethodCall, factoryContext);
@@ -555,7 +555,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
                         Expression.IfThen(Expression.Equal(argument, Expression.Constant(null)),
                             Expression.Block(
                                 Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
-                                Expression.Call(LogRequiredParameterNotProvidedMethod, 
+                                Expression.Call(LogRequiredParameterNotProvidedMethod,
                                     HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
                             )
                         )
@@ -643,7 +643,7 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
             var failBlock = Expression.Block(
                 Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
                 Expression.Call(LogParameterBindingFailureMethod,
-                    HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TempSourceStringExpr));
+                    HttpContextExpr, parameterTypeNameConstant, parameterNameConstant, TryParseMethodCache.TempSourceStringExpr));
 
             var tryParseCall = tryParseMethodCall(parsedValue);
 
@@ -682,14 +682,14 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
             var fullParamCheckBlock = !isOptional
                 ? Expression.Block(
                     // tempSourceString = httpContext.RequestValue["id"];
-                    Expression.Assign(TempSourceStringExpr, valueExpression),
+                    Expression.Assign(TryParseMethodCache.TempSourceStringExpr, valueExpression),
                     // if (tempSourceString == null) { ... } only produced when parameter is required
                     checkRequiredParaseableParameterBlock,
                     // if (tempSourceString != null) { ... }
-                    ifNotNullTryParse) 
+                    ifNotNullTryParse)
                 : Expression.Block(
                     // tempSourceString = httpContext.RequestValue["id"];
-                    Expression.Assign(TempSourceStringExpr, valueExpression),
+                    Expression.Assign(TryParseMethodCache.TempSourceStringExpr, valueExpression),
                     // if (tempSourceString != null) { ... }
                     ifNotNullTryParse);
 
@@ -737,7 +737,7 @@ private static Expression BindParameterFromBody(ParameterInfo parameter, bool al
                     Expression.Equal(BodyValueExpr, Expression.Constant(null)),
                         Expression.Block(
                             Expression.Assign(WasParamCheckFailureExpr, Expression.Constant(true)),
-                            Expression.Call(LogRequiredParameterNotProvidedMethod, 
+                            Expression.Call(LogRequiredParameterNotProvidedMethod,
                                     HttpContextExpr, Expression.Constant(parameter.ParameterType.Name), Expression.Constant(parameter.Name))
                         )
                     )
@@ -863,8 +863,8 @@ static async Task ExecuteAwaited(Task<string> task, HttpContext httpContext)
 
         private static Task ExecuteWriteStringResponseAsync(HttpContext httpContext, string text)
         {
-             SetPlaintextContentType(httpContext);
-             return httpContext.Response.WriteAsync(text);
+            SetPlaintextContentType(httpContext);
+            return httpContext.Response.WriteAsync(text);
         }
 
         private static Task ExecuteValueTask(ValueTask task)
diff --git a/src/Http/Http.Extensions/test/TryParseMethodCacheTests.cs b/src/Http/Http.Extensions/test/TryParseMethodCacheTests.cs
@@ -27,12 +27,13 @@ public class TryParseMethodCacheTests
         [InlineData(typeof(ulong))]
         public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCulture(Type @type)
         {
-            var methodFound = TryParseMethodCache.FindTryParseMethod(@type);
+            var methodFound = new TryParseMethodCache().FindTryParseMethod(@type);
 
             Assert.NotNull(methodFound);
 
-            var call = methodFound!(Expression.Variable(type, "parsedValue"));
-            var parameters = call.Method.GetParameters();
+            var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression;
+            Assert.NotNull(call);
+            var parameters = call!.Method.GetParameters();
 
             Assert.Equal(4, parameters.Length);
             Assert.Equal(typeof(string), parameters[0].ParameterType);
@@ -49,12 +50,13 @@ public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCult
         [InlineData(typeof(TimeSpan))]
         public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCultureDateType(Type @type)
         {
-            var methodFound = TryParseMethodCache.FindTryParseMethod(@type);
+            var methodFound = new TryParseMethodCache().FindTryParseMethod(@type);
 
             Assert.NotNull(methodFound);
 
-            var call = methodFound!(Expression.Variable(type, "parsedValue"));
-            var parameters = call.Method.GetParameters();
+            var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression;
+            Assert.NotNull(call);
+            var parameters = call!.Method.GetParameters();
 
             if (@type == typeof(TimeSpan))
             {
@@ -77,12 +79,13 @@ public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCult
         [InlineData(typeof(TryParsableInvariantRecord))]
         public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCultureCustomType(Type @type)
         {
-            var methodFound = TryParseMethodCache.FindTryParseMethod(@type);
+            var methodFound = new TryParseMethodCache().FindTryParseMethod(@type);
 
             Assert.NotNull(methodFound);
 
-            var call = methodFound!(Expression.Variable(type, "parsedValue"));
-            var parameters = call.Method.GetParameters();
+            var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression;
+            Assert.NotNull(call);
+            var parameters = call!.Method.GetParameters();
 
             Assert.Equal(3, parameters.Length);
             Assert.Equal(typeof(string), parameters[0].ParameterType);
@@ -91,6 +94,56 @@ public void FindTryParseMethod_ReturnsTheExpectedTryParseMethodWithInvariantCult
             Assert.True(((call.Arguments[1] as ConstantExpression)!.Value as CultureInfo)!.Equals(CultureInfo.InvariantCulture));
         }
 
+        [Fact]
+        public void FindTryParseMethodForEnums()
+        {
+            var type = typeof(Choice);
+            var methodFound = new TryParseMethodCache().FindTryParseMethod(type);
+
+            Assert.NotNull(methodFound);
+
+            var call = methodFound!(Expression.Variable(type, "parsedValue")) as MethodCallExpression;
+            Assert.NotNull(call);
+            var method = call!.Method;
+            var parameters = method.GetParameters();
+
+            // By default, we use Enum.TryParse<T>
+            Assert.True(method.IsGenericMethod);
+            Assert.Equal(2, parameters.Length);
+            Assert.Equal(typeof(string), parameters[0].ParameterType);
+            Assert.True(parameters[1].IsOut);
+        }
+
+        [Fact]
+        public void FindTryParseMethodForEnumsWhenNonGenericEnumParseIsUsed()
+        {
+            var type = typeof(Choice);
+            var cache = new TryParseMethodCache(preferNonGenericEnumParseOverload: true);
+            var methodFound = cache.FindTryParseMethod(type);
+
+            Assert.NotNull(methodFound);
+
+            var parsedValue = Expression.Variable(type, "parsedValue");
+            var block = methodFound!(parsedValue) as BlockExpression;
+            Assert.NotNull(block);
+            Assert.Equal(typeof(bool), block!.Type);
+
+            var parseEnum = Expression.Lambda<Func<string, Choice>>(Expression.Block(new[] { parsedValue },
+                block,
+                parsedValue), cache.TempSourceStringExpr).Compile();
+
+            Assert.Equal(Choice.One, parseEnum("One"));
+            Assert.Equal(Choice.Two, parseEnum("Two"));
+            Assert.Equal(Choice.Three, parseEnum("Three"));
+        }
+
+        enum Choice
+        {
+            One,
+            Two,
+            Three
+        }
+
         private record TryParsableInvariantRecord(int value)
         {
             public static bool TryParse(string? value, IFormatProvider formatProvider, out TryParsableInvariantRecord? result)
diff --git a/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs b/src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
@@ -27,6 +27,7 @@ internal class EndpointMetadataApiDescriptionProvider : IApiDescriptionProvider
         private readonly EndpointDataSource _endpointDataSource;
         private readonly IHostEnvironment _environment;
         private readonly IServiceProviderIsService? _serviceProviderIsService;
+        private readonly TryParseMethodCache TryParseMethodCache = new();
 
         // Executes before MVC's DefaultApiDescriptionProvider and GrpcHttpApiDescriptionProvider for no particular reason.
         public int Order => -1100;
diff --git a/src/Shared/TryParseMethodCache.cs b/src/Shared/TryParseMethodCache.cs
@@ -15,35 +15,64 @@
 
 namespace Microsoft.AspNetCore.Http
 {
-    internal static class TryParseMethodCache
+    internal sealed class TryParseMethodCache
     {
-        private static readonly MethodInfo EnumTryParseMethod = GetEnumTryParseMethod();
+        private readonly MethodInfo _enumTryParseMethod;
 
         // Since this is shared source, the cache won't be shared between RequestDelegateFactory and the ApiDescriptionProvider sadly :(
-        private static readonly ConcurrentDictionary<Type, Func<Expression, MethodCallExpression>?> MethodCallCache = new();
-        internal static readonly ParameterExpression TempSourceStringExpr = Expression.Variable(typeof(string), "tempSourceString");
+        private readonly ConcurrentDictionary<Type, Func<Expression, Expression>?> _methodCallCache = new();
 
-        public static bool HasTryParseMethod(ParameterInfo parameter)
+        internal readonly ParameterExpression TempSourceStringExpr = Expression.Variable(typeof(string), "tempSourceString");
+
+        public TryParseMethodCache() : this(preferNonGenericEnumParseOverload: false)
+        {
+        }
+
+        // This is for testing
+        public TryParseMethodCache(bool preferNonGenericEnumParseOverload)
+        {
+            _enumTryParseMethod = GetEnumTryParseMethod(preferNonGenericEnumParseOverload);
+        }
+
+        public bool HasTryParseMethod(ParameterInfo parameter)
         {
             var nonNullableParameterType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType;
             return FindTryParseMethod(nonNullableParameterType) is not null;
         }
 
-        public static Func<Expression, MethodCallExpression>? FindTryParseMethod(Type type)
+        public Func<Expression, Expression>? FindTryParseMethod(Type type)
         {
-            static Func<Expression, MethodCallExpression>? Finder(Type type)
+            Func<Expression, Expression>? Finder(Type type)
             {
                 MethodInfo? methodInfo;
 
                 if (type.IsEnum)
                 {
-                    methodInfo = EnumTryParseMethod.MakeGenericMethod(type);
-                    if (methodInfo != null)
+                    if (_enumTryParseMethod.IsGenericMethod)
                     {
+                        methodInfo = _enumTryParseMethod.MakeGenericMethod(type);
+
                         return (expression) => Expression.Call(methodInfo!, TempSourceStringExpr, expression);
                     }
 
-                    return null;
+                    return (expression) =>
+                    {
+                        var enumAsObject = Expression.Variable(typeof(object), "enumAsObject");
+                        var success = Expression.Variable(typeof(bool), "success");
+
+                        // object enumAsObject;
+                        // bool success;
+                        // success = Enum.TryParse(type, tempSourceString, out enumAsObject);
+                        // parsedValue = success ? (Type)enumAsObject : default;
+                        // return success;
+
+                        return Expression.Block(new[] { success, enumAsObject },
+                            Expression.Assign(success, Expression.Call(_enumTryParseMethod, Expression.Constant(type), TempSourceStringExpr, enumAsObject)),
+                            Expression.Assign(expression,
+                                Expression.Condition(success, Expression.Convert(enumAsObject, type), Expression.Default(type))),
+                            success);
+                    };
+
                 }
 
                 if (TryGetDateTimeTryParseMethod(type, out methodInfo))
@@ -87,32 +116,59 @@ public static bool HasTryParseMethod(ParameterInfo parameter)
                 return null;
             }
 
-            return MethodCallCache.GetOrAdd(type, Finder);
+            return _methodCallCache.GetOrAdd(type, Finder);
         }
 
-        private static MethodInfo GetEnumTryParseMethod()
+        private static MethodInfo GetEnumTryParseMethod(bool preferNonGenericEnumParseOverload)
         {
             var staticEnumMethods = typeof(Enum).GetMethods(BindingFlags.Public | BindingFlags.Static);
 
+            // With NativeAOT, if there's no static usage of Enum.TryParse<T>, it will be removed
+            // we fallback to the non-generic version if that is the case
+            MethodInfo? genericCandidate = null;
+            MethodInfo? nonGenericCandidate = null;
+
             foreach (var method in staticEnumMethods)
             {
-                if (!method.IsGenericMethod || method.Name != nameof(Enum.TryParse) || method.ReturnType != typeof(bool))
+                if (method.Name != nameof(Enum.TryParse) || method.ReturnType != typeof(bool))
                 {
                     continue;
                 }
 
                 var tryParseParameters = method.GetParameters();
 
-                if (tryParseParameters.Length == 2 &&
+                // Enum.TryParse<T>(string, out object)
+                if (method.IsGenericMethod &&
+                    tryParseParameters.Length == 2 &&
                     tryParseParameters[0].ParameterType == typeof(string) &&
                     tryParseParameters[1].IsOut)
                 {
-                    return method;
+                    genericCandidate = method;
                 }
+
+                // Enum.TryParse(type, string, out object)
+                if (!method.IsGenericMethod &&
+                    tryParseParameters.Length == 3 &&
+                    tryParseParameters[0].ParameterType == typeof(Type) &&
+                    tryParseParameters[1].ParameterType == typeof(string) &&
+                    tryParseParameters[2].IsOut)
+                {
+                    nonGenericCandidate = method;
+                }
+            }
+
+            if (genericCandidate is null && nonGenericCandidate is null)
+            {
+                Debug.Fail("No suitable System.Enum.TryParse method found.");
+                throw new MissingMethodException("No suitable System.Enum.TryParse method found.");
+            }
+
+            if (preferNonGenericEnumParseOverload)
+            {
+                return nonGenericCandidate!;
             }
 
-            Debug.Fail("static bool System.Enum.TryParse<TEnum>(string? value, out TEnum result) not found.");
-            throw new Exception("static bool System.Enum.TryParse<TEnum>(string? value, out TEnum result) not found.");
+            return genericCandidate ?? nonGenericCandidate!;
         }
 
         private static bool TryGetDateTimeTryParseMethod(Type type, [NotNullWhen(true)] out MethodInfo? methodInfo)
