Roslyn-analyzers: "Do not use Enumerable methods" encourages degrading readability

[jnm2]

Analyzer package: Microsoft.CodeAnalysis.FxCopAnalyzers
Package Version: 2.6.2-beta2
Diagnostic ID: CA1826

"Do not use Enumerable methods on indexable collections. Instead use the collection directly" makes sense for .First() and .Last(), but not for .FirstOrDefault() or .LastOrDefault(). There's no readable replacement for those methods; unless FirstOrDefault is showing up in a performance profiling tool, readability is more important.

Even .Last() is a stretch since the replacement is list[list.Count - 1]. That's a big drop in readability.

[jnm2]

The end mark is missing from the message, too, and the description Going through LINQ here would match the style of other analyzer messages better as Using LINQ.

This collection is directly indexable. Going through LINQ here causes unnecessary allocations and CPU work.

[reduckted]

I find this warning quite annoying for FirstOrDefault(), so I'd be happy to help out. What's the actual fix though? Is the fix to stop warning for FirstOrDefault() and LastOrDefault()?

[Evangelink]

@reduckted That's what I get when reading the previous comments. @mavasani could you confirm?

[mavasani]

@Evangelink We can possibly introduce an editorconfig option to turn off the diagnostic for FirstOrDefault and LastOrDefault cases.

[Evangelink]

Indeed it's probably better to leave the option to the user. I know this would imply a breaking-change for the default case but I would advise to have it turned off by default. This way that reduces the feeling of False-Positive while letting the users able to have the extra checks if they want. WDYT?

[mavasani]

If we are thinking of turning it off by default, then as well not have an option until we get user request.

[Evangelink]

I don't have any strong preference here so I am fine with either way, what's your preference?

[mavasani]

IMO, this rule was written as primarily a performance rule, so it is likely that someone might want or expect all the cases to be flagged by default. Given there are cases which observably degrade readability, we want a split so users can easily choose between one or the other (perf only vs perf whenever it does not degrade readability). I believe either of the following options are acceptable:

1. Create a separate rule ID for OrDefault cases.
2. Add an editorconfig option to turn off flagging the OrDefault cases, while retaining the default to match the current analyzer behavior.