Merge pull request #4621 from Evangelink/CA1820-expression-tree

Update CA1820 to not report on expression trees

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/SpecifyStringComparison.cs

@@ -49,9 +49,6 @@ public sealed class SpecifyStringComparisonAnalyzer : DiagnosticAnalyzer
                                                                              isPortedFxCopRule: false,
                                                                              isDataflowRule: false);
 
-        private static readonly ImmutableArray<OperationKind> s_LambdaOrLocalFunctionKinds =
-            ImmutableArray.Create(OperationKind.AnonymousFunction, OperationKind.LocalFunction);
-
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule_CA1307, Rule_CA1310);
 
         public override void Initialize(AnalysisContext analysisContext)
@@ -86,17 +83,11 @@ public override void Initialize(AnalysisContext analysisContext)
                         return;
                     }
 
-                    if (linqExpressionType != null)
+                    // Check if we are in a Expression<Func<T...>> context, in which case it is possible
+                    // that the underlying call doesn't have the comparison option so we want to bail-out.
+                    if (invocationExpression.IsWithinExpressionTree(linqExpressionType))
                     {
-                        var enclosingLambdaOrLocalFunc = invocationExpression.GetAncestor(s_LambdaOrLocalFunctionKinds);
-
-                        // Check if we are in a Expression<Func<T...>> context, in which case it is possible
-                        // that the underlying call doesn't have the comparison option so we want to bail-out.
-                        if (enclosingLambdaOrLocalFunc?.Parent?.Type?.OriginalDefinition is { } lambdaType
-                            && linqExpressionType.Equals(lambdaType))
-                        {
-                            return;
-                        }
+                        return;
                     }
 
                     // Report correctness issue CA1310 for known string comparison methods that default to culture specific string comparison:

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/TestForEmptyStringsUsingStringLength.cs

@@ -46,26 +46,43 @@ public sealed override void Initialize(AnalysisContext context)
             context.EnableConcurrentExecution();
             context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
 
-            context.RegisterOperationAction(
-                operationAnalysisContext => AnalyzeInvocationExpression((IInvocationOperation)operationAnalysisContext.Operation, operationAnalysisContext.ReportDiagnostic),
-                OperationKind.Invocation);
-
-            context.RegisterOperationAction(
-                operationAnalysisContext => AnalyzeBinaryExpression((IBinaryOperation)operationAnalysisContext.Operation, operationAnalysisContext.ReportDiagnostic),
-                OperationKind.BinaryOperator);
+            context.RegisterCompilationStartAction(context =>
+            {
+                var linqExpressionType = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemLinqExpressionsExpression1);
+
+                context.RegisterOperationAction(
+                    operationAnalysisContext => AnalyzeInvocationExpression(
+                        (IInvocationOperation)operationAnalysisContext.Operation, linqExpressionType,
+                        operationAnalysisContext.ReportDiagnostic),
+                    OperationKind.Invocation);
+
+                context.RegisterOperationAction(
+                    operationAnalysisContext => AnalyzeBinaryExpression(
+                        (IBinaryOperation)operationAnalysisContext.Operation, linqExpressionType,
+                        operationAnalysisContext.ReportDiagnostic),
+                    OperationKind.BinaryOperator);
+            });
         }
 
         /// <summary>
         /// Check to see if we have an invocation to string.Equals that has an empty string as an argument.
         /// </summary>
