Address feedback from config binding gen PR to improve enum parsing (#…

…89952)
dotnet · Oct 11, 2023 · f1b4930 · f1b4930
1 parent 5685318
commit f1b4930
Show file tree

Hide file tree

Showing 66 changed files with 5,038 additions and 72 deletions.
diff --git a/...es/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs b/...es/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Emitter.cs
@@ -13,6 +13,9 @@ private sealed partial class Emitter
             private readonly InterceptorInfo _interceptorInfo;
             private readonly BindingHelperInfo _bindingHelperInfo;
             private readonly TypeIndex _typeIndex;
+            private readonly bool _emitEnumParseMethod;
+            private readonly bool _emitGenericParseEnum;
+            private readonly bool _emitThrowIfNullMethod;
 
             private readonly SourceWriter _writer = new();
 
@@ -21,6 +24,9 @@ public Emitter(SourceGenerationSpec sourceGenSpec)
                 _interceptorInfo = sourceGenSpec.InterceptorInfo;
                 _bindingHelperInfo = sourceGenSpec.BindingHelperInfo;
                 _typeIndex = new TypeIndex(sourceGenSpec.ConfigTypes);
+                _emitEnumParseMethod = sourceGenSpec.EmitEnumParseMethod;
+                _emitGenericParseEnum = sourceGenSpec.EmitGenericParseEnum;
+                _emitThrowIfNullMethod = sourceGenSpec.EmitThrowIfNullMethod;
             }
 
             public void Emit(SourceProductionContext context)

diff --git a/...ies/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs b/...ies/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
@@ -26,6 +26,8 @@ internal sealed partial class Parser(CompilationData compilationData)
 
             private readonly InterceptorInfo.Builder _interceptorInfoBuilder = new();
             private BindingHelperInfo.Builder? _helperInfoBuilder; // Init'ed with type index when registering interceptors, after creating type specs.
+            private bool _emitEnumParseMethod;
+            private bool _emitGenericParseEnum;
 
             public List<DiagnosticInfo>? Diagnostics { get; private set; }
 
@@ -45,12 +47,16 @@ internal sealed partial class Parser(CompilationData compilationData)
                 ParseInvocations(invocations);
                 CreateTypeSpecs(cancellationToken);
                 RegisterInterceptors();
+                CheckIfToEmitParseEnumMethod();
 
                 return new SourceGenerationSpec
                 {
                     InterceptorInfo = _interceptorInfoBuilder.ToIncrementalValue(),
                     BindingHelperInfo = _helperInfoBuilder!.ToIncrementalValue(),
                     ConfigTypes = _createdTypeSpecs.Values.OrderBy(s => s.TypeRef.FullyQualifiedName).ToImmutableEquatableArray(),
+                    EmitEnumParseMethod = _emitEnumParseMethod,
+                    EmitGenericParseEnum = _emitGenericParseEnum,
+                    EmitThrowIfNullMethod = IsThrowIfNullMethodToBeEmitted()
                 };
             }
 
@@ -840,6 +846,45 @@ private void RecordDiagnostic(DiagnosticDescriptor descriptor, Location trimmedL
                 Diagnostics ??= new List<DiagnosticInfo>();
                 Diagnostics.Add(DiagnosticInfo.Create(descriptor, trimmedLocation, messageArgs));
             }
