Merge pull request #6776 from mavasani/ArrayAccess

Improve handling of indexed array access in flow analysis

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

@@ -1,6 +1,7 @@
 #nullable enable
 abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractDataFlowAnalysisContext<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.ComputeEqualsByHashCodeParts(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractDataFlowAnalysisContext<TAnalysisData!, TAnalysisContext!, TAnalysisResult!, TAbstractAnalysisValue>! obj) -> bool
 abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractDataFlowAnalysisContext<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.ComputeHashCodePartsSpecific(ref Analyzer.Utilities.RoslynHashCode hashCode) -> void
+abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityBasedPredicateAnalysisData<TValue>.ValueDomain.get -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractValueDomain<TValue>!
 abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CacheBasedEquatable<T>.ComputeEqualsByHashCodeParts(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CacheBasedEquatable<T!>! obj) -> bool
 abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CacheBasedEquatable<T>.ComputeHashCodeParts(ref Analyzer.Utilities.RoslynHashCode hashCode) -> void
 abstract Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.GetAbstractDefaultValue(Microsoft.CodeAnalysis.ITypeSymbol? type) -> TAbstractAnalysisValue

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

@@ -6762,6 +6762,103 @@ public void M(S[] arguments)
 }");
         }
 
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)]
+        [Fact, WorkItem(6616, "https://github.com/dotnet/roslyn-analyzers/issues/6616")]
+        public async Task IndexedArrayAccessWithConstantsAndNonConstants_NoDiagnosticsAsync()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+#nullable enable
+
+using System;
+
+namespace Test
+{
+    public static class Test
+    {
+        public static void Method1()
+        {
+            // default
+            var objPlayers = new object?[] { null };
+
+            for (var idx = 0; idx < 1; idx++)
+            {
+                objPlayers[idx] = $""{idx}"";
+            }
+
+            // validate
+            if (objPlayers[0] is null)    // CA1508: Always true (should probably be unknown)
+            {
+            }
+        }
+
+        public static void Method2(object?[] objPlayers, int idx)
+        {
+            objPlayers[0] = null;
+            objPlayers[idx] = 0;
+            if (objPlayers[0] is null)    // CA1508: Always true (should probably be unknown)
+            {
+            }
+        }
+    }
+}",
+                LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8,
+            }.RunAsync();
+        }
+
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)]
+        [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)]
+        [Fact, WorkItem(6616, "https://github.com/dotnet/roslyn-analyzers/issues/6616")]
+        public async Task IndexedArrayAccessWithConstantsAndNonConstants_DiagnosticsAsync()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+#nullable enable
+
+using System;
+
+namespace Test
+{
+    public static class Test
+    {
+        public static void Method1(object?[][] objPlayers, int idx)
+        {
+            objPlayers[0][0] = null;
+
+            for (idx = 0; idx < 1; idx++)
+            {
+                objPlayers[1][idx] = 0;
+            }
+
+            if (objPlayers[0][0] is null)    // CA1508: Always true
+            {
+            }
+        }
+
+        public static void Method2(object?[][] objPlayers, int idx)
+        {
+            objPlayers[0][0] = null;
+            objPlayers[1][idx] = 0;
+            if (objPlayers[0][0] is null)    // CA1508: Always true
+            {
+            }
+        }
+    }
+}",
+                LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8,
+                ExpectedDiagnostics =
+                {
+                    // /0/Test0.cs(19,17): warning CA1508: 'objPlayers[0][0] is null' is always 'true'. Remove or refactor the condition(s) to avoid dead code.
+                    GetCSharpResultAt(19, 17, "objPlayers[0][0] is null", "true"),
+                    // /0/Test0.cs(28,17): warning CA1508: 'objPlayers[0][0] is null' is always 'true'. Remove or refactor the condition(s) to avoid dead code.
+                    GetCSharpResultAt(28, 17, "objPlayers[0][0] is null", "true"),
+                },
+            }.RunAsync();
+        }
+
         [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)]
         [Fact]
         public async Task ArrayInitializerNotParentedByArrayCreationAsync()

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/CopyAnalysis/CopyAnalysis.cs