-        private static void AnalyzeInvocationExpression(IInvocationOperation invocationOperation, Action<Diagnostic> reportDiagnostic)
+        private static void AnalyzeInvocationExpression(IInvocationOperation invocationOperation,
+            INamedTypeSymbol? linqExpressionTreeType, Action<Diagnostic> reportDiagnostic)
         {
             if (!invocationOperation.Arguments.IsEmpty)
             {
                 IMethodSymbol methodSymbol = invocationOperation.TargetMethod;
-                if (methodSymbol != null &&
-                    IsStringEqualsMethod(methodSymbol) &&
-                    HasAnEmptyStringArgument(invocationOperation))
+                if (methodSymbol == null
+                    || !IsStringEqualsMethod(methodSymbol)
+                    || !HasAnEmptyStringArgument(invocationOperation))
+                {
+                    return;
+                }
+
+                // Check if we are in a Expression<Func<T...>> context, in which case it is possible
+                // that the underlying call doesn't have the helper so we want to bail-out.
+                if (!invocationOperation.IsWithinExpressionTree(linqExpressionTreeType))
                 {
                     reportDiagnostic(invocationOperation.Syntax.CreateDiagnostic(s_rule));
                 }
@@ -76,7 +93,8 @@ private static void AnalyzeInvocationExpression(IInvocationOperation invocationO
         /// Check to see if we have a equals or not equals expression where an empty string is being
         /// compared.
         /// </summary>
-        private static void AnalyzeBinaryExpression(IBinaryOperation binaryOperation, Action<Diagnostic> reportDiagnostic)
+        private static void AnalyzeBinaryExpression(IBinaryOperation binaryOperation,
+            INamedTypeSymbol? linqExpressionTreeType, Action<Diagnostic> reportDiagnostic)
         {
             if (binaryOperation.OperatorKind is not BinaryOperatorKind.Equals and
                 not BinaryOperatorKind.NotEquals)
@@ -90,7 +108,15 @@ private static void AnalyzeBinaryExpression(IBinaryOperation binaryOperation, Ac
                 return;
             }
 
-            if (IsEmptyString(binaryOperation.LeftOperand) || IsEmptyString(binaryOperation.RightOperand))
+            if (!IsEmptyString(binaryOperation.LeftOperand)
+                && !IsEmptyString(binaryOperation.RightOperand))
+            {
+                return;
+            }
+
+            // Check if we are in a Expression<Func<T...>> context, in which case it is possible
+            // that the underlying call doesn't have the helper so we want to bail-out.
+            if (!binaryOperation.IsWithinExpressionTree(linqExpressionTreeType))
             {
                 reportDiagnostic(binaryOperation.Syntax.CreateDiagnostic(s_rule));
             }

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/TestForEmptyStringsUsingStringLengthTests.cs

@@ -2,6 +2,7 @@
 
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Testing;
+using Test.Utilities;
 using Xunit;
 using VerifyCS = Test.Utilities.CSharpCodeFixVerifier<
     Microsoft.NetCore.Analyzers.Runtime.TestForEmptyStringsUsingStringLengthAnalyzer,
@@ -108,5 +109,24 @@ void Method()
         }
 
         #endregion
+
+        [Fact, WorkItem(1508, "https://github.com/dotnet/roslyn-analyzers/issues/1508")]
+        public async Task CA1820_ExpressionTree_NoDiagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+using System.Linq;
+
+class C
+{
+    void M(IQueryable<string> strings)
+    {
+        var q1 = from s in strings
+                where s == """"
+                select s;
+
+        var q2 = strings.Where(s => s.Equals(""""));
+    }
+}");
+        }
     }
 }

src/Utilities/Compiler/Extensions/IOperationExtensions.cs

@@ -508,12 +508,18 @@ void ProcessLocalOrParameter(ISymbol symbol)
 
         private static readonly ImmutableArray<OperationKind> s_LambdaAndLocalFunctionKinds =
             ImmutableArray.Create(OperationKind.AnonymousFunction, OperationKind.LocalFunction);
+
         public static bool IsWithinLambdaOrLocalFunction(this IOperation operation, [NotNullWhen(true)] out IOperation? containingLambdaOrLocalFunctionOperation)
         {
             containingLambdaOrLocalFunctionOperation = operation.GetAncestor(s_LambdaAndLocalFunctionKinds);
             return containingLambdaOrLocalFunctionOperation != null;
         }
 
+        public static bool IsWithinExpressionTree(this IOperation operation, [NotNullWhen(true)] INamedTypeSymbol? linqExpressionTreeType)
+            => linqExpressionTreeType != null
+                && operation.GetAncestor(s_LambdaAndLocalFunctionKinds)?.Parent?.Type?.OriginalDefinition is { } lambdaType
+                && linqExpressionTreeType.Equals(lambdaType);
+
         public static ITypeSymbol? GetPatternType(this IPatternOperation pattern)
         {
             return pattern switch