Merge pull request #6466 from mavasani/Fix6453

Fix flow analysis to stop tracking entities which have unknown instance location

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.