Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance CA1305 analyzer to cover interpolated strings #88902

Open
Alex-Sob opened this issue Jul 14, 2023 · 4 comments
Open

Enhance CA1305 analyzer to cover interpolated strings #88902

Alex-Sob opened this issue Jul 14, 2023 · 4 comments
Labels
area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@Alex-Sob
Copy link

Alex-Sob commented Jul 14, 2023

Describe the problem you are trying to solve

The existing CA1305: Specify IFormatProvider rule does no seem to cover interpolated strings. Currently compiler emits no warning if a value that can be formatted is used inside an interpolation expression:

var text = $"Date: {DateTime.Now}";

Describe suggestions on how to achieve the rule

The existing analyzer could be enhanced, so that it would report diagnostic and recommend using new string.Create(IFormatProvider, ref DefaultInterpolatedStringHandler) overloads if type of the value used in interpolation expression implements ISpanFormattable (I'm aware that it's actually used instead of calling ToString). Does that make sense? If so, I could try contributing myself as I have experience writing analyzers for Microsoft projects.

Examples

If type of the value implements IFormattable/ISpanFormattable which are types that default interpolated string handler special-cases when formatting a value, then compiler could recommend a call to string.Create with format provider supplied explicitly:

// Violation
var text = $"Date: {DateTime.Now}";

// Fix
text = string.Create(CultureInfo.CurrentCulture, $"Date: {DateTime.Now}"); // Or whatever culture is appropriate

If an interpolated string is implicitly cast to FormattableString, then string.Create would also be a more efficient alternative:

// Violation
FormattableString formattableString = $"Date: {DateTime.Now}";
var text = formattableString.ToString(CultureInfo.CurrentCulture);

// Fix
text = string.Create(CultureInfo.CurrentCulture, $"Date: {DateTime.Now}");

Or

// Violation
FormattableString formattableString = $"Date: {DateTime.Now}";
var text = FormattableString.Invariant(formattableString);

// Fix
text = string.Create(CultureInfo.InvariantCulture, $"Date: {DateTime.Now}");

In case of IFormattable, it allows passing null as a format provider. Maybe this could be addressed as well, but it's not related specifically to interpolated strings:

IFormattable formattable = $"Date: {DateTime.Now}";
var text = formattable.ToString("format", null);
@mavasani mavasani transferred this issue from dotnet/roslyn-analyzers Jul 14, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 14, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 14, 2023
@mavasani
Copy link

Moving to dotnet/runtime for triage. I guess the primary question would be whether we want to enhance the existing rule or use a different rule ID?

@ghost
Copy link

ghost commented Jul 14, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Describe the problem you are trying to solve

The existing CA1305: Specify IFormatProvider rule does no seem to cover interpolated strings. Currently compiler emits no warnings if a value that can be formatted is used inside an interpolation expression:

var formatted = $"Date: {DateTime.Now}";

Describe suggestions on how to achieve the rule

The existing analyzer could be enhanced, so that it would report diagnostic and recommend using new string.Create(IFormatProvider, ref DefaultInterpolatedStringHandler) overloads if an object of a type containing ToString(IFormatProvider) or an object implementing ISpanFormattable (I'm aware that it's actually used instead of calling ToString on values) is injected into an interpolated string. Does that make sense? If so, I could try contributing myself as I have experience writing analyzers for Microsoft projects.

Author: Alex-Sob
Assignees: -
Labels:

area-System.Runtime, untriaged, needs-area-label

Milestone: -

@vcsjones vcsjones removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 14, 2023
@tannergooding tannergooding added code-analyzer Marks an issue that suggests a Roslyn analyzer needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jul 21, 2023
@tannergooding tannergooding added this to the Future milestone Jul 21, 2023
@tannergooding
Copy link
Member

This needs more thought/consideration, namely updating the top post to more clearly provide some example code showing what's happening today vs what's expected to happen and be suggested by the analyzer.

We should take into account https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated#implicit-conversions-and-how-to-specify-iformatprovider-implementation and the different ways format providers can be provided alongside string interpolation

@Alex-Sob
Copy link
Author

@tannergooding I added some examples to the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime code-analyzer Marks an issue that suggests a Roslyn analyzer needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

5 participants