C#: Implement IFormattable for Variant structs

[Repiteo]

Implements the IFormattable interface to the Variant structs (more specifically, the structs that already had an associated ToString() function). This will allow these structs to return strings from a different culture and/or strings with an invariant culture. Because these are two new functions (one with just an IFormatProvider argument) that would share syntax with the existing two functions (just one for Rid), the code has been simplified by having everything direct to the IFormattable function

[raulsntos]

I've been meaning to implement ISpanFormattable in these types, but this is a good first step. For .NET 8.0 we should also implement IUtf8SpanFormattable.

The IFormattable interface has a single method with the signature:

string ToString(string? format, IFormatProvider? formatProvider)

So I don't know why you added an overload that only takes IFormatProvider. From what I can see, the numerics BCL types don't have this overload. See System.Numerics.Vector3.

[raulsntos]

When implementing IFormattable we should likely use the number separator from the IFormatProvider (and not a comma). See System.Numerics.Vector3.

[Repiteo]

Woah, I had no clue that even existed!

[Repiteo]

Actually, looking more into it, I disagree with this implementation. The documentation of NumberGroupSeparator specifies it's for digits to the left of the decimal point. That doesn't feel like it's applicable in this context, as the comma acts as a separator in a list format. I have it changed on the repo for now, but I'm considering a revert

[raulsntos]

It's what System.Numerics vectors use for separation, so we'll want to be consistent if we are going to be supporting cultures; otherwise, we shouldn't implement IFormattable in these types.

There was a similar discussion in the .NET repository about the Vector64/128/256 types. The conclusion was that these types didn't need to implement IFormattable.

• Incorrect culture-specific formatting for Vector64/128/256 types dotnet/runtime#11604
• Make Vector64/128/256<T>.ToString() Culture invariant dotnet/coreclr#25343

The current Vector256<T>.ToString implementation actually uses NumberGroupSeparator internally, but since it doesn't implement IFormattable, it always uses the Invariant Culture (which uses comma: ,).

[Repiteo]

Though we will be supporting cultures by nature of the actual values being formatted by them. The discussions even seemed to agree that NumberGroupSeparator wasn't the best choice for the seperation of a list of elements, before ultimately falling back to not bothering with formatting in the first place. The takeaway I got was that they would've opted for hardcoded commas, much like the hardcoded wrappers (<, >), if they hadn't gone with the overall invariant style. That, and the Vector types in System.Numerics look like the only ones that implemented IFormattable, so I don't believe they should be taken as something to copy in this instance

[raulsntos]

What I mean is that if we use a hardcoded separator (a comma), that's not supporting cultures. Not every culture uses commas to separate numbers. Vector64/128/256 don't implement IFormattable because those types don't support cultures, their ToString implementation always uses the Invariant Culture.

I think you may be suggesting implementing IFormattable using comma as a separator and only using the IFormatProvider for the numbers, but that's mixing cultures and in my opinion it's more confusing. For example the german culture uses comma for the decimal separator so a Vector2 would look like (4,2, 4,2) with commas everywhere, that doesn't look right to me. I also don't know if parenthesis is the right thing to use for every culture.

So, the more I think about it the more I think these types shouldn't implement IFormattable. We may want to wait until Vector64/128/256 implement it to see what they do there. It seems there are plans to implement the interface at some point (see dotnet/runtime#90764 (comment)).

[Repiteo]

That is largely what I'm saying, yes. And thats because, while I understand that other cultures might use different separators for numbers, I'm not convinced that NumberGroupSeparator is the way to go about it. They specify a different separator for currency for instance, and this separator is specified for splitting large numbers to the left of the decimal place. NumberGroupSeparator, as far as I can tell, doesn't specify if it should be used as a separator for different batches of numbers; just that it separates the portions of a single, large number

I'm fine waiting out and seeing how these other types are implemented, but I wouldn't want to discard IFormattable as a whole, especially if ISpanFormattable is planned down the road. Something that could be considered as well is Unity's implementation of Vector3, which I would imagine many newer Godot C# users would expect. Failing that, we could always just... Add formatting options? Haven't messed with that much before, but I know there's some format string shenanigans in Godot already, so it wouldn't be totally unprecedented

[raulsntos]

I agree with you that NumberGroupSeparator may not be the right separator, but it would at least be consistent with the BCL types, so that's why I suggested it.

If we end up deciding that IFormattable shouldn't be implemented for these types, then neither should ISpanFormattable because that's just the Span version of the same interface (the version 2.0). But I'm not really saying we should discard it altogether, I just think we should wait and see what the BCL does and follow their lead.

I don't think Unity is a good example because their APIs don't usually follow normal .NET conventions. Also, I don't think the repository you linked is open source so I'll refrain from looking at that code.

