-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Promote "Convert to interpolated string" to an analyzer/fixer #68469
Comments
@arkalyanms This woudl be good to do. Can we try to get this in next sprint? Would be good to help devs on team get experience writing analyzers as well. |
@CyrusNajmabadi @arkalyanms What's the status of this issue? |
@Bartleby2718 Nothing has changed here. if you'd like to help out, we welcome contributions. Thanks! :) |
@CyrusNajmabadi I can take a look, but I don't have too much experience with Roslyn. Could you give me some pointers? Specifically:
|
@Bartleby2718 I recommend reading https://github.com/dotnet/roslyn/blob/main/CONTRIBUTING.md first. We also have a good discord channel over at: https://discord.com/channels/143867839282020352/598678594750775301 You can find examples of anlayzers and fixers here https://github.com/dotnet/roslyn/tree/main/src/Analyzers/Core/Analyzers and https://github.com/dotnet/roslyn/tree/main/src/Analyzers/Core/CodeFixes. Note that as this is a cross language feature, there will likely be work here, and in the corresponding C# and VB projects.
For the ID, pick the next multiple of 10 after the main set of IDs not used in IDEDiagnosticIds.cs. Severity should be 'hidden' or 'info'. We can decide with the PR. The description can be figured out with the PR as well.
We already have the refactoring. So this would just be about moving all its code, wholesale, over to the analyzer. So it would be the same set of supported cases.
You both can be assigned to it :) |
@CyrusNajmabadi Actually, I see that the file next to it-- |
We probably want to move them all over. With separate diagnostic IDs for each |
Note: VB's interpolated string is much less optimized than C#. |
@tats-u If you don't mind, could you point me to an example where different |
@Bartleby2718 Sorry I don't know much about it because I'm just an outsider, but I can only say that values like it are just constants and used only in the second argument of the constructor of the abstract class I wonder if the priority can be changed depending on languages. |
Yeah I should be able to update this line to use an |
How about adding an optional argument to the constructor? protected AbstractUseInterpolatedStringDiagnosticAnalyzer(EnforceOnBuildValues enforceOnBuildValues = EnforceOnBuildValues.UseInterpolatedString) |
@Bartleby2718 Sorry I've misunderstood. Now we don't have to prepare a dedicated |
Summary
The current "Convert to interpolated string" refactoring is really helpful, but it can't be enabled for builds and it doesn't have a fix-all. Ideally it would be converted into an IDEXXXX or CAXXXX analyzer/fixer.
Background and Motivation
"Convert to interpolated string" used to be primarily about code style, but with the improvements to interpolated strings that came in .NET 6 and C# 10, it's now a performance feature. Anywhere code is doing
string.Format("...", ...)
, it's leaving performance (throughput and allocation) on the table by not using an interpolated string.Proposed Feature
P0:
Take the existing functionality and just make it an analyzer/fixer with fix-all support. That way, for example dotnet/runtime can enable it in builds to flag any places we're missing out on free wins, and also auto-fix everything so that we don't have to do the painstaking transformations manually.
P1:
Since the refactoring was initially written, the new
string.Create(...)
method was introduced, which enables supporting culture. Currently the refactoring supportsstring.Format("...", ...)
andstring.Format(null, "...", ...)
, but it gives up on e.g.string.Format(CultureInfo.InvariantCulture, "...", ...)
. That can now be fixed intostring.Create(CultureInfo.InvariantCulture, $"...")
.P2:
Other APIs now have interpolated string handlers that allow for similar efficiency gains. In particular,
StringBuilder
has long providedAppendFormat
methods that take the same arguments asstring.Format
, and appends the results to the builder. As of .NET 6,StringBuilder.Append
andStringBuilder.AppendLine
have overloads that take interpolated string handlers, so the analyzer/fixer can also flag a call likesb.AppendFormat("...", ...)
and fix it intosb.Append($"...")
.Alternative Designs
dotnet/roslyn-analyzers#6674 is adding an analyzer (fixer to follow) focused on string.Format and StringBuilder.AppendFormat calls where the format string isn't a literal; in such cases, we can use
CompositeFormat
in .NET 8 to achieve the same throughput/allocation efficiencies at run-time (paying some start-up cost that interpolated strings avoid when the format string is known at compile-time). It also tentatively currently includes a separate diagnostic for when the string is a literal, in particular because it can flag cases where the format provider argument isn't missing or null. It's unfortunate to have two of these. If we can convert the current refactoring into a proper analyzer/fixer, than we can delete this diagnostic from roslyn-analyzers. Alternatively, we could delete the refactoring, and move the current refactoring logic into this analyzer/fixer.The text was updated successfully, but these errors were encountered: