Bump AnalysisLevel to 8

[Morilli]

This bumps the AnalysisLevel for code analyzers from 6 to 8, enabling more analyzers by default. This is now safe as we require the .NET 8 SDK to build.

I've applied fixes and severity changes to all new inspections. Open to feedback on whether any of those are disagreed on.

Check if completed:

• I have run any relevant test suites
• I, the committer, have read the licensing terms for contributors (last updated 2024-06-22) and am compliant

[YoshiRulz]

Rebased, dropping commits which are already on master. My thoughts:

• CA1852 "Seal internal types": I approve, but I think making them structs or otherwise rethinking the data representation would have a far greater impact than letting the JIT maybe elide some type checks. I would support your alternative idea of muting it for the time being.
• CA2019 re: [ThreadStatic]: Having written about this footgun, I guess I never checked for it in our codebase.
• CA1854 re: TryGetValue: Good. I think the ControllerConfig callsite can be simplified further.
• CA1859 "Use concrete types when possible for improved performance": Terrible idea, especially if it changes a method signature. The only time I would consider doing something this is if we define an interface with only 1 impl., defined in the same project, and even then that might be for good reason. Mute rule.
• CA1868 re: Contains: Good.
• CA1861 "Avoid constant arrays as arguments": Seems like a helpful warning, but if it's flooding the console then you're right it's not.

I also needed to mute CA1855 due to newer changes. It's just a hint to replace span.Fill(0) w/ Clear. IMO you're not going to write that accidentally, and it can be good to be explicit.

I resolved the warning in MDS_Format.MountBlobs in a different way, and opened a separate PR #4228 for that.

[Morilli]

• CA1852 "Seal internal types": I approve, but I think making them structs or otherwise rethinking the data representation would have a far greater impact than letting the JIT maybe elide some type checks. I would support your alternative idea of muting it for the time being.

Yeah I'm still not too sure about it. How about we leave the ones I converted in this PR and mute the rule for all others that might come up to not annoy people unnecesarily?

• CA1859 "Use concrete types when possible for improved performance": Terrible idea, especially if it changes a method signature. The only time I would consider doing something this is if we define an interface with only 1 impl., defined in the same project, and even then that might be for good reason. Mute rule.

This actually only fires for internal or private methods, so this will only ever report suggestions when all existing callsites support the method signature change and it actually reduces virtual or interface calls (that's what it says at least). So in general this seems like a free performance optimization hint, the only reason I haven't applied all inspections and have lowered it to suggestion is because

1. as mentioned in the commit description it doesn't always show in the IDE (CA1859 fails to show in IDE in specific scenarios dotnet/roslyn-analyzers#7496) and looking through issues on the roslyn-analyzers repo the analyzer does seem to have some bugs (although I haven't encountered any of them on the bizhawk repo)
2. some interface abstraction are useful when they are IReadOnlyX to signal intent, even if the code is only a private method

• CA1861 "Avoid constant arrays as arguments": Seems like a helpful warning, but if it's flooding the console then you're right it's not.

Yeah this one basically reports every single method call with inline array creation, even if those methods are only called once and even if they are part of a static readonly initialization. The annoying thing here is that in most cases, fixing the inspection would only move the array initializer to a random field instead of having it inline, saving nothing.

I also needed to mute CA1855 due to newer changes. It's just a hint to replace span.Fill(0) w/ Clear. IMO you're not going to write that accidentally, and it can be good to be explicit.

I disagree with this one. If I'm seeing it right, the only case of this in the repo is

BizHawk/src/BizHawk.Emulation.Cores/Computers/Commodore64/Serial/Drive1541.cs

Line 203 in 59b2dba

_ram.AsSpan().Fill(0);

It seems quite clear to me that this IS in a fact an "accident" and was just fixing the garbage code from before. This should either be Array.Clear(_ram, 0, _ram.Length) as it's being done in countless other places or at least the suggested _ram.AsSpan().Clear(). This does look like a very valid suggestion to me, especially if you are not aware that Clear() exists.
A Clear method being synonymous to setting everything to 0 is logical to me, so I don't see how you'd get more clarity by using a Fill method instead.

