Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 22 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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; }

Expand All @@ -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()
};
}

Expand Down Expand Up @@ -842,6 +848,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);
tarekgh marked this conversation as resolved.
Show resolved Hide resolved
return;
}
}
}

private bool IsThrowIfNullMethodToBeEmitted()
{
if (_typeSymbols.ArgumentNullException is not null)
{
var throwIfNullMethods = _typeSymbols.ArgumentNullException.GetMembers("ThrowIfNull");
tarekgh marked this conversation as resolved.
Show resolved Hide resolved

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.Equals(_typeSymbols.Object, SymbolEqualityComparer.Default)
tarekgh marked this conversation as resolved.
Show resolved Hide resolved
&& secondParam.Name == "paramName" && secondParam.Type.Equals(_typeSymbols.String, SymbolEqualityComparer.Default))
{
return true;
}
}
}
}

return false;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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}});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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}})
{
Expand All @@ -614,8 +607,6 @@ private void EmitPrimitiveParseMethod(ParsableFromStringSpec type)

switch (typeKind)
{
case StringParsableTypeKind.Enum:
return;
case StringParsableTypeKind.ByteArray:
{
parsedValueExpr = $"Convert.FromBase64String({Identifier.value})";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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});"
tarekgh marked this conversation as resolved.
Show resolved Hide resolved
: $$"""
if ({{paramName}} is null)
{
throw new ArgumentNullException(nameof({{paramName}}));
}
""";

_writer.WriteLine(throwIfNullExpr);
}

_writer.WriteLine();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}});
Expand All @@ -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}});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}}));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ 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 INamedTypeSymbol Object { get; }

public KnownTypeSymbols(CSharpCompilation compilation)
{
Expand Down Expand Up @@ -113,6 +116,13 @@ 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));

Object = compilation.GetBestTypeByMetadataName(typeof(object));
String = compilation.GetBestTypeByMetadataName(typeof(string));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
}
}
Loading