Codecov Report

Merging #3584 into master will decrease coverage by 0.02%.
The diff coverage is 93.22%.

@@            Coverage Diff             @@
##           master    #3584      +/-   ##
==========================================
- Coverage   94.67%   94.65%   -0.03%     
==========================================
  Files        1074     1075       +1     
  Lines      244939   245459     +520     
  Branches    14787    14847      +60     
==========================================
+ Hits       231899   232339     +440     
- Misses      11006    11072      +66     
- Partials     2034     2048      +14     

Few things to note:

1. CA1827-CA1829 were contributed very recently by a community member and we should feel free to tune its heuristic and/or make them more general, as appropriate.

2. The fact that CA1829 is implemented as a separate analyzer already causes bad user experience in certain cases where we flag both CA1827 and CA1829 on same code, leading to different end result for the user based on which fix they chose, instead of the most efficient one. I was planning to merge these analyzers to avoid multiple suggestions on same code.

3. New analysis added in this PR adds to these scenarios and makes for a bad user experience. For example, consider user code “.Count() != 0” when IsEmpty and Count property are both available. The most efficient check here is “!IsEmpty”, but we end up suggesting CA1827 and CA1829, and user either ends up with “.Any()” or “.Count != 0”. For the latter case, we will now end up with a follow-up CA1836 violation from the analysis added here that would suggest “!IsEmpty”. It is unfortunate that user ended up with different end result in both cases, and the code generated by CA1829 code fix itself is always flagged with CA1836 rule added here.

4. I suggest we merge all these analyses into a single analyzer for a seamless user experience:

   1. At most one of these CA rules should fire for any given code, and that should be recommending the most efficient end result for the user
   2. Applying a code fix from any of the rules in this bucket should not lead to a follow-up violation from another rule in this bucket
      IMO, this is only possible if we merge CA1827-CA1829 and the analysis added in this PR into a single analyzer.

Thanks @mavasani, I appreciate the feedback and recommendations. We had previously looked at the "Use Any when applicable over Count" analyzer, but our assessment at that time was that it was targeting different code and using different heuristics, but from your analysis here, I see how there is overlap in the experience.

Generalizing this group of analyzers into a single analyzer is intriguing. I think we'd need to do some more design work to figure out the right path forward on that though.

• We'd need to expand @jozkee's mapping shown to capture the combined set of warnings and fixes
• The implementation would likely not be able to target specific types or interfaces, but instead target member names.
• We were already unsure if this could truly be considered a "Performance" category analyzer--I suspect that a general-purpose analyzer tackling all of these scenarios should be categorized as "Maintainability" since it's really about code readability/clarity.
• We'd have to figure out which (if any) of the diagnostic IDs to keep and what the downstream effects of that are

I was scratching my head trying to see how we could merge the 4 rules all together in order to diagnose one rule at most as you mention and this is what I got.

First, I identified the rules and the types affected by them; the next table tries to illustrate that.
For example, CA1829 is only registered when the compilation contains System.Linq.Enumerable which extends IEnumerable<T>, hence, Applies To shows the type IEnumerable<T> and Applies When shows System.Linq.Enumerable, the same for the other 3 rules.

Second, I tried to sketch a routine that could allow us to funnel into returning one rule following this principles:

1. What's the offending expression?
2. What's the most optimal expression that you can use to express the same?

And this is what I got. The comments below each condition says what are the possible rules that can be returned from that code branch.
Please disregard the fact that this is a mix of pseudo-code and C#

// Offending expression e.g: obj.Count() != 0
offendingExpression = context.Operation; 

if context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqEnumerable)
// CA1827, 28, 29 & 36.
  if offendingExpression is (valid binary operation)* or (valid Equals invocation operation)**
    // CA1827, 28, 36. 
    IOperation operation = Get the non-constant side of the expression
    
    if operation is Count() method invocation
      // CA1827, 36
      if operation.ContainingType contains (valid IsEmpty property)***
        return CA1836
      else 
        return CA1827

    else if operation is CountAsync() method invocation
      return CA1828
    else if operation is Count property member accessor
      return CA1836

  else if offendingExpression is Count() method invocation
    return CA1829

