Roslyn: Suggestion to replace ToString at end of interpolation expression with colon and format code

[jnm2]

I see this now and then:

_ = $"...{foo.ToString("bar")}...";

The .ToString(" and ") spans (or maybe just the ToString token?) should be faded and a code fix should be available inside .ToString("bar") to convert to:

_ = $"...{foo:bar}...";

The degenerate case .ToString() could also have the same treatment with a code fix to entirely remove.

[jnm2]

In a similar vein, PadLeft and PadRight could be replaced using the alignment component:

_ = $"...{foo.ToString("bar").PadRight(3)}...";
_ = $"...{foo2.PadLeft(4, ' ')}...";

⬇

_ = $"...{foo,3:bar}...";
_ = $"...{foo2,-4}...";

[CyrusNajmabadi]

Looking into this.

[0xd4d]

This code fix will box value types. The reason ToString() was used could've been to avoid boxing it.

[sharwell]

This code fix will box value types. The reason ToString() was used could've been to avoid boxing it.

@dotnet/roslyn-compiler is there any reason the compiler cannot optimize this case (string interpolation to string, as opposed to IFormattable) as was done for concatenation?

[jaredpar]

@sharwell

Overall better efficiency around string interpolation is a feature we're looking at for C# 9.0.

The comment chain is a bit ambiguous but I assume the ask is to implicitly call ToString on value types before passing them into the string formatting functions for interpolated strings? If so then the reason why is:

1. Compat: it's observably to move the ToString calls before the boxing operation
2. Makes assumptions about how interpolation works which the compiler in general tries not to do.

That is the reason we haven't made any changes up through C# 8.0. Going into C# 9 though better performance here, at the possible expense of corner case compat changes, is a goal.

[sharwell]

@jaredpar Thanks! Based on this, I'm going to push for the readability of {value} outweighing the performance advantage of {value.ToString()} for value types that either don't implement IFormattable or do so in a way that is known to ignore the culture.

[martinstenhoff]

In a similar vein, PadLeft and PadRight could be replaced using the alignment component:

_ = $"...{foo.ToString("bar").PadRight(3)}...";
_ = $"...{foo2.PadLeft(4, ' ')}...";

⬇

_ = $"...{foo,3:bar}...";
_ = $"...{foo2,-4}...";

@jnm2 @CyrusNajmabadi @sharwell

Hasn't the PadLeft/PadRight to alignment component fix been implemented to do the reverse here?
A PadLeft becomes a PadRight when simplifying to alignment component and vice versa?

Console.WriteLine($"|{"Bug".PadLeft(10)}|");
Console.WriteLine($"|{"Bug",-10}|");

=>

|       Bug|
|Bug       |

[jnm2]

@martinstenhoff Thanks for checking on this. That's a bit embarrassing! I'm especially happy that you caught this before it got further than a preview. We read this but then accidentally equated 'left-aligned' with PadLeft, when really PadLeft is right-aligning and PadRight is left-aligning:

The formatted data in the field is right-aligned if alignment is positive and left-aligned if alignment is negative.

https://docs.microsoft.com/en-us/dotnet/standard/base-types/composite-formatting#alignment-component

Is this something you'd be willing to submit a PR to fix? I think I'd do it by fixing this line:

https://github.com/dotnet/roslyn/blob/508ed762c35e83f316fefed4e0c6f56a438eda60/src/Features/Core/Portable/SimplifyInterpolation/Helpers.cs#L134

and then fixing up the tests that begin failing in https://github.com/dotnet/roslyn/blob/master/src/EditorFeatures/CSharpTest/SimplifyInterpolation/SimplifyInterpolationTests.cs.

[jnm2]

I have some time here, so I think I'll get a PR up.