-
Notifications
You must be signed in to change notification settings - Fork 4.1k
WIP: Move to Resx MVP #79297
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
base: features/move-to-resx
Are you sure you want to change the base?
WIP: Move to Resx MVP #79297
Conversation
- Detects hardcoded strings in source code. - Auto-generates resource names (with potential for AI-assisted naming). - Inserts new entries into a single .resx file. - Replaces original strings in code with references to the new resource entries. This lays the foundation for improved localization and maintainability. Currently supports one .resx file; future updates may expand multi-file support and smarter naming strategies.
@Fanominals please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement ( “Agreement” ) is agreed to by the party signing below ( “You” ), 1. Definitions. “Code” means the computer software code, whether in human-readable or machine-executable form, “Project” means any of the projects owned or managed by .NET Foundation and offered under a license “Submit” is the act of uploading, submitting, transmitting, or distributing code or other content to any “Submission” means the Code and any other copyrightable material Submitted by You, including any 2. Your Submission. You must agree to the terms of this Agreement before making a Submission to any 3. Originality of Work. You represent that each of Your Submissions is entirely Your 4. Your Employer. References to “employer” in this Agreement include Your employer or anyone else 5. Licenses. a. Copyright License. You grant .NET Foundation, and those who receive the Submission directly b. Patent License. You grant .NET Foundation, and those who receive the Submission directly or c. Other Rights Reserved. Each party reserves all rights not expressly granted in this Agreement. 6. Representations and Warranties. You represent that You are legally entitled to grant the above 7. Notice to .NET Foundation. You agree to notify .NET Foundation in writing of any facts or 8. Information about Submissions. You agree that contributions to Projects and information about 9. Governing Law/Jurisdiction. This Agreement is governed by the laws of the State of Washington, and 10. Entire Agreement/Assignment. This Agreement is the entire agreement between the parties, and .NET Foundation dedicates this Contribution License Agreement to the public domain according to the Creative Commons CC0 1. |
|
||
private static TextDocument? FindBestResxFile(Project project, string? documentPath) | ||
{ | ||
var allResx = project.AdditionalDocuments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are resx files always included in additional files? I think they are only when ResxSourceGenerator package is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, so in my experience ResX Files are not included in additional files and need to be manually added. When I was researching this, from what I read it was best practice to get the Resx files from additional documents for codefixes and analyzers but there could be something I am missing. I am reaching out to you on teams and maybe we can figure out a better implementation for this?
@JoeRobich, @dibarbet please take a look |
public override ImmutableArray<string> FixableDiagnosticIds | ||
=> ImmutableArray.Create(MoveToResx.DiagnosticId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public override ImmutableArray<string> FixableDiagnosticIds | |
=> ImmutableArray.Create(MoveToResx.DiagnosticId); | |
public override ImmutableArray<string> FixableDiagnosticIds | |
=> [MoveToResx.DiagnosticId]; |
Prefer collection expressions
|
||
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var diagnostic = context.Diagnostics[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have more than one diagnostic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaredpar no. This was based on an early iteration of code fixes that thought that might occur (for fix all). but that's not how fix all actually ended up working. but thsi was never revised. So this is fine to do.
var diagnostic = context.Diagnostics[0]; | ||
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
var node = root?.FindNode(diagnostic.Location.SourceSpan) as LiteralExpressionSyntax; | ||
if (node == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (node == null) | |
if (node is null) |
//private static IEnumerable<TextDocument> GetResxAdditionalFiles(Project project) | ||
//{ | ||
// foreach (var file in project.AdditionalDocuments) | ||
// { | ||
// if (file.FilePath != null && | ||
// string.Equals(".resx", Path.GetExtension(file.FilePath), StringComparison.OrdinalIgnoreCase)) | ||
// { | ||
// yield return file; | ||
// } | ||
// } | ||
//} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove commented out code before merging.
namespace Microsoft.CodeAnalysis.CSharp.MoveToResx; | ||
|
||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
internal class MoveToResx : DiagnosticAnalyzer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, update filename to match.
internal class MoveToResx : DiagnosticAnalyzer | |
internal class CSharpMoveToResxDiagnosticAnalyzer : DiagnosticAnalyzer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, seal the type.
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
internal class MoveToResx : DiagnosticAnalyzer | ||
{ | ||
public const string DiagnosticId = "MoveToResx"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new diagnostic id to IDEDiagnosticIds.cs instead of defining one here. Maybe IDE0360
makes sense.
|
||
private static void AnalyzeStringLiteral(SyntaxNodeAnalysisContext context) | ||
{ | ||
var stringLiteral = (LiteralExpressionSyntax)context.Node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are not creating resource files yet, check whether a .resx file is reachable before reporting diagnostics. Meaning FindBestResxFile
may need to be moved to a utilities class that your analyzer and codefix can share.
// 2. Parse the SourceText with XDocument | ||
var xdoc = XDocument.Parse(resxSourceText.ToString()); | ||
|
||
// 3. Add or update the resource entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check to see if the string (not just the name) already exists in the resource file. If so we can grab the name and skip updating the .resx.
If we checked for this in the Analyzer, we may want to offer a separate diagnostic and fix for that. UseResxString or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey good idea! Making this change for the code fix. For the analyzer use case, how would the analyzer know which resx to check for the string. Would it check all available resx files?
using System.Collections.Immutable; | ||
using System.IO; | ||
using System.Xml.Linq; | ||
using Microsoft.CodeAnalysis.CSharp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sort :)
using System.Xml.Linq; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
|
||
namespace Microsoft.CodeAnalysis.CSharp.MoveToResx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we use file scoped namespaces.
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(CSharpMoveToResxCodeFixProvider)), Shared] | ||
internal class CSharpMoveToResxCodeFixProvider : CodeFixProvider | ||
{ | ||
[ImportingConstructor] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, this should have the obsolete attribute stating it should not be called directly (see other code fix providers).
Nit: use a primary constructor.
namespace Microsoft.CodeAnalysis.CSharp.MoveToResx | ||
{ | ||
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(CSharpMoveToResxCodeFixProvider)), Shared] | ||
internal class CSharpMoveToResxCodeFixProvider : CodeFixProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sealed
=> ImmutableArray.Create(MoveToResx.DiagnosticId); | ||
|
||
public override FixAllProvider GetFixAllProvider() | ||
=> WellKnownFixAllProviders.BatchFixer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how effective this will be. we may want to write a custom fixer here. Do you have tests showing fix-all behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, great point. I am working on a custom fixer for this.
{ | ||
var diagnostic = context.Diagnostics[0]; | ||
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
var node = root?.FindNode(diagnostic.Location.SourceSpan) as LiteralExpressionSyntax; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var node = root?.FindNode(diagnostic.Location.SourceSpan) as LiteralExpressionSyntax; | |
var node = root?.FindNode(diagnostic.Location.SourceSpan, getInnermostNodeForTie: true) as LiteralExpressionSyntax; |
This is needed for the scenario where you have Foo("SomeString")
(and please test this). Your above code will get an ArgumentSyntax not its child LiteralExpressionSyntax here.
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var diagnostic = context.Diagnostics[0]; | ||
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either do GetRequiredSyntaxRootAsync
or use our existing helper, which is called something like context.TryGetRelevantNodeAsync<LiteralExpressionSyntax>()
if (node == null) | ||
return; | ||
|
||
var resxFile = FindBestResxFile(context.Document.Project, context.Document.FilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are many forms of literals (like numbers, booleans, etc). you need to actually validate that this is a string literal/
var resxName = Path.GetFileName(resxFile.FilePath); | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: $"Move string to .resx resource ({resxName})", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ironic that the title itself is not localized :)
also, this indicates an interesting case we should support. We should find interpolated strings and offer to make them resources, converting it to then use string.Format at the call site.
{ | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "No .resx file found. Please create a .resx file in your project or folder.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we def would not do things this way normally. this will pop up a lightbulb (on every string) with something that does nothing. In this case, we should add the resx file to the project and populate it properly. This may require you to write a component at a higher layer, and not offer anything if you can't import that component here.
.ToList(); | ||
|
||
if (allResx.Count == 0 || documentPath == null) | ||
return allResx.FirstOrDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should always have a document path. so this second clause won't ever hit.
fort he first clause, it's odd since you knwo the count is 0, so returning FirstOrDefualt is equivalent to jsut returning null.
var resxSourceText = await resxFile.GetTextAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
// 2. Parse the SourceText with XDocument | ||
var xdoc = XDocument.Parse(resxSourceText.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can fail, throwing an exception. that woudl be bad.
} | ||
else | ||
{ | ||
dataElement.Element("value")!.Value = value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no guarantee that the resx file will be formed in the way you want. it's important to check for that and act gracefully there. these are also good test cases.
// 7. Replace the string literal in the code with a resource reference, including namespace if available | ||
var resourceClass = Path.GetFileNameWithoutExtension(resxFile.Name); | ||
var ns = document.Project.DefaultNamespace; | ||
string resourceAccessString = !string.IsNullOrEmpty(ns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't abbr in the code. so use something like defaultNamespace, not 'ns'.
also, we should annotate this expression with a simplification annotation, so that the extraneous namespace/type name can be trimmed if possible int he destiation.
return newSolution; | ||
} | ||
|
||
private static string ToDeterministicResourceKey(string value, int maxWords = 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this an optional argument?
words.Add(sb.ToString().ToLowerInvariant()); | ||
|
||
// Limit to maxWords | ||
if (words.Count > maxWords) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems quite low. What i'd probably do instead is make the resource string as long as needed without going over a max length (like 60 chars) vs a max number of words (which could be any any length).
} | ||
} | ||
if (sb.Length > 0) | ||
words.Add(sb.ToString().ToLowerInvariant()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we lowercasing the txt?
|
||
// Split the string into words using non-alphanumeric as delimiters | ||
var words = new List<string>(); | ||
var sb = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
builder, or stringBuilder
private static string ToDeterministicResourceKey(string value, int maxWords = 6) | ||
{ | ||
if (string.IsNullOrWhiteSpace(value)) | ||
return "emptyString"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when does this happen?
words = words.Take(maxWords).ToList(); | ||
|
||
// Convert to lowerCamelCase | ||
var keyBuilder = new StringBuilder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var keyBuilder = new StringBuilder(); | |
using var _ = PooledStringBuilder.GetInstance(out var keyBuilder); |
public override void Initialize(AnalysisContext context) | ||
{ | ||
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
context.EnableConcurrentExecution(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have existing DiagnosticAnalyzer subtypes that you can subclass yourself. They will take care of these sorts of common pieces of code.
|
||
// Report a diagnostic for every string literal | ||
var diagnostic = Diagnostic.Create(s_moveToResxRule, stringLiteral.GetLocation(), stringLiteral.Token.ValueText); | ||
context.ReportDiagnostic(diagnostic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to do a bunch more up front checking. As an example, we really never want the code fixer part to ever fail (unless validating that it wouldn't fail woudl be prohibitively expensive).
So any locations where the fixer no-ops, we should check up here (for example, for things like the empty string).
NOte: the analyzer and fixer should both be low-pri. This will show up on every string litearl, and will often not be what the user wants. So we should deprioritize it against existing analzyers/refactorings/fixers. |
…-resx' into dev/gosingh/move-to-resx
@JoeRobich @CyrusNajmabadi @jaredpar Ive added a fixall provider that works + addressed most of the things brought up so far. Requesting a review on the new changes |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@@ -0,0 +1,64 @@ | |||
//// Licensed to the .NET Foundation under one or more agreements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want to commit commented out code. Remove until we are ready to move forward with this.
var resx = allResx.First(); | ||
|
||
return CodeAction.Create( | ||
"Move all strings to .resx resource", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this string to the Resx. =)
|
||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
"Move string to .resx resource", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to Resx
{ | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "No .resx file found. Please create a .resx file in your project or folder.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still believe we should be checking for .resx files in the Analyzer.
Document document, | ||
LiteralExpressionSyntax stringLiteral, | ||
TextDocument resxFile, | ||
CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the code in the codefix and the fixall are fairly similar. Could we share it to reduce duplication?
Add initial support for string extraction and .resx integration
This lays the foundation for improved localization and maintainability. Currently supports one .resx file; future updates may expand multi-file support and smarter naming strategies.