Apply outstanding FxCop rules #3007

huoyaoyuan · 2019-11-17T17:42:32Z

Oh...there're some rules I've already cleaned 3 years ago.
FxCop was old and containing some unsuitable rules. But it's now maintained by the official compiler team, and adding many new morden rules, and more official rules are going into them, so I pick it.
I've turned on the outstanding ones, and turned off ones conflicting with our current design.
I intended to add comments in the rule set, but VS designer will always remove them.

Supressing some hard-to-apply instances to very valuable diagnostics.

They even don't have agreement on arguments order.

huoyaoyuan · 2019-11-17T17:44:43Z

osu.Framework/Bindables/BindableNumber.cs

                    throw new ArgumentOutOfRangeException(
-                        $"Can not weld bindable longs with non-overlapping min/max-ranges. The ranges were [{MinValue} - {MaxValue}] and [{other.MinValue} - {other.MaxValue}].", nameof(them));
+                        nameof(them), $"Can not weld bindable longs with non-overlapping min/max-ranges. The ranges were [{MinValue} - {MaxValue}] and [{other.MinValue} - {other.MaxValue}].");


ArgumentOutOfRangeException puts param name at first, but ArgumentException puts it at last. I'm not realizing this.

huoyaoyuan · 2019-11-17T17:45:50Z

osu.Framework/Graphics/OpenGL/Textures/TextureGLSingle.cs

-        private static readonly Action<TexturedVertex2D> default_quad_action;
-
-        static TextureGLSingle()
-        {
-            QuadBatch<TexturedVertex2D> quadBatch = new QuadBatch<TexturedVertex2D>(100, 1000);
-            default_quad_action = quadBatch.AddAction;
-        }
+        private static readonly Action<TexturedVertex2D> default_quad_action = new QuadBatch<TexturedVertex2D>(100, 1000).AddAction;


Static constructor has performance impact on every instance method call of the type, according to FxCop doc.

Got a link for this?

Here's one. It has to do with introducing a flag checking whether the static ctor was ever called before to all static methods and instance ctors.

Here's a sharplab snippet showing the difference between an explicit and an implicit static ctor (note the beforefieldinit flag). As stated in the fxcop documentation the implicit static ctor is not guaranteed to finish before any static method call or instance ctor, but is guaranteed to run before all static field accesses.

But then again, it's just a flag check. I really doubt it'll have real and noticeable performance repercussions.

Seems to be a bit over-the-top in terms of optimisation. That said, no harm if the changes don't affect readability, which seems to be the case for the two in this PR.

There are places we use static ctor for more than just initialisation though, so as long as the fxcop rule accounts (and allows) for that, there shouldn't be an issue.

huoyaoyuan · 2019-11-17T17:46:52Z

osu.Framework/Graphics/Sprites/FontUsage.cs

-        public bool Equals(FontUsage other) => string.Equals(Family, other.Family) && string.Equals(Weight, other.Weight) && Italics == other.Italics && Size.Equals(other.Size) && FixedWidth == other.FixedWidth;
+        public bool Equals(FontUsage other) => Family == other.Family && Weight == other.Weight && Italics == other.Italics && Size.Equals(other.Size) && FixedWidth == other.FixedWidth;


== on string uses SequenceEquals. It should be always used unless absolute reason.

huoyaoyuan · 2019-11-17T17:48:15Z

osu.Framework/IO/Network/WebRequest.cs

-        private static readonly Logger logger;
-
-        private static readonly HttpClient client;
-
-        static WebRequest()
+        private static readonly HttpClient client = new HttpClient(new HttpClientHandler { AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate })
        {
-            client = new HttpClient(new HttpClientHandler { AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate });
-            client.DefaultRequestHeaders.UserAgent.ParseAdd("osu!");
-
+            DefaultRequestHeaders =
+            {
+                UserAgent = { ProductInfoHeaderValue.Parse("osu!") }
+            },
            // Timeout is controlled manually through cancellation tokens because
            // HttpClient does not properly timeout while reading chunked data
-            client.Timeout = System.Threading.Timeout.InfiniteTimeSpan;
+            Timeout = System.Threading.Timeout.InfiniteTimeSpan
+        };


