Open chucker opened 2 years ago
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.
Tagging subscribers to this area: @eiriktsarpalis See info in area-owners.md if you want to be subscribed.
Author: | chucker |
---|---|
Assignees: | - |
Labels: | `area-System.Linq`, `untriaged`, `code-analyzer` |
Milestone: | - |
Thanks for the proposal, @chucker. A couple of questions:
OrderBy
/OrderByDescending
calls are chained next to each other? What about a case like the following?:var q = Customers
.OrderByDescending(c => c.RankTotalRevenues)
.Where(c => some condition)
.OrderBy(c => c.Name); // should we warn here?
cc @eiriktsarpalis @layomia
Great questions!
Should this analyzer only warn if the OrderBy/OrderByDescending calls are chained next to each other? What about a case like the following?:
I don't believe that OrderBy
ever affects Where
. Maybe I'm mistaken?
However, regarding the first question, there are two trickier cases:
1
var customersByName = Customers.OrderBy(c => c.Name);
// we're taking a query that's _already_ ordered and then ordering it differently
var customersByRevenue = customersByName.OrderByDescending(c => c.RankTotalRevenues) // should we warn here?
My original hunch about this one was that we should warn. But in the above scenario, the behavior may be intended by the developer: they simply want two different sort orders. If the behavior is intended, I can't think of a way for the developer to signal that.
(Plus, what if customersByName
gets produced in an entirely different method, which implicitly returns an IOrderedEnumerable
?)
2
Your code led me to another tricky case:
var q = Customers
.OrderByDescending(c => c.RankTotalRevenues)
.ToList()
.OrderBy(c => c.Name); // should we warn here?
This is subtly different. ToList()
will force the enumerable to materialize; e.g., if it is LINQ to SQL, it will actually perform a SELECT … ORDER BY
query. OrderBy
will then order a second time, by name. This may be intentional behavior by the dev, such as to take advantage of certain indexes.
And, surely if Take()
is used, the original order is definitely relevant (SELECT TOP 5 … ORDER BY x
, then order again).
Therefore, TL;DR:
Should this analyzer only warn if the OrderBy/OrderByDescending calls are chained next to each other?
For the above reasons (and probably more), I think the answer is yes, we should only warn if they are immediately chained next to each other, even though we'd have some false negatives.
There are multiple overloads of OrderBy and OrderByDescending. How should the analyzer and the fixer behave when they encounter the different combination of possible parameters in those overloads?
I took a look, and the overloads seem to be equivalent to the ones for ThenBy
and ThenByDescending
. So IMHO, the answer to this one is quite simply: the symbol name of the method invocation gets replaced; the actual parameters remain as is.
Brief description:
LINQ has
OrderBy
andOrderByDescending
methods. One might be tempted to chain calls of those to order by multiple fields. However, subsequent calls result in only the final call having an effect.An analyzer should detect such incorrect uses, and a fixer should offer replacing them with
ThenBy
andThenByDescending
.Languages applicable:
C# 10, VB 16.9
Code example that the analyzer should report:
Proposed warning text: "Do not call 'OrderBy' or 'OrderByDescending' multiple times. Only the last call would have an effect. Instead, for subsequent calls, use 'ThenBy' and 'ThenByDecending'."