Fix flow analysis to stop tracking entities which have unknown instan…

…ce location

Fixes #6453
While performing flow analysis for loops, we clear out the instance location for analysis entities when processing the back edges. This is primarily done because same operation when processed again might refer to a different runtime object/entity for a different loop iteration, and we have no way to known if any members accessed of this object point to the same runtime instance. Even though we were correctly setting the instance location for such entities to Unknown location on back edges, we were still continuining to track the analysis data for such entities. This led to multiple such distinct entities being considered identical and sharing the same flow analysis values, as seen in the bug repro case.

The fix here is to avoid tracking such child or instance member entities which have an unknown instance location for flow analysis.

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

@@ -7059,5 +7059,38 @@ public void M(bool flag)
                 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(6453, "https://github.com/dotnet/roslyn-analyzers/issues/6453")]
+        public async Task IndexedNullCompare_NoDiagnosticAsync()
+        {
+            await VerifyCSharpAnalyzerAsync(@"
+using System.Collections.Generic;
+
+sealed class Data
+{
+    public object Value { get; }
+    public Data(object value)
+    {
+        Value = value;
+    }
+}
+
+static class Test
+{
+    static void Filter(List<Data> list, int j)
+    {
+        for (int i = 0; i < list.Count - 1; i++)
+        {
+            if (list[i + 1].Value != null) continue;
+            if (list[i].Value == null) continue; // <-------- CA1508 False positive
+            list.RemoveAt(i);
+        }
+    }
+}
+");
+        }
     }
 }

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

@@ -3152,5 +3152,37 @@ public void M(object o)
 ";
             await VerifyCS.VerifyCodeFixAsync(source, source);
         }
+
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.ValueContentAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.CopyAnalysis)]
+        [Fact, WorkItem(6453, "https://github.com/dotnet/roslyn-analyzers/issues/6453")]
+        public async Task IndexedValueCompare_NoDiagnosticAsync()
+        {
+            await VerifyCSharpAnalyzerAsync(@"
+using System.Collections.Generic;
+
+sealed class Data
+{
+    public int Value { get; }
+    public Data(int value)
+    {
+        Value = value;
+    }
+}
+
+static class Test
+{
+    static void Filter(List<Data> list, int j)
+    {
+        for (int i = 0; i < list.Count - 1; i++)
+        {
+            if (list[i + 1].Value != 0) continue;
+            if (list[i].Value == 0) continue; // <-------- CA1508 False positive
+            list.RemoveAt(i);
+        }
+    }
+}
+");
+        }
     }
 }

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs

@@ -43,8 +43,7 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti
                 value = PointsToAbstractValue.Create(AbstractLocation.CreateAnalysisEntityDefaultLocation(analysisEntity), mayBeNull: true);
 
                 // PERF: Do not track entity and its points to value for partial analysis for entities requiring complete analysis.
-                if (PointsToAnalysisKind == PointsToAnalysisKind.Complete ||
-                    !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis())
+                if (analysisEntity.ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind))
                 {
                     _trackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value);
                     _defaultPointsToValueMapBuilder.Add(analysisEntity, value);

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs

@@ -103,7 +103,7 @@ internal static bool ShouldBeTracked(AnalysisEntity analysisEntity, PointsToAnal
                 return false;
             }
 
-            return pointsToAnalysisKind == PointsToAnalysisKind.Complete || !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis();
+            return analysisEntity.ShouldBeTrackedForPointsToAnalysis(pointsToAnalysisKind);
         }
 
         [Conditional("DEBUG")]

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs

@@ -45,7 +45,7 @@ public void Dispose()
 
         public void AddEntityAndPointsToValue(AnalysisEntity analysisEntity, PointsToAbstractValue value)
         {
-            Debug.Assert(PointsToAnalysisKind == PointsToAnalysisKind.Complete || !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis());
+            Debug.Assert(analysisEntity.ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind));
 
             AllEntities.Add(analysisEntity);
             AddTrackedPointsToValue(value);

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs

@@ -34,7 +34,10 @@ protected override void ResetAbstractValue(AnalysisEntity analysisEntity)
             protected override void SetAbstractValue(AnalysisEntity analysisEntity, ValueContentAbstractValue value)
                 => SetAbstractValue(CurrentAnalysisData, analysisEntity, value);
 
-            private static void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value)
+            private void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value)
+                => SetAbstractValue(analysisData, analysisEntity, value, HasCompletePointsToAnalysisResult);
+
+            private static void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value, bool hasCompletePointsToAnalysisResult)
             {
                 // PERF: Avoid creating an entry if the value is the default unknown value.
                 if (value == ValueContentAbstractValue.MayBeContainsNonLiteralState &&
@@ -43,7 +46,10 @@ private static void SetAbstractValue(ValueContentAnalysisData analysisData, Anal
                     return;
                 }
 
-                analysisData.SetAbstractValue(analysisEntity, value);
+                if (analysisEntity.ShouldBeTrackedForAnalysis(hasCompletePointsToAnalysisResult))
+                {
+                    analysisData.SetAbstractValue(analysisEntity, value);
+                }
             }
 
             protected override bool HasAbstractValue(AnalysisEntity analysisEntity)

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntity.cs

@@ -154,17 +154,28 @@ public bool IsChildOrInstanceMember
             }
         }
 
-        internal bool IsChildOrInstanceMemberNeedingCompletePointsToAnalysis()
+        internal bool ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind pointsToAnalysisKind)
+            => ShouldBeTrackedForAnalysis(pointsToAnalysisKind == PointsToAnalysisKind.Complete);
+
+        internal bool ShouldBeTrackedForAnalysis(bool hasCompletePointsToAnalysisResult)
         {
             if (!IsChildOrInstanceMember)
             {
-                return false;
+                // We can always perform analysis for entities which are not child or instance members.
+                return true;
+            }
+
+            // If we have complete points to analysis result, and this child or instance member has a
+            // known instance location, we can perform analysis for this entity.
+            if (hasCompletePointsToAnalysisResult && !HasUnknownInstanceLocation)
+            {
+                return true;
             }
 
-            // PERF: This is the core performance optimization for partial PointsToAnalysisKind.
-            // We avoid tracking PointsToValues for all entities that are child or instance members,
+            // PERF: This is the core performance optimization when we have partial points to analysis result.
+            // We avoid tracking analysis values for all entities that are child or instance members,
             // except when they are fields or members of a value type (for example, tuple elements or struct members).
-            return Parent == null || !Parent.Type.HasValueCopySemantics();
+            return Parent != null && Parent.Type.HasValueCopySemantics();
         }
 
         public bool HasConstantValue => Symbol switch

src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs

@@ -217,8 +217,7 @@ protected override void SetAbstractValueForAssignment(IOperation target, IOperat
         {
             if (AnalysisEntityFactory.TryCreate(target, out var targetAnalysisEntity))
             {
-                if (!HasCompletePointsToAnalysisResult &&
-                    targetAnalysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis())
+                if (!targetAnalysisEntity.ShouldBeTrackedForAnalysis(HasCompletePointsToAnalysisResult))
                 {
                     // We are not tracking points to values for fields and properties.
                     // So, it is not possible to accurately track value changes to target entity which is a member.