IEnumerable inspections and quick-fixes in ReSharper and Rider

ReSharper and Rider continuously analyze the code we’re working on and help us detect errors and problems early on. They provide suggestions and hints as well, and for many of the 2300+ code inspections that are available, quick-fixes exist to automatically resolve detected issues. ReSharper 2017.2, as well as the Rider 2017.2 EAP, introduce several new code inspections and quick-fixes that help us write better code. Let’s have a look.

IEnumerable – Possible multiple enumeration

The Possible multiple enumeration of IEnumerable code inspection has been around for a while. It helps making sure we’re not enumerating a collection twice – something which could lead to excess work of a database or even to mutated state and strange results. ReSharper 2017.2 adds support for IOrderedEnumerable and ParallelQuery, next to the already existing inspection for IEnumerable. Let’s look at what it does.

Consider the following code snippet:

In this piece of code, we’re enumerating the names sequence twice: once when calling names.Any(), and a second time when iterating over it.

Now what could happen? A couple of things. First of  all, we’re probably running the query executed by GetNamesFromDatabase() twice. This is wasteful and puts unnecessary load on the database. And while in this example the chance is slim, the database may have new or deleted records between the call to Any() and iterating over the sequence, which could lead to strange behaviour of our application.

The solution? Making sure we iterate only once, by converting the sequence to an array or a list using .ToArray() or .ToList(). And that’s exactly what the Possible multiple enumeration code inspection and its corresponding quick-fix does:

Code inspection and quick-fix - Possible multiple enumeration

Note that the quick-fix checks for the type of our IEnumerable: if it’s already a List or array, there is no use in converting it again.

Possibly unintended transformation from IQueryable to IEnumerable

When working with objects that implement the IQueryable interface, we’re most probably working with their underlying LINQ provider. For example when using Entity Framework, we could have a DbSet which is an IQueryable. Whenever we run a LINQ query against it, we’re usually making a database call.

Since the IQueryable interface inherits from IEnumerable, there could be cases where we implicitly convert the IQueryable to IEnumerable. This could result in memory spikes (converting lots of objects into memory) or slow performance (local processing instead of processing on the database)

Consider the following code:

When we call the .WhereNameStartsWith() extension method, we’re implicitly converting the IQueryable provided by the database context to an IEnumerable and running the filter in code (and not on the database).

That’s probably fine and intended, but team members (or our future self) could add additional LINQ queries on top of this, expecting them to run on the database and unwillingly introducing a potential performance issue. We can let the IDE add an explicit .AsEnumerable() for us:

Possibly unintended transformation from IQueryable to IEnumerable

Initialize auto-property from parameter context action

When using the Introduce auto-property from parameter quick-fix, we could already generate an auto-property with various options, such as making it get-only, adding a private setter,  making it mutable, …

The Initialize auto-property from parameter context action now provides similar options, making it very easy to add a property from a constructor parameter:

Initialize auto-property from parameter context action

Download ReSharper Ultimate 2017.2 now or check out Rider 2017.2 EAP! We’d love to hear your thoughts.

This entry was posted in How-To's and tagged , , , , . Bookmark the permalink.

4 Responses to IEnumerable inspections and quick-fixes in ReSharper and Rider

  1. Alex Povar says:

    It might be not the best idea to cast IEnumerable to IList as a part of the quick fix as that could have side effects. Sometimes, when you return IEnumerable you expect that object will be used as a read-only collection and nobody will modify it (as interface doesn’t allow that). However, if you downcast value back to IList you obtain an ability to modify the returned value. After you have the downcasted variable in the scope, you can later accidentally modify it (or pass it to the other method that e.g. receives IList). If that is a shared object, it might be not expected and you will introduce bugs in your application that are really hard to find.

    The approach you suggest works, however it should be used in performance-critical code only when you _clearly understand_ the risks and the maintenance dept you introduce. It’s unsafe to include it to the quick fix that will be used by thousands of developers with different experience and level of knowledge. Moreover, you are teaching them that such things are completely “safe” (assuming that JetBrains wouldn’t recommend unsafe code transformations). Later they will follow this approach and their code will start to smell as they introduced places for potential issues in future.

    Of course, if you follow the defensive programming approach you will not return something mutable as IEnumerable hoping that nobody will downcast it later. However, in real life you think about potential down-cast rarely and rely on the interface constraints. Often you simply overlook such things and even don’t think about the down-cast. Given that you make quick-fixes available for projects of different quality, it’s a very dangerous way to make the quick-fix behave in the current way.

    If you really care about performance (while that’s still an open question whether the introduced change is not a premature optimization as now code looks worse) I’d suggest to change the quick fix to use the IReadOnlyCollection instead. You will still keep the read-only semantics, while now you know that instance is a collection with a known number of items. Such code will be safer and will not break user’s solution or lead to bugs that are really hard to troubleshoot.

  2. Pingback: The Morning Brew - Chris Alcock » The Morning Brew #2420

  3. dev says:

    It should make developers fix the source – overuse of lazy sequences, rather than hack-patching it various ways at consumption point.

  4. Pingback: Dew Drop - September 6, 2017 (#2555) - Morning Dew

Leave a Reply

Your email address will not be published. Required fields are marked *