I don't know what you mean by formatting options and format string shenanigans. But it sounds like it would grow the scope of just implementing IFormatting and that would need a proposal and some justification.

[Repiteo]

Hmm, it's true that would expand the scope of IFormattable, and consequently the scope of this PR. I'm beginning to see why you've become hesitant. A proposal is probably be warranted for knowing what direction would be preferable

In any case, it'll likely be well worth implementing some sort of testing system into #82955 to output different formatting styles across various IFormattable operations. Actually seeing firsthand what kinds of outputs could happen with different formatting options would probably go a long way towards making sure the implementations make sense & aren't off-base in some way

[Repiteo]

Actually, I think we've got something that we could use here: CultureInfo.TextData.ListSeparator!

The idea came to me while making a csv file (CultureInfo.csv) to display various values supported by CultureInfo against different cultures. ListSeparator was actually the value brought up as the reasonable formattable alternative in the Vector64/128/256 discussion, but I somehow didn't realize until setting this up that it was accessable from CultureInfo directly! The two values it seems to use are , and ;, with the latter being frequently paired with cultures that use a comma as their decimal point. When using ListSeparator, the above German culture Vector2 example would end up looking like (4,2; 4,2), which is much more reasonable

[Repiteo]

I added an overload only taking IFormatProvider because that's what the system types seem to do (eg: System.Single)

Also my Unity .NET Framework background is showing, I didn't realize ISpanFormattable was a thing to begin with

[Repiteo]

As long as I'm fixing up IFormattable, I may as well look into ISpanFormattable too. Hardly ever messed with spans before, but would this approach do the trick?

Original

public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider provider)
{
    try
    {
        ToString(format.ToString(), provider).AsSpan().CopyTo(destination);
        charsWritten = destination.Length;
        return true;
    }
    catch
    {
        charsWritten = 0;
        return false;
    }
}

EDIT: Scratch that, read more on the documentation & I had the right idea. The main change needed was it should only return false if the copy fails (all other exceptions throw). Here's v2:

public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format = default, IFormatProvider provider = null)
{
    ReadOnlySpan<char> span = ToString(format.ToString(), provider).AsSpan();
    if (span.TryCopyTo(destination))
    {
        charsWritten = span.Length;
        return true;
    }
    else
    {
        charsWritten = 0;
        return false;
    }
}

[raulsntos]

No, the purpose of ISpanFormattable is to avoid allocations, calling ToString would allocate a String, so it would defeat the purpose of implementing the interface. If anything, it should be the other way around, the ToString implementation should call TryFormat.

Here's an example for how TryFormat could look like for Vector3 (this is a naive and untested implementation):

public bool TryFormat(Span<char> destination, out int charsWritten, ReadOnlySpan<char> format, IFormatProvider? provider)
{
    charsWritten = 0;
    if (destination.IsEmpty)
    {
        return false;
    }

    var numberFormat = NumberFormatInfo.GetInstance(formatProvider);

    destination[charsWritten++] = '(';

    if (!TryWriteMember(destination, X, format, ref charsWritten))
    {
        return false;
    }

    if (!TryWriteSpan(destination, numberFormat.NumberGroupSeparator, ref charsWritten))
    {
        return false;
    }

    if (!TryWriteMember(destination, Y, format, ref charsWritten))
    {
        return false;
    }

    if (!TryWriteSpan(destination, numberFormat.NumberGroupSeparator, ref charsWritten))
    {
        return false;
    }

    if (!TryWriteMember(destination, Z, format, ref charsWritten))
    {
        return false;
    }

    destination[charsWritten++] = ')';

    return true;

    static bool TryWriteMember<T>(Span<char> destination, T member, ReadOnlySpan<char> format, ref int charsWritten) where T : ISpanFormattable
    {
        if (member.TryFormat(destination.Slice(charsWritten), out int memberCharsWritten, format))
        {
            charsWritten += memberCharsWritten;
            return true;
        }

        return false;
    }

    static bool TryWriteSpan(Span<char> destination, ReadOnlySpan<char> span, ref int charsWritten)
    {
        if (span.TryCopyTo(destination))
        {
            charsWritten += span.Length;
            return true;
        }

        return false;
    }
}

This is not really correct:

• I'm not checking if there's enough size in the destination Span before writing the ( and ) characters.
• I'm not writing the space after the number group separator.
• Probably has other issues too, I didn't test it.

But it was already getting quite long, I think you probably get the idea though. Implementing ISpanFormattable may be a bit more work, so it's fine to leave it for a separate PR.

[Repiteo]

I'll leave it be for now. I'd love to take a stab at it, but I'm at quite a loss as to how you'd get ToString to call TryFormat without somehow knowing the span size beforehand

[Repiteo]

Closing in favor of #89547