Implement proper parsing of primitives in config binder generator

[layomia]

from @eerhardt in #82179 (comment) (fyi @stephentoub)

A couple things here:

Parsing:

1. In order to be consistent with the reflection based behavior, we need to catch the int.Parse exception thrown here, and wrap it in a InvalidOperationException and add the configuration path of the invalid data. This allows devs to easily know where the invalid data exists from the exception message.
2. This should be int.Parse(valueString, NumberStyles.Integer, NumberFormatInfo.InvariantInfo);

See the following for how I did it when manually writing the bind code for Console Logging:

runtime/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerOptions.cs

Lines 222 to 239 in 967250c

	internal static bool ParseInt(IConfiguration configuration, string key, out int value)
	{
	if (configuration[key] is string valueString)
	{
	try
	{
	value = int.Parse(valueString, NumberStyles.Integer, NumberFormatInfo.InvariantInfo);
	return true;
	}
	catch (Exception e)
	{
	ThrowInvalidConfigurationException(configuration, key, typeof(int), e);
	}
	}
	value = default;
	return false;
	}

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

---

from @eerhardt in #82179 (comment) (fyi @stephentoub)

A couple things here:

Parsing:

1. In order to be consistent with the reflection based behavior, we need to catch the int.Parse exception thrown here, and wrap it in a InvalidOperationException and add the configuration path of the invalid data. This allows devs to easily know where the invalid data exists from the exception message.
2. This should be int.Parse(valueString, NumberStyles.Integer, NumberFormatInfo.InvariantInfo);

See the following for how I did it when manually writing the bind code for Console Logging:

runtime/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerOptions.cs

Lines 222 to 239 in 967250c

	internal static bool ParseInt(IConfiguration configuration, string key, out int value)
	{
	if (configuration[key] is string valueString)
	{
	try
	{
	value = int.Parse(valueString, NumberStyles.Integer, NumberFormatInfo.InvariantInfo);
	return true;
	}
	catch (Exception e)
	{
	ThrowInvalidConfigurationException(configuration, key, typeof(int), e);
	}
	}
	value = default;
	return false;
	}

Author:	layomia
Assignees:	layomia
Labels:	area-Extensions-Configuration, source-generator
Milestone:	8.0.0

[ericstj]

If we decide to directly use converters we should measure the size impact -- maybe directly referencing a converter is less expensive then GetCoverter (which I think @eerhardt said is huge), but it still looks pretty big. Maybe we can just look to the converters for inspiration of which parse/tryparse signatures to call, then generate those?

[tarekgh]

I think the idea is not using the converters at all and just having the parse code embedded inside the generated code. no dependency and not calling type converter.

[eerhardt]

For built-in types like int, bool, and enum, I don't see a benefit pulling TypeConverter into the app. Using the parsing logic directly makes sense to me.

See also #44493 (comment) about TypeConverter extensibility being opt-in.

The one use of config binding in Console Logging shouldn't pull in TypeConverter at all. See #81931 for why and numbers.

[layomia]

I think the idea is not using the converters at all and just having the parse code embedded inside the generated code. no dependency and not calling type converter.

Yeah this is what I meant.

[eerhardt]

One option for the "opt in" model of TypeConverters is to respect the TypeConverterAttribute if either:

• Directly applied to the property
• Directly applied to the Type of the property

So, for example:

[TypeConverter(typeof(PointConverter))]
public struct Point
{
   public int X;
   public int Y;
}

public class CustomOptions
{
    public Point CurrentPoint { get; set }
}

or

public struct Point
{
   public int X;
   public int Y;
}

public class CustomOptions
{
    [TypeConverter(typeof(PointConverter))]
    public Point CurrentPoint { get; set }
}

See also:

• Developers can safely trim apps which need Configuration Binder #44493 (comment)
• Support TypeConverterAttribute on bound properties #36545

[layomia]

@eerhardt I opened #83599 to discuss this.

[layomia]

@tarekgh @eerhardt I'm using the list of "intrinsic" types specified in the core TypeConverter assembly to determine which primitives need support -

runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs

