[WIP] resx analyzer #3566

Evangelink · 2020-04-28T12:04:59Z

Contributes to #3525

Evangelink · 2020-04-28T12:05:34Z

src/Roslyn.Diagnostics.Analyzers/Core/AnalyzerReleases.Unshipped.md

@@ -6,6 +6,7 @@ RS0042 | RoslynDiagnosticsReliability | Warning | DoNotCopyValue
 RS0043 | RoslynDiagnosticsMaintainability | Warning | DoNotCallGetTestAccessor
 RS0044 | RoslynDiagnosticsMaintainability | Hidden | CreateTestAccessor
 RS0045 | RoslynDiagnosticsMaintainability | Hidden | ExposeMemberForTesting
+RS0047 | RoslynDiagnosticsDesign | Info | DefineResourceEntryCorrectly


#3352 is taking RS0046

Evangelink · 2020-04-28T12:07:48Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+                    }
+
+                    var isDescription = false;
+                    var sourceText = file.GetText(context.CancellationToken);


I have tried a couple of resx/xml loader but we either don't have the right dll or this is failing because of the first line of the resx which doesn't seem to be xml compliant. Let me know if you know a better way that doing line by line reading.

Evangelink · 2020-04-28T12:08:21Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+        private static readonly Regex s_dataNameRegex = new Regex("<data.*?name=\"(.*?)\"", RegexOptions.Compiled);
+        private static readonly Regex s_valueRegex = new Regex("<value>(.*?)</value>", RegexOptions.Compiled);


It's never 100% clear to me when Compiled is better.

Evangelink · 2020-04-28T12:09:16Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+        private static readonly Regex s_dataNameRegex = new Regex("<data.*?name=\"(.*?)\"", RegexOptions.Compiled);
+        private static readonly Regex s_valueRegex = new Regex("<value>(.*?)</value>", RegexOptions.Compiled);


FYI .*? allows a non-greedy version of .* this prevents unwanting characters! Besides, it's usually better in term of performances because it reduces the backtracking.

