Set up-to-date .NET code quality analyzers #6434

huoyaoyuan · 2024-11-27T16:12:41Z

This is an overhaul from previous FxCop (#3007). Now the rules are shipped in-box with .NET SDK, with concept of mode/level, allowing to set aggressiveness of categories instead of enumerating them.

The analysis rules are grouped by aggressiveness of None < Default < Minimum < Recommended < All. I avoided None and All because they would impact critical/too flaky ones, and didn't involve rules with too much noise/high design impact.

The performance concern for inner loop should be low, since many analyzers are already running at suggestion level. If there's evidence of performance impact, we can set EnableNETAnalyzers to false in projects and pass true in CI.

huoyaoyuan · 2024-11-27T16:14:18Z

osu.Framework.SourceGeneration.Tests/GeneratorTestHelper.cs

@@ -23,7 +23,7 @@ public static void VerifyZeroDiagnostics(this GeneratorDriverRunResult runResult
                                                .Where(d => d.Severity == DiagnosticSeverity.Error)
                                                .ToArray();

-            if (compilationDiagnostics.Any() || generatorDiagnostics.Any())


I see this rule as controversial. As a non-English speaker, Any() or !Any() is less readable for me comparing to length tests. I separated all changes with this rule in one commit, in case if you'd like to revert it.

huoyaoyuan · 2024-11-27T16:14:43Z

CodeAnalysis/BannedSymbols.txt

-M:System.Char.ToLower(System.Char);char.ToLower() changes behaviour depending on CultureInfo.CurrentCulture. Use char.ToLowerInvariant() instead. If wanting culture-sensitive behaviour, explicitly provide CultureInfo.CurrentCulture.
-M:System.Char.ToUpper(System.Char);char.ToUpper() changes behaviour depending on CultureInfo.CurrentCulture. Use char.ToUpperInvariant() instead. If wanting culture-sensitive behaviour, explicitly provide CultureInfo.CurrentCulture.
-M:System.String.ToLower();string.ToLower() changes behaviour depending on CultureInfo.CurrentCulture. Use string.ToLowerInvariant() instead. If wanting culture-sensitive behaviour, explicitly provide CultureInfo.CurrentCulture or use LocalisableString.
-M:System.String.ToUpper();string.ToUpper() changes behaviour depending on CultureInfo.CurrentCulture. Use string.ToUpperInvariant() instead. If wanting culture-sensitive behaviour, explicitly provide CultureInfo.CurrentCulture or use LocalisableString.


These are covered in CA1304.

huoyaoyuan · 2024-11-27T16:17:14Z

Directory.Build.props

@@ -28,7 +28,17 @@
    <AdditionalFiles Include="$(MSBuildThisFileDirectory)CodeAnalysis\BannedSymbols.txt" />
  </ItemGroup>
  <PropertyGroup Label="Code Analysis">
-    <CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)CodeAnalysis\osu-framework.ruleset</CodeAnalysisRuleSet>
+    <AnalysisMode>Default</AnalysisMode>


Actually I tested Recommended as baseline for every category, and revert to Default if there's too much noise or controversary.

turbedi · 2024-11-27T17:04:08Z

osu.Framework/Graphics/Containers/SearchContainer.cs

@@ -121,7 +121,7 @@ protected void Filter()
        private void performFilter()
        {
            string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);


To avoid the unnecessary allocation. Not related to this PR, but noticed it while reading through it. Wanna include it in this PR while you are at it?
I'm surprised that there is no analyzer for this 😅

Suggested change

string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);

string[] terms = (searchTerm ?? string.Empty).Split(' ', StringSplitOptions.RemoveEmptyEntries);

Yeah surprisingly, there isn't a single rule for switching from collection parameter to singleton. CA1861 is about using collections more efficiently.
I'd like to avoid including unrelated changes for this PR, because the discovered instance numbers can be non-trivial.

Susko3

More care needs to be given when changing ContainsKey() & item access to TryGetValue(). I assume you've just run an automatic fixer?

Due the to currently chosen code structure and variable names, it's harder to understand than the original code.

Susko3 · 2024-11-27T19:31:37Z

osu.Framework/Graphics/UserInterface/TabControl.cs

@@ -545,10 +545,11 @@ protected override IEnumerable<Vector2> ComputeLayoutPositions()

            private void updateChildIfNeeded(TabItem<T> child, bool isVisible)
            {
-                if (!tabVisibility.ContainsKey(child) || tabVisibility[child] != isVisible)
+                if (!tabVisibility.TryGetValue(child, out bool value) || value != isVisible)


Please don't name it value. Maybe currentVisibility?

Susko3 · 2024-11-27T19:32:24Z

osu.Framework/Graphics/UserInterface/TabControl.cs

-                    tabVisibility[child] = isVisible;
+                    value = isVisible;
+                    tabVisibility[child] = value;


Just revert this change, it doesn't really make sense.

The code fixer was too dumb to include this

Susko3 · 2024-11-27T19:34:31Z

osu.Framework/Input/Handlers/Midi/MidiHandler.cs

+                if (!runningStatus.TryGetValue(senderId, out byte value))
                    throw new InvalidDataException($"Received running status of sender {senderId}, but no event type was stored");

-                eventType = runningStatus[senderId];
+                eventType = value;


-if (!runningStatus.TryGetValue(senderId, out byte value)) +if (!runningStatus.TryGetValue(senderId, out eventType))

huoyaoyuan · 2024-11-27T20:04:48Z

I assume you've just run an automatic fixer?

Correct. I didn't even look through the code in detail since the fix is quite mature.

peppy

No issues from my end 👍

smoogipoo

Seems ok

huoyaoyuan added 10 commits November 27, 2024 22:13

Update and remove duplicated entries from SolutionItems

f638e2a

Port legacy ruleset into globalconfig

a5cf8b6

Enable recommended documentation rules

a69c2e0

Enable recommended globalization rules

1f604fd

Enable recommended interoperability and maintainability rules

a91fef2

Enable basic performance rules

f24f397

Performance improvement for LINQ usages

b531e25

Enable recommended reliability rules

3887b9b

Selectively opt-in some usage rules

b723add

Remove banned ToUpper and ToLower covered by CA1304

2b68a6a

pull-request-size bot added the size/L label Nov 27, 2024

huoyaoyuan commented Nov 27, 2024

View reviewed changes

Revert change for rule non being enabled

ab9552b

turbedi reviewed Nov 27, 2024

View reviewed changes

Susko3 suggested changes Nov 27, 2024

View reviewed changes

Improve fixed code

a80a4be

peppy self-requested a review November 28, 2024 04:35

peppy approved these changes Nov 28, 2024

View reviewed changes

smoogipoo approved these changes Nov 28, 2024

View reviewed changes

smoogipoo added the type:code-quality label Nov 28, 2024

peppy merged commit 0c99586 into ppy:master Nov 28, 2024
13 of 14 checks passed

huoyaoyuan deleted the netcode-codeanalysis branch November 28, 2024 11:07

huoyaoyuan mentioned this pull request Nov 29, 2024

Set up-to-date .NET code quality analyzers ppy/osu#30921

Merged

frenzibyte mentioned this pull request Dec 4, 2024

Fix recent warnings seen on Rider 2024.3 #6443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up-to-date .NET code quality analyzers #6434

Set up-to-date .NET code quality analyzers #6434

huoyaoyuan commented Nov 27, 2024

huoyaoyuan Nov 27, 2024

huoyaoyuan Nov 27, 2024

huoyaoyuan Nov 27, 2024

turbedi Nov 27, 2024

huoyaoyuan Nov 27, 2024

Susko3 left a comment

Susko3 Nov 27, 2024

Susko3 Nov 27, 2024

huoyaoyuan Nov 27, 2024

Susko3 Nov 27, 2024

huoyaoyuan commented Nov 27, 2024

peppy left a comment

smoogipoo left a comment

	string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
	string[] terms = (searchTerm ?? string.Empty).Split(' ', StringSplitOptions.RemoveEmptyEntries);

Set up-to-date .NET code quality analyzers #6434

Set up-to-date .NET code quality analyzers #6434

Conversation

huoyaoyuan commented Nov 27, 2024

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

Susko3 left a 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

huoyaoyuan commented Nov 27, 2024

peppy left a comment

Choose a reason for hiding this comment

smoogipoo left a comment

Choose a reason for hiding this comment