Implement analyzer for unused resources #6338

Youssef1313 · 2022-12-13T10:20:13Z

Currently having a false positive due to IPropertyReferenceOperation isn't produced for nameof(Prop) only in VB roslyn#65969
False positives for IVTs. The resource could be used by other projects.
As there is no way to suppress the warning for individual cases, I think we could have an .editorconfig option to specify suppressions, e.g, dotnet_code_quality.ignore_unused_resources = MyResources.Resource1,MyResources2.Resource2

codecov · 2022-12-13T15:15:58Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (8a92037) 96.44% compared to head (ee737f7) 96.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6338      +/-   ##
==========================================
- Coverage   96.44%   96.43%   -0.02%     
==========================================
  Files        1411     1412       +1     
  Lines      337272   337334      +62     
  Branches    11155    11163       +8     
==========================================
+ Hits       325289   325294       +5     
- Misses       9179     9235      +56     
- Partials     2804     2805       +1

mavasani · 2022-12-16T10:56:54Z

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUnusedResources.cs

+        public override void Initialize(AnalysisContext context)
+        {
+            context.EnableConcurrentExecution();
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);


Why do we need to report diagnostics on generated code?

@mavasani I report the diagnostic in the C# code that is generated from resx. The other way I can think of is reporting a diagnostic with Location.None, but I think it's clearer to have a location.

mavasani · 2022-12-16T11:07:09Z

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUnusedResources.cs

+                var resourceFileNames = context.Options.AdditionalFiles
+                    .Where(f => Path.GetExtension(f.Path).Equals(".resx", StringComparison.Ordinal)).Select(f => Path.GetFileNameWithoutExtension(f.Path)).ToImmutableHashSet();
+
+                var resourceTypes = context.Compilation.GetSymbolsWithName(n => resourceFileNames.Contains(n), SymbolFilter.Type, context.CancellationToken)


This seems to be quite expensive - we are basically force completing symbols in CompilationStartAction callback. I think a better approach would be to register a SymbolAction callback with SymbolKind.NamedType, check in the callback if the named type is static type which is the generated class for a resx file and add them to a set of resx types. The IPropertyReferenceOperation callback will also collect the set of used properties which are static properties within a static named type and are of type string. The compilation end action can then analyze both these sets and report diagnostics.

mavasani · 2022-12-16T11:11:53Z

False positives for IVTs. The resource could be used by other projects.

I wouldn't classify these as false positives, though we may want to have an editorconfig option that bails out of resx analysis for projects that give IVT access to any assembly. IMO, if a project defines a resx that is not used within that project but used by a referencing project, then the developer should move the resx out of that project into a resx file in the referencing project.

mavasani · 2022-12-16T11:14:48Z

As there is no way to suppress the warning for individual cases, I think we could have an .editorconfig option to specify suppressions, e.g, dotnet_code_quality.ignore_unused_resources = MyResources.Resource1,MyResources2.Resource2

This seems like a good idea. I don't see any other better alternative for specifying suppressions. We may want to follow the standard pattern we use in our editorconfig options when we want to specify one or more symbol names in the option value, for example: https://github.com/dotnet/roslyn-analyzers/blob/main/docs/Analyzer%20Configuration.md#excluded-type-names-with-derived-types

mavasani · 2022-12-16T11:17:34Z

@Youssef1313 I understand the usefulness of this analyzer, but wanted to confirm if you opened an issue on dotnet/runtime to take this through triage. Even though the analyzer is not .NET API specific, I think it would be good to do so to ensure there are no surprises on their side when this analyzer gets added.

mavasani · 2022-12-16T11:30:23Z

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidUnusedResources.cs

+            CreateLocalizableResourceString(nameof(AvoidUnusedResourcesTitle)),
+            CreateLocalizableResourceString(nameof(AvoidUnusedResourceMessage)),
+            DiagnosticCategory.Maintainability,
+            RuleLevel.Disabled,


I am wondering if we should have this as RuleLevel.Ide_Suggestion instead of disabled by default. I understand that the rule will not report suggestion diagnostics by default as it is a compilation end analyzer, but having this as an Info diagnostic by default will mean that specifying any non-default value for AnalysisMode will bump this to a warning and start reporting unused resource warnings on command line builds. We can also implicitly pass in all .resx files in the project as additional files when AnalysisMode or AnalysisLevel properties end with minimum, recommended or all similar to the below added to MS.CA.Analyzers package:

roslyn-analyzers/src/Tools/GenerateDocumentationAndConfigFiles/Program.cs

Lines 1601 to 1608 in 79cc002

CodeAnalysisAnalyzersPackageName => @"



<Target Name=""AddAllResxFilesAsAdditionalFiles"" BeforeTargets=""CoreCompile"" Condition=""'@(EmbeddedResource)' != '' AND '$(SkipAddAllResxFilesAsAdditionalFiles)' != 'true'"">

