Consider `IParsable`

[KathleenDollard]

ArgumentConverter.StringConverters defines a dictionary of delegates that perform parsing for specific types. The dictionary approach allows new types to be included, and alternative mechanisms to parsing, such as calling a constructor with a single string parameter.

It also requires maintenance of the types in the list.

IParseable<T> is provided for a long list of types in the .NET libraries, and using it would be an awesome way to provide support for many types. However, it is not clear that we can determine use it - at least I do not have my head around how we can manipulate the generic without reflection.

If it is possible to use it, we would need to fall back to not using it in .NET Standard 2.0.

[KalleOlaviNiemitalo]

how we can manipulate the generic without reflection

Related feature requests https://github.com/dotnet/csharplang/discussions/905, https://github.com/dotnet/csharplang/discussions/6308

[KathleenDollard]

I have done some more work on this. The things that define T are Option<T> and Argument<T>. If they create a ValueResult<T>, then we get a typed ValueResult. ValueResult<T> would then be responsible for the type conversion, either through IParseable or the type conversion dictionary. The problem here is this code:

public class ValueResult<T> : ValueResult
{
    internal ValueResult(
        CliSymbol valueSymbol,
        string rawValue,
        IEnumerable<Location> locations,
        ValueResultOutcome outcome)
        : base(valueSymbol, locations, outcome, error)
    {
        RawValue = rawValue;
        var conversionDone = false;
#if NET7_0_OR_GREATER
        if (Value is IParsable<T> parseable)
        {

This code is IParsable<T> gives an error on <T> that it is not an IParsable<T> because of the self-constraint.

If someone can solve this, I would love to hear about it.

If we can not treat a T as IParsable, even when it is

I believe support for IParsable is very important as we move to the .NET Libraries because it gives us automatic support for future types, and supports people writing custom types that are parsable. It will also be faster and more consistent with many less allocations.

This is a touch ugly, but it is one way we can get the genric available in the right place (air code):

public record TypeConverter<T>(Func<string, T> Converter) // used in next sample
{}

public class ValueResult
{
  internal string RawValue{ get; }
  // All ValueResult functionality accept storing the value
  public abstract TRequested GetValue<TRequested>(CliArgument argument);  // similar for option
}

public class ValueResult<T> : ValueResult
#if NET7_0_OR_GREATER
   where T : IParsable<T>
#endif
{
  private T Value<T> { get; internal set; }
  public override TRequested GetValue<TRequested>()
  {
    if (!typeof(TRequested).IsAssignableFrom(typeof(T))
    { throw new ...}
#if NET7_0_OR_GREATER
    if (T.TryParse(RawValue, out var retValue)
    { return (TRequested)retValue; } // This cast may be problematic, will work if boxed to object
    else
    { throw new ... }
#else
    if (TypeConversionDictionary.TryParse(symbol, out var conversion)
    { return (TRequested)conversion(RawValue); }
    else
    { throw new ... }
#endif
  }
  // Rest of class
}

public abstract class CliTypeConverter<T>
{
   public static Func<string,  out T, bool> Converter{ get; }

   // forcer converter to be set in derived classes. Avoids allocations, but might be 
   // surprising if multiple derived classes set different values
   protected CliTypeConverter(Func<string, T> converter)
      => Converter = converter;
}

public class ValueResult<T, TConverter> :  ValueResult<T>
  where TConverter is CliTypeConverter<T>
{
  private T Value<T> { get; internal set; }
  public override TRequested GetValue<TRequested>()
  {
    if (!typeof(TRequested).IsAssignableFrom(typeof(T))
    { throw new ...}
    return TConverter.TryParse(RawValue, out var retValue)
         ? (TRequested)retValue
         :  throw new...
  }
  // Rest of class
}

This works fine, except that it also needs to accommodate a .NET 7+ type that is not IParsable. This might be OK if a future happens where we can add interfaces via extensions, but at least for .NET 7-9 we need a solution, and I think it needs to be a type differentiated from the ValueResult, probably a ValueResult<T, TConverter>. That will leak back to the declaration:

// int is IParsable<int> in .NET 7+
// MyType is not IParsable<MyType>

var arg1 = new CliArgument<int>("-x");
var arg2 = new CliArgument<MyType>("-y"); // Succeeds below .NET 7 if there is a converter in the dictionary, otherwise fails at runtime
var arg3 = new CliArgument<MyType, MyTypeConverter>("-z"); // Succeeds, uses local converter
var arg4 = var arg1 = new CliArgument<int, MyCustomIntConverter>("-x"); // Figure out which converter to use

We could probably do almost the same thing with factory methods, but I wanted to make it easy to compare with what we do today. Since factory methods for symbols were rejected earlier in the project, I do not want to reopen that here, although I think it could save us two types.

I certainly may be missing something in this, but if we are to type values, it has to come from the generic symbol, as that is the thing that knows the type, until the user requests it on retrieval. That means ValueResult and anything requiring the type like a converter must come from the symbol.

[KathleenDollard]

On further consideration, I like the .NET 6 plus design of requiring either IParsable or a type converter as a type parameter. It leans into IParsable, which I think is a good decision. Where that does not work, we still have a guarantee of having a converter.

Down-level and multi-targeting might be a bit messier than needed, but it would provide a good upgrade story.