Evangelink · 2020-04-28T12:13:37Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+                    {
+                        var text = line.ToString().Trim();
+
+                        var match = s_dataNameRegex.Match(text);


If we go for this line by line check, do we want to have a StartsWith check before the regex match?

Evangelink · 2020-04-28T12:14:24Z

Tagging @mavasani and @sharwell to get your opinion on whether or not the direction looks good to you?!

Evangelink · 2020-04-28T12:25:41Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+                            if (endsWithPeriod && !isDescription)
+                            {
+                                var linePositionSpan = sourceText.Lines.GetLinePositionSpan(line.Span);
+                                var location = Location.Create(file.Path, line.Span, linePositionSpan);


Where do we want to report? Full line? Only the .?

Evangelink · 2020-04-28T12:26:40Z

src/Roslyn.Diagnostics.Analyzers/Core/DefineResourceEntryCorrectly.cs

+                            else if (isDescription && !endsWithPeriod)
+                            {
+                                var linePositionSpan = sourceText.Lines.GetLinePositionSpan(line.Span);
+                                var location = Location.Create(file.Path, line.Span, linePositionSpan);


Where do we want to report? Full line? The last character of the sentence?

mavasani · 2020-04-28T14:28:44Z

@Evangelink Looking at the current implementation, I think my proposed idea of reporting diagnostics on resx files has few big drawbacks:

We cannot implement or offer a code fix. Code fixes are currently only supported for diagnostics on source file
CompilationAction: This diagnostic will not be reported in live analysis when you open a resx file. This is unfortunately the drawback of diagnostics with location in additional files, which I had not completely thought through.
False positives: We are assuming the relationship between between resource strings and their usage in analyzer based on just the resx name suffix, like Title, Description, etc. This makes it not useful as a generic analyzer for all analyzer authors.

I think we should take the following direction:

Lets target this analyzer should be added to 'Microsoft.CodeAnalysis.Analyzers', so it can be used by all analyzer authors.
Instead of directly reading resx and flagging the resource strings, let us try to approach this from the view of types deriving from DiagnosticAnalyzer, so we generate diagnostics in these source files. More specifically:
1. Enhance DiagnosticDescriptorCreationAnalyzer.AnalyzeTitle, so if title is a constant string, and has any periods or newlines, we report a diagnostic that title is recommended to be a short string without periods or newlines.
2. For cases where title argument is not a constant string, but a LocalizableString, we need to handle 2 cases:
  1. Argument is an object creation of type LocalizableResourceString. We can process the arguments to this creation and identify the name of resource string used for title (const string for the 1st argument) + name/type of the resources file (System.Type argument for the 3rd argument). We can map this to the resource string in resx file and then generate a similar diagnostic if it has periods or newlines.
  2. Argument is a field reference of type LocalizableString or LocalizableResourceString or const string. For const string, it should be easy to report the diagnostic. For LocalizableString or LocalizableResourceString, we need to actually analyze the field intializer. We can setup this analysis by registering a SymbolStartAction for named types. We register an Operation action with OperationKind.FieldInitializer and use this callback to analyze each field initializer of type LocalizableResourceString, and build a map from field symbol to a tuple (string resourceName, string resxFileName). In DiagnosticDescriptorCreationAnalyzer.AnalyzeTitle callback, we build a set of field symbols of type LocalizableString or LocalizableResourceString that are used as title arguments to descriptors. We register a SymbolEndAction which analyzes both these data structures to then link which resx string is used as title argument, and then report a similar diagnostic in this end action. NOTE: We should probably guard this analysis from executing only if at least one resx file was provided as an additional file, otherwise we cannot report any diagnostics here.
We can repeat similar analysis for Message and Description arguments as done for Title above, but with different validation: Message should not have any newline + it should not end with a period, unless it has at least one more period (long, multi sentence messages are common). Description should always have a period to end.
All the above diagnostics would be enabled by default, and also valid for any third party analyzer.
Additionally, we can add separate diagnostic descriptors for: Resource strings used for title, message or description should always end with "Title", "Message", "Description" respectively. I am in two minds if these diagnostics will be too noisy or not - we should maybe have these off by default and just enable them for our repo.
Code Fix: We should write a code fix that fixes all the above cases, with a FixAll operation. That is going to be the biggest benefit here.

Sorry for the long comment, we can discuss more offline for further ideas.

First version of resx analyzer

6241bce

Evangelink requested a review from a team as a code owner April 28, 2020 12:04

Evangelink commented Apr 28, 2020

View reviewed changes

Evangelink changed the title ~~First version of resx analyzer~~ [WIP] resx analyzer Apr 28, 2020

Evangelink commented Apr 28, 2020

View reviewed changes

Evangelink closed this Apr 29, 2020

Evangelink deleted the resx-analyzer branch July 13, 2020 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] resx analyzer #3566

[WIP] resx analyzer #3566

Evangelink commented Apr 28, 2020

Evangelink Apr 28, 2020

Evangelink Apr 28, 2020

Evangelink Apr 28, 2020

Evangelink Apr 28, 2020

Evangelink Apr 28, 2020

Evangelink commented Apr 28, 2020

Evangelink Apr 28, 2020 •

edited

Loading

Evangelink Apr 28, 2020

mavasani commented Apr 28, 2020 •

edited

Loading

		private static readonly Regex s_dataNameRegex = new Regex("<data.?name=\"(.?)\"", RegexOptions.Compiled);
		private static readonly Regex s_valueRegex = new Regex("<value>(.*?)</value>", RegexOptions.Compiled);

[WIP] resx analyzer #3566

[WIP] resx analyzer #3566

Conversation

Evangelink commented Apr 28, 2020

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

Evangelink commented Apr 28, 2020

Evangelink Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Evangelink Apr 28, 2020

Choose a reason for hiding this comment

mavasani commented Apr 28, 2020 • edited Loading

Evangelink Apr 28, 2020 •

edited

Loading

mavasani commented Apr 28, 2020 •

edited

Loading