+
+            private void CheckIfToEmitParseEnumMethod()
+            {
+                foreach (var typeSymbol in _createdTypeSpecs.Keys)
+                {
+                    if (IsEnum(typeSymbol))
+                    {
+                        _emitEnumParseMethod = true;
+                        _emitGenericParseEnum = _typeSymbols.Enum.GetMembers("Parse").Any(m => m is IMethodSymbol methodSymbol && methodSymbol.IsGenericMethod);
+                        return;
+                    }
+                }
+            }
+
+            private bool IsThrowIfNullMethodToBeEmitted()
+            {
+                if (_typeSymbols.ArgumentNullException is not null)
+                {
+                    var throwIfNullMethods = _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull");
+
+                    foreach (var throwIfNullMethod in throwIfNullMethods)
+                    {
+                        if (throwIfNullMethod is IMethodSymbol throwIfNullMethodSymbol && throwIfNullMethodSymbol.IsStatic && throwIfNullMethodSymbol.Parameters.Length == 2)
+                        {
+                            var parameters = throwIfNullMethodSymbol.Parameters;
+                            var firstParam = parameters[0];
+                            var secondParam = parameters[1];
+
+                            if (firstParam.Name == "argument" && firstParam.Type.SpecialType == SpecialType.System_Object
+                                && secondParam.Name == "paramName" && secondParam.Type.Equals(_typeSymbols.String, SymbolEqualityComparer.Default))
+                            {
+                                return true;
+                            }
+                        }
+                    }
+                }
+
+                return false;
+            }
         }
     }
 }
diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs
@@ -144,8 +144,8 @@ void EmitMethods(ImmutableEquatableArray<TypedInterceptorInvocationInfo>? interc
                             Debug.Assert(!type.IsValueType);
                             string binderOptionsArg = configureOptions ? $"{Identifier.GetBinderOptions}({Identifier.configureOptions})" : $"{Identifier.binderOptions}: null";
 
-                            EmitCheckForNullArgument_WithBlankLine(Identifier.configuration);
-                            EmitCheckForNullArgument_WithBlankLine(Identifier.instance, voidReturn: true);
+                            EmitCheckForNullArgument_WithBlankLine(Identifier.configuration, _emitThrowIfNullMethod);
+                            EmitCheckForNullArgument_WithBlankLine(Identifier.instance, _emitThrowIfNullMethod, voidReturn: true);
                             _writer.WriteLine($$"""
                                 var {{Identifier.typedObj}} = ({{type.DisplayString}}){{Identifier.instance}};
                                 {{nameof(MethodsToGen_CoreBindingHelper.BindCore)}}({{configExpression}}, ref {{Identifier.typedObj}}, defaultValueIfNotFound: false, {{binderOptionsArg}});

diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
@@ -92,7 +92,7 @@ private void EmitGetCoreMethod()
                 EmitBlankLineIfRequired();
                 EmitStartBlock($"public static object? {nameof(MethodsToGen_CoreBindingHelper.GetCore)}(this {Identifier.IConfiguration} {Identifier.configuration}, Type {Identifier.type}, Action<{Identifier.BinderOptions}>? {Identifier.configureOptions})");
 
-                EmitCheckForNullArgument_WithBlankLine(Identifier.configuration);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.configuration, _emitThrowIfNullMethod);
 
                 _writer.WriteLine($"{Identifier.BinderOptions}? {Identifier.binderOptions} = {Identifier.GetBinderOptions}({Identifier.configureOptions});");
                 _writer.WriteLine();
@@ -178,7 +178,7 @@ private void EmitGetValueCoreMethod()
                 EmitBlankLineIfRequired();
                 EmitStartBlock($"public static object? {nameof(MethodsToGen_CoreBindingHelper.GetValueCore)}(this {Identifier.IConfiguration} {Identifier.configuration}, Type {Identifier.type}, string {Identifier.key})");
 
-                EmitCheckForNullArgument_WithBlankLine(Identifier.configuration);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.configuration, _emitThrowIfNullMethod);
                 _writer.WriteLine($@"{Identifier.IConfigurationSection} {Identifier.section} = {GetSectionFromConfigurationExpression(Identifier.key, addQuotes: false)};");
                 _writer.WriteLine();
 
@@ -224,7 +224,7 @@ private void EmitBindCoreMainMethod()
 
                 EmitBlankLineIfRequired();
                 EmitStartBlock($"public static void {nameof(MethodsToGen_CoreBindingHelper.BindCoreMain)}({Identifier.IConfiguration} {Identifier.configuration}, object {Identifier.instance}, Type {Identifier.type}, {TypeDisplayString.NullableActionOfBinderOptions} {Identifier.configureOptions})");
-                EmitCheckForNullArgument_WithBlankLine(Identifier.instance, voidReturn: true);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.instance, _emitThrowIfNullMethod, voidReturn: true);
                 EmitIConfigurationHasValueOrChildrenCheck(voidReturn: true);
                 _writer.WriteLine($"{Identifier.BinderOptions}? {Identifier.binderOptions} = {Identifier.GetBinderOptions}({Identifier.configureOptions});");
                 _writer.WriteLine();
@@ -475,24 +475,20 @@ private void EmitHelperMethods()
                     EmitGetBinderOptionsHelper();
                 }
 
-                if (_bindingHelperInfo.TypesForGen_ParsePrimitive is { Count: not 0 } stringParsableTypes)
+                if (_emitEnumParseMethod)
                 {
-                    bool enumTypeExists = false;
+                    _writer.WriteLine();
+                    EmitEnumParseMethod();
+                    _emitBlankLineBeforeNextStatement = true;
+                }
 
+                if (_bindingHelperInfo.TypesForGen_ParsePrimitive is { Count: not 0 } stringParsableTypes)
+                {
                     foreach (ParsableFromStringSpec type in stringParsableTypes)
                     {
-                        EmitBlankLineIfRequired();
-
-                        if (type.StringParsableTypeKind == StringParsableTypeKind.Enum)
-                        {
-                            if (!enumTypeExists)
-                            {
-                                EmitEnumParseMethod();
-                                enumTypeExists = true;
-                            }
-                        }
-                        else
+                        if (type.StringParsableTypeKind is not StringParsableTypeKind.Enum)
                         {
+                            EmitBlankLineIfRequired();
                             EmitPrimitiveParseMethod(type);
                         }
                     }
@@ -585,16 +581,13 @@ private void EmitEnumParseMethod()
             {
                 string exceptionArg1 = string.Format(ExceptionMessages.FailedBinding, $"{{{Identifier.getPath}()}}", $"{{typeof(T)}}");
 
+                string parseEnumCall = _emitGenericParseEnum ? "Enum.Parse<T>(value, ignoreCase: true)" : "(T)Enum.Parse(typeof(T), value, ignoreCase: true)";
                 _writer.WriteLine($$"""
                     public static T ParseEnum<T>(string value, Func<string?> getPath) where T : struct
                     {
                         try
                         {
-                            #if NETFRAMEWORK || NETSTANDARD2_0
-                                return (T)Enum.Parse(typeof(T), value, ignoreCase: true);
-                            #else
-                                return Enum.Parse<T>(value, ignoreCase: true);
-                            #endif
+                            return {{parseEnumCall}};
                         }
                         catch ({{Identifier.Exception}} {{Identifier.exception}})
                         {
@@ -614,8 +607,6 @@ private void EmitPrimitiveParseMethod(ParsableFromStringSpec type)
 
                 switch (typeKind)
                 {
-                    case StringParsableTypeKind.Enum:
-                        return;
                     case StringParsableTypeKind.ByteArray:
                         {
                             parsedValueExpr = $"Convert.FromBase64String({Identifier.value})";

diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs
@@ -229,18 +229,30 @@ private void EmitBlankLineIfRequired()
                 _emitBlankLineBeforeNextStatement = true;
             }
 
-            private void EmitCheckForNullArgument_WithBlankLine(string paramName, bool voidReturn = false)
+            private void EmitCheckForNullArgument_WithBlankLine(string paramName, bool useThrowIfNullMethod, bool voidReturn = false)
             {
-                string returnExpr = voidReturn
-                    ? "return"
-                    : $"throw new ArgumentNullException(nameof({paramName}))";
-
-                _writer.WriteLine($$"""
+                if (voidReturn)
+                {
+                    _writer.WriteLine($$"""
                     if ({{paramName}} is null)
                     {
-                        {{returnExpr}};
+                        return;
                     }
                     """);
+                }
+                else
+                {
+                    string throwIfNullExpr = useThrowIfNullMethod
+                    ? $"ArgumentNullException.ThrowIfNull({paramName});"
+                    : $$"""
+                    if ({{paramName}} is null)
+                    {
+                        throw new ArgumentNullException(nameof({{paramName}}));
+                    }
+                    """;
+
+                    _writer.WriteLine(throwIfNullExpr);
+                }
 
                 _writer.WriteLine();
             }

diff --git a/...soft.Extensions.Configuration.Binder/gen/Emitter/OptionsBuilderConfigurationExtensions.cs b/...soft.Extensions.Configuration.Binder/gen/Emitter/OptionsBuilderConfigurationExtensions.cs
@@ -43,7 +43,7 @@ private void EmitBindMethods_Extensions_OptionsBuilder()
                     paramList + $", {TypeDisplayString.NullableActionOfBinderOptions} {Identifier.configureBinder}",
                     documentation);
 
-                EmitCheckForNullArgument_WithBlankLine(Identifier.optionsBuilder);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.optionsBuilder, _emitThrowIfNullMethod);
 
                 _writer.WriteLine($$"""
                     {{Identifier.Configure}}<{{Identifier.TOptions}}>({{Identifier.optionsBuilder}}.{{Identifier.Services}}, {{Identifier.optionsBuilder}}.Name, {{Identifier.config}}, {{Identifier.configureBinder}});
@@ -65,11 +65,11 @@ private void EmitBindConfigurationMethod()
 
                 EmitMethodStartBlock(MethodsToGen.OptionsBuilderExt_BindConfiguration, "BindConfiguration", paramList, documentation);
 
-                EmitCheckForNullArgument_WithBlankLine(Identifier.optionsBuilder);
-                EmitCheckForNullArgument_WithBlankLine(Identifier.configSectionPath);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.optionsBuilder, _emitThrowIfNullMethod);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.configSectionPath, _emitThrowIfNullMethod);
 
                 EmitStartBlock($"{Identifier.optionsBuilder}.{Identifier.Configure}<{Identifier.IConfiguration}>(({Identifier.instance}, {Identifier.config}) =>");
-                EmitCheckForNullArgument_WithBlankLine(Identifier.config);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.config, _emitThrowIfNullMethod);
                 _writer.WriteLine($$"""
                     {{Identifier.IConfiguration}} {{Identifier.section}} = string.Equals(string.Empty, {{Identifier.configSectionPath}}, StringComparison.OrdinalIgnoreCase) ? {{Identifier.config}} : {{Identifier.config}}.{{Identifier.GetSection}}({{Identifier.configSectionPath}});
                     {{nameof(MethodsToGen_CoreBindingHelper.BindCoreMain)}}({{Identifier.section}}, {{Identifier.instance}}, typeof({{Identifier.TOptions}}), {{Identifier.configureBinder}});

diff --git a/...sions.Configuration.Binder/gen/Emitter/OptionsConfigurationServiceCollectionExtensions.cs b/...sions.Configuration.Binder/gen/Emitter/OptionsConfigurationServiceCollectionExtensions.cs
@@ -53,8 +53,8 @@ private void EmitConfigureMethods()
                 // Like the others, it is public API that could be called directly by users.
                 // So, it is always generated whenever a Configure overload is called.
                 EmitStartMethod(MethodsToGen.ServiceCollectionExt_Configure_T_name_BinderOptions, paramList: $"string? {Identifier.name}, " + configParam + $", {TypeDisplayString.NullableActionOfBinderOptions} {Identifier.configureOptions}");
-                EmitCheckForNullArgument_WithBlankLine(Identifier.services);
-                EmitCheckForNullArgument_WithBlankLine(Identifier.config);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.services, _emitThrowIfNullMethod);
+                EmitCheckForNullArgument_WithBlankLine(Identifier.config, _emitThrowIfNullMethod);
                 _writer.WriteLine($$"""
                     OptionsServiceCollectionExtensions.AddOptions({{Identifier.services}});
                     {{Identifier.services}}.{{Identifier.AddSingleton}}<{{Identifier.IOptionsChangeTokenSource}}<{{Identifier.TOptions}}>>(new {{Identifier.ConfigurationChangeTokenSource}}<{{Identifier.TOptions}}>({{Identifier.name}}, {{Identifier.config}}));

diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Parser/KnownTypeSymbols.cs
@@ -56,6 +56,8 @@ internal sealed class KnownTypeSymbols
         public INamedTypeSymbol? ISet_Unbound { get; }
         public INamedTypeSymbol? ISet { get; }
         public INamedTypeSymbol? List { get; }
+        public INamedTypeSymbol Enum { get; }
+        public INamedTypeSymbol? ArgumentNullException { get; }
 
         public KnownTypeSymbols(CSharpCompilation compilation)
         {
@@ -113,6 +115,12 @@ public KnownTypeSymbols(CSharpCompilation compilation)
             IReadOnlyList_Unbound = compilation.GetBestTypeByMetadataName(typeof(IReadOnlyList<>))?.ConstructUnboundGenericType();
             IReadOnlySet_Unbound = compilation.GetBestTypeByMetadataName("System.Collections.Generic.IReadOnlySet`1")?.ConstructUnboundGenericType();
             ISet_Unbound = ISet?.ConstructUnboundGenericType();
+
+            // needed to be able to know if a member exist inside the compilation unit
+            Enum = compilation.GetSpecialType(SpecialType.System_Enum);
+            ArgumentNullException = compilation.GetBestTypeByMetadataName(typeof(ArgumentNullException));
+
+            String = compilation.GetBestTypeByMetadataName(typeof(string));
         }
     }
 }
diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/SourceGenerationSpec.cs
@@ -10,5 +10,8 @@ public sealed record SourceGenerationSpec
         public required InterceptorInfo InterceptorInfo { get; init; }
         public required BindingHelperInfo BindingHelperInfo { get; init; }
         public required ImmutableEquatableArray<TypeSpec> ConfigTypes { get; init; }
+        public required bool EmitEnumParseMethod { get; set; }
+        public required bool EmitGenericParseEnum { get; set; }
+        public required bool EmitThrowIfNullMethod { get; set; }
     }
 }
diff --git a/...Tests/Baselines/Collections.generated.txt → ...aselines/net462/Collections.generated.txt b/...Tests/Baselines/Collections.generated.txt → ...aselines/net462/Collections.generated.txt
diff --git a/...es/ConfigurationBinder/Bind.generated.txt → ...62/ConfigurationBinder/Bind.generated.txt b/...es/ConfigurationBinder/Bind.generated.txt → ...62/ConfigurationBinder/Bind.generated.txt
diff --git a/...urationBinder/Bind_Instance.generated.txt → ...urationBinder/Bind_Instance.generated.txt b/...urationBinder/Bind_Instance.generated.txt → ...urationBinder/Bind_Instance.generated.txt
diff --git a/...Bind_Instance_BinderOptions.generated.txt → ...Bind_Instance_BinderOptions.generated.txt b/...Bind_Instance_BinderOptions.generated.txt → ...Bind_Instance_BinderOptions.generated.txt
diff --git a/...ionBinder/Bind_Key_Instance.generated.txt → ...ionBinder/Bind_Key_Instance.generated.txt b/...ionBinder/Bind_Key_Instance.generated.txt → ...ionBinder/Bind_Key_Instance.generated.txt
diff --git a/...nd_ParseTypeFromMethodParam.generated.txt → ...nd_ParseTypeFromMethodParam.generated.txt b/...nd_ParseTypeFromMethodParam.generated.txt → ...nd_ParseTypeFromMethodParam.generated.txt
diff --git a/...nes/ConfigurationBinder/Get.generated.txt → ...462/ConfigurationBinder/Get.generated.txt b/...nes/ConfigurationBinder/Get.generated.txt → ...462/ConfigurationBinder/Get.generated.txt
diff --git a/...onfigurationBinder/GetValue.generated.txt → ...onfigurationBinder/GetValue.generated.txt b/...onfigurationBinder/GetValue.generated.txt → ...onfigurationBinder/GetValue.generated.txt
diff --git a/...rationBinder/GetValue_T_Key.generated.txt → ...rationBinder/GetValue_T_Key.generated.txt b/...rationBinder/GetValue_T_Key.generated.txt → ...rationBinder/GetValue_T_Key.generated.txt
diff --git a/...GetValue_T_Key_DefaultValue.generated.txt → ...GetValue_T_Key_DefaultValue.generated.txt b/...GetValue_T_Key_DefaultValue.generated.txt → ...GetValue_T_Key_DefaultValue.generated.txt
diff --git a/...nBinder/GetValue_TypeOf_Key.generated.txt → ...nBinder/GetValue_TypeOf_Key.generated.txt b/...nBinder/GetValue_TypeOf_Key.generated.txt → ...nBinder/GetValue_TypeOf_Key.generated.txt
diff --git a/...lue_TypeOf_Key_DefaultValue.generated.txt → ...lue_TypeOf_Key_DefaultValue.generated.txt b/...lue_TypeOf_Key_DefaultValue.generated.txt → ...lue_TypeOf_Key_DefaultValue.generated.txt
diff --git a/...onBinder/Get_PrimitivesOnly.generated.txt → ...onBinder/Get_PrimitivesOnly.generated.txt b/...onBinder/Get_PrimitivesOnly.generated.txt → ...onBinder/Get_PrimitivesOnly.generated.txt
diff --git a/...s/ConfigurationBinder/Get_T.generated.txt → ...2/ConfigurationBinder/Get_T.generated.txt b/...s/ConfigurationBinder/Get_T.generated.txt → ...2/ConfigurationBinder/Get_T.generated.txt
diff --git a/...nBinder/Get_T_BinderOptions.generated.txt → ...nBinder/Get_T_BinderOptions.generated.txt b/...nBinder/Get_T_BinderOptions.generated.txt → ...nBinder/Get_T_BinderOptions.generated.txt
diff --git a/...figurationBinder/Get_TypeOf.generated.txt → ...figurationBinder/Get_TypeOf.generated.txt b/...figurationBinder/Get_TypeOf.generated.txt → ...figurationBinder/Get_TypeOf.generated.txt
diff --git a/...er/Get_TypeOf_BinderOptions.generated.txt → ...er/Get_TypeOf_BinderOptions.generated.txt b/...er/Get_TypeOf_BinderOptions.generated.txt → ...er/Get_TypeOf_BinderOptions.generated.txt
diff --git a/...s/Baselines/EmptyConfigType.generated.txt → ...ines/net462/EmptyConfigType.generated.txt b/...s/Baselines/EmptyConfigType.generated.txt → ...ines/net462/EmptyConfigType.generated.txt
diff --git a/...nsBuilder/BindConfiguration.generated.txt → ...nsBuilder/BindConfiguration.generated.txt b/...nsBuilder/BindConfiguration.generated.txt → ...nsBuilder/BindConfiguration.generated.txt
diff --git a/...lines/OptionsBuilder/Bind_T.generated.txt → ...et462/OptionsBuilder/Bind_T.generated.txt b/...lines/OptionsBuilder/Bind_T.generated.txt → ...et462/OptionsBuilder/Bind_T.generated.txt
diff --git a/...uilder/Bind_T_BinderOptions.generated.txt → ...uilder/Bind_T_BinderOptions.generated.txt b/...uilder/Bind_T_BinderOptions.generated.txt → ...uilder/Bind_T_BinderOptions.generated.txt