Enable Code Quality rule CA1851

[RoosterDragon]

Following on from #20957

Enforces https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1851

We fix violations in a few ways:

1. Enumerating the input only once. This provides the best result - we can continue to accept any enumerable and don't have to allocate memory to materialize a list.
2. Changing the input to IReadOnlyCollection<T>. This reduces the flexibility of inputs, but often input is already a materialized collection and thus this isn't a problem in many callsites. We can safely enumerate a collection multiple times.
3. Materialize the source via ToList. This means allocating an intermediate list but is always an option if the above options cannot be employed.

Specific changes of interest:

• ActorInfo.cs - as we want to enumerate this enumerable fresh each time due to the changing results, we suppress the warning.
• TypeDictionary.cs - we borrow a technique already employed in TraitDictionary of a container interface. This allows us to access the backing list after casting the container. In turn we can expose the list as IReadOnlyCollection which helps a lot of callsites not have to deal with IEnumerable.

[PunkPun]

is ToList better for perf than a ToArray?

[RoosterDragon]

is ToList better for perf than a ToArray?

• ToArray involves an extra array allocation and copy to resize the final collection to the correct size. If the collection will be long lived or iterated many times than an array offers memory saving and iterating perf benefits over a list.
• ToList avoids the extra array allocation/copy as it will be returned with excess capacity. For a short lived collection that isn't iterated very much then we save on the step of copying this to a correctly sized array.

So as a rule of thumb ToList for throwaway collections and ToArray for something you'll keep around for a while.

[PunkPun]

LGTM, went through the changes and also ran a large AI game

[PunkPun]

Needs a rebase

[abcdefg30]

Changelog

[Mailaender]

This does not appear in VS Code nor with make check. It only fails in CI, which is unfortunate.

[RoosterDragon]

It's working for me locally, my dotnet --version is 7.0.400. The CI runner appears to be using 6.0.413.

What SDK version are you running locally? And if you update to at least the same version as CI does that improve the situation?

[RoosterDragon]

The docs for CA1851 also state it was introduced in .NET 7. A list of rules introduced in .NET 7 is here. https://github.com/dotnet/roslyn-analyzers/blob/main/src/NetAnalyzers/Core/AnalyzerReleases.Shipped.md#release-70

If it turns out you actually need a .NET 7 SDK for the rule then that would explain the gap.

[RoosterDragon]

It does need a .NET 7 SDK. The Windows CI installs 6 and won't flag if the rule is violated. The Linux CI runner comes with 7, so installing 6 is a no-op, and it flags if the rule is violated.