if context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqEnumerable)
// CACA1827, 28 & 36.
  if offendingExpression is (valid binary operation)* or (valid Equals invocation operation)**
    // CA1827, 28, 36. 
    IOperation operation = Determine the non-constant side of the expression.
    
    if operation is Count() method invocation
      // CA1827, 36
      if operation.ContainingType contains (valid IsEmpty property)***
        return CA1836
      else 
        return CA1827

    else if operation is CountAsync() method invocation
      return CA1828
    else if operation is Count property member accessor
      if operation.ContainingType contains (valid IsEmpty property)***
        return CA1836

// For all types
if offendingExpression is (valid binary operation)* or (valid Equals invocation operation)**
  IOperation operation = Determine the non-constant side of the expression.
  if operation is Count() method invocation OR operation is Count property member accessor
    if operation.ContainingType contains (valid IsEmpty property)***
      return CA1836

@mavasani @jeffhandley Would you consider this good enough?
Would it be better to do it in this PR or in a separate PR?

@mavasani Should we re-arrange the sequence of Rule IDs so the 4 rules (CA1827-CA1829 & CA1836) show as consecutive numbers since they are somewhat related?

Applying a code fix from any of the rules in this bucket should not lead to a follow-up violation from another rule in this bucket

I have the concern that this might keep happening even if we merge the rules.
Consider the following:

List<string> list = new List<string>();
bool isEmpty = list.Count() == 0;

As of today, this snippet triggers both rules CA1827 and CA1829 but I think that is because there are two offending expressions which are two separate analysis, one for list.Count() == 0 and the other for list.Count().

Is there a way to query the current diagnosis results so we can either avoid emitting rule CA1829 if the expression is within the expression that triggered CA1827 OR remove diagnosis of CA1829 if CA1827 encompasses it?

Thanks @jozkee - that seems to match pretty much what I had envisioned. You can ensure that we do not report duplicate diagnostics by writing separate analysis callbacks for method invocation and property reference as follows.

NOTE: I think CA1829 should be relaxed so semantics are equivalent to CA1836, i.e. flags Count() invocation if there is any valid property named Length/Count on containing type.

void Initialize(AnalysisContext context)
{
   context.RegisterCompilationStartAction(OnCompilationStart);
}

void OnCompilationStart(CompilationStartAnalysisContext context)
{
  var systemLinqEnumerable = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqEnumerable)
  var enumerableCountMethod = systemLinqEnumerable?.GetMembers("Count").OfType<IMethodSymbol>().FirstOrDefault(m => m.Parameters.Length == 0)

  var queryableExtensionsType = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemDataEntityQueryableExtensions)
  var queryableCountAsyncMethod = queryableExtensionsType?.GetMembers("CountAsync").OfType<IMethodSymbol>().FirstOrDefault(m => m.Parameters.Length == 0)

  if (enumerableCountMethod != null or queryableCountAsyncMethod != null)
    compilationStartContext.RegisterOperationAction(
      c => AnalyzeMethodInvocation(c, enumerableCountMethod, queryableCountAsyncMethod),
      OperationKind.Invocation)  
  compilationStartContext.RegisterOperationAction(AnalyzePropertyReference, OperationKind.PropertyReference)
}

void AnalyzeMethodInvocation(OperationAnalysisContext context, IMethodSymbol enumerableCountMethod, IMethodSymbol queryableCountAsyncMethod)
{
  var invocation = (IInvocationOperation)context.Operation;
  var isCountMethodInvocation = invocation.TargetMethod.Equals(enumerableCountMethod)
  var isCountAsyncMethodInvocation = invocation.TargetMethod.Equals(queryableCountAsyncMethod)
  if (!isCountMethodInvocation and !isCountAsyncMethodInvocation)
    return

  // Define extension similar to WalkDownConversion
  var parentIgnoringConversions = invocation.WalkUpConversions();

  // Check if invocation is parented by an Offending expression e.g: obj.Count() != 0
  if parentIgnoringConversions is (valid binary operation)* or (valid Equals invocation operation)**
    if isCountMethodInvocation
      if invocation.ContainingType contains (valid IsEmpty property)***
        report CA1836
      else if invocation.ContainingType contains (valid Length/Count property)***
        report CA1829
      else
        report CA1827

    else if isCountAsyncMethodInvocation
      report CA1828

  else if invocation.ContainingType contains (valid Length/Count property)***
    report CA1829
}