@@ -16,6 +16,8 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis
     /// </summary>
     public partial class CopyAnalysis : ForwardDataFlowAnalysis<CopyAnalysisData, CopyAnalysisContext, CopyAnalysisResult, CopyBlockAnalysisResult, CopyAbstractValue>
     {
+        internal static readonly AbstractValueDomain<CopyAbstractValue> ValueDomainInstance = CopyAbstractValueDomain.Default;
+
         private CopyAnalysis(CopyDataFlowOperationVisitor operationVisitor)
             : base(operationVisitor.AnalysisDomain, operationVisitor)
         {
@@ -43,7 +45,7 @@ private CopyAnalysis(CopyDataFlowOperationVisitor operationVisitor)
                     wellKnownTypeProvider, pointsToAnalysisKind, interproceduralAnalysisConfig,
                     interproceduralAnalysisPredicate, pessimisticAnalysis, performCopyAnalysis: false, exceptionPathsAnalysis) :
                 null;
-            var analysisContext = CopyAnalysisContext.Create(CopyAbstractValueDomain.Default, wellKnownTypeProvider,
+            var analysisContext = CopyAnalysisContext.Create(ValueDomainInstance, wellKnownTypeProvider,
                 cfg, owningSymbol, analyzerOptions, interproceduralAnalysisConfig, pessimisticAnalysis, exceptionPathsAnalysis, pointsToAnalysisResult,
                 TryGetOrComputeResultForAnalysisContext, interproceduralAnalysisPredicate);
             return TryGetOrComputeResultForAnalysisContext(analysisContext);

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/CopyAnalysis/CopyAnalysisData.cs

@@ -7,6 +7,7 @@
 
 namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis
 {
+    using static Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.CopyAnalysis.CopyAnalysis;
     using CoreCopyAnalysisData = DictionaryAnalysisData<AnalysisEntity, CopyAbstractValue>;
 
     /// <summary>
@@ -38,6 +39,7 @@ private CopyAnalysisData(CopyAnalysisData data1, CopyAnalysisData data2, MapAbst
             AssertValidCopyAnalysisData();
         }
 
+        protected override AbstractValueDomain<CopyAbstractValue> ValueDomain => ValueDomainInstance;
         public override AnalysisEntityBasedPredicateAnalysisData<CopyAbstractValue> Clone() => new CopyAnalysisData(this);
 
         public override int Compare(AnalysisEntityBasedPredicateAnalysisData<CopyAbstractValue> other, MapAbstractDomain<AnalysisEntity, CopyAbstractValue> coreDataAnalysisDomain)

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

@@ -14,7 +14,7 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis<PointsToAnalysis
         private sealed class PointsToAnalysisDomain : PredicatedAnalysisDataDomain<PointsToAnalysisData, PointsToAbstractValue>
         {
             public PointsToAnalysisDomain(DefaultPointsToValueGenerator defaultPointsToValueGenerator)
-                : base(new CorePointsToAnalysisDataDomain(defaultPointsToValueGenerator, PointsToAbstractValueDomainInstance))
+                : base(new CorePointsToAnalysisDataDomain(defaultPointsToValueGenerator, ValueDomainInstance))
             {
             }
 

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

@@ -17,7 +17,7 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis
     /// </summary>
     public partial class PointsToAnalysis : ForwardDataFlowAnalysis<PointsToAnalysisData, PointsToAnalysisContext, PointsToAnalysisResult, PointsToBlockAnalysisResult, PointsToAbstractValue>
     {
-        internal static readonly AbstractValueDomain<PointsToAbstractValue> PointsToAbstractValueDomainInstance = PointsToAbstractValueDomain.Default;
+        internal static readonly AbstractValueDomain<PointsToAbstractValue> ValueDomainInstance = PointsToAbstractValueDomain.Default;
 
         private PointsToAnalysis(PointsToAnalysisDomain analysisDomain, PointsToDataFlowOperationVisitor operationVisitor)
             : base(analysisDomain, operationVisitor)

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

@@ -51,6 +51,7 @@ private PointsToAnalysisData(PointsToAnalysisData data1, PointsToAnalysisData da
             AssertValidPointsToAnalysisData();
         }
 
+        protected override AbstractValueDomain<PointsToAbstractValue> ValueDomain => PointsToAnalysis.ValueDomainInstance;
         public override AnalysisEntityBasedPredicateAnalysisData<PointsToAbstractValue> Clone() => new PointsToAnalysisData(this);
 
         public override int Compare(AnalysisEntityBasedPredicateAnalysisData<PointsToAbstractValue> other, MapAbstractDomain<AnalysisEntity, PointsToAbstractValue> coreDataAnalysisDomain)

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs

@@ -16,6 +16,8 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis
 
     internal partial class TaintedDataAnalysis : ForwardDataFlowAnalysis<TaintedDataAnalysisData, TaintedDataAnalysisContext, TaintedDataAnalysisResult, TaintedDataBlockAnalysisResult, TaintedDataAbstractValue>
     {
+        internal static readonly AbstractValueDomain<TaintedDataAbstractValue> ValueDomainInstance = TaintedDataAbstractValueDomain.Default;
+
         private TaintedDataAnalysis(TaintedDataAnalysisDomain analysisDomain, TaintedDataOperationVisitor operationVisitor)
             : base(analysisDomain, operationVisitor)
         {
@@ -94,7 +96,7 @@ private TaintedDataAnalysis(TaintedDataAnalysisDomain analysisDomain, TaintedDat
             }
 
             TaintedDataAnalysisContext analysisContext = TaintedDataAnalysisContext.Create(
-                TaintedDataAbstractValueDomain.Default,
+                ValueDomainInstance,
                 wellKnownTypeProvider,
                 cfg,
                 containingMethod,

src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysisData.cs

@@ -29,6 +29,7 @@ public TaintedDataAnalysisData(TaintedDataAnalysisData fromData, TaintedDataAnal
         {
         }
 
+        protected override AbstractValueDomain<TaintedDataAbstractValue> ValueDomain => TaintedDataAnalysis.ValueDomainInstance;
         public override AnalysisEntityBasedPredicateAnalysisData<TaintedDataAbstractValue> Clone()
         {
             return new TaintedDataAnalysisData(this);

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

@@ -17,6 +17,8 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis
     /// </summary>
     public partial class ValueContentAnalysis : ForwardDataFlowAnalysis<ValueContentAnalysisData, ValueContentAnalysisContext, ValueContentAnalysisResult, ValueContentBlockAnalysisResult, ValueContentAbstractValue>
     {
+        internal static readonly AbstractValueDomain<ValueContentAbstractValue> ValueDomainInstance = ValueContentAbstractValueDomain.Default;
+
         private ValueContentAnalysis(ValueContentAnalysisDomain analysisDomain, ValueContentDataFlowOperationVisitor operationVisitor)
             : base(analysisDomain, operationVisitor)
         {
@@ -98,7 +100,7 @@ private ValueContentAnalysis(ValueContentAnalysisDomain analysisDomain, ValueCon
                 null;
 
             var analysisContext = ValueContentAnalysisContext.Create(
-                ValueContentAbstractValueDomain.Default, wellKnownTypeProvider, cfg, owningSymbol, analyzerOptions,
+                ValueDomainInstance, wellKnownTypeProvider, cfg, owningSymbol, analyzerOptions,
                 interproceduralAnalysisConfig, pessimisticAnalysis, copyAnalysisResult,
                 pointsToAnalysisResult, TryGetOrComputeResultForAnalysisContext,
                 additionalSupportedValueTypes, getValueContentValueForAdditionalSupportedValueTypeOperation,

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

@@ -42,6 +42,7 @@ internal ValueContentAnalysisData(
         {
         }
 
+        protected override AbstractValueDomain<ValueContentAbstractValue> ValueDomain => ValueContentAnalysis.ValueDomainInstance;
         public override AnalysisEntityBasedPredicateAnalysisData<ValueContentAbstractValue> Clone() => new ValueContentAnalysisData(this);
 
         public override int Compare(AnalysisEntityBasedPredicateAnalysisData<ValueContentAbstractValue> other, MapAbstractDomain<AnalysisEntity, ValueContentAbstractValue> coreDataAnalysisDomain)

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

@@ -11,5 +11,7 @@ public abstract partial class AbstractIndex : CacheBasedEquatable<AbstractIndex>
         public static AbstractIndex Create(int index) => new ConstantValueIndex(index);
         public static AbstractIndex Create(AnalysisEntity analysisEntity) => new AnalysisEntityBasedIndex(analysisEntity);
         public static AbstractIndex Create(IOperation operation) => new OperationBasedIndex(operation);
+
+        internal bool IsConstant() => this is ConstantValueIndex;
     }
 }

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

@@ -120,7 +120,7 @@ public AnalysisEntity WithMergedInstanceLocation(AnalysisEntity analysisEntityTo
             Debug.Assert(EqualsIgnoringInstanceLocation(analysisEntityToMerge));
             Debug.Assert(!InstanceLocation.Equals(analysisEntityToMerge.InstanceLocation));
 
-            var mergedInstanceLocation = PointsToAnalysis.PointsToAnalysis.PointsToAbstractValueDomainInstance.Merge(InstanceLocation, analysisEntityToMerge.InstanceLocation);
+            var mergedInstanceLocation = PointsToAnalysis.PointsToAnalysis.ValueDomainInstance.Merge(InstanceLocation, analysisEntityToMerge.InstanceLocation);
             return new AnalysisEntity(Symbol, Indices, InstanceReferenceOperationSyntax, CaptureId, mergedInstanceLocation, Type, Parent, IsThisOrMeInstance);
         }
 
@@ -204,6 +204,9 @@ or PointsToAbstractValueKind.UnknownNull
 
         public bool IsLValueFlowCaptureEntity => CaptureId.HasValue && CaptureId.Value.IsLValueFlowCapture;
 
+        internal AnalysisEntity WithIndices(ImmutableArray<AbstractIndex> indices)
+            => new(Symbol, indices, InstanceReferenceOperationSyntax, CaptureId, InstanceLocation, Type, Parent, IsThisOrMeInstance);
+
         public bool EqualsIgnoringInstanceLocation(AnalysisEntity? other)
         {
             // Perform fast equality checks first.

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

@@ -5,6 +5,7 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
+using System.Linq;
 
 namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow
 {
@@ -58,6 +59,7 @@ protected AnalysisEntityBasedPredicateAnalysisData(
 
         public virtual bool HasAnyAbstractValue => CoreAnalysisData.Count > 0 || HasPredicatedData;
 
+        protected abstract AbstractValueDomain<TValue> ValueDomain { get; }
         public abstract AnalysisEntityBasedPredicateAnalysisData<TValue> Clone();
         public abstract AnalysisEntityBasedPredicateAnalysisData<TValue> WithMergedData(AnalysisEntityBasedPredicateAnalysisData<TValue> data, MapAbstractDomain<AnalysisEntity, TValue> coreDataAnalysisDomain);
         public abstract int Compare(AnalysisEntityBasedPredicateAnalysisData<TValue> other, MapAbstractDomain<AnalysisEntity, TValue> coreDataAnalysisDomain);
@@ -100,6 +102,8 @@ public virtual void SetAbstractValue(AnalysisEntity key, TValue value)
             }
 
             CoreAnalysisData[key] = value;
+
+            ClearOverlappingAnalysisDataForIndexedEntity(key, value);
         }
 
         public void RemoveEntries(AnalysisEntity key)
@@ -159,6 +163,46 @@ public PredicateValueKind ApplyPredicatedDataForEntity(AnalysisEntity predicated
 
         public void AddTrackedEntities(HashSet<AnalysisEntity> builder) => builder.UnionWith(CoreAnalysisData.Keys);
 
+        private void ClearOverlappingAnalysisDataForIndexedEntity(AnalysisEntity analysisEntity, TValue value)
+        {
+            if (!analysisEntity.Indices.Any(index => !index.IsConstant()))
+            {
+                return;
+            }
+
+            foreach (var entity in CoreAnalysisData.Keys)
+            {
+                if (entity.Indices.Length != analysisEntity.Indices.Length ||
+                    entity == analysisEntity)
+                {
+                    continue;
+                }
+
+                var canOverlap = true;
+                for (var i = 0; i < entity.Indices.Length; i++)
+                {
+                    if (entity.Indices[i].IsConstant() &&
+                        analysisEntity.Indices[i].IsConstant() &&
+                        !entity.Indices[i].Equals(analysisEntity.Indices[i]))
+                    {
+                        canOverlap = false;
+                        break;
+                    }
+                }
+
+                if (canOverlap &&
+                    entity.WithIndices(analysisEntity.Indices).Equals(analysisEntity) &&
+                    CoreAnalysisData.TryGetValue(entity, out var existingValue))
+                {
+                    var mergedValue = ValueDomain.Merge(value, existingValue);
+                    if (!existingValue!.Equals(mergedValue))
+                    {
+                        SetAbstractValue(entity, mergedValue);
+                    }
+                }
+            }
+        }
+
         protected override void Dispose(bool disposing)
         {
             if (IsDisposed)