Microsoft.CodeAnalysis.ResxSourceGenerator Format methods risk collisions, and could read better

[AArnott]

Analyzer

Microsoft.CodeAnalysis.ResxSourceGenerator

Describe the improvement

When Microsoft.CodeAnalysis.ResxSourceGenerator generates Format methods (because EmitFormatMethods="true" metadata is set to true), the Format methods are mixed into the same class as the string properties. As a result, they have to have Format prefixed to them since C# doesn't let a class have a property and a method with the same name. This is a bit weird.
This also creates possibility of collisions with another property that already has the same name but with Format in front.

What if we solve both problems by moving the Format methods into a nested class?

For example, change this:

/// <summary>'{0}' cannot be an empty guid.</summary>
internal static string? @Argument_EmptyGuid => GetResourceString("Argument_EmptyGuid");

/// <summary>'{0}' cannot be an empty guid.</summary>
internal static string FormatArgument_EmptyGuid(object p0)
   => string.Format(Culture, GetResourceString("Argument_EmptyGuid") ?? "", p0);

To this:

/// <summary>'{0}' cannot be an empty guid.</summary>
internal static string? @Argument_EmptyGuid => GetResourceString("Argument_EmptyGuid");

internal static partial class Format
{
    /// <summary>'{0}' cannot be an empty guid.</summary>
    internal static string Argument_EmptyGuid(object p0)
       => string.Format(Culture, GetResourceString("Argument_EmptyGuid") ?? "", p0);
}

Then uses change like this:

-Strings.FormatArgument_EmptyGuid("arg");
+Strings.Format.Argument_EmptyGuid("arg");

[sharwell]

💭 What if it just always emitted methods instead of properties when EmitFormatMethods is set to true and the string contains one or more placeholders. The property would be emitted as a method without parameters (just add () to the existing code), and the format methods would not have the Format prefix.

[AArnott]

I thought of that. I think that would be ideal for most cases. It would likely avoid a lot of bugs where folks use the string without realizing it needs formatting args.

But it would also make the string impossible to consume when you explicitly don't want to format it right away. For example when calling StringBuilder.AppendFormat, or when you want to defer or avoid formatting when using TraceSource.TraceEvent if there are no listeners attached.

I'd love to capture the best of both worlds, somehow. Maybe the strings with formatting args could be moved into two nested subclasses: Format and Unformatted.

[sharwell]

But it would also make the string impossible to consume when you explicitly don't want to format it right away.

You'd just call the parameterless overload.

[sharwell]

So here is one option:

/// <summary>'{0}' cannot be an empty guid.</summary>
internal static string Argument_EmptyGuid();

/// <summary>'{0}' cannot be an empty guid.</summary>
internal static string Argument_EmptyGuid(object? p0);

[AArnott]

True. The requirement to add () to what used to be a property doesn't seem like a bad thing, as it shows intention to not include formatting args.