Enable more Roslynator rules #21202
Enable more Roslynator rules #21202
Conversation
|
IMO these three rules don't bring any readability improvements, so whether we want them should hinge on their potential performance impact. Do you have an idea on what kind of impact these will have? |
|
I haven't looked at the files changed but I'm a fan of RCS1246. I also like RCS107 but I suppose the devil's in the details there as for RCS1146 IMO it only reduces readability and if it at all has a performance impact I really doubt its measurable. There's nothing faster than boolean math |
|
it even decompiles into the same low level c# 
|
|
RCS1146 is purely a style choice. Bench for RCS1077 and RCS1246: public class BenchmarkJob
{
readonly int[] array = Enumerable.Range(0, 1000).ToArray();
readonly List<int> list = Enumerable.Range(0, 1000).ToList();
[Benchmark]
public int ArrayFind() => Array.Find(array, x => x == 500);
[Benchmark]
public int ArrayFirstOrDefault() => array.FirstOrDefault(x => x == 500);
[Benchmark]
public int ListFind() => list.Find(x => x == 500);
[Benchmark]
public int ListFirstOrDefault() => list.FirstOrDefault(x => x == 500);
[Benchmark]
public int ArrayFirst() => array.First();
[Benchmark]
public int ArrayIndexer() => array[0];
[Benchmark]
public int ListFirst() => list.First();
[Benchmark]
public int ListIndexer() => list[0];
[Benchmark]
public List<int> ListSelectToList() => list.Select(x => x * 2).ToList();
[Benchmark]
public List<int> ListConvertAll() => list.ConvertAll(x => x * 2);
[Benchmark]
public int SelectMax() => list.Select(x => x * 2).Max();
[Benchmark]
public int Max() => list.Max(x => x * 2);
[Benchmark]
public bool WhereAny() => list.Where(x => x % 2 == 0).Any(x => x > 500);
[Benchmark]
public bool Any() => list.Any(x => x % 2 == 0 && x > 500);
} |
|
Based on the benchmark and sentiments expressed thus far, I'm leaning towards the following approach:
That gets us the worthwile perf wins, whilst skipping everything else. |
|
Can we not enforce RCS1077 once we have the extension method(s)? I'd vote for enforcing it. |
|
As VSCode let's me automatically fix RCS1077 my preference would be to enforce it. I'm also not a massive fan of custom extension methods as their discoverability if very low. As for RCS1146 I would definitely like to see that commit dropped |
RoosterDragon
force-pushed
the
style-ros-hard
branch
from
36e063a
to
352187b
Compare
|
Ok, changes pushed with the following flavour:
|
|
Sorry, final verdict was FOR extension methods for RCS1077. |
This allows the LINQ spelling to be used, but benefits from the performance improvement of the specific methods for these classes that provide the same result.
RoosterDragon
force-pushed
the
style-ros-hard
branch
from
352187b
to
1a7c510
Compare
|
Rebased and added a separate commit for extension methods for RCS1077. |
Following on from #21192
Enforces 3 Roslynator rules across the project.
This is the last set of rules that I personally think might be worth enabling from Roslynator. However I think there's room for bikeshedding on these ones so I'm showing what the rules do out of the box, but I don't think we'll want to adjust this PR before taking any changes (if we even take any changes here).
See https://josefpihrt.github.io/docs/roslynator/analyzers for explanation of each rule.
RCS1077
I don't mind the enumerable transforms the rule applies, but the array and list changes I find quite annoying. If I'm dealing with an input enumerable that happens to be a list or an array, the rule kicks in and wants me to stop using the LINQ enumerable methods and swap to something else. This is doubly annoying if I switch the input back into an enumerable and now everything breaks.
I'm tempted to add extensions method such as
T FirstOrDefault<T>(this T[] array, Predicate<T>) => Array.Find(array, predicate);so that you can keep using the same methods without having to swap back and forth. However even doing that won't stop the rule suggesting the obtuseConvertAlltransformation for list-> list projections.We could forget about this rule, or maybe we could manually fix the violations we care about, but not enforce the rule as a warning. It could be a lower severity or disabled entirely. We could add the extension method suggestion or go without.
I think some parts of this rule are useful, but I wouldn't want to enforce it straight up like this. Open to ideas.
RCS1146
I like the first transformation, but I find the transformation for nullable Booleans deeply obtuse and would rather it kept it as a seperate null check. Again we could forget about the rule entirely, or manually apply some fixes but not all, or maybe enforce it at a lower severity, etc. Open to thoughts.
RCS1246
Again, I think an extension method for
Firstmight be nicer here, to avoid having to change back and forth depending on the concrete type of your collection. Thoughts?