Quite tricky here - object initializer syntax allows you to set nested properties without assigning new objects in the first-level property. I learned this just recently.

huoyaoyuan · 2019-11-17T17:49:39Z

osu.Framework/Platform/MacOS/Native/Cocoa.cs

        private static readonly MethodInfo method_cocoa_to_ns_string = type_cocoa.GetMethod("ToNSString");
        private static readonly MethodInfo method_cocoa_get_string_constant = type_cocoa.GetMethod("GetStringConstant");

-        public static IntPtr AppKitLibrary;
-        public static IntPtr FoundationLibrary;
+        public static IntPtr AppKitLibrary = (IntPtr)type_cocoa.GetField("AppKitLibrary").GetValue(null);
+        public static IntPtr FoundationLibrary = (IntPtr)type_cocoa.GetField("FoundationLibrary").GetValue(null);


Static inline initializations are executed in the file order. The code is actually already depending on this in the previous lines.

huoyaoyuan · 2019-11-17T18:01:08Z

CodeFactor should be aware about the member complexity before this PR 😟

bdach · 2019-11-17T18:02:28Z

I sort of disagree with enabling the ones that have to then be explicitly suppressed due to a lack of better alternative.

I believe a blanket ban for static initialisers is too harsh. It leads to suppressions in test code, which is kind of unwarranted since initialisation performance in test code should not matter. Yes, everywhere else it's useful - but not in tests.
The zero-length array inspection really shouldn't complain on things like int[0][].

Personally I'd be fine with enabling these if those two were plain warnings. But it seems we currently have a very binary approach of error/no error.

Oh, and I think codefactor just re-complains whenever a file with previously-dismissed warnings is touched.

huoyaoyuan · 2019-11-17T18:31:03Z

I sort of disagree with enabling the ones that have to then be explicitly suppressed due to a lack of better alternative.

I'm neutual on this. Just tried it and find it's only complained in very few places.

Yes, everywhere else it's useful - but not in tests.

I just think plain supression is less work than creating a splited rule set for tests. It supports inheriting, so not much work neither.

The zero-length array inspection really shouldn't complain on things like int[0][].

The analyzer seems not aware on what's the element type.

Personally I'd be fine with enabling these if those two were plain warnings.

Big project should have all won't-fixes suppressed, or it will be hard to distinguish meaningful ones from them.

smoogipoo · 2019-11-19T07:10:13Z

osu.Framework/Platform/Windows/TimePeriod.cs