void AnalyzePropertyReference(OperationAnalysisContext context)
{
  var propertyReference = (IPropertyReferenceOperation)context.Operation;
  if propertyReference.Name != "Count" and propertyReference.Name != "Length"
       return
  if Not propertyReference.ContainingType contains (valid IsEmpty property)***
       return

  var parentIgnoringConversions = propertyReference.WalkUpConversions();
  if parentIgnoringConversions is (valid binary operation)* or (valid Equals invocation operation)**
        report CA1836
}

Should we re-arrange the sequence of Rule IDs so the 4 rules (CA1827-CA1829 & CA1836) show as consecutive numbers since they are somewhat related?

That is tricky, as we have already released an FxCop analyzers package with CA1827-CA1829. Users might have added source suppressions or ruleset/editorconfig entries to turn off the diagnostic, and we don't want them to be broken by us changing IDs of shipped rules. We have faced similar situations in the past, and come to terms with the fact that having non-sequential IDs is better then changing existing rule IDs from an end user perspective as it avoids breaking change for users.

@jozkee Let me know if you have any other questions, we can discuss offline as well. Feel free to ping me anytime on teams.

@mavasani thanks for replying! Few more questions:

NOTE: I think CA1829 should be relaxed so semantics are equivalent to CA1836, i.e. flags Count() invocation if there is any valid property named Length/Count on containing type.

If we relax it, then the rule wouldn't be proper to Microsoft.NetCore.Analyzers, don't you think?

As per the documentation (Section 4.ii):

Analyzers related to pure code quality improvements, which are not specific to any API should go into Microsoft.CodeQuality.Analyzers. Analyzers specific to usage of a specific .NetCore/.NetStandard API should go into Microsoft.NetCore.Analyzers package.

However, since the rule already shipped, it wouldn't be wise to just move it to Microsoft.CodeQuality.Analyzers. Should we include rule CA1829 in both projects?

Also, since we are planning on merging API specific rules (CA1827 and CA1828) with non-API specific (CA1829 if relaxed, and CA1836), in which project should those rules exist?

If we relax it, then the rule wouldn't be proper to Microsoft.NetCore.Analyzers, don't you think

I would not worrying about the sub-package/folder that the rule belongs to - feel free to move the analyzer(s) around so all analysis for these related rules is in a single analyzer. End users will either install FxCop analyzers package, which includes both Microsoft.NetCore.Analyzers and Microsoft.CodeQuality.Analyzers OR get all the CA analyzers in .NET5 SDK. So, we should be fine.

Also, since we are planning on merging API specific rules (CA1827 and CA1828) with non-API specific (CA1829 if relaxed, and CA1836), in which project should those rules exist?

I think it makes sense to keep them in Microsoft.NetCore.Analyzers as these are all API centric analyzers. We normally put API-agnostic analyzers that recommend pure code quality improvements in Microsoft.CodeQuality.Analyzers, say remove unnecessary parameter, make method static, etc. etc.

I am almost finishing up with merging the analyses, and I found a couple of bugs in the current analyzer implementation for rule CA1827/28.

• bool any = 0 >= list.Count();
  fixer suggests bool any = list.Any();
  The condition will only be true when Count = 0, so Any() should be negated.

• bool any = 0 < list.Count();
  fixer suggests bool any = !list.Any();
  This case is the flipped version of Count > 0; Any() should not be negated.

