Merge pull request #6794 from mavasani/CA1508_Enums

Make flow analysis more conservative in presence of entities that can point to multiple different objects

src/Microsoft.CodeAnalysis.AnalyzerUtilities/PublicAPI.Unshipped.txt

@@ -38,6 +38,7 @@ Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractAnalysisDomain<TAnalysisDat
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractBlockAnalysisResult
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractBlockAnalysisResult.AbstractBlockAnalysisResult(Microsoft.CodeAnalysis.FlowAnalysis.BasicBlock! basicBlock) -> void
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractBlockAnalysisResult.BasicBlock.get -> Microsoft.CodeAnalysis.FlowAnalysis.BasicBlock!
+Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.EntityForInstanceLocation.get -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity?
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityFactory.TryGetCopyValueForFlowCapture(Microsoft.CodeAnalysis.FlowAnalysis.CaptureId captureId, out Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis.CopyAbstractValue! copyValue) -> bool
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowAnalysisResult<TBlockAnalysisResult, TAbstractAnalysisValue>.LambdaAndLocalFunctionAnalysisInfo.get -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.LambdaAndLocalFunctionAnalysisInfo!
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.StandaloneLocalFunctionAnalysisResultsMap.get -> System.Collections.Immutable.ImmutableDictionary<Microsoft.CodeAnalysis.IMethodSymbol!, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.IDataFlowAnalysisResult<TAbstractAnalysisValue>!>!
@@ -73,6 +74,7 @@ static Analyzer.Utilities.RoslynHashCode.Combine<T1, T2, T3, T4>(T1 value1, T2 v
 static Analyzer.Utilities.RoslynHashCode.Combine<T1, T2, T3>(T1 value1, T2 value2, T3 value3) -> int
 static Analyzer.Utilities.RoslynHashCode.Combine<T1, T2>(T1 value1, T2 value2) -> int
 static Analyzer.Utilities.RoslynHashCode.Combine<T1>(T1 value1) -> int
+static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.Create(Microsoft.CodeAnalysis.ISymbol? symbol, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractIndex!> indices, Microsoft.CodeAnalysis.ITypeSymbol! type, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue! instanceLocation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity? parent, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity? entityForInstanceLocation) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DisposeAnalysis.DisposeAnalysis.TryGetOrComputeResult(Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph! cfg, Microsoft.CodeAnalysis.ISymbol! owningSymbol, Analyzer.Utilities.WellKnownTypeProvider! wellKnownTypeProvider, Microsoft.CodeAnalysis.Diagnostics.AnalyzerOptions! analyzerOptions, Microsoft.CodeAnalysis.DiagnosticDescriptor! rule, System.Collections.Immutable.ImmutableHashSet<Microsoft.CodeAnalysis.INamedTypeSymbol!>! disposeOwnershipTransferLikelyTypes, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAnalysisKind defaultPointsToAnalysisKind, bool trackInstanceFields, bool exceptionPathsAnalysis, out Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAnalysisResult? pointsToAnalysisResult, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisKind interproceduralAnalysisKind = Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisKind.ContextSensitive, bool performCopyAnalysisIfNotUserConfigured = false, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisPredicate? interproceduralAnalysisPredicate = null, bool defaultDisposeOwnershipTransferAtConstructor = false, bool defaultDisposeOwnershipTransferAtMethodCall = false) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DisposeAnalysis.DisposeAnalysisResult?
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisConfiguration.Create(Microsoft.CodeAnalysis.Diagnostics.AnalyzerOptions! analyzerOptions, Microsoft.CodeAnalysis.DiagnosticDescriptor! rule, Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph! cfg, Microsoft.CodeAnalysis.Compilation! compilation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisKind defaultInterproceduralAnalysisKind, uint defaultMaxInterproceduralMethodCallChain = 3, uint defaultMaxInterproceduralLambdaOrLocalFunctionCallChain = 3) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisConfiguration
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisConfiguration.Create(Microsoft.CodeAnalysis.Diagnostics.AnalyzerOptions! analyzerOptions, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.DiagnosticDescriptor!> rules, Microsoft.CodeAnalysis.FlowAnalysis.ControlFlowGraph! cfg, Microsoft.CodeAnalysis.Compilation! compilation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisKind defaultInterproceduralAnalysisKind, uint defaultMaxInterproceduralMethodCallChain = 3, uint defaultMaxInterproceduralLambdaOrLocalFunctionCallChain = 3) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralAnalysisConfiguration
@@ -655,7 +657,6 @@ static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocation.CreateThisO
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocationDataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.GetClonedAnalysisDataHelper(System.Collections.Generic.IDictionary<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocation!, TAbstractAnalysisValue>! analysisData) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DictionaryAnalysisData<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocation!, TAbstractAnalysisValue>!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocationDataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.GetEmptyAnalysisDataHelper() -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DictionaryAnalysisData<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractLocation!, TAbstractAnalysisValue>!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.Create(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.InterproceduralCaptureId interproceduralCaptureId, Microsoft.CodeAnalysis.ITypeSymbol! type, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue! instanceLocation) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity!
-static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.Create(Microsoft.CodeAnalysis.ISymbol? symbol, System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractIndex!> indices, Microsoft.CodeAnalysis.ITypeSymbol! type, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue! instanceLocation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity? parent) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.Create(Microsoft.CodeAnalysis.Operations.IInstanceReferenceOperation! instanceReferenceOperation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue! instanceLocation) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity.CreateThisOrMeInstance(Microsoft.CodeAnalysis.INamedTypeSymbol! typeSymbol, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue! instanceLocation) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity!
 static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.IsChildAnalysisEntity(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity! entity, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity! ancestorEntity) -> bool

src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_NullAnalysis.cs

@@ -7280,5 +7280,124 @@ private void Method3(IReadOnlyList<int>? items3 = null)
                 },
             }.RunAsync();
         }