[YoshiRulz]

General point: Having a rule muted doesn't preclude a human from noticing and refactoring bad code. There are rules for pointing out code smells and there are rules for banning harmful practices, and the former aren't as binary.

CA1852

How about we leave the ones I converted in this PR and mute the rule for all others that might come up to not annoy people unnecesarily?

It's can-kicking either way so idrc.

CA1859

This actually only fires for internal or private methods, so this will only ever report suggestions when all existing callsites support the method signature change [...]

So even when it's working it's not very useful. Aren't all of our hot paths declared public?
Looking through the commit, I'd guess only the InputRoll and UndoHistory changes are worth testing, drop the rest, let humans decide what needs fixing.

CA1855

It seems quite clear to me that this IS in a fact an "accident" [...] A Clear method being synonymous to setting everything to 0 is logical to me, so I don't see how you'd get more clarity by using a Fill method instead.

Saxxon replaced the loop with Clear, then I replaced that with Fill(0) to preserve the original intent.
I'm applying the same reasoning as for writing = 0 instead of = default when zeroing e.g. an int field. And my above point re: humans applies to this too.

[Morilli]

Having a rule muted doesn't preclude a human from noticing and refactoring bad code.

Sure, however if you're against changing certain code then it doesn't matter whether there is a rule suggesting the change or not. At the end of the day these changes were all done and checked manually (by me, a human).
I would argue that having a rule muted DOES reduce the chance of noticing bad code.

CA1859
This actually only fires for internal or private methods [...]

So even when it's working it's not very useful. Aren't all of our hot paths declared public?
Looking through the commit, I'd guess only the InputRoll and UndoHistory changes are worth testing, drop the rest, let humans decide what needs fixing.

I would assume most hot paths are actually private, not public, but this isn't primarily about performance (none of these type changes will have noticable performance impact) and more about not overcomplicating code needlessly. Using interfaces like IList instead of List does nothing but increase abstraction, which is useless and I'm personally against. I have checked all inspections myself and only applied the ones I found reasonable (as a baseline: if the code was written from scratch, would I use the suggested types or the ones currently used).

For example in CheckHeaderHeuristics, you used an IList<string> as parameter, but passing an array instead of a List might throw a runtime exception because the array does not implement the Add or Clear methods, so being explicit with the List type is actually required here.

CA1855

Saxxon replaced the loop with Clear, then I replaced that with Fill(0) to preserve the original intent.

I doubt there was any intent behind the original code besides "I want to clear this array but I don't know how to do it efficiently, so I'll write a loop". Funnily enough, the original code was also written by Saxxon and they are the ones who changed the code to a Clear call, so surely the "original intent" argument doesn't make sense here.

[YoshiRulz]

CA1859

Using interfaces like IList instead of List does nothing but increase abstraction, which is useless and I'm personally against. [...] For example in CheckHeaderHeuristics, you used an IList<string> as parameter, but passing an array instead of a List might throw a runtime exception because the array does not implement the Add or Clear methods, so being explicit with the List type is actually required here.

Using a collection interface is correct in almost every case—it does do more than just abstracting over a class. The choice of IList<T> over IReadOnlyList<T> or ISet<T> communicates the semantics of how elements are stored and retrieved. Arrays are special and I expect every C# dev to be aware of that. And those aren't the only 2 options for IList<T> implementations.
IList<string> is the correct type for a collection of warnings which may be appended to.

CA1855

[...]

Regardless of the intent there, I still value explicitness. 0 is distinct from uninit (which default is semantically) and, for example, if we used Span more we could have made a ClearFast helper which was a noop in Release config.

[Morilli]

IList<string> is the correct type for a collection of warnings which may be appended to.

No. Quoting from the documentation: IList Represents a non-generic collection of objects that can be individually accessed by index.
Also: IList implementations fall into three categories: read-only, fixed-size, and variable-size.

This is precisely the problem here: By allowing an abstract type but then not handling this abstract type, you're just complicating things unncessarily.

[YoshiRulz]

You want to Debug.Assert(!list.IsReadOnly); everywhere? Go ahead.