search

dotnet / roslyn-analyzers

MIT License
1.55k stars 460 forks source link

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

Open eiriktsarpalis opened 2 months ago

eiriktsarpalis commented 2 months ago

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 https://github.com/dotnet/installer/issues/19577#issuecomment-2075909386

hez2010 commented 2 months ago

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

tannergooding commented 2 months ago

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 commented 2 months ago

This should move to roslyn-anlayzers for tracking?

eiriktsarpalis commented 2 months ago

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

buyaa-n commented 2 months ago

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