+
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.CopyAnalysis)]
+        [Fact, WorkItem(6520, "https://github.com/dotnet/roslyn-analyzers/issues/6520")]
+        public async Task CompareTwoUnrelatedEnumVariables_NoDiagnosticsAsync()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+public class Parser
+{
+    private static ParsedPredicate ParsePredicate(ParsedPredicate predicate, PredicateOperand identifier, PredicateOperand literal)
+    {
+        if (predicate.Left.TypePrimitive == PredicateTypePrimitive.F1)
+        {
+            identifier = predicate.Left;
+            literal = predicate.Right;
+        }
+        else
+        {
+            identifier = predicate.Right;
+            literal = predicate.Left;
+        }
+
+        if (identifier.TypePrimitive != PredicateTypePrimitive.F1)
+        {
+            return predicate;
+        }
+
+        if (literal.TypePrimitive == PredicateTypePrimitive.F2)
+        {
+        }
+
+        return predicate;
+    }
+}
+
+public class PredicateOperand
+{
+    public PredicateTypePrimitive TypePrimitive { get; set; }
+}
+
+public enum PredicateTypePrimitive
+{
+    F1,
+    F2,
+}
+
+public class ParsedPredicate
+{
+    public PredicateOperand Left { get; }
+    public PredicateOperand Right { get; }
+}",
+                LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8,
+            }.RunAsync();
+        }
+
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.CopyAnalysis)]
+        [Fact, WorkItem(6520, "https://github.com/dotnet/roslyn-analyzers/issues/6520")]
+        public async Task CompareTwoRelatedEnumVariables_DiagnosticsAsync()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+public class Parser
+{
+    private static ParsedPredicate ParsePredicate(ParsedPredicate predicate, PredicateOperand identifier, PredicateOperand literal)
+    {
+        if (predicate.Left.TypePrimitive == PredicateTypePrimitive.F1)
+        {
+            identifier = predicate.Left;
+            literal = predicate.Right;
+        }
+        else
+        {
+            identifier = predicate.Right;
+            literal = predicate.Left;
+        }
+
+        if (identifier.TypePrimitive != PredicateTypePrimitive.F1)
+        {
+            return predicate;
+        }
+
+        if (identifier.TypePrimitive == PredicateTypePrimitive.F2)
+        {
+        }
+
+        return predicate;
+    }
+}
+
+public class PredicateOperand
+{
+    public PredicateTypePrimitive TypePrimitive { get; set; }
+}
+
+public enum PredicateTypePrimitive
+{
+    F1,
+    F2,
+}
+
+public class ParsedPredicate
+{
+    public PredicateOperand Left { get; }
+    public PredicateOperand Right { get; }
+}",
+                LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8,
+                ExpectedDiagnostics =
+                {
+                    // /0/Test0.cs(22,13): warning CA1508: 'identifier.TypePrimitive == PredicateTypePrimitive.F2' is always 'false'. Remove or refactor the condition(s) to avoid dead code.
+                    GetCSharpResultAt(22, 13, "identifier.TypePrimitive == PredicateTypePrimitive.F2", "false")
+                }
+            }.RunAsync();
+        }
     }
 }

src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs

@@ -2733,7 +2733,9 @@ public void Load(Uri productFileUrl, Uri originalLocation = null)
         [InlineData("class", "class")]
         public async Task DataflowAcrossBranchesAsync(string typeTest, string typeA)
         {
-            await VerifyCS.VerifyAnalyzerAsync($@"
+            var test = new VerifyCS.Test
+            {
+                TestCode = $@"
 using System;
 
 namespace TestNamespace
@@ -2752,27 +2754,46 @@ public void Something(int param)
             Test t = new Test();
             t.A = new A();
             t.A.IntProperty = param;
+            A a = new A();
+            a.IntProperty = param;
+            A a1 = new A();
+            a1.IntProperty = param;
+            A a2 = new A();
+            a2.IntProperty = param;
             if (param >= 0)
             {{
-                A a1 = new A();
                 a1.IntProperty = 1;
                 t.A = a1;                    // t.A now contains/points to a1
+                a = a2;
             }}
             else
             {{
-                A a2 = new A();
                 a2.IntProperty = 1;
-                t.A = a2;                    // t.A now contains/points to a2     
+                t.A = a2;                    // t.A now contains/points to a2   
+                a = a1;
             }}
         
             if (t.A.IntProperty == 1)        // t.A now contains/points either a1 or a2, both of which have .IntProperty = """"
+                                             // However, we conservatively don't report it when 'A' is a class
+            {{
+            }}
+
+            if (a.IntProperty == 1)          // a points to a1 or a2, and a.IntProperty = param for both cases.
             {{
             }}
         }}
     }}
-}}",
-            // Test0.cs(33,17): warning CA1508: 't.A.IntProperty == 1' is always 'true'. Remove or refactor the condition(s) to avoid dead code.
-            GetCSharpResultAt(33, 17, "t.A.IntProperty == 1", "true"));
+}}"
+            };
+
+            if (typeA != "class")
+            {
+                test.ExpectedDiagnostics.Add(
+                    // Test0.cs(33,17): warning CA1508: 't.A.IntProperty == 1' is always 'true'. Remove or refactor the condition(s) to avoid dead code.
+                    GetCSharpResultAt(39, 17, "t.A.IntProperty == 1", "true"));
+            }
+
+            await test.RunAsync();
         }
 
         [Fact, WorkItem(4056, "https://github.com/dotnet/roslyn-analyzers/issues/4056")]

src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Data/ReviewSQLQueriesForSecurityVulnerabilitiesTests_FlowAnalysis.cs

@@ -3349,7 +3349,9 @@ End Sub
         [Fact]
         public async Task FlowAnalysis_PointsTo_ReferenceType_BaseDerived_IfStatement_NoDiagnosticAsync()
         {
-            await VerifyCS.VerifyAnalyzerAsync($@"
+            await new VerifyCS.Test
+            {
+                TestCode = $@"
 {SetupCodeCSharp}
 
 class Command1 : Command
@@ -3389,12 +3391,22 @@ void M1(string param)
         }}
 
         string str = t.B.Field;         // t.B now points to either b or d, both of which have .Field = """"
+                                        // However, we are forced to be conservative in our analysis due to
+                                        // potential false positives from multiple variables pointing to the same set, i.e. b or d.
+                                        // See https://github.com/dotnet/roslyn-analyzers/issues/6520 for an example.
         Command c = new Command1(str, str);
     }}
-}}
-");
+}}",
+                ExpectedDiagnostics =
+                {
+                    // /0/Test0.cs(126,21): warning CA2100: Review if the query string passed to 'Command1.Command1(string cmd, string parameter2)' in 'M1', accepts any user input
+                    GetCSharpResultAt(126, 21, "Command1.Command1(string cmd, string parameter2)", "M1"),
+                }
+            }.RunAsync();
 
-            await VerifyVB.VerifyAnalyzerAsync($@"
+            await new VerifyVB.Test
+            {
+                TestCode = $@"
 {SetupCodeBasic}
 
 Class Command1
@@ -3429,9 +3441,18 @@ Dim b As New Base()
             t.B = b                             ' t.B now points to b
         End If
         Dim str As String = t.B.Field           ' t.B now points to either b or d, both of which have .Field = """"
+                                                ' However, we are forced to be conservative in our analysis due to
+                                                ' potential false positives from multiple variables pointing to the same set, i.e. b or d.
+                                                ' See https://github.com/dotnet/roslyn-analyzers/issues/6520 for an example.
         Dim c As Command = New Command1(str, str)
     End Sub
-End Class");
+End Class",
+                ExpectedDiagnostics =
+                {
+                    // /0/Test0.vb(159,28): warning CA2100: Review if the query string passed to 'Sub Command1.New(cmd As String, parameter2 As String)' in 'M1', accepts any user input
+                    GetBasicResultAt(159, 28, "Sub Command1.New(cmd As String, parameter2 As String)", "M1")
+                }
+            }.RunAsync();
         }
 
         [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)]

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/GlobalFlowStateAnalysis/GlobalFlowStateDataFlowOperationVisitor.cs

@@ -60,7 +60,8 @@ private static AnalysisEntity GetGlobalEntity(TAnalysisContext analysisContext)
                 ImmutableArray<AbstractIndex>.Empty,
                 owningSymbol.GetMemberOrLocalOrParameterType()!,
                 instanceLocation: PointsToAbstractValue.Unknown,
-                parent: null);
+                parent: null,
+                entityForInstanceLocation: null);
         }
 
         public sealed override DictionaryAnalysisData<AnalysisEntity, TAbstractAnalysisValue> Flow(