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 #6434

Merged
merged 12 commits into from
Nov 28, 2024
Merged
45 changes: 43 additions & 2 deletions .globalconfig
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,49 @@ 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
# Not sure why it's performance, but causing too much noise
dotnet_diagnostic.CA1806.severity = none

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

# CA1826: Do not use Enumerable methods on indexable collections
# Noise for FirstOrDefault and LastOrDefault
dotnet_diagnostic.CA1826.severity = none

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

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

# CA1868: Unnecessary call to 'Contains(item)'
# Causing noises only
dotnet_diagnostic.CA1868.severity = none

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

# 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 = warning

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

# Banned APIs
dotnet_diagnostic.RS0030.severity = error
4 changes: 0 additions & 4 deletions CodeAnalysis/BannedSymbols.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,4 @@ M:System.Threading.Tasks.Task.Wait();Don't use Task.Wait. Use Task.WaitSafely()
M:System.Guid.#ctor;Probably meaning to use Guid.NewGuid() instead. If actually wanting empty, use Guid.Empty.
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.
Comment on lines -11 to -14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are covered in CA1304.

M:System.Reflection.Assembly.GetEntryAssembly();Use osu.Framework.RuntimeInfo.EntryAssembly instead
72 changes: 0 additions & 72 deletions CodeAnalysis/osu-framework.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 @@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<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="NuGet">
<Authors>ppy Pty Ltd</Authors>
Expand Down
3 changes: 1 addition & 2 deletions osu-framework.sln
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
.globalconfig = .globalconfig
Directory.Build.props = Directory.Build.props
.config\dotnet-tools.json = .config\dotnet-tools.json
CodeAnalysis\osu-framework.ruleset = CodeAnalysis\osu-framework.ruleset
global.json = global.json
osu-framework.sln.DotSettings = osu-framework.sln.DotSettings
osu.Framework.Android.props = osu.Framework.Android.props
osu.Framework.iOS.props = osu.Framework.iOS.props
Expand Down Expand Up @@ -74,7 +74,6 @@ EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "CodeAnalysis", "CodeAnalysis", "{50334A1F-990D-45FA-A1FE-C92F1994D97B}"
ProjectSection(SolutionItems) = preProject
CodeAnalysis\BannedSymbols.txt = CodeAnalysis\BannedSymbols.txt
CodeAnalysis\osu-framework.ruleset = CodeAnalysis\osu-framework.ruleset
EndProjectSection
EndProject
Global
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public static void VerifyZeroDiagnostics(this GeneratorDriverRunResult runResult
.Where(d => d.Severity == DiagnosticSeverity.Error)
.ToArray();

if (compilationDiagnostics.Any() || generatorDiagnostics.Any())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (compilationDiagnostics.Length > 0 || generatorDiagnostics.Length > 0)
{
var sb = new StringBuilder();

Expand Down
3 changes: 2 additions & 1 deletion osu.Framework.Tests/Audio/AudioThreadTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Threading.Tasks;
using NUnit.Framework;
using osu.Framework.Audio.Track;
using osu.Framework.Extensions;
using osu.Framework.IO.Stores;
using osu.Framework.Threading;

Expand Down Expand Up @@ -87,7 +88,7 @@ void runScheduled() => thread.Scheduler.Add(() =>

runScheduled();

Task.WaitAll(cts.Task);
cts.Task.WaitSafely();
}
}
}
2 changes: 2 additions & 0 deletions osu.Framework.Tests/Containers/TestSceneContainerState.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
using osu.Framework.Tests.Visual;
using osuTK;

#pragma warning disable CA1826 // Performance for test is not important

namespace osu.Framework.Tests.Containers
{
[System.ComponentModel.Description("ensure valid container state in various scenarios")]
Expand Down
4 changes: 4 additions & 0 deletions osu.Framework.Tests/IO/TestWebRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,9 @@ private class HttpBinPostResponse
[JsonProperty("json")]
public TestObject Json { get; set; }

#pragma warning disable CA1507 // Happens to name the same because of casing preference
[JsonProperty("form")]
#pragma warning restore CA1507
private Dictionary<string, object> form { get; set; }
}

Expand All @@ -906,7 +908,9 @@ private class HttpBinPutResponse
[JsonProperty("args")]
private Dictionary<string, object> arguments { get; set; }

#pragma warning disable CA1507 // Happens to name the same because of casing preference
[JsonProperty("form")]
#pragma warning restore CA1507
private Dictionary<string, object> form { get; set; }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
using osu.Framework.Localisation;
using osuTK;

#pragma warning disable CA1826 // Performance for test is not important

