dotnet / roslyn-analyzers

MIT License
1.57k stars 464 forks source link

Code fix for CA1826 "Do not use Enumerable methods on indexable collections. Instead use the collection directly" #1932

Open Neme12 opened 5 years ago

Neme12 commented 5 years ago

It would be nice to have a code fix at least for the case of First() and Last().

TKharaishvili commented 5 years ago

Is this issue labeled correctly?

I just installed Microsoft.CodeQuality.Analyzers in the .NET 4.7 console app and couldn't find CA1826 anywhere in the analyzer list.

After searching around some more I found it in the Microsoft.NetCore.Analyzers package:

image

but writing this piece of code gives me no warnings:

var list = new List<string> { "a", "b", "c" };
var f = list.First();
var l = list.Last();
var count = list.Count();

since it says NetCore.Analyzers I then tried it on a .net core console app but still got no warnings.

Needless to say I'm quite confused.

TKharaishvili commented 5 years ago

Just looked at the source code and found out that the warning is not issued for IList<T>-s. Probably because those enumerable methods are optimized for them.

But the question of this issue not being labeled correctly still remains.

TKharaishvili commented 5 years ago

Hi @mavasani I just created a PR for this - https://github.com/dotnet/roslyn-analyzers/pull/2173

It's not ideal but I'm hoping you can guide me to the right direction with some code reviews. Cheers.

Evangelink commented 4 years ago

A code fix now exists for First, Last and Count so I think the intent of this issue is actually solved.

The remaining work is:

paul1956 commented 4 years ago

This warning should not apply to VB or at least there should be a Fix to locally suppress it and I don't see what the fix would be for the OrDefault versions without the code getting much more complex.

Evangelink commented 4 years ago

@paul1956 Could you be a little more specific, are you talking about the rule in general? Are you referring only to Last and LastOrDefault? I am not experienced with vbnet but this does seem or for First, FirstOrDefault and Count. If you disagree could you add some examples to better explain your point?

paul1956 commented 4 years ago

@Evangelink Using the collection directly in VB is not easy and makes the code very complex and there is no CodeFix for VB. Where List is some collection: First and maybe Count when "Length" is available, is easy List.First vs List(0) is a wash as is List.Count vs List.Length VB does not have Index expressions, there is no easy way to do FirstOrDefault or LastOrDefault. Last is one that could be debated but Last seems simpler List.Last vs List(List.Count-1)

Generally It would be better if this was just disabled for VB