Set up-to-date .NET code quality analyzers #30921
Conversation
.globalconfig
Outdated
| # 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 |
There was a problem hiding this comment.
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.
| #pragma warning disable CA1507 // Happens to name the same because of casing preference | ||
| [JsonProperty(@"users")] | ||
| #pragma warning restore CA1507 |
There was a problem hiding this comment.
I don't know about this, I don't feel the presence of this inspection makes having these sparkled in multiple JSON models worth it.
There was a problem hiding this comment.
@huoyaoyuan Please disable CA1507 project-wide. This having to be disabled here is proof to me that the inspection is directly harmful. Unless the dotnet team can somehow make it not fire on members related to serialisation where it is explicitly unwanted to have C# member renames also change the field names used in serialisation, I'm not interested.
It's not a "don't know about this" from me, it's a very strong "this is bad and will lead to bad things happening".
| @@ -173,10 +173,10 @@ public void ApplyToDrawableHitObject(DrawableHitObject drawable) | |||
| }; | |||
| drawable.OnRevertResult += (_, result) => | |||
| { | |||
| if (!ratesForRewinding.ContainsKey(result.HitObject)) return; | |||
| if (!ratesForRewinding.TryGetValue(result.HitObject, out double rewindValue)) return; | |||
osu.Game/Overlays/ChatOverlay.cs
Outdated
| @@ -386,9 +386,8 @@ private void joinedChannelsChanged(object? sender, NotifyCollectionChangedEventA | |||
| { | |||
| channelList.RemoveChannel(channel); | |||
|
|
|||
| if (loadedChannels.ContainsKey(channel)) | |||
| if (loadedChannels.TryGetValue(channel, out var loaded)) | |||
There was a problem hiding this comment.
Code inspector suggests changing this to:
if (loadedChannels.Remove(channel, out var loaded))
{
// DrawableChannel removed from cache must be manually disposed
loaded.Dispose();
}Which sounds good.
There was a problem hiding this comment.
There are CA1853 and CA1854 for interaction with getter and Remove, but no rule for them three. Looks fine.
| @@ -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)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The question is whether to prefer
.Count > 0overAny()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.
There was a problem hiding this comment.
Meets my expectation to keep things as-is and review in future.
| #pragma warning disable CA1507 // Happens to name the same because of casing preference | ||
| [JsonProperty(@"users")] | ||
| #pragma warning restore CA1507 |
There was a problem hiding this comment.
@huoyaoyuan Please disable CA1507 project-wide. This having to be disabled here is proof to me that the inspection is directly harmful. Unless the dotnet team can somehow make it not fire on members related to serialisation where it is explicitly unwanted to have C# member renames also change the field names used in serialisation, I'm not interested.
It's not a "don't know about this" from me, it's a very strong "this is bad and will lead to bad things happening".
|
Worth to mention that similar to ppy/osu-framework#6444 this should be updated to make the |
Similar to framework side PR ppy/osu-framework#6434.
The severity of some analyzers was tuned to reduce noise or changes required.