In C# IENumerable<T>.All() and List<T>.All() methods incorrectly assumed to be pure

SonarQube Cloud
1

Sonarcube v7.8.0.26217 analyzing C# code

The sample program below generates a pair of incorrect warnings on the All() method calls. In both cases it is assuming the invocation is a pure method and the line of code is actually a no-op; but the collection is modified.

This example came from a Stackoverflow question with 462k views, so I’m reasonably sure I’m not the only person who has seen the warning, looked at the code, and thought he could safely delete the line while wondering what the original author was trying to do. This is cut down from actual code where the original data came from a ToList()ed LINQ query via EF6.

using System.Linq;
 
namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            BlameStackoverflowForThis();
        }
 
        static void BlameStackoverflowForThis()
        {
            //https://stackoverflow.com/a/6233655/85661
            //the abuse of All() seen below is from the above answer
 
            List<TestObject> testList = new List<TestObject> { new TestObject(), new TestObject(), new TestObject() };
            IEnumerable<TestObject> testEnum = testList;
 
            //ReSharper warning:  Return value of pure method is not used
            //but text is changed from null to "foo"
            testEnum.All(c => { c.text = "foo"; return true; });
 
            //ReSharper warning:  Return value of pure method is not used
            //but text is changed from "foo" to "bar"            
            //in this case testList.ForEach(c => { c.text = "bar"; }); is a 
            //clean way to do it,  but IEnumerable doesn't have a Foreach() method
            testList.All(c => { c.text = "bar"; return true; });
         }
    }
 
    public class TestObject
    {
        public  string text { get; set; }
    }
}```
2

hi @Dan_Neely and welcome to the SonarSource Community forum!

From the comments inside the code snippet, I understand you are talking about ReSharper warnings. What does this have to do with the SonarSource products (SonarQube, SonarCloud, SonarLint)?

3

That was a copy/paste error on my part. Both Sonarqube and Resharper made the same incorrect analysis of the real code I was looking at. I just forgot to update the code comments for my second bug report.

4

I don’t see an option to edit my original sample, the sonarqube bug message is:

Use the return value of method ‘All’, which has no side effect. L1035

9

Thanks for reporting this @Dan_Neely.

After discussing this internally, we decided on the following: we will keep the current implementation, as using such Linq methods to loop over a collection is a corner case and an abuse of its original purpose.

We will update the rule description, so that it mentions this case, explaining users should be careful when they come across such code.

Best,

-Chris

10

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.