Subtype relationship between collection and read-only collection interfaces impacts behavior of analyzers

[eiriktsarpalis]

Cc @tannergooding @eiriktsarpalis on the error CA1826: (NETCORE_ENGINEERING_TELEMETRY=Build) Do not use Enumerable methods on indexable collections. Instead use the collection directly. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1826) failures ("This rule analyzes collection types that implement IReadOnlyList<T> but not IList<T>.")

Originally posted by @MichalStrehovsky in dotnet/installer#19577 (comment)

[hez2010]

It seems like an outdated analyzer rule which should be disabled.

[tannergooding]

Just noting that this was discussed a bit offline...

The general intent of CA1826 has been to catch places where x.First() or similar calls would not be specal cased and therefore would fallback to the slower behavior where it actually enumerates, which up until this point has historically included IReadOnlyList<T> and similar interfaces, but not IList<T> itself (which most LINQ APIs explicitly special cased).

With the new change, IList<T> implements IReadOnlyList<T> by default now and because ASP.NET has overridden it's severity from suggestion to warning and enabled warnaserror, it's triggering unnecessarily in more places.

https://github.com/dotnet/runtime/pull/101469/files#diff-dcec95b9c555c8a762091e5ed283baa292291e6fb5af994ecb8d464c4a5fa5b4L81 will effectively obsolete the analyzer on modern .NET, as IROList<T> will be the specialized type and will include the prior set of IList<T>, so it's something we'll likely change to just not run for modern TFMs (or tweak the logic around what exactly its intent is and when/where it should fire). -- CC. @stephentoub

[CyrusNajmabadi]

This should move to roslyn-anlayzers for tracking?

[eiriktsarpalis]

We track at least some analyzer issues in this repo. I defer to @buyaa-n on picking the appropriate location.

[buyaa-n]

As it is not a new rule that needs API review moving to roslyn-analyzers repo