I will fix those two cases in the upcoming commits, I just wanted to point them out in this thread to keep track in case is needed, and also ask if there is a tracking issue for this that we may close after merging this, I doubt it since those look like very uncommon cases.

@jozkee Thanks - awesome!

also ask if there is a tracking issue for this that we may close after merging this, I doubt it since those look like very uncommon cases.

I don't think these are known issues.

Recap:

• Removed the following classes (the fixers still exist, though):

  • DoNotUseCountWhenAnyCanBeUsedAnalyzer
  • UsePropertyInsteadOfCountMethodWhenAvailableAnalyzer

• Created class UseCountProperlyAnalyzer (feel free to suggest a better name) which supports rules 1827, 28, 29 & 36 (added in this PR as well).

All tests are passing with the exception of a few ones related to relaxation of rule 1829, (will call those out individually as a review comment).

@mavasani Can you grant me permission to add more reviewers or add below names yourself?
cc @pgovind @buyaa-n @carlossanlop @bartonjs

When user code has Count(), why are we not preferring CA1829 (Use property Count/Length) over CA1827 (Use Any())?

I was thinking that the right thing to do was to replace the code with its most semantically correct version (IsEmpty or Any()), if it was a candidate (part of a valid binary or equals invocation operation); otherwise, just report replacing the invocation operation.

I do understand your point that a property check is better that an Any() method, although I think Any() reads better.

I do understand your point that a property check is better that an Any() method, although I think Any() reads better.

I think given this is a performance rule, property access would be a better suggestion. I am also fine with having a user configurable option here to allow users to choose if they prefer performant code or readable code in this space. I will let you, @jeffhandley and the runtime team decide the preferred default semantics here.

I do understand your point that a property check is better that an Any() method, although I think Any() reads better.

If there's an IsEmpty, it's almost certainly better. We should add Any() => !IsEmpty and !Any() => IsEmpty into the list of things to correct.

(LINQ Any() is better than LINQ Count(), may be worse than implemented Count() and is almost definitely worse than property Count/Length/IsEmpty)

If there's an IsEmpty, it's almost certainly better. We should add Any() => !IsEmpty and !Any() => IsEmpty into the list of things to correct.

New rule for IsEmpty takes priority over the rule for Any() so that should be already addressed in this PR.

(LINQ Any() is better than LINQ Count(), may be worse than implemented Count() and is almost definitely worse than property Count/Length/IsEmpty)

Better in the sense of performance? @bartonjs so you would suggest to place rule "Use Property Count/Length instead of LINQ Count()" above "Use LINQ Any() instead of LINQ Count()"?

Better in the sense of performance?

Yeah. Any() has to spin up an iterator, try to pump it, then throw it away. A property defined on the type is (assuming it follows guidelines) not going to do anywhere near that much work, or have GC impact.

so you would suggest to place rule "Use Property Count/Length instead of LINQ Count()" above "Use LINQ Any() instead of LINQ Count()"?

Yep. I think the best-to-worst order for "has stuff or not" is

1. IsEmpty property
2. Any() instance method
3. Count/Length instance property
4. Any() extension method
5. Count() instance method
6. Count() extension method

The Count() instance method could arguably float up one, but it's probably just trading one kind of complication with another vs LINQ Any() (otherwise it would have been a property).

OK, I did the proper changes to prefer CA1829 over CA1827, except when the offending method contains a predicate e.g. Count(_ => true), in that case, we can only suggest Any(_ => true).

@mavasani Thanks, just a few more changes to fix some bugs that I found addressing your feedback.

• AnalyzePropertyReference must only report CA1836, before it could incorrectly report CA1829 if you were using property Count and there was also property Length available.
• AnalyzePropertyReference should not report if the property name is not Count or Length, before this, it was incorrectly suggesting CA1836 for all properties.
• Refactored AnalyzeParent* methods to only indicate if the parent operation should be replaced; I also moved the logic to determine which rule we should report to separate methods, DetermineReportForInvocationAnalysis and DetermineReportForPropertyReference to correctly honor the statement I already made on the first bullet.