@@ -59,7 +59,7 @@ internal bool Active
                    }
                    else
                    {
-                        timeEndPeriod(period);
+                        _ = timeEndPeriod(period);


Disagree with this style.

smoogipoo · 2019-11-19T07:13:14Z

osu.Framework/MathUtils/Interpolation.cs

@@ -405,7 +405,9 @@ internal static class Interpolation<TValue>
    {
        public static readonly InterpolationFunc<TValue> FUNCTION;

+#pragma warning disable CA1810 // Initialize reference type static fields inline


You can make these into fields. I'd previously done it in the past, though it didn't lead to significant performance differences (generally the case with static ctor pre-checks that the JIT/EE does) so I didn't commit it.

smoogipoo · 2019-11-19T07:18:12Z

osu.Framework.Tests/Lists/TestArrayExtensions.cs

@@ -38,7 +38,9 @@ public void TestEmptyRectangularToJagged()
        public void TestEmptyJaggedToRectangular()
        {
            int[,] result = null;
-            Assert.DoesNotThrow(() => result = new int[0][].ToRectangular());
+#pragma warning disable CA1825 // Avoid zero-length array allocations.


Does Array.Empty<int[]> not work?

It just feels not natural in this test case. The test is dealing with [][] and [,], using < []> doesn't look good.

Empty<int[]> makes more sense to me than the original case. It's an empty array containing int[]s, whereas the original code doesn't have the same impact.

smoogipoo · 2019-11-19T07:26:02Z

osu.Framework.Tests/IO/TestWebRequest.cs

@@ -26,9 +26,11 @@ public class TestWebRequest
        private static readonly string host;
        private static readonly IEnumerable<string> protocols;

+#pragma warning disable CA1810 // Initialize reference type static fields inline


I personally disagree with disallowing static ctors. The only reason is performance differences which are extremely hard to benchmark in the first place. I'm ok with converting them over on a per-case basis where appropriate, but even benchmarkdotnet gives BenchmarkClass.StaticCtor: Default -> The method duration is indistinguishable from the empty method duration.

I would generally benchmarks when making such changes.

I'll change the warning to be disabled. The ones able to be written inline should be fine.

…rper config.

smoogipoo · 2019-11-20T02:34:37Z

osu.Framework/MathUtils/Interpolation.cs

-
-        static Interpolation()
+        // Should match the definition of InterpolationFunc and ValueAt
+        private static readonly Type[] parameters =


Why this and not using reflection?

Reflection will make the code into a complex expression that's too complicated.
The arguments and its reflection target (InterpolationFunc) live in the same file. There shouldn't be risk of out of sync.

What do you mean by "complex expression"? From which perspective? Because it'd be just:

private static Type[] parameters = typeof(InterpolationFunc<TValue>) .GetMethod(nameof(InterpolationFunc<TValue>.Invoke)) ?.GetParameters().Select(p => p.ParameterType).ToArray();

I also don't understand what you mean "shouldn't be risk of out of sync", if we change the arguments of ValueAt then we'll have to knowingly update this array without the compiler warning us. So yes there is a risk of being out of sync, even if it's in the same file.

If inline initialization is used, the whole initialization of FUNCTION will be a huge expression. The expression getting the parameters will be inlined.
Reverted the changes that makes it inline, with some small adjust.

This is still risk of out of sync between ValueAt declarations and InterpolationFunc. Creating unused delegates (using direct creation instead of reflection) for them can make compiler error then, but may looks too verbose.

peppy · 2019-12-09T16:35:51Z

osu.Framework/Allocation/CachedAttribute.cs

@@ -160,4 +160,15 @@ internal static CacheDependencyDelegate CreateActivator(Type type)
            }
        }
    }
+
+    /// <summary>
+    /// The exception that is thrown when intending to cache <see langword="null"/> using <see cref="CachedAttribute"/>.


Suggested change

/// The exception that is thrown when intending to cache <see langword="null"/> using <see cref="CachedAttribute"/>.

/// The exception that is thrown when attempting to cache <see langword="null"/> using <see cref="CachedAttribute"/>.

peppy · 2019-12-09T16:36:59Z

osu.Framework/Graphics/OpenGL/Textures/TextureGLSingle.cs

-        private static readonly Action<TexturedVertex2D> default_quad_action;
-
-        static TextureGLSingle()
-        {
-            QuadBatch<TexturedVertex2D> quadBatch = new QuadBatch<TexturedVertex2D>(100, 1000);
-            default_quad_action = quadBatch.AddAction;
-        }
+        private static readonly Action<TexturedVertex2D> default_quad_action = new QuadBatch<TexturedVertex2D>(100, 1000).AddAction;


Got a link for this?

huoyaoyuan added 15 commits November 17, 2019 22:07

Run code analysis on every project.

fd0b551

Add FxCop with everything turned off.

33f6be1

Turn on unrelated rules.

8060e0c

CA1810: don't use static constructor for reference type

4d7a3d3

CA1825: use Array.Empty instead of new [0].

34ec36c

CA1401: P/Invoke should not be public.

8ceac30

CA1820: use string.IsNullOrEmpty.

9b7acc8

CA1827 is accidentally disabled.

d287819

Specify CharSet on Windows P/Invoke with string.

b68a8de

CA2208: create Argument*Exception correctly.

c649422

They even don't have agreement on arguments order.

CA1041: provide message for [Obsolete].

372a567

CA1052: use static class.

fe52be8

CA1806: use discards for ignored returns.

14a5e53

CA2201: throw correct exception type.

3fd1c79

CA1309: compare strings correctly.

c6e2996

bdach added the type:code-quality label Nov 17, 2019

huoyaoyuan commented Nov 17, 2019

View reviewed changes

Assign a new exception type for null cached dependency.

4f1b5b9

huoyaoyuan mentioned this pull request Nov 18, 2019

[WIP] Migrate ReSharper rules to StyleCop rules #2514

Closed

smoogipoo requested changes Nov 19, 2019

View reviewed changes