Lines 162 to 198 in 967250c

	// Add the intrinsics
	//
	// When modifying this list, be sure to update the initial dictionary capacity above
	[typeof(bool)] = new IntrinsicTypeConverterData((type) => new BooleanConverter()),
	[typeof(byte)] = new IntrinsicTypeConverterData((type) => new ByteConverter()),
	[typeof(sbyte)] = new IntrinsicTypeConverterData((type) => new SByteConverter()),
	[typeof(char)] = new IntrinsicTypeConverterData((type) => new CharConverter()),
	[typeof(double)] = new IntrinsicTypeConverterData((type) => new DoubleConverter()),
	[typeof(string)] = new IntrinsicTypeConverterData((type) => new StringConverter()),
	[typeof(int)] = new IntrinsicTypeConverterData((type) => new Int32Converter()),
	[typeof(Int128)] = new IntrinsicTypeConverterData((type) => new Int128Converter()),
	[typeof(short)] = new IntrinsicTypeConverterData((type) => new Int16Converter()),
	[typeof(long)] = new IntrinsicTypeConverterData((type) => new Int64Converter()),
	[typeof(float)] = new IntrinsicTypeConverterData((type) => new SingleConverter()),
	[typeof(Half)] = new IntrinsicTypeConverterData((type) => new HalfConverter()),
	[typeof(UInt128)] = new IntrinsicTypeConverterData((type) => new UInt128Converter()),
	[typeof(ushort)] = new IntrinsicTypeConverterData((type) => new UInt16Converter()),
	[typeof(uint)] = new IntrinsicTypeConverterData((type) => new UInt32Converter()),
	[typeof(ulong)] = new IntrinsicTypeConverterData((type) => new UInt64Converter()),
	[typeof(object)] = new IntrinsicTypeConverterData((type) => new TypeConverter()),
	[typeof(CultureInfo)] = new IntrinsicTypeConverterData((type) => new CultureInfoConverter()),
	[typeof(DateOnly)] = new IntrinsicTypeConverterData((type) => new DateOnlyConverter()),
	[typeof(DateTime)] = new IntrinsicTypeConverterData((type) => new DateTimeConverter()),
	[typeof(DateTimeOffset)] = new IntrinsicTypeConverterData((type) => new DateTimeOffsetConverter()),
	[typeof(decimal)] = new IntrinsicTypeConverterData((type) => new DecimalConverter()),
	[typeof(TimeOnly)] = new IntrinsicTypeConverterData((type) => new TimeOnlyConverter()),
	[typeof(TimeSpan)] = new IntrinsicTypeConverterData((type) => new TimeSpanConverter()),
	[typeof(Guid)] = new IntrinsicTypeConverterData((type) => new GuidConverter()),
	[typeof(Uri)] = new IntrinsicTypeConverterData((type) => new UriTypeConverter()),
	[typeof(Version)] = new IntrinsicTypeConverterData((type) => new VersionConverter()),
	// Special cases for things that are not bound to a specific type
	//
	[typeof(Array)] = new IntrinsicTypeConverterData((type) => new ArrayConverter()),
	[typeof(ICollection)] = new IntrinsicTypeConverterData((type) => new CollectionConverter()),
	[typeof(Enum)] = new IntrinsicTypeConverterData((type) => CreateEnumConverter(type), cacheConverterInstance: false),
	[s_intrinsicNullableKey] = new IntrinsicTypeConverterData((type) => CreateNullableConverter(type), cacheConverterInstance: false),
	[s_intrinsicReferenceKey] = new IntrinsicTypeConverterData((type) => new ReferenceConverter(type), cacheConverterInstance: false),

Are there any other strategies for picking these types that come to mind?

[tarekgh]

I think the collected list of primitives is good enough and we can watch if we get more cases we need to support.

[eerhardt]

Looking at:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 838 to 873 in bd6ed47

	error = null;
	result = null;
	if (type == typeof(object))
	{
	result = value;
	return true;
	}
	if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>))
	{
	if (string.IsNullOrEmpty(value))
	{
	return true;
	}
	return TryConvertValue(Nullable.GetUnderlyingType(type)!, value, path, out result, out error);
	}
	TypeConverter converter = TypeDescriptor.GetConverter(type);
	if (converter.CanConvertFrom(typeof(string)))
	{
	try
	{
	result = converter.ConvertFromInvariantString(value);
	}
	catch (Exception ex)
	{
	error = new InvalidOperationException(SR.Format(SR.Error_FailedBinding, path, type), ex);
	}
	return true;
	}
	if (type == typeof(byte[]))
	{
	try
	{
	result = Convert.FromBase64String(value);

I think starting with those built-in Types makes a lot of sense, as those will be the types that can be converted out of the box with TypeDescriptor.GetConverter(type). Note you will only need the Types that have a TypeConverter that CanConvertFrom(typeof(string)) == true.

1. Looks like Nullable<> should be tested to ensure it works correctly.
2. The last Type to consider would be byte[], since it is built directly into ConfigurationBinder.

[layomia]

Looks like Nullable<> should be tested to ensure it works correctly.
The last Type to consider would be byte[], since it is built directly into ConfigurationBinder.

Yup we already have tests for these two; will just doublecheck that the implementation lines up with reflection in all cases.