<ItemGroup>

<EmbeddedResourceWithResxExtension Include=""@(EmbeddedResource)"" Condition=""'%(Extension)' == '.resx'"" />

<AdditionalFiles Include=""%(EmbeddedResourceWithResxExtension.Identity)"" />

</ItemGroup>

</Target>

Youssef1313 · 2022-12-16T11:45:02Z

Thanks for review @mavasani. I opened dotnet/runtime#79745 and will get back to the PR once dotnet/runtime team approves the analyzer.

stephentoub · 2023-04-25T01:05:36Z

This was approved in dotnet/runtime#79765.

mavasani · 2024-01-02T09:15:04Z

@Youssef1313 Do you plan to continue working on this PR?

sharwell · 2024-01-02T14:51:10Z

though we may want to have an editorconfig option that bails out of resx analysis for projects that give IVT access to any assembly

I don't see the value of an option here. Resources can't be defined as private, so if you want to disable analysis for an assembly with IVTs just turn off the rule completely.

Youssef1313 · 2024-01-06T16:29:27Z

@mavasani Sure. I rebased and fixed the merge conflicts, updated RuleLevel, and updated the implementation per #6338 (comment).

Let me know what do you think of the current implementation. Then, I think the obvious remaining task is to add an MSBuild target to the package that adds resx files as AdditionalFiles. Maybe we also need to add AdditionalFiles to CompilerVisibleItemMetadata? Not sure.

Sorry for leaving this for long.

Youssef1313 marked this pull request as ready for review December 13, 2022 16:21

Youssef1313 requested a review from a team as a code owner December 13, 2022 16:21

mavasani reviewed Dec 16, 2022

View reviewed changes

Youssef1313 mentioned this pull request Dec 16, 2022

Unused resources analyzer dotnet/runtime#79745

Closed

Youssef1313 mentioned this pull request Dec 16, 2022

Analyzer proposal: Remove unused resources dotnet/runtime#79765

Open

Youssef1313 added 3 commits January 6, 2024 18:06

Implement analyzer for unused resources

f770f40

Update RuleLevel

0f317dd

Adjust the implementation per feedback

931bb8b

Youssef1313 force-pushed the resx-analyzer branch from 8131b24 to 931bb8b Compare January 6, 2024 16:24

chore: Run pack

ee737f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement analyzer for unused resources #6338

Implement analyzer for unused resources #6338

Youssef1313 commented Dec 13, 2022 •

edited

codecov bot commented Dec 13, 2022 •

edited

mavasani Dec 16, 2022

Youssef1313 Dec 16, 2022

mavasani Dec 16, 2022

mavasani commented Dec 16, 2022

mavasani commented Dec 16, 2022 •

edited

mavasani commented Dec 16, 2022

mavasani Dec 16, 2022

Youssef1313 commented Dec 16, 2022

stephentoub commented Apr 25, 2023

mavasani commented Jan 2, 2024

sharwell commented Jan 2, 2024

Youssef1313 commented Jan 6, 2024

	CodeAnalysisAnalyzersPackageName => @"
	<!-- Target to add all 'EmbeddedResource' files with '.resx' extension as analyzer additional files -->
	<Target Name=""AddAllResxFilesAsAdditionalFiles"" BeforeTargets=""CoreCompile"" Condition=""'@(EmbeddedResource)' != '' AND '$(SkipAddAllResxFilesAsAdditionalFiles)' != 'true'"">
	<ItemGroup>
	<EmbeddedResourceWithResxExtension Include=""@(EmbeddedResource)"" Condition=""'%(Extension)' == '.resx'"" />
	<AdditionalFiles Include=""%(EmbeddedResourceWithResxExtension.Identity)"" />
	</ItemGroup>
	</Target>

Implement analyzer for unused resources #6338

Are you sure you want to change the base?

Implement analyzer for unused resources #6338

Conversation

Youssef1313 commented Dec 13, 2022 • edited

codecov bot commented Dec 13, 2022 • edited

Codecov Report

mavasani Dec 16, 2022

Choose a reason for hiding this comment

Youssef1313 Dec 16, 2022

Choose a reason for hiding this comment

mavasani Dec 16, 2022

Choose a reason for hiding this comment

mavasani commented Dec 16, 2022

mavasani commented Dec 16, 2022 • edited

mavasani commented Dec 16, 2022

mavasani Dec 16, 2022

Choose a reason for hiding this comment

Youssef1313 commented Dec 16, 2022

stephentoub commented Apr 25, 2023

mavasani commented Jan 2, 2024

sharwell commented Jan 2, 2024

Youssef1313 commented Jan 6, 2024

Youssef1313 commented Dec 13, 2022 •

edited

codecov bot commented Dec 13, 2022 •

edited

mavasani commented Dec 16, 2022 •

edited