namespace osu.Framework.Tests.Visual.UserInterface
{
public partial class TestSceneTabControl : FrameworkTestScene
Expand Down
18 changes: 9 additions & 9 deletions osu.Framework/Audio/Mixing/Bass/BassAudioMixer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public long ChannelGetPosition(IBassAudioChannel channel, PositionFlags mode = P
/// <param name="mode">How to set the position.</param>
/// <returns>
/// If successful, then <see langword="true"/> is returned, else <see langword="false"/> is returned.
/// Use <see cref="P:ManagedBass.Bass.LastError"/> to get the error code.
/// Use <see cref="ManagedBass.Bass.LastError"/> to get the error code.
/// </returns>
public bool ChannelSetPosition(IBassAudioChannel channel, long position, PositionFlags mode = PositionFlags.Bytes)
{
Expand Down Expand Up @@ -222,11 +222,11 @@ public bool ChannelGetLevel(IBassAudioChannel channel, [In, Out] float[] levels,
/// <remarks>See: <see cref="ManagedBass.Bass.ChannelGetData(int, float[], int)"/>.</remarks>
/// <param name="channel">The <see cref="IBassAudioChannel"/> to retrieve the data of.</param>
/// <param name="buffer">float[] to write the data to.</param>
/// <param name="length">Number of bytes wanted, and/or <see cref="T:ManagedBass.DataFlags"/>.</param>
/// <returns>If an error occurs, -1 is returned, use <see cref="P:ManagedBass.Bass.LastError"/> to get the error code.
/// <param name="length">Number of bytes wanted, and/or <see cref="DataFlags"/>.</param>
/// <returns>If an error occurs, -1 is returned, use <see cref="ManagedBass.Bass.LastError"/> to get the error code.
/// <para>When requesting FFT data, the number of bytes read from the channel (to perform the FFT) is returned.</para>
/// <para>When requesting sample data, the number of bytes written to buffer will be returned (not necessarily the same as the number of bytes read when using the <see cref="F:ManagedBass.DataFlags.Float"/> or DataFlags.Fixed flag).</para>
/// <para>When using the <see cref="F:ManagedBass.DataFlags.Available"/> flag, the number of bytes in the channel's buffer is returned.</para>
/// <para>When requesting sample data, the number of bytes written to buffer will be returned (not necessarily the same as the number of bytes read when using the <see cref="DataFlags.Float"/> or DataFlags.Fixed flag).</para>
/// <para>When using the <see cref="DataFlags.Available"/> flag, the number of bytes in the channel's buffer is returned.</para>
/// </returns>
public int ChannelGetData(IBassAudioChannel channel, float[] buffer, int length)
=> BassMix.ChannelGetData(channel.Handle, buffer, length);
Expand All @@ -240,24 +240,24 @@ public int ChannelGetData(IBassAudioChannel channel, float[] buffer, int length)
/// <param name="parameter">The sync parameters, depending on the sync type.</param>
/// <param name="procedure">The callback function which should be invoked with the sync.</param>
/// <param name="user">User instance data to pass to the callback function.</param>
/// <returns>If successful, then the new synchroniser's handle is returned, else 0 is returned. Use <see cref="P:ManagedBass.Bass.LastError" /> to get the error code.</returns>
/// <returns>If successful, then the new synchroniser's handle is returned, else 0 is returned. Use <see cref="ManagedBass.Bass.LastError" /> to get the error code.</returns>
public int ChannelSetSync(IBassAudioChannel channel, SyncFlags type, long parameter, SyncProcedure procedure, IntPtr user = default)
=> BassMix.ChannelSetSync(channel.Handle, type, parameter, procedure, user);

/// <summary>
/// Removes a synchroniser from a mixer source channel.
/// </summary>
/// <param name="channel">The <see cref="IBassAudioChannel"/> to remove the synchroniser for.</param>
/// <param name="sync">Handle of the synchroniser to remove (return value of a previous <see cref="M:ManagedBass.Mix.BassMix.ChannelSetSync(System.Int32,ManagedBass.SyncFlags,System.Int64,ManagedBass.SyncProcedure,System.IntPtr)" /> call).</param>
/// <returns>If successful, <see langword="true" /> is returned, else <see langword="false" /> is returned. Use <see cref="P:ManagedBass.Bass.LastError" /> to get the error code.</returns>
/// <param name="sync">Handle of the synchroniser to remove (return value of a previous <see cref="BassMix.ChannelSetSync(int,SyncFlags,long,SyncProcedure,IntPtr)" /> call).</param>
/// <returns>If successful, <see langword="true" /> is returned, else <see langword="false" /> is returned. Use <see cref="ManagedBass.Bass.LastError" /> to get the error code.</returns>
public bool ChannelRemoveSync(IBassAudioChannel channel, int sync)
=> BassMix.ChannelRemoveSync(channel.Handle, sync);

/// <summary>
/// Frees a channel's resources.
/// </summary>
/// <param name="channel">The <see cref="IBassAudioChannel"/> to free.</param>
/// <returns>If successful, <see langword="true" /> is returned, else <see langword="false" /> is returned. Use <see cref="P:ManagedBass.Bass.LastError" /> to get the error code.</returns>
/// <returns>If successful, <see langword="true" /> is returned, else <see langword="false" /> is returned. Use <see cref="ManagedBass.Bass.LastError" /> to get the error code.</returns>
public bool StreamFree(IBassAudioChannel channel)
{
Remove(channel, false);
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework/Audio/Track/Waveform.cs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ private float computeIntensity(ChannelInfo info, float[] bins, float startFreque
/// <returns>An async task for the generation of the <see cref="Waveform"/>.</returns>
public async Task<Waveform> GenerateResampledAsync(int pointCount, CancellationToken cancellationToken = default)
{
if (pointCount < 0) throw new ArgumentOutOfRangeException(nameof(pointCount));
ArgumentOutOfRangeException.ThrowIfNegative(pointCount);

if (pointCount == 0)
return new Waveform(null);
Expand Down
4 changes: 2 additions & 2 deletions osu.Framework/Graphics/Containers/FlowContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,10 @@ public void Insert(int position, T drawable)
/// <returns>The position of the drawable in the layout.</returns>
public float GetLayoutPosition(Drawable drawable)
{
if (!layoutChildren.ContainsKey(drawable))
if (!layoutChildren.TryGetValue(drawable, out float value))
throw new InvalidOperationException($"Cannot get layout position of drawable which is not contained within this {nameof(FlowContainer<T>)}.");

return layoutChildren[drawable];
return value;
}

protected override bool UpdateChildrenLife()
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework/Graphics/Containers/SearchContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ protected void Filter()
private void performFilter()
{
string[] terms = (searchTerm ?? string.Empty).Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

matchSubTree(this, terms, terms.Any(), allowNonContiguousMatching);
matchSubTree(this, terms, terms.Length > 0, allowNonContiguousMatching);
}

private bool matchSubTree(Drawable drawable, IReadOnlyList<string> searchTerms, bool searchActive, bool nonContiguousMatching)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ internal class GLShaderStorageBufferObject<TData> : IShaderStorageBufferObject<T
public int Id { get; }

private readonly TData[] data;
private readonly uint elementSize;
private readonly int elementSize;

public GLShaderStorageBufferObject(GLRenderer renderer, int uboSize, int ssboSize)
{
Expand All @@ -27,10 +27,10 @@ public GLShaderStorageBufferObject(GLRenderer renderer, int uboSize, int ssboSiz
Id = GL.GenBuffer();
Size = renderer.UseStructuredBuffers ? ssboSize : uboSize;
data = new TData[Size];
elementSize = (uint)Marshal.SizeOf(default(TData));
elementSize = Marshal.SizeOf(default(TData));

GL.BindBuffer(BufferTarget.UniformBuffer, Id);
GL.BufferData(BufferTarget.UniformBuffer, (IntPtr)(elementSize * Size), ref data[0], BufferUsageHint.DynamicDraw);
GL.BufferData(BufferTarget.UniformBuffer, elementSize * Size, ref data[0], BufferUsageHint.DynamicDraw);
GL.BindBuffer(BufferTarget.UniformBuffer, 0);
}

Expand Down Expand Up @@ -73,7 +73,7 @@ public void Flush()
return;

GL.BindBuffer(BufferTarget.UniformBuffer, Id);
GL.BufferSubData(BufferTarget.UniformBuffer, (IntPtr)(changeBeginIndex * elementSize), (IntPtr)(elementSize * changeCount), ref data[changeBeginIndex]);
GL.BufferSubData(BufferTarget.UniformBuffer, changeBeginIndex * elementSize, elementSize * changeCount, ref data[changeBeginIndex]);
GL.BindBuffer(BufferTarget.UniformBuffer, 0);

changeBeginIndex = -1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public DeferredVertexBatch(DeferredRenderer renderer, PrimitiveTopology topology
throw new NotImplementedException($"Topology '{topology}' is not yet implemented for this renderer.");

default:
throw new ArgumentOutOfRangeException();
throw new ArgumentOutOfRangeException(nameof(topology));
}
}
else
Expand Down
2 changes: 1 addition & 1 deletion osu.Framework/Graphics/UserInterface/Dropdown.cs
Original file line number Diff line number Diff line change
Expand Up @@ -701,7 +701,7 @@ protected override bool OnKeyDown(KeyDownEvent e)
{
var visibleMenuItemsList = VisibleMenuItems.ToList();

if (visibleMenuItemsList.Any())
if (visibleMenuItemsList.Count > 0)
{
var currentPreselected = PreselectedItem;
int targetPreselectionIndex = visibleMenuItemsList.IndexOf(currentPreselected);
Expand Down
Loading
Loading