Roslyn: IDE0071 may change type of objects passed to FormattableString

[stephentoub]

Version Used:
3.8.0-4.20464.1+56f747b362e15a0763bad5ce4702a9b7c8949d7e

Steps to Reproduce:
```C#
using System;

class Program
{
static void Main(string[] args) => M($"{args.Length.ToString()}");

static void M(FormattableString fs)
{
    foreach (object o in fs.GetArguments())
        Console.WriteLine(o?.GetType());
}

}
```

Expected Behavior:
IDE0071 does not fire (or a distinct diagnostic ID should be used for targeting FormattableString instead of string, or something like that)

Actual Behavior:
IDE0071 fires, and applying the fix (removing the .ToString()) ends up changing the program's behavior, outputting "System.Int32" instead of "System.String". This is relevant when a FormattableString is being used by some utility that processes the objects in some way, based on type, e.g. in https://github.com/dotnet/runtime/blob/b39c698c628079b464650cda2540653533a84342/src/libraries/Common/src/System/Net/Logging/NetEventSource.Common.cs#L388-L392 where FormattableString is being used by logging and exerts customized formatting behavior based on argument type... in which case using ToString() in the interpolation is a way for the caller to opt-out of that policy.

[CyrusNajmabadi]

Yes, i believe this was a known potential subtlke change that was possible, but was felt to be too niche to block the feature on. i.e. if the code depends on that .ToString being called for correctness, that seems like a big code smell. So we went by our bar that such code would not be representative and the suggestion was more for the common and normal coding patterns where there wouldn't be a perceivable difference between .ToString() being there or not.

[stephentoub]

That makes sense to me when the target is String. Makes less sense to me when the target is FormattableString, where the whole point is to be given raw access to the objects used to fill the holes so that the code processing the FormattableString can make its own decisions (if there's only one right way to format these, then there's no reason for FormattableString to exist).

[CyrusNajmabadi]

Note: the linked code is a good example here. In this case, the .ToString exists to not get the IntPtr printing behavior. If that's something important, then that's a good example of code that should have comments/suppression making it explicit that that's the goal here.

[stephentoub]

a good example of code that should have comments/suppression

That requires someone to examine every single change being made by a solution-wide fix, which negates the benefit. On top of that, the apply-all UI doesn't let you edit the changes being applied, only turn them individually on and off, which means in such an experience you can't even add such a comment or suppression at the time you're choosing not to apply it. It's not a great experience.

[CyrusNajmabadi]

Makes less sense to me when the target is FormattableString, where the whole point is to be given raw access to the objects used to fill the holes so that the code processing the FormattableString can make its own decisions

We thought about that, but still felt that an explicit .ToString to sidestep built in formattable behavior was a smell and not common. As such, a more appropriate solution in that case would be to suppress the suggestion (or just not apply it).

i.e. in the vast majority of cases it would be something someone would want to remove, and in the rare case it wasn't, that would be acceptable.

[CyrusNajmabadi]

That requires someone to examine every single change being made by a solution-wide fix, which negates the benefit.

Yes. That's our bar. We do not and have never had the philosophy that we will not change semantics. We take an assessment on what are the coding patterns we will thing are expected and normal and err toward that. AS i've mentioned in most issues around this, an audit of our features showed that we would effectively not be able to ever support fix all if we needed to maintain exact observable behavior (including runtime behavior like this).

It's a balance between being generally useful the majority of the time, with the caveat that there may be rare cases that might be a problem.

It's not a great experience.

We can ask teh platform team to update the 'fix all ui' to support more flexibility here.

[stephentoub]

We thought about that, but still felt that an explicit .ToString to sidestep built in formattable behavior was a smell and not common.

I disagree. It could be there to opt-out of the formatting applied by the call site, it could be there to change the allocation behavior, etc. As for suppressing the suggestion, that's even more of a smell to me, having to litter a code base with pragma warning disable and pragma warning restore.

It's a balance between being generally useful the majority of the time, with the caveat that there may be rare cases that might be a problem.

Then why not simply have a different diagnostic ID, so that someone can choose to blanketly apply it when the target is string and not when it's FormattableString? Let the dev decide rather than saying that they must be treated the same or else the dev is doing something wrong. The scenarios for string and the scenarios for FormattableString are different.

[CyrusNajmabadi]

I disagree. It could be there to opt-out of the formatting applied by the call site, it could be there to change the allocation behavior, etc. As for suppressing the suggestion, that's even more of a smell to me, having to litter a code base with pragma warning disable and pragma warning restore.

Then i would turn off this suggestion entirely. It exists precisely for the use case of people for whom this sort of extra .ToString is likely just redundant an unnecessary. If your coding practices make it more important that that .ToString must stay there, then i would disable this.

Then why not simply have a different diagnostic ID, so that someone can choose to blanketly apply it when the target is string and not when it's FormattableString? Let the dev decide rather than saying that they must be treated the same or else the dev is doing something wrong.

Primarily because it would be hard to reign in control over that. What is the total set of diagnostic IDs we need per feature? Would we need a diagnostic ID if this migth have an allocation impact? If it might have a CPU impact? If it might have a runtime behavior impact? etc. etc.

We'd also then need multiple ways to present this in the UI: i.e.

1. prefer removing ToString when totally safe to do. (note: this is also something hard to even validate in the first place)
2. prefer removing ToString even if there is a chance it might change semantics.

We'd need that sort of split (maybe going even further) with all our features. And, as mentioned above, nearly all features would fall into '2' anyways. So, instead of that, if your position is that any sort of runtime change isn't really suitable for you, then i would suggest literally not using the suggestions. The bar you have, and the bar we provide aren't in line with each other.

[stephentoub]

I don't understand the response. I'm not suggesting a separate ID for when it might allocate or not. I'm suggesting a separate ID based on the target, whether it's String or FormattableString. They represent different scenarios. There is a clear split between them.

[CyrusNajmabadi]

I'm suggesting a separate ID based on the target, whether it's String or FormattableString. They represent different scenarios. There is a clear split between them.

Fair, we can look into that as a particular fix for exactly this issue. However, as per above i'm wary about this as it would no mean extra user options for people to have to know about, extra UI to enable and distinguish these, etc. I'm not sure this case warrants it, but we can take to the team to decide.

[sharwell]

Given that FormattableString is an edge case usage, I would be fine with simply excluding instances of that type from analysis altogether. We could create a separate diagnostic for it, but it's just not common enough to matter in practice.

[stephentoub]

That's reasonable as well.