huoyaoyuan added 6 commits November 19, 2019 17:56

Revert "CA1806: use discards for ignored returns", but keep the ReSha…

a3c8787

…rper config.

Don't warn on CA1810.

7a7f955

Remove supression of CA1825.

73b327c

Merge branch 'master' into fxcop

2d57d5e

Resolve newly caught CA1200.

18b7c99

Cleanup interpolation.cs

99a7c0f

huoyaoyuan force-pushed the fxcop branch from 58aa6a5 to 99a7c0f Compare November 19, 2019 11:27

smoogipoo requested changes Nov 20, 2019

View reviewed changes

huoyaoyuan added 6 commits November 21, 2019 23:07

Merge branch 'master' into fxcop

faadafe

Don't use inline static initialize on Interpolation type.

40397f4

Merge branch 'master' into fxcop

57d184d

Update analyzers version after merge.

3994fc2

Merge branch 'master' into fxcop

473cbb1

Merge branch 'master' into fxcop

53b980b

peppy requested changes Dec 9, 2019

View reviewed changes

huoyaoyuan added 3 commits December 10, 2019 17:51

Fix word in xml doc.

79d91d6

Merge branch 'master' into fxcop

b18c0db

CA1715: use prefix for generic parameters.

b7b25ab

peppy approved these changes Dec 11, 2019

View reviewed changes

smoogipoo approved these changes Dec 12, 2019

View reviewed changes

Merge branch 'master' into fxcop

950446e

smoogipoo merged commit ca513ac into ppy:master Dec 12, 2019

huoyaoyuan deleted the fxcop branch December 12, 2019 10:22

huoyaoyuan mentioned this pull request Dec 17, 2019

Apply FxCop at game side ppy/osu#7238

Merged

huoyaoyuan mentioned this pull request Nov 27, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply outstanding FxCop rules #3007

Apply outstanding FxCop rules #3007

huoyaoyuan commented Nov 17, 2019

huoyaoyuan Nov 17, 2019

huoyaoyuan Nov 17, 2019

peppy Dec 9, 2019

bdach Dec 9, 2019 •

edited

Loading

peppy Dec 9, 2019

huoyaoyuan Nov 17, 2019

huoyaoyuan Nov 17, 2019

huoyaoyuan Nov 17, 2019

huoyaoyuan commented Nov 17, 2019

bdach commented Nov 17, 2019

huoyaoyuan commented Nov 17, 2019

smoogipoo Nov 19, 2019

smoogipoo Nov 19, 2019 •

edited

Loading

smoogipoo Nov 19, 2019

huoyaoyuan Nov 19, 2019

smoogipoo Nov 19, 2019 •

edited

Loading

smoogipoo Nov 19, 2019

huoyaoyuan Nov 19, 2019

smoogipoo Nov 20, 2019

huoyaoyuan Nov 20, 2019

smoogipoo Nov 21, 2019

huoyaoyuan Nov 21, 2019

huoyaoyuan Nov 21, 2019

peppy Dec 9, 2019

peppy Dec 9, 2019

		public bool Equals(FontUsage other) => string.Equals(Family, other.Family) && string.Equals(Weight, other.Weight) && Italics == other.Italics && Size.Equals(other.Size) && FixedWidth == other.FixedWidth;
		public bool Equals(FontUsage other) => Family == other.Family && Weight == other.Weight && Italics == other.Italics && Size.Equals(other.Size) && FixedWidth == other.FixedWidth;

	/// The exception that is thrown when intending to cache <see langword="null"/> using <see cref="CachedAttribute"/>.
	/// The exception that is thrown when attempting to cache <see langword="null"/> using <see cref="CachedAttribute"/>.

Apply outstanding FxCop rules #3007

Apply outstanding FxCop rules #3007

Conversation

huoyaoyuan commented Nov 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Dec 9, 2019 • edited Loading

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 17, 2019

bdach commented Nov 17, 2019

huoyaoyuan commented Nov 17, 2019

Choose a reason for hiding this comment

smoogipoo Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Nov 19, 2019 • edited Loading

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

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

bdach Dec 9, 2019 •

edited

Loading

smoogipoo Nov 19, 2019 •

edited

Loading

smoogipoo Nov 19, 2019 •

edited

Loading