Merge pull request #3058 from mavasani/PerfFixes

Performance fix for AnalysisEntityMapAbstractDomain.Merge algorithm

src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt

@@ -124,7 +124,7 @@ Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityFactory.TryCreateForT
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityFactory.TryGetForFlowCapture(Microsoft.CodeAnalysis.FlowAnalysis.CaptureId captureId, out Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity analysisEntity) -> bool
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityFactory.TryGetForInterproceduralAnalysis(Microsoft.CodeAnalysis.IOperation operation, out Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity analysisEntity) -> bool
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain<TValue>
-Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain<TValue>.AnalysisEntityMapAbstractDomain(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractValueDomain<TValue> valueDomain) -> void
+Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain<TValue>.AnalysisEntityMapAbstractDomain(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractValueDomain<TValue> valueDomain, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAnalysisResult pointsToAnalysisResultOpt) -> void
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo<TAbstractAnalysisValue>
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo<TAbstractAnalysisValue>.AnalysisEntityOpt.get -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity
 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo<TAbstractAnalysisValue>.ArgumentInfo(Microsoft.CodeAnalysis.IOperation operation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity analysisEntityOpt, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue instanceLocation, TAbstractAnalysisValue value) -> void
@@ -660,6 +660,7 @@ virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOpera
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.GetDefaultValueForParameterOnExit(Microsoft.CodeAnalysis.ITypeSymbol parameterType) -> TAbstractAnalysisValue
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.SetAbstractValueForAssignment(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity targetAnalysisEntity, Microsoft.CodeAnalysis.IOperation assignedValueOperationOpt, TAbstractAnalysisValue assignedValue) -> void
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain<TValue>.AssertValidAnalysisData(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DictionaryAnalysisData<Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity, TValue> map) -> void
+virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain<TValue>.OnNewMergedValue(TValue value) -> void
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.ApplyInterproceduralAnalysisResult(TAnalysisData resultData, bool isLambdaOrLocalFunction, bool hasDelegateTypeArgument, TAnalysisResult analysisResult) -> void
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.ApplyPredicatedDataForEntity(TAnalysisData analysisData, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity predicatedEntity, bool trueData) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PredicateValueKind
 virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor<TAnalysisData, TAnalysisContext, TAnalysisResult, TAbstractAnalysisValue>.AssertValidAnalysisData(TAnalysisData analysisData) -> void

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

@@ -2672,5 +2672,54 @@ public void Load(Uri productFileUrl, Uri originalLocation = null)
 }
 ", GetEditorConfigAdditionalFile(editorconfig));
         }
+
+        [Theory]
+        [InlineData("struct", "struct")]
+        [InlineData("struct", "class")]
+        [InlineData("class", "struct")]
+        [InlineData("class", "class")]
+        public void DataflowAcrossBranches(string typeTest, string typeA)
+        {
+            VerifyCSharp($@"
+using System;
+
+namespace TestNamespace
+{{
+    public {typeA} A
+    {{
+        public int IntProperty {{ get; set; }}
+    }}
+
+    public {typeTest} Test
+    {{
+        public A A;
+
+        public void Something(int param)
+        {{
+            Test t = new Test();
+            t.A = new A();
+            t.A.IntProperty = param;
+            if (param >= 0)
+            {{
+                A a1 = new A();
+                a1.IntProperty = 1;
+                t.A = a1;                    // t.A now contains/points to a1
+            }}
+            else
+            {{
+                A a2 = new A();
+                a2.IntProperty = 1;
+                t.A = a2;                    // t.A now contains/points to a2     
+            }}
+        
+            if (t.A.IntProperty == 1)        // t.A now contains/points either a1 or a2, both of which have .IntProperty = """"
+            {{
+            }}
+        }}
+    }}
+}}",
+            // 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"));
+        }
     }
 }

src/Microsoft.NetCore.Analyzers/UnitTests/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs

@@ -3066,5 +3066,197 @@ protected virtual bool HasUrl(IContext filterContext)
 }
 ");
         }
+
+        [Fact]
+        public void LotsOfAnalysisEntities_1()
+        {
+            VerifyCSharpWithDependencies(@"
+using System;
+using System.Data;
+using System.Data.SqlClient;
+using System.Web;
+
+namespace TestNamespace
+{
+    public class DataStructure
+    {
+        public string StringProperty { get; set; }
+    }
+
+    public class ExampleClass
+    {
+        public SqlCommand Something(HttpRequest request)
+        {
+            string name = request.Form[""product_name""];
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            if ((new Random()).Next(6) == 4) 
+            {
+                return null;
+            }
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            for (int i = 0; i < 3; i++)
+            {
+                new DataStructure()
+                {
+                    StringProperty = ""This is tainted: "" + name,
+                };
+            }
+
+            return null;
+        }
+    }
+}");
+        }
+
+        [Fact]
+        public void LotsOfAnalysisEntities_2()
+        {
+            VerifyCSharpWithDependencies(@"
+using System;
+using System.Data;
+using System.Data.SqlClient;
+using System.Web;
+
+namespace TestNamespace
+{
+    public class DataStructure
+    {
+        public string StringProperty { get; set; }
+    }
+
+    public class ExampleClass
+    {
+        public SqlCommand Something(HttpRequest request)
+        {
+            string name = request.Form[""product_name""];
+
+            var d1 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d2 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d3 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d4 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d5 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d6 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            if ((new Random()).Next(6) == 4) 
+            {
+                return null;
+            }
+
+            var d7 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d8 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d9 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d10 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            var d11 = new DataStructure()
+            {
+                StringProperty = ""This is tainted: "" + name,
+            };
+
+            for (int i = 0; i < 3; i++)
+            {
+                var d12 = new DataStructure()
+                {
+                    StringProperty = ""This is tainted: "" + name,
+                };
+            }
+
+            return null;
+        }
+    }
+}");
+        }
     }
 }

src/Utilities/Compiler/PooledObjects/PooledHashSet.cs

@@ -49,6 +49,9 @@ public ImmutableHashSet<T> ToImmutableAndFree()
             return result;
         }
 
+        public ImmutableHashSet<T> ToImmutable()
+            => Count == 0 ? ImmutableHashSet<T>.Empty : this.ToImmutableHashSet(Comparer);
+
         // global pool
         private static readonly ObjectPool<PooledHashSet<T>> s_poolInstance = CreatePool();
         private static readonly ConcurrentDictionary<IEqualityComparer<T>, ObjectPool<PooledHashSet<T>>> s_poolInstancesByComparer

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

@@ -39,14 +39,16 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti
                 }
 
                 value = PointsToAbstractValue.Create(AbstractLocation.CreateAnalysisEntityDefaultLocation(analysisEntity), mayBeNull: true);
-                _trackedEntitiesBuilder.AllEntities.Add(analysisEntity);
+                _trackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value);
                 _defaultPointsToValueMapBuilder.Add(analysisEntity, value);
             }
 
             return value;
         }
 
         public bool IsTrackedEntity(AnalysisEntity analysisEntity) => _defaultPointsToValueMapBuilder.ContainsKey(analysisEntity);
+        public bool IsTrackedPointsToValue(PointsToAbstractValue value) => _trackedEntitiesBuilder.IsTrackedPointsToValue(value);
+        public void AddTrackedPointsToValue(PointsToAbstractValue value) => _trackedEntitiesBuilder.AddTrackedPointsToValue(value);
         public bool HasAnyTrackedEntity => _defaultPointsToValueMapBuilder.Count > 0;
     }
 }

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

@@ -18,7 +18,7 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis<PointsToAnalysis
         private sealed class CorePointsToAnalysisDataDomain : AnalysisEntityMapAbstractDomain<PointsToAbstractValue>
         {
             public CorePointsToAnalysisDataDomain(DefaultPointsToValueGenerator defaultPointsToValueGenerator, AbstractValueDomain<PointsToAbstractValue> valueDomain)
-                : base(valueDomain)
+                : base(valueDomain, defaultPointsToValueGenerator.IsTrackedEntity, defaultPointsToValueGenerator.IsTrackedPointsToValue)
             {
                 DefaultPointsToValueGenerator = defaultPointsToValueGenerator;
             }
@@ -29,8 +29,10 @@ public CorePointsToAnalysisDataDomain(DefaultPointsToValueGenerator defaultPoint
 
             protected override bool CanSkipNewEntry(AnalysisEntity analysisEntity, PointsToAbstractValue value)
                 => value.Kind == PointsToAbstractValueKind.Unknown ||
-                    !DefaultPointsToValueGenerator.IsTrackedEntity(analysisEntity) ||
-                    value == GetDefaultValue(analysisEntity);
+                   value == GetDefaultValue(analysisEntity);
+
+            protected override void OnNewMergedValue(PointsToAbstractValue value)
+                => DefaultPointsToValueGenerator.AddTrackedPointsToValue(value);
 
             protected override void AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, PointsToAbstractValue value)
             {

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

@@ -20,7 +20,6 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis<PointsToAnalysis
         private sealed class PointsToDataFlowOperationVisitor :
             AnalysisEntityDataFlowOperationVisitor<PointsToAnalysisData, PointsToAnalysisContext, PointsToAnalysisResult, PointsToAbstractValue>
         {
-            private readonly TrackedEntitiesBuilder _trackedEntitiesBuilder;
             private readonly DefaultPointsToValueGenerator _defaultPointsToValueGenerator;
             private readonly PointsToAnalysisDomain _pointsToAnalysisDomain;
             private readonly PooledDictionary<IOperation, ImmutableHashSet<AbstractLocation>.Builder> _escapedOperationLocationsBuilder;
@@ -34,7 +33,7 @@ private sealed class PointsToDataFlowOperationVisitor :
                 PointsToAnalysisContext analysisContext)
                 : base(analysisContext)
             {
-                _trackedEntitiesBuilder = trackedEntitiesBuilder;
+                TrackedEntitiesBuilder = trackedEntitiesBuilder;
                 _defaultPointsToValueGenerator = defaultPointsToValueGenerator;
                 _pointsToAnalysisDomain = pointsToAnalysisDomain;
                 _escapedOperationLocationsBuilder = PooledDictionary<IOperation, ImmutableHashSet<AbstractLocation>.Builder>.GetInstance();
@@ -44,6 +43,8 @@ private sealed class PointsToDataFlowOperationVisitor :
                 analysisContext.InterproceduralAnalysisDataOpt?.InitialAnalysisData.AssertValidPointsToAnalysisData();
             }
 
+            internal TrackedEntitiesBuilder TrackedEntitiesBuilder { get; }
+
             public ImmutableDictionary<IOperation, ImmutableHashSet<AbstractLocation>> GetEscapedLocationsThroughOperationsMap()
                 => GetEscapedAbstractLocationsMapAndFreeBuilder(_escapedOperationLocationsBuilder);
 
@@ -119,7 +120,7 @@ protected override void AddTrackedEntities(PointsToAnalysisData analysisData, Ha
                     return;
                 }
 
-                foreach (var entity in _trackedEntitiesBuilder.AllEntities)
+                foreach (var entity in TrackedEntitiesBuilder.EnumerateEntities())
                 {
                     if (analysisData.HasAbstractValue(entity) ||
                         !forInterproceduralAnalysis && _defaultPointsToValueGenerator.IsTrackedEntity(entity))
@@ -174,7 +175,7 @@ protected override void SetAbstractValue(AnalysisEntity analysisEntity, PointsTo
                     }
 
                     SetAbstractValueCore(CurrentAnalysisData, analysisEntity, value);
-                    _trackedEntitiesBuilder.AllEntities.Add(analysisEntity);
+                    TrackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value);
                 }
             }
 

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

@@ -1,5 +1,6 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System.Collections.Immutable;
 using System.Diagnostics;
 using Analyzer.Utilities;
 using Analyzer.Utilities.Extensions;
@@ -97,7 +98,8 @@ protected override PointsToAnalysisResult ToResult(PointsToAnalysisContext analy
                 dataFlowAnalysisResult,
                 operationVisitor.GetEscapedLocationsThroughOperationsMap(),
                 operationVisitor.GetEscapedLocationsThroughReturnValuesMap(),
-                operationVisitor.GetEscapedLocationsThroughEntitiesMap());
+                operationVisitor.GetEscapedLocationsThroughEntitiesMap(),
+                operationVisitor.TrackedEntitiesBuilder);
         }
         protected override PointsToBlockAnalysisResult ToBlockResult(BasicBlock basicBlock, PointsToAnalysisData blockAnalysisData)
             => new PointsToBlockAnalysisResult(basicBlock, blockAnalysisData);