Bump AnalysisLevel and enable more analyzers #3681

Morilli · 2023-06-09T18:47:34Z

This bumps the .NET analyzer level from 5 to 6 and enables more built-in rules from the most useful categories imho.

Note that all analyzers will still only be ran with -p:RunAnalyzersDuringBuild=true currently as build time does increase substantially with those enabled.

When fixing inspections I've tried my best to keep the behavior 100% the same, although some things could break, for example the locale fixes (like float.ToString()).

Some more links: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-options, https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/code-quality-rule-options

Check if completed:

I have run any relevant test suites
I, the committer, have read the licensing terms for contributors (last updated 2022-07-15) and am compliant

fixes CA1304, CA1309, CA1310, CA1311 temporarily silences CA1305 reduce severity of CA2101

fixes CA1841 silence CA1805 and CA1822 because of sheer amount of violations reduce severity of CA1838 because I don't wanna fix it lol

fixes CA2211, CA2215, CA2241, CA2251 silence CA1816 because it's unnecessary reduce severity of CA2201 because I like the concept but don't wanna fix them all

YoshiRulz

Was in the "middle" (just started really) of a review, will just come back later and commit fixes directly

YoshiRulz · 2023-08-14T18:49:55Z

Common.ruleset

@@ -461,233 +461,6 @@
 		<!-- Use the Regex source generator -->
 		<Rule Id="MA0110" Action="Error" />
 	</Rules>
-	<Rules AnalyzerId="Microsoft.CodeAnalysis.FxCopAnalyzers" RuleNamespace="Microsoft.CodeAnalysis.FxCopAnalyzers">


When you say "legacy," have these been removed from the FxCop NuGet package?

The FxCop nuget package itself was no longer included in BizHawk and has been deprecated in favor of the .NET analyzers themselves, see for example https://learn.microsoft.com/en-us/visualstudio/code-quality/migrate-from-legacy-analysis-to-net-analyzers?view=vs-2022.

YoshiRulz · 2023-08-14T19:03:13Z

src/BizHawk.Common/Extensions/StringExtensions.cs

@@ -165,5 +166,9 @@ public static byte[] ToCharCodepointArray(this string str)
 		/// <remarks><c>"abc,def,ghi".TransformFields(',', s => s.Reverse()) == "cba,fed,ihg"</c></remarks>
 		public static string TransformFields(this string str, char delimiter, Func<string, string> transform)
 			=> string.Join(delimiter.ToString(), str.Split(delimiter).Select(transform));
+
+		public static bool StartsWithOrdinal(this string str, string value) => str.StartsWith(value, StringComparison.Ordinal);


This is a good idea to include if the stdlib is missing it, but the downside of rolling your own is that it's going to be overlooked unless there's an Analyzer rule for it. I can add one to BizHawk.Analyzer if you'd like. Also they should take ReadOnlySpan<char>, I've been meaning to do that for the others.

This specific function could be overlooked (I'd appreciate an analyzer rule for that in case you agree with this being here), but the general warning will still display due to CA1310.

YoshiRulz · 2023-08-14T19:15:16Z

src/BizHawk.Client.Common/ArgParser.cs

-				var argDowncased = arg.ToLower();
-				if (argDowncased.StartsWith("--load-slot="))
+				var argDowncased = arg.ToLowerInvariant();
+				if (argDowncased.StartsWithOrdinal("--load-slot="))


Here we see the limits of low-effort (no offence) find+replace: these could all be StartsWith(..., StringComparison.OrdinalIgnoreCase) (or a helper for brevity). Maybe this way happens to be more performant, but I'd argue readability is more important for this method.

I don't consider this blocking for merge as you're just resolving warnings.

YoshiRulz · 2023-08-14T19:34:55Z

src/BizHawk.Client.Common/RecentFiles.cs

@@ -71,7 +71,7 @@ public bool Remove(string newFile)
 		{
 			if (!Frozen)
 			{
-				return recentlist.RemoveAll(recent => string.Compare(newFile, recent, StringComparison.CurrentCultureIgnoreCase) == 0) != 0; // none removed => return false
+				return recentlist.RemoveAll(recent => string.Equals(newFile, recent, StringComparison.OrdinalIgnoreCase)) != 0; // none removed => return false


string.Compare(a, b, m) == 0 => string.Equals(a, b, m)?

(Should write an Analyzer rule for this too...)

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2251

YoshiRulz · 2023-08-14T19:35:30Z

src/BizHawk.Client.Common/RomGame.cs

@@ -65,7 +65,7 @@ public RomGame(HawkFile file, string patch)

 				if (!string.IsNullOrWhiteSpace(GameInfo.Name) && GameInfo.Name == GameInfo.Name.ToUpperInvariant())
 				{
-					GameInfo.Name = Thread.CurrentThread.CurrentCulture.TextInfo.ToTitleCase(GameInfo.Name.ToLower());
+					GameInfo.Name = Thread.CurrentThread.CurrentCulture.TextInfo.ToTitleCase(GameInfo.Name.ToLowerInvariant());


What is even going on here

YoshiRulz · 2023-08-14T19:41:29Z

src/BizHawk.Common/Extensions/StringExtensions.cs

+
+		public static bool StartsWithOrdinal(this string str, string value) => str.StartsWith(value, StringComparison.Ordinal);
+
+		public static bool EndsWithOrdinal(this string str, string value) => str.EndsWith(value, StringComparison.Ordinal);


Could do with {,Last}IndexOfOrdinal too

YoshiRulz · 2023-08-14T19:55:57Z

src/BizHawk.Client.Common/cheats/GameSharkDecoder.cs

@@ -109,13 +111,13 @@ private static IDecodeResult Gen(string code)
 		private static IDecodeResult Sms(string code)
 		{
 			// Game Genie
-			if (code.LastIndexOf("-") == 7 && code.IndexOf("-") == 3)
+			if (code.LastIndexOf("-", StringComparison.Ordinal) == 7 && code.IndexOf("-", StringComparison.Ordinal) == 3)


These checks are dumb, this main class should only do a length check, if applicable, then delegate to the individual decoders and return if successful, trying the next decoder if not. But all of this is dumb and needs rewriting with unit tests.

For this PR, you could replace s.IndexOf(c, Ordinal) == n with s[n] is c and it would be no more broken while resolving the warnings.

YoshiRulz · 2023-08-14T20:04:51Z

src/BizHawk.Client.Common/config/PathEntryCollectionExtensions.cs

@@ -90,12 +90,12 @@ private static string AbsolutePathForInner(this PathEntryCollection collection,

 			if (path == "%recent%") return PathUtils.SpecialRecentsDir;


I think the == operator is .Equals(..., Ordinal), but it would be nice to be explicit—is there not a rule for that?

There is https://www.jetbrains.com/help/resharper/SpecifyStringComparison.html, I'm not aware of any global code inspection though.

src/BizHawk.Client.Common/input/InputCoalescerControllers.cs

Morilli force-pushed the more-analyzer branch from 960e761 to 8599fba Compare July 23, 2023 19:22

Morilli added 7 commits August 14, 2023 00:09

Bump AnalysisLevel, remove legacy rules

54248f5

bump AnalysisModeGlobalization to Recommended

9ec6a0e

fixes CA1304, CA1309, CA1310, CA1311 temporarily silences CA1305 reduce severity of CA2101

Fix CA1305 and bump to error

d7f0dc4

Bump AnalysisModeMaintainability to Recommended

288060e

bump AnalysisModeReliability to Recommended

5ed1114

bump AnalysisModePerformance to Recommended

de198cb

fixes CA1841 silence CA1805 and CA1822 because of sheer amount of violations reduce severity of CA1838 because I don't wanna fix it lol

bump AnalysisModeUsage to Recommended

a0f5487

fixes CA2211, CA2215, CA2241, CA2251 silence CA1816 because it's unnecessary reduce severity of CA2201 because I like the concept but don't wanna fix them all

Morilli force-pushed the more-analyzer branch from 8599fba to a0f5487 Compare August 14, 2023 10:54

Morilli merged commit c10d292 into master Aug 14, 2023

Morilli deleted the more-analyzer branch August 14, 2023 21:18

YoshiRulz reviewed Aug 14, 2023

View reviewed changes

TiKevin83 mentioned this pull request Aug 20, 2023

Standardize and Optimize Code with dotnet format #3754

Draft

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump AnalysisLevel and enable more analyzers #3681

Bump AnalysisLevel and enable more analyzers #3681

Morilli commented Jun 9, 2023

YoshiRulz left a comment •

edited

Loading

YoshiRulz Aug 14, 2023

Morilli Aug 15, 2023

YoshiRulz Aug 14, 2023

Morilli Aug 15, 2023

YoshiRulz Aug 14, 2023

YoshiRulz Aug 14, 2023

Morilli Aug 15, 2023

YoshiRulz Aug 14, 2023

YoshiRulz Aug 14, 2023

YoshiRulz Aug 14, 2023

YoshiRulz Aug 14, 2023

Morilli Aug 15, 2023


		public static bool StartsWithOrdinal(this string str, string value) => str.StartsWith(value, StringComparison.Ordinal);

		public static bool EndsWithOrdinal(this string str, string value) => str.EndsWith(value, StringComparison.Ordinal);

		@@ -90,12 +90,12 @@ private static string AbsolutePathForInner(this PathEntryCollection collection,

		if (path == "%recent%") return PathUtils.SpecialRecentsDir;

Bump AnalysisLevel and enable more analyzers #3681

Bump AnalysisLevel and enable more analyzers #3681

Conversation

Morilli commented Jun 9, 2023

YoshiRulz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoshiRulz left a comment •

edited

Loading