Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 14 commits into from
Dec 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8f6e5c4
Convert legacy ruleset to globalconfig
huoyaoyuan Nov 28, 2024
c57ace0
Enable recommended rules for documentation
huoyaoyuan Nov 28, 2024
cd9b592
Enable recommended rules for globalization
huoyaoyuan Nov 28, 2024
13d7c6a
Enable recommended rules for maintainability
huoyaoyuan Nov 28, 2024
f5a7716
Apply minor performance rules
huoyaoyuan Nov 28, 2024
5b63d72
Mark CA1826/CA1860 as suggestion
huoyaoyuan Nov 28, 2024
0a8ec4d
Enable recommended rules for reliability
huoyaoyuan Nov 28, 2024
fced254
Enable selected rules for usage
huoyaoyuan Nov 28, 2024
fa3c95c
Merge branch 'master'
huoyaoyuan Nov 29, 2024
68f2170
Fix CA1865
huoyaoyuan Nov 29, 2024
68f4fa5
Turn off CA1507
huoyaoyuan Dec 2, 2024
7ece8ec
Update for suggestions
huoyaoyuan Dec 2, 2024
d92ea91
Merge branch 'master' into netcore-analyzer
huoyaoyuan Dec 5, 2024
68e400d
Put globalconfig into seperated folder and reference explicitly
huoyaoyuan Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 0 additions & 57 deletions .globalconfig
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions CodeAnalysis/BannedSymbols.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ M:Realms.CollectionExtensions.SubscribeForNotifications`1(System.Collections.Gen
M:System.Threading.Tasks.Task.Wait();Don't use Task.Wait. Use Task.WaitSafely() to ensure we avoid deadlocks.
P:System.Threading.Tasks.Task`1.Result;Don't use Task.Result. Use Task.GetResultSafely() to ensure we avoid deadlocks.
M:System.Threading.ManualResetEventSlim.Wait();Specify a timeout to avoid waiting forever.
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.
M:Humanizer.InflectorExtensions.Pascalize(System.String);Humanizer's .Pascalize() extension method changes behaviour depending on CultureInfo.CurrentCulture. Use StringDehumanizeExtensions.ToPascalCase() instead.
M:Humanizer.InflectorExtensions.Camelize(System.String);Humanizer's .Camelize() extension method changes behaviour depending on CultureInfo.CurrentCulture. Use StringDehumanizeExtensions.ToCamelCase() instead.
M:Humanizer.InflectorExtensions.Underscore(System.String);Humanizer's .Underscore() extension method changes behaviour depending on CultureInfo.CurrentCulture. Use StringDehumanizeExtensions.ToSnakeCase() instead.
Expand Down
109 changes: 109 additions & 0 deletions CodeAnalysis/osu.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# .NET Code Style
# IDE styles reference: https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/
is_global = true

# IDE0001: Simplify names
dotnet_diagnostic.IDE0001.severity = warning

# IDE0002: Simplify member access
dotnet_diagnostic.IDE0002.severity = warning

# IDE0003: Remove qualification
dotnet_diagnostic.IDE0003.severity = warning

# IDE0004: Remove unnecessary cast
dotnet_diagnostic.IDE0004.severity = warning

# IDE0005: Remove unnecessary imports
dotnet_diagnostic.IDE0005.severity = warning

# IDE0034: Simplify default literal
dotnet_diagnostic.IDE0034.severity = warning

# IDE0036: Sort modifiers
dotnet_diagnostic.IDE0036.severity = warning

# IDE0040: Add accessibility modifier
dotnet_diagnostic.IDE0040.severity = warning

# IDE0049: Use keyword for type name
dotnet_diagnostic.IDE0040.severity = warning

# IDE0055: Fix formatting
dotnet_diagnostic.IDE0055.severity = warning

# IDE0051: Private method is unused
dotnet_diagnostic.IDE0051.severity = silent

# IDE0052: Private member is unused
dotnet_diagnostic.IDE0052.severity = silent

# IDE0073: File header
dotnet_diagnostic.IDE0073.severity = warning

# IDE0130: Namespace mismatch with folder
dotnet_diagnostic.IDE0130.severity = warning

# IDE1006: Naming style
dotnet_diagnostic.IDE1006.severity = warning

# CA1305: Specify IFormatProvider
# Too many noisy warnings for parsing/formatting numbers
dotnet_diagnostic.CA1305.severity = none

# CA1507: Use nameof to express symbol names
# Flaggs serialization name attributes
dotnet_diagnostic.CA1507.severity = suggestion

# CA1806: Do not ignore method results
# The usages for numeric parsing are explicitly optional
dotnet_diagnostic.CA1806.severity = suggestion

# CA1822: Mark members as static
# Potential false positive around reflection/too much noise
dotnet_diagnostic.CA1822.severity = none

# CA1826: Do not use Enumerable method on indexable collections
dotnet_diagnostic.CA1826.severity = suggestion

# CA1859: Use concrete types when possible for improved performance
# Involves design considerations
dotnet_diagnostic.CA1859.severity = suggestion

# CA1860: Avoid using 'Enumerable.Any()' extension method
dotnet_diagnostic.CA1860.severity = suggestion

# CA1861: Avoid constant arrays as arguments
# Outdated with collection expressions
dotnet_diagnostic.CA1861.severity = suggestion

# CA2007: Consider calling ConfigureAwait on the awaited task
dotnet_diagnostic.CA2007.severity = warning

# CA2016: Forward the 'CancellationToken' parameter to methods
# Some overloads are having special handling for debugger
dotnet_diagnostic.CA2016.severity = suggestion

# CA2021: Do not call Enumerable.Cast<T> or Enumerable.OfType<T> with incompatible types
# Causing a lot of false positives with generics
dotnet_diagnostic.CA2021.severity = none

# CA2101: Specify marshaling for P/Invoke string arguments
# Reports warning for all non-UTF16 usages on DllImport; consider migrating to LibraryImport
dotnet_diagnostic.CA2101.severity = none

# CA2201: Do not raise reserved exception types
dotnet_diagnostic.CA2201.severity = warning

# CA2208: Instantiate argument exceptions correctly
dotnet_diagnostic.CA2208.severity = suggestion

# CA2242: Test for NaN correctly
dotnet_diagnostic.CA2242.severity = warning

# Banned APIs
dotnet_diagnostic.RS0030.severity = error

# Temporarily disable analysing CanBeNull = true in NRT contexts due to mobile issues.
# See: https://github.com/ppy/osu/pull/19677
dotnet_diagnostic.OSUF001.severity = none
58 changes: 0 additions & 58 deletions CodeAnalysis/osu.ruleset
View file
Open in desktop

This file was deleted.

14 changes: 13 additions & 1 deletion Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,21 @@
<ItemGroup Label="Code Analysis">
<PackageReference Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.3.4" PrivateAssets="All" />
<AdditionalFiles Include="$(MSBuildThisFileDirectory)CodeAnalysis\BannedSymbols.txt" />
<!-- Rider compatibility: .globalconfig needs to be explicitly referenced instead of using the global file name. -->
<GlobalAnalyzerConfigFiles Include="$(MSBuildThisFileDirectory)CodeAnalysis\osu.globalconfig" />
</ItemGroup>
<PropertyGroup Label="Code Analysis">
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)CodeAnalysis\osu.ruleset</CodeAnalysisRuleSet>
<AnalysisMode>Default</AnalysisMode>
<AnalysisModeDesign>Default</AnalysisModeDesign>
<AnalysisModeDocumentation>Recommended</AnalysisModeDocumentation>
<AnalysisModeGlobalization>Recommended</AnalysisModeGlobalization>
<AnalysisModeInteroperability>Recommended</AnalysisModeInteroperability>
<AnalysisModeMaintainability>Recommended</AnalysisModeMaintainability>
<AnalysisModeNaming>Default</AnalysisModeNaming>
<AnalysisModePerformance>Minimum</AnalysisModePerformance>
<AnalysisModeReliability>Recommended</AnalysisModeReliability>
<AnalysisModeSecurity>Default</AnalysisModeSecurity>
<AnalysisModeUsage>Default</AnalysisModeUsage>
</PropertyGroup>
<PropertyGroup Label="Documentation">
<GenerateDocumentationFile>true</GenerateDocumentationFile>
Expand Down
20 changes: 10 additions & 10 deletions osu.Game.Rulesets.Mania/Skinning/Argon/ManiaArgonSkinTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 1: return colour_cyan;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 3:
Expand All @@ -176,7 +176,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 2: return colour_cyan;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 4:
Expand All @@ -190,7 +190,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 3: return colour_purple;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 5:
Expand All @@ -206,7 +206,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 4: return colour_cyan;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 6:
Expand All @@ -224,7 +224,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 5: return colour_pink;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 7:
Expand All @@ -244,7 +244,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 6: return colour_pink;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 8:
Expand All @@ -266,7 +266,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 7: return colour_purple;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 9:
Expand All @@ -290,7 +290,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 8: return colour_purple;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}

case 10:
Expand All @@ -316,7 +316,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 9: return colour_purple;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}
}

Expand All @@ -339,7 +339,7 @@ private Color4 getColourForLayout(int columnIndex, StageDefinition stage)

case 5: return colour_green;

default: throw new ArgumentOutOfRangeException();
default: throw new ArgumentOutOfRangeException(nameof(columnIndex));
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions osu.Game.Tests/CodeAnalysis.tests.globalconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# Higher global_level has higher priority, the default global_level
# is 100 for root .globalconfig and 0 for others
# https://learn.microsoft.com/dotnet/fundamentals/code-analysis/configuration-files#precedence
is_global = true
global_level = 101

dotnet_diagnostic.CA2007.severity = none
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/Gameplay/TestScenePlayerLocalScoreImport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public void TestReplayExport()
string? filePath = null;

// Files starting with _ are temporary, created by CreateFileSafely call.
AddUntilStep("wait for export file", () => filePath = LocalStorage.GetFiles("exports").SingleOrDefault(f => !Path.GetFileName(f).StartsWith("_", StringComparison.Ordinal)), () => Is.Not.Null);
AddUntilStep("wait for export file", () => filePath = LocalStorage.GetFiles("exports").SingleOrDefault(f => !Path.GetFileName(f).StartsWith('_')), () => Is.Not.Null);
AddUntilStep("filesize is non-zero", () =>
{
try
Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerParticipantsList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ public void TestKickButtonOnlyPresentWhenHost()

AddStep("make second user host", () => MultiplayerClient.TransferHost(3));

AddUntilStep("kick buttons not visible", () => this.ChildrenOfType<ParticipantPanel.KickButton>().Count(d => d.IsPresent) == 0);
AddUntilStep("kick buttons not visible", () => !this.ChildrenOfType<ParticipantPanel.KickButton>().Any(d => d.IsPresent));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this debatable. This is test code, so performance isn't a matter, and .Count() > 0 is way clearer than .Any() in my book. To the point that rider has begun flagging .Any() usages and suggesting to replace them with count checks at hint level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The story about Any vs Count is beyond this. CA1860 is about using .Count > 0 instead of predicate-less .Any(), and has a lot of violations in both game and test code. When using a predicate, the performance characteristic reverses.

To the point that rider has begun flagging .Any() usages and suggesting to replace them with count checks at hint level.

Do you mean predicate-less Any? This is the same as CA1860.

To be straight, performance-wise, .Count is better than .Any(), but Any() is better than .Count(), and .Any(predicate) is better than .Count(predicate). This leads to inconsistent style for maximizing performance.

I'm very sure that the performance is not important here. The question is whether to prefer .Count > 0 over Any() for every cases - it has about 30 violations in game code base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is whether to prefer .Count > 0 over Any() for every cases

I'd say it's situational, at least for test code. At the end of the day it's up to the reviewers to be able to understand the code and feel whether it's maintainable or not.

For operational code, I think we can do manual review to keep things simple.

Personally, whenever I see !x.Any(y), I always have to go and check the documentation for the empty case especially as it relates to x.All(!y). With this context, Count() is easier to mentally parse for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meets my expectation to keep things as-is and review in future.


AddStep("make local user host again", () => MultiplayerClient.TransferHost(API.LocalUser.Value.Id));

Expand Down
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/UserInterface/TestSceneDeleteLocalScore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public void TestDeleteViaRightClick()

AddStep("click delete option", () =>
{
InputManager.MoveMouseTo(contextMenuContainer.ChildrenOfType<DrawableOsuMenuItem>().First(i => i.Item.Text.Value.ToString().ToLowerInvariant() == "delete"));
InputManager.MoveMouseTo(contextMenuContainer.ChildrenOfType<DrawableOsuMenuItem>().First(i => string.Equals(i.Item.Text.Value.ToString(), "delete", System.StringComparison.OrdinalIgnoreCase)));
InputManager.Click(MouseButton.Left);
});

Expand Down
6 changes: 3 additions & 3 deletions osu.Game.Tests/osu.Game.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
<OutputType>WinExe</OutputType>
<TargetFramework>net8.0</TargetFramework>
</PropertyGroup>
<PropertyGroup Label="Code Analysis">
<CodeAnalysisRuleSet>tests.ruleset</CodeAnalysisRuleSet>
</PropertyGroup>
<ItemGroup Label="Code Analysis">
<GlobalAnalyzerConfigFiles Include="CodeAnalysis.tests.globalconfig" />
</ItemGroup>
<ItemGroup Label="Project References">
<ProjectReference Include="..\osu.Game.Rulesets.Osu\osu.Game.Rulesets.Osu.csproj" />
<ProjectReference Include="..\osu.Game.Rulesets.Catch\osu.Game.Rulesets.Catch.csproj" />
Expand Down
Loading
Loading