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.

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
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
51 changes: 49 additions & 2 deletions .globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,55 @@ dotnet_diagnostic.IDE0130.severity = warning
# IDE1006: Naming style
dotnet_diagnostic.IDE1006.severity = warning

#Disable operator overloads requiring alternate named methods
dotnet_diagnostic.CA2225.severity = none
# CA1305: Specify IFormatProvider
# Too many noisy warnings for parsing/formatting numbers
dotnet_diagnostic.CA1305.severity = none

# 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
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 LINQ analyzers were major changes comparing to framework side PR. The flagged use cases are much more than framework side, and the performance impact should be lower, so I prefer to not turn on warnings. Instead, suggestions will still be visible in IDE.


# 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
Expand Down
4 changes: 0 additions & 4 deletions CodeAnalysis/BannedSymbols.txt
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
58 changes: 0 additions & 58 deletions CodeAnalysis/osu.ruleset

This file was deleted.

12 changes: 11 additions & 1 deletion Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,17 @@
<AdditionalFiles Include="$(MSBuildThisFileDirectory)CodeAnalysis\BannedSymbols.txt" />
</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
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
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
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
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
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
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
6 changes: 0 additions & 6 deletions osu.Game.Tests/tests.ruleset

This file was deleted.

22 changes: 12 additions & 10 deletions osu.Game/Database/RealmObjectExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -248,29 +248,30 @@ public static Live<T> ToLive<T>(this T realmObject, RealmAccess realm)
return new RealmLive<T>(realmObject, realm);
}

#pragma warning disable RS0030 // mentioning banned symbols in documentation
/// <summary>
/// Register a callback to be invoked each time this <see cref="T:Realms.IRealmCollection`1" /> changes.
/// Register a callback to be invoked each time this <see cref="IRealmCollection{T}" /> changes.
/// </summary>
/// <remarks>
/// <para>
/// This adds osu! specific thread and managed state safety checks on top of <see cref="IRealmCollection{T}.SubscribeForNotifications"/>.
/// </para>
/// <para>
/// The first callback will be invoked with the initial <see cref="T:Realms.IRealmCollection`1" /> after the asynchronous query completes,
/// The first callback will be invoked with the initial <see cref="IRealmCollection{T}" /> after the asynchronous query completes,
/// and then called again after each write transaction which changes either any of the objects in the collection, or
/// which objects are in the collection. The <c>changes</c> parameter will
/// be <c>null</c> the first time the callback is invoked with the initial results. For each call after that,
/// it will contain information about which rows in the results were added, removed or modified.
/// </para>
/// <para>
/// If a write transaction did not modify any objects in this <see cref="T:Realms.IRealmCollection`1" />, the callback is not invoked at all.
/// If a write transaction did not modify any objects in this <see cref="IRealmCollection{T}" />, the callback is not invoked at all.
/// If an error occurs the callback will be invoked with <c>null</c> for the <c>sender</c> parameter and a non-<c>null</c> <c>error</c>.
/// Currently the only errors that can occur are when opening the <see cref="T:Realms.Realm" /> on the background worker thread.
/// Currently the only errors that can occur are when opening the <see cref="Realm" /> on the background worker thread.
/// </para>
/// <para>
/// At the time when the block is called, the <see cref="T:Realms.IRealmCollection`1" /> object will be fully evaluated
/// At the time when the block is called, the <see cref="IRealmCollection{T}" /> object will be fully evaluated
/// and up-to-date, and as long as you do not perform a write transaction on the same thread
/// or explicitly call <see cref="M:Realms.Realm.Refresh" />, accessing it will never perform blocking work.
/// or explicitly call <see cref="Realm.Refresh" />, accessing it will never perform blocking work.
/// </para>
/// <para>
/// Notifications are delivered via the standard event loop, and so can't be delivered while the event loop is blocked by other activity.
Expand All @@ -279,13 +280,14 @@ public static Live<T> ToLive<T>(this T realmObject, RealmAccess realm)
/// </para>
/// </remarks>
/// <param name="collection">The <see cref="IRealmCollection{T}"/> to observe for changes.</param>
/// <param name="callback">The callback to be invoked with the updated <see cref="T:Realms.IRealmCollection`1" />.</param>
/// <param name="callback">The callback to be invoked with the updated <see cref="IRealmCollection{T}" />.</param>
/// <returns>
/// A subscription token. It must be kept alive for as long as you want to receive change notifications.
/// To stop receiving notifications, call <see cref="M:System.IDisposable.Dispose" />.
/// To stop receiving notifications, call <see cref="IDisposable.Dispose" />.
/// </returns>
/// <seealso cref="M:Realms.CollectionExtensions.SubscribeForNotifications``1(System.Collections.Generic.IList{``0},Realms.NotificationCallbackDelegate{``0})" />
/// <seealso cref="M:Realms.CollectionExtensions.SubscribeForNotifications``1(System.Linq.IQueryable{``0},Realms.NotificationCallbackDelegate{``0})" />
/// <seealso cref="Realms.CollectionExtensions.SubscribeForNotifications{T}(IList{T}, NotificationCallbackDelegate{T})" />
/// <seealso cref="Realms.CollectionExtensions.SubscribeForNotifications{T}(IQueryable{T}, NotificationCallbackDelegate{T})" />
#pragma warning restore RS0030
public static IDisposable QueryAsyncWithNotifications<T>(this IRealmCollection<T> collection, NotificationCallbackDelegate<T> callback)
where T : RealmObjectBase
{
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Extensions/StringDehumanizeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public static string ToPascalCase(this string input)
public static string ToCamelCase(this string input)
{
string word = input.ToPascalCase();
return word.Length > 0 ? word.Substring(0, 1).ToLowerInvariant() + word.Substring(1) : word;
return word.Length > 0 ? char.ToLowerInvariant(word[0]) + word.Substring(1) : word;
}

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions osu.Game/Online/API/Requests/Responses/APIKudosuHistory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ public class KudosuGiver

public KudosuAction Action;

#pragma warning disable CA1507 // Happens to name the same because of casing preference
[JsonProperty("action")]
#pragma warning restore CA1507
private string action
{
set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ public class APIUserMostPlayedBeatmap
[JsonProperty("count")]
public int PlayCount { get; set; }

#pragma warning disable CA1507 // Happens to name the same because of casing preference
[JsonProperty("beatmap")]
#pragma warning restore CA1507
private APIBeatmap beatmap { get; set; }

public APIBeatmap BeatmapInfo
Expand Down
Loading
Loading