From 5f67d699e066d9c0f4bd1c1ed87e71161ff70728 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Thu, 21 Nov 2019 05:17:32 -0800 Subject: [PATCH 1/6] Performance fix for AnalysisEntityMapAbstractDomain.Merge algorithm We now ensure that we only create new entries in the result map if the entity key is a valid tracked entity --- .../PublicAPI.Unshipped.txt | 2 +- ...CodeForSqlInjectionVulnerabilitiesTests.cs | 192 ++++++++++++++++++ ...Analysis.CorePointsToAnalysisDataDomain.cs | 5 +- ...alysis.PointsToDataFlowOperationVisitor.cs | 9 +- .../PointsToAnalysis/PointsToAnalysis.cs | 4 +- .../PointsToAnalysisResult.cs | 8 +- ...lysis.CoreTaintedDataAnalysisDataDomain.cs | 6 +- ...ataAnalysis.TaintedDataOperationVisitor.cs | 7 +- .../TaintedDataAnalysis.cs | 11 +- ...eContentAnalysis.CoreAnalysisDataDomain.cs | 6 +- ...tentAnalysis.ValueContentAnalysisDomain.cs | 7 +- ...is.ValueContentDataFlowOperationVisitor.cs | 9 +- .../ValueContentAnalysis.cs | 9 +- .../AnalysisEntityMapAbstractDomain.cs | 17 +- 14 files changed, 255 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt b/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt index 869544e9c9..be954671c4 100644 --- a/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt +++ b/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 -Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain.AnalysisEntityMapAbstractDomain(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractValueDomain valueDomain) -> void +Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain.AnalysisEntityMapAbstractDomain(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AbstractValueDomain valueDomain, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAnalysisResult pointsToAnalysisResultOpt) -> void Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo.AnalysisEntityOpt.get -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ArgumentInfo.ArgumentInfo(Microsoft.CodeAnalysis.IOperation operation, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity analysisEntityOpt, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis.PointsToAbstractValue instanceLocation, TAbstractAnalysisValue value) -> void diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs index a4d4057a25..46b2c8900d 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/ReviewCodeForSqlInjectionVulnerabilitiesTests.cs +++ b/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; + } + } +}"); + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs index 661d5e954f..8612cbd984 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs @@ -18,7 +18,7 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis { public CorePointsToAnalysisDataDomain(DefaultPointsToValueGenerator defaultPointsToValueGenerator, AbstractValueDomain valueDomain) - : base(valueDomain) + : base(valueDomain, defaultPointsToValueGenerator.IsTrackedEntity) { DefaultPointsToValueGenerator = defaultPointsToValueGenerator; } @@ -29,8 +29,7 @@ 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 AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, PointsToAbstractValue value) { diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs index 9de04aa94d..cc415b51d8 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs @@ -20,7 +20,6 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis { - private readonly TrackedEntitiesBuilder _trackedEntitiesBuilder; private readonly DefaultPointsToValueGenerator _defaultPointsToValueGenerator; private readonly PointsToAnalysisDomain _pointsToAnalysisDomain; private readonly PooledDictionary.Builder> _escapedOperationLocationsBuilder; @@ -34,7 +33,7 @@ private sealed class PointsToDataFlowOperationVisitor : PointsToAnalysisContext analysisContext) : base(analysisContext) { - _trackedEntitiesBuilder = trackedEntitiesBuilder; + TrackedEntitiesBuilder = trackedEntitiesBuilder; _defaultPointsToValueGenerator = defaultPointsToValueGenerator; _pointsToAnalysisDomain = pointsToAnalysisDomain; _escapedOperationLocationsBuilder = PooledDictionary.Builder>.GetInstance(); @@ -44,6 +43,8 @@ private sealed class PointsToDataFlowOperationVisitor : analysisContext.InterproceduralAnalysisDataOpt?.InitialAnalysisData.AssertValidPointsToAnalysisData(); } + internal TrackedEntitiesBuilder TrackedEntitiesBuilder { get; } + public ImmutableDictionary> 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.AllEntities) { 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.AllEntities.Add(analysisEntity); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs index e75ada110c..cf6c6f1a8b 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs +++ b/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.AllEntities.ToImmutableHashSet()); } protected override PointsToBlockAnalysisResult ToBlockResult(BasicBlock basicBlock, PointsToAnalysisData blockAnalysisData) => new PointsToBlockAnalysisResult(basicBlock, blockAnalysisData); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs index 0ca375aee4..4488b0054f 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs @@ -12,17 +12,20 @@ public sealed class PointsToAnalysisResult : DataFlowAnalysisResult> _escapedLocationsThroughOperationsMap; private readonly ImmutableDictionary> _escapedLocationsThroughReturnValuesMap; private readonly ImmutableDictionary> _escapedLocationsThroughEntitiesMap; + private readonly ImmutableHashSet _trackedEntities; internal PointsToAnalysisResult( DataFlowAnalysisResult corePointsToAnalysisResult, ImmutableDictionary> escapedLocationsThroughOperationsMap, ImmutableDictionary> escapedLocationsThroughReturnValuesMap, - ImmutableDictionary> escapedLocationsThroughEntitiesMap) + ImmutableDictionary> escapedLocationsThroughEntitiesMap, + ImmutableHashSet trackedEntities) : base(corePointsToAnalysisResult) { _escapedLocationsThroughOperationsMap = escapedLocationsThroughOperationsMap; _escapedLocationsThroughReturnValuesMap = escapedLocationsThroughReturnValuesMap; _escapedLocationsThroughEntitiesMap = escapedLocationsThroughEntitiesMap; + _trackedEntities = trackedEntities; } public ImmutableHashSet GetEscapedAbstractLocations(IOperation operation) @@ -44,5 +47,8 @@ public ImmutableHashSet GetEscapedAbstractLocations(AnalysisEn return ImmutableHashSet.Empty; } + + internal bool IsTrackedEntity(AnalysisEntity analysisEntity) + => _trackedEntities.Contains(analysisEntity); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.CoreTaintedDataAnalysisDataDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.CoreTaintedDataAnalysisDataDomain.cs index 9356f92cff..8d27c947fd 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.CoreTaintedDataAnalysisDataDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.CoreTaintedDataAnalysisDataDomain.cs @@ -1,6 +1,7 @@ // 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 Microsoft.CodeAnalysis.FlowAnalysis.DataFlow; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis { @@ -8,9 +9,8 @@ internal partial class TaintedDataAnalysis { private sealed class CoreTaintedDataAnalysisDataDomain : AnalysisEntityMapAbstractDomain { - public static readonly CoreTaintedDataAnalysisDataDomain Instance = new CoreTaintedDataAnalysisDataDomain(TaintedDataAbstractValueDomain.Default); - - private CoreTaintedDataAnalysisDataDomain(TaintedDataAbstractValueDomain valueDomain) : base(valueDomain) + public CoreTaintedDataAnalysisDataDomain(PointsToAnalysisResult pointsToAnalysisResultOpt) + : base(TaintedDataAbstractValueDomain.Default, pointsToAnalysisResultOpt) { } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs index 981c823df9..054e9eb722 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.TaintedDataOperationVisitor.cs @@ -21,15 +21,18 @@ internal partial class TaintedDataAnalysis { private sealed class TaintedDataOperationVisitor : AnalysisEntityDataFlowOperationVisitor { + private readonly TaintedDataAnalysisDomain _taintedDataAnalysisDomain; + /// /// Mapping of a tainted data sinks to their originating sources. /// /// Keys are sinks where the tainted data entered, values are s where the tainted data originated from. private Dictionary.Builder SinkKinds, ImmutableHashSet.Builder SourceOrigins)> TaintedSourcesBySink { get; } - public TaintedDataOperationVisitor(TaintedDataAnalysisContext analysisContext) + public TaintedDataOperationVisitor(TaintedDataAnalysisDomain taintedDataAnalysisDomain, TaintedDataAnalysisContext analysisContext) : base(analysisContext) { + _taintedDataAnalysisDomain = taintedDataAnalysisDomain; this.TaintedSourcesBySink = new Dictionary.Builder SinkKinds, ImmutableHashSet.Builder SourceOrigins)>(); } @@ -83,7 +86,7 @@ protected override bool HasAnyAbstractValue(TaintedDataAnalysisData data) protected override TaintedDataAnalysisData MergeAnalysisData(TaintedDataAnalysisData value1, TaintedDataAnalysisData value2) { - return TaintedDataAnalysisDomainInstance.Merge(value1, value2); + return _taintedDataAnalysisDomain.Merge(value1, value2); } protected override void UpdateValuesForAnalysisData(TaintedDataAnalysisData targetAnalysisData) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs index c15383cb11..ac81824fee 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/TaintedDataAnalysis/TaintedDataAnalysis.cs @@ -17,10 +17,8 @@ namespace Analyzer.Utilities.FlowAnalysis.Analysis.TaintedDataAnalysis internal partial class TaintedDataAnalysis : ForwardDataFlowAnalysis { - private static readonly TaintedDataAnalysisDomain TaintedDataAnalysisDomainInstance = new TaintedDataAnalysisDomain(CoreTaintedDataAnalysisDataDomain.Instance); - - private TaintedDataAnalysis(TaintedDataOperationVisitor operationVisitor) - : base(TaintedDataAnalysisDomainInstance, operationVisitor) + private TaintedDataAnalysis(TaintedDataAnalysisDomain analysisDomain, TaintedDataOperationVisitor operationVisitor) + : base(analysisDomain, operationVisitor) { } @@ -116,8 +114,9 @@ private TaintedDataAnalysis(TaintedDataOperationVisitor operationVisitor) private static TaintedDataAnalysisResult TryGetOrComputeResultForAnalysisContext(TaintedDataAnalysisContext analysisContext) { - TaintedDataOperationVisitor visitor = new TaintedDataOperationVisitor(analysisContext); - TaintedDataAnalysis analysis = new TaintedDataAnalysis(visitor); + TaintedDataAnalysisDomain analysisDomain = new TaintedDataAnalysisDomain(new CoreTaintedDataAnalysisDataDomain(analysisContext.PointsToAnalysisResultOpt)); + TaintedDataOperationVisitor visitor = new TaintedDataOperationVisitor(analysisDomain, analysisContext); + TaintedDataAnalysis analysis = new TaintedDataAnalysis(analysisDomain, visitor); return analysis.TryGetOrComputeResultCore(analysisContext, cacheResult: true); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.CoreAnalysisDataDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.CoreAnalysisDataDomain.cs index 3bbbce9496..e8ae677f55 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.CoreAnalysisDataDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.CoreAnalysisDataDomain.cs @@ -2,6 +2,7 @@ using System.Diagnostics; using System.Linq; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis { @@ -15,9 +16,8 @@ public partial class ValueContentAnalysis : ForwardDataFlowAnalysis private sealed class CoreAnalysisDataDomain : AnalysisEntityMapAbstractDomain { - public static readonly CoreAnalysisDataDomain Instance = new CoreAnalysisDataDomain(ValueContentAbstractValueDomain.Default); - - private CoreAnalysisDataDomain(AbstractValueDomain valueDomain) : base(valueDomain) + public CoreAnalysisDataDomain(AbstractValueDomain valueDomain, PointsToAnalysisResult pointsToAnalysisResultOpt) + : base(valueDomain, pointsToAnalysisResultOpt) { } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentAnalysisDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentAnalysisDomain.cs index 1e331d607b..04f8b23c01 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentAnalysisDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentAnalysisDomain.cs @@ -1,6 +1,7 @@ // 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.Diagnostics; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis { @@ -13,10 +14,8 @@ public partial class ValueContentAnalysis : ForwardDataFlowAnalysis private sealed class ValueContentAnalysisDomain : PredicatedAnalysisDataDomain { - public static readonly ValueContentAnalysisDomain Instance = new ValueContentAnalysisDomain(); - - private ValueContentAnalysisDomain() - : base(CoreAnalysisDataDomain.Instance) + public ValueContentAnalysisDomain(PointsToAnalysisResult pointsToAnalysisResult) + : base(new CoreAnalysisDataDomain(ValueContentAbstractValueDomain.Default, pointsToAnalysisResult)) { } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs index 06bf6fc6f4..35e89bd822 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs @@ -17,9 +17,12 @@ public partial class ValueContentAnalysis : ForwardDataFlowAnalysis private sealed class ValueContentDataFlowOperationVisitor : AnalysisEntityDataFlowOperationVisitor { - public ValueContentDataFlowOperationVisitor(ValueContentAnalysisContext analysisContext) + private readonly ValueContentAnalysisDomain _valueContentAnalysisDomain; + + public ValueContentDataFlowOperationVisitor(ValueContentAnalysisDomain valueContentAnalysisDomain, ValueContentAnalysisContext analysisContext) : base(analysisContext) { + _valueContentAnalysisDomain = valueContentAnalysisDomain; } protected override void AddTrackedEntities(ValueContentAnalysisData analysisData, HashSet builder, bool forInterproceduralAnalysis) @@ -134,9 +137,9 @@ private void SetValueForComparisonOperator(IOperation target, IOperation assigne #endregion protected override ValueContentAnalysisData MergeAnalysisData(ValueContentAnalysisData value1, ValueContentAnalysisData value2) - => ValueContentAnalysisDomain.Instance.Merge(value1, value2); + => _valueContentAnalysisDomain.Merge(value1, value2); protected override ValueContentAnalysisData MergeAnalysisDataForBackEdge(ValueContentAnalysisData value1, ValueContentAnalysisData value2) - => ValueContentAnalysisDomain.Instance.MergeAnalysisDataForBackEdge(value1, value2); + => _valueContentAnalysisDomain.MergeAnalysisDataForBackEdge(value1, value2); protected override void UpdateValuesForAnalysisData(ValueContentAnalysisData targetAnalysisData) => UpdateValuesForAnalysisData(targetAnalysisData.CoreAnalysisData, CurrentAnalysisData.CoreAnalysisData); protected override ValueContentAnalysisData GetClonedAnalysisData(ValueContentAnalysisData analysisData) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs index b268dfb46a..78080f7a8c 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.cs @@ -17,8 +17,8 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.ValueContentAnalysis /// public partial class ValueContentAnalysis : ForwardDataFlowAnalysis { - private ValueContentAnalysis(ValueContentDataFlowOperationVisitor operationVisitor) - : base(ValueContentAnalysisDomain.Instance, operationVisitor) + private ValueContentAnalysis(ValueContentAnalysisDomain analysisDomain, ValueContentDataFlowOperationVisitor operationVisitor) + : base(analysisDomain, operationVisitor) { } @@ -98,8 +98,9 @@ private ValueContentAnalysis(ValueContentDataFlowOperationVisitor operationVisit private static ValueContentAnalysisResult TryGetOrComputeResultForAnalysisContext(ValueContentAnalysisContext analysisContext) { - var operationVisitor = new ValueContentDataFlowOperationVisitor(analysisContext); - var nullAnalysis = new ValueContentAnalysis(operationVisitor); + var analysisDomain = new ValueContentAnalysisDomain(analysisContext.PointsToAnalysisResultOpt); + var operationVisitor = new ValueContentDataFlowOperationVisitor(analysisDomain, analysisContext); + var nullAnalysis = new ValueContentAnalysis(analysisDomain, operationVisitor); return nullAnalysis.TryGetOrComputeResultCore(analysisContext, cacheResult: true); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs index fcdf8dbab2..f5f8870921 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs @@ -1,8 +1,10 @@ // 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; using System.Diagnostics; using System.Linq; using Analyzer.Utilities.PooledObjects; +using Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis; namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow { @@ -11,13 +13,24 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow /// public abstract class AnalysisEntityMapAbstractDomain : MapAbstractDomain { - protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomain) + private static readonly Func s_defaultIsTrackedEntity = new Func(_ => true); + private readonly Func _isTrackedEntity; + + private protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomain, Func isTrackedEntity) : base(valueDomain) + { + _isTrackedEntity = isTrackedEntity ?? throw new ArgumentNullException(nameof(isTrackedEntity)); + } + + protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomain, PointsToAnalysisResult pointsToAnalysisResultOpt) + : this(valueDomain, pointsToAnalysisResultOpt != null ? pointsToAnalysisResultOpt.IsTrackedEntity : s_defaultIsTrackedEntity) { } protected abstract TValue GetDefaultValue(AnalysisEntity analysisEntity); protected abstract bool CanSkipNewEntry(AnalysisEntity analysisEntity, TValue value); + private bool IsNonTrackedEntityOrCanSkipNewEntry(AnalysisEntity analysisEntity, TValue value) + => !_isTrackedEntity(analysisEntity) || CanSkipNewEntry(analysisEntity, value); protected abstract void AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, TValue value); protected virtual void AssertValidAnalysisData(DictionaryAnalysisData map) @@ -125,7 +138,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) Debug.Assert(ValueDomain.Compare(mergedValue, newMergedValue) <= 0); mergedValue = newMergedValue; - if (!isExistingKeyInInput && !isExistingKeyInResult && CanSkipNewEntry(mergedKey, mergedValue)) + if (!isExistingKeyInInput && !isExistingKeyInResult && IsNonTrackedEntityOrCanSkipNewEntry(mergedKey, mergedValue)) { // PERF: Do not add a new key-value pair to the resultMap if the value can be skipped. continue; From 3794cc9c75e900e33de0c3a0f7c087e02745a1b4 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Thu, 21 Nov 2019 05:58:37 -0800 Subject: [PATCH 2/6] Further optimization to delay merging values until required --- .../AnalysisEntityMapAbstractDomain.cs | 60 +++++++++++++------ 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs index f5f8870921..be364662c3 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs @@ -29,8 +29,6 @@ protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomai protected abstract TValue GetDefaultValue(AnalysisEntity analysisEntity); protected abstract bool CanSkipNewEntry(AnalysisEntity analysisEntity, TValue value); - private bool IsNonTrackedEntityOrCanSkipNewEntry(AnalysisEntity analysisEntity, TValue value) - => !_isTrackedEntity(analysisEntity) || CanSkipNewEntry(analysisEntity, value); protected abstract void AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, TValue value); protected virtual void AssertValidAnalysisData(DictionaryAnalysisData map) @@ -62,7 +60,8 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) } var resultMap = new DictionaryAnalysisData(); - var newKeys = PooledHashSet.GetInstance(); + using var newKeys = PooledHashSet.GetInstance(); + using var valuesToMergeBuilder = ArrayBuilder.GetInstance(5); var map2LookupIgnoringInstanceLocation = map2.Keys.Where(IsAnalysisEntityForFieldOrProperty) .ToLookup(entity => entity.EqualsIgnoringInstanceLocationId); @@ -94,12 +93,14 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) } TValue value2 = map2[key2]; - TValue mergedValue = ValueDomain.Merge(value1, value2); - Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); - Debug.Assert(ValueDomain.Compare(value2, mergedValue) <= 0); + + valuesToMergeBuilder.Clear(); + valuesToMergeBuilder.Add(value1); + valuesToMergeBuilder.Add(value2); if (key1.InstanceLocation.Equals(key2.InstanceLocation)) { + var mergedValue = GetMergedValue(valuesToMergeBuilder); AssertValidEntryForMergedMap(key1, mergedValue); resultMap[key1] = mergedValue; } @@ -112,33 +113,40 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) } AnalysisEntity mergedKey = key1.WithMergedInstanceLocation(key2); - TValue newMergedValue = mergedValue; + var isExistingKeyInInput = false; var isExistingKeyInResult = false; if (resultMap.TryGetValue(mergedKey, out var existingValue)) { - newMergedValue = ValueDomain.Merge(newMergedValue, existingValue); + valuesToMergeBuilder.Add(existingValue); isExistingKeyInResult = true; } if (map1.TryGetValue(mergedKey, out existingValue)) { - newMergedValue = ValueDomain.Merge(newMergedValue, existingValue); + valuesToMergeBuilder.Add(existingValue); isExistingKeyInInput = true; } if (map2.TryGetValue(mergedKey, out existingValue)) { - newMergedValue = ValueDomain.Merge(newMergedValue, existingValue); + valuesToMergeBuilder.Add(existingValue); isExistingKeyInInput = true; } - Debug.Assert(ValueDomain.Compare(value1, newMergedValue) <= 0); - Debug.Assert(ValueDomain.Compare(value2, newMergedValue) <= 0); - Debug.Assert(ValueDomain.Compare(mergedValue, newMergedValue) <= 0); - mergedValue = newMergedValue; + var isCandidateToBeSkipped = !isExistingKeyInInput && !isExistingKeyInResult; + if (isCandidateToBeSkipped && !_isTrackedEntity(mergedKey)) + { + // PERF: Do not add a new key-value pair to the resultMap if the key is not a valid tracked entity. + continue; + } - if (!isExistingKeyInInput && !isExistingKeyInResult && IsNonTrackedEntityOrCanSkipNewEntry(mergedKey, mergedValue)) + var mergedValue = GetMergedValue(valuesToMergeBuilder); + + Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); + Debug.Assert(ValueDomain.Compare(value2, mergedValue) <= 0); + + if (isCandidateToBeSkipped && CanSkipNewEntry(mergedKey, mergedValue)) { // PERF: Do not add a new key-value pair to the resultMap if the value can be skipped. continue; @@ -195,8 +203,6 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) } } - newKeys.Free(); - Debug.Assert(Compare(map1, resultMap) <= 0); Debug.Assert(Compare(map2, resultMap) <= 0); AssertValidAnalysisData(resultMap); @@ -204,6 +210,26 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) return resultMap; static bool IsAnalysisEntityForFieldOrProperty(AnalysisEntity entity) => entity.SymbolOpt?.Kind == SymbolKind.Field || entity.SymbolOpt?.Kind == SymbolKind.Property; + + TValue GetMergedValue(ArrayBuilder values) + { + Debug.Assert(values.Count > 0); + var mergedValue = values[0]; + for (var i = 1; i < values.Count; i++) + { + mergedValue = GetMergedValueCore(mergedValue, values[i]); + } + + return mergedValue; + + TValue GetMergedValueCore(TValue value1, TValue value2) + { + TValue mergedValue = ValueDomain.Merge(value1, value2); + Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); + Debug.Assert(ValueDomain.Compare(value2, mergedValue) <= 0); + return mergedValue; + } + } } } } \ No newline at end of file From efd756e6b97966aa515a68556ea13833731e9e96 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Thu, 21 Nov 2019 06:19:58 -0800 Subject: [PATCH 3/6] Refactor TrackedEntitiesBuilder to not expose the underlying tracked entities set --- .../DefaultPointsToValueGenerator.cs | 2 +- ...alysis.PointsToDataFlowOperationVisitor.cs | 4 ++-- .../PointsToAnalysis/PointsToAnalysis.cs | 2 +- .../TrackedEntitiesBuilder.cs | 23 +++++++++++++++---- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs index 169a98a1d2..1fac60180d 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs @@ -39,7 +39,7 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti } value = PointsToAbstractValue.Create(AbstractLocation.CreateAnalysisEntityDefaultLocation(analysisEntity), mayBeNull: true); - _trackedEntitiesBuilder.AllEntities.Add(analysisEntity); + _trackedEntitiesBuilder.AddEntity(analysisEntity); _defaultPointsToValueMapBuilder.Add(analysisEntity, value); } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs index cc415b51d8..508b23584d 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs @@ -120,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)) @@ -175,7 +175,7 @@ protected override void SetAbstractValue(AnalysisEntity analysisEntity, PointsTo } SetAbstractValueCore(CurrentAnalysisData, analysisEntity, value); - TrackedEntitiesBuilder.AllEntities.Add(analysisEntity); + TrackedEntitiesBuilder.AddEntity(analysisEntity); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs index cf6c6f1a8b..d8f21bb09a 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs @@ -99,7 +99,7 @@ protected override PointsToAnalysisResult ToResult(PointsToAnalysisContext analy operationVisitor.GetEscapedLocationsThroughOperationsMap(), operationVisitor.GetEscapedLocationsThroughReturnValuesMap(), operationVisitor.GetEscapedLocationsThroughEntitiesMap(), - operationVisitor.TrackedEntitiesBuilder.AllEntities.ToImmutableHashSet()); + operationVisitor.TrackedEntitiesBuilder.ToImmutable()); } protected override PointsToBlockAnalysisResult ToBlockResult(BasicBlock basicBlock, PointsToAnalysisData blockAnalysisData) => new PointsToBlockAnalysisResult(basicBlock, blockAnalysisData); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs index 2dc200d097..a51150f9bf 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs @@ -1,6 +1,8 @@ // 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; +using System.Collections.Generic; +using System.Collections.Immutable; using Analyzer.Utilities.PooledObjects; namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis @@ -10,16 +12,29 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis /// internal sealed class TrackedEntitiesBuilder : IDisposable { + /// + /// Stores all the tracked entities. + /// NOTE: Entities added to this set should not be removed. + /// + private PooledHashSet _allEntities { get; } + public TrackedEntitiesBuilder() { - AllEntities = PooledHashSet.GetInstance(); + _allEntities = PooledHashSet.GetInstance(); } - public PooledHashSet AllEntities { get; } - public void Dispose() { - AllEntities.Free(); + _allEntities.Free(); } + + public void AddEntity(AnalysisEntity analysisEntity) + => _allEntities.Add(analysisEntity); + + public IEnumerable EnumerateEntities() + => _allEntities; + + public ImmutableHashSet ToImmutable() + => _allEntities.ToImmutableHashSet(); } } From 4e94ce44b419afd09c571b9462f1300a302502ad Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Fri, 22 Nov 2019 04:00:16 -0800 Subject: [PATCH 4/6] Add more unit tests - couple of these pass prior to this fix and fail with this fix. Working on addressing these failures in a follow-up commit. --- ...eadConditionalCode_ValueContentAnalysis.cs | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/Microsoft.CodeQuality.Analyzers/UnitTests/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs b/src/Microsoft.CodeQuality.Analyzers/UnitTests/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs index 649eeb97b4..428912c098 100644 --- a/src/Microsoft.CodeQuality.Analyzers/UnitTests/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs +++ b/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")); + } } } From 3a02a748bdd3b96a7fde93e1e14298155cbb4a1f Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Fri, 22 Nov 2019 05:24:06 -0800 Subject: [PATCH 5/6] Track the reachable PointsTo values during PointsToAnalysis to ensure that new reachable entities from Merge algorithm are not skipped. This commit fixes the analysis for tests added in previous commit. --- .../PublicAPI.Unshipped.txt | 1 + .../Compiler/PooledObjects/PooledHashSet.cs | 3 + .../DefaultPointsToValueGenerator.cs | 4 +- ...Analysis.CorePointsToAnalysisDataDomain.cs | 5 +- ...alysis.PointsToDataFlowOperationVisitor.cs | 2 +- .../PointsToAnalysis/PointsToAnalysis.cs | 2 +- .../PointsToAnalysisResult.cs | 8 +- .../TrackedEntitiesBuilder.cs | 28 +++++- .../AnalysisEntityMapAbstractDomain.cs | 95 +++++++++++++------ 9 files changed, 110 insertions(+), 38 deletions(-) diff --git a/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt b/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt index be954671c4..1d909a8bbd 100644 --- a/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt +++ b/src/Microsoft.CodeAnalysis.FlowAnalysis.Utilities/PublicAPI.Unshipped.txt @@ -660,6 +660,7 @@ virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOpera virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOperationVisitor.GetDefaultValueForParameterOnExit(Microsoft.CodeAnalysis.ITypeSymbol parameterType) -> TAbstractAnalysisValue virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityDataFlowOperationVisitor.SetAbstractValueForAssignment(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity targetAnalysisEntity, Microsoft.CodeAnalysis.IOperation assignedValueOperationOpt, TAbstractAnalysisValue assignedValue) -> void virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain.AssertValidAnalysisData(Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DictionaryAnalysisData map) -> void +virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntityMapAbstractDomain.OnNewMergedValue(TValue value) -> void virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor.ApplyInterproceduralAnalysisResult(TAnalysisData resultData, bool isLambdaOrLocalFunction, bool hasDelegateTypeArgument, TAnalysisResult analysisResult) -> void virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor.ApplyPredicatedDataForEntity(TAnalysisData analysisData, Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.AnalysisEntity predicatedEntity, bool trueData) -> Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PredicateValueKind virtual Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.DataFlowOperationVisitor.AssertValidAnalysisData(TAnalysisData analysisData) -> void diff --git a/src/Utilities/Compiler/PooledObjects/PooledHashSet.cs b/src/Utilities/Compiler/PooledObjects/PooledHashSet.cs index b3536766c8..c139c64349 100644 --- a/src/Utilities/Compiler/PooledObjects/PooledHashSet.cs +++ b/src/Utilities/Compiler/PooledObjects/PooledHashSet.cs @@ -49,6 +49,9 @@ public ImmutableHashSet ToImmutableAndFree() return result; } + public ImmutableHashSet ToImmutable() + => Count == 0 ? ImmutableHashSet.Empty : this.ToImmutableHashSet(Comparer); + // global pool private static readonly ObjectPool> s_poolInstance = CreatePool(); private static readonly ConcurrentDictionary, ObjectPool>> s_poolInstancesByComparer diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs index 1fac60180d..63cebd1e73 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs @@ -39,7 +39,7 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti } value = PointsToAbstractValue.Create(AbstractLocation.CreateAnalysisEntityDefaultLocation(analysisEntity), mayBeNull: true); - _trackedEntitiesBuilder.AddEntity(analysisEntity); + _trackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value); _defaultPointsToValueMapBuilder.Add(analysisEntity, value); } @@ -47,6 +47,8 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti } 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; } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs index 8612cbd984..bbc142dd81 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.CorePointsToAnalysisDataDomain.cs @@ -18,7 +18,7 @@ public partial class PointsToAnalysis : ForwardDataFlowAnalysis { public CorePointsToAnalysisDataDomain(DefaultPointsToValueGenerator defaultPointsToValueGenerator, AbstractValueDomain valueDomain) - : base(valueDomain, defaultPointsToValueGenerator.IsTrackedEntity) + : base(valueDomain, defaultPointsToValueGenerator.IsTrackedEntity, defaultPointsToValueGenerator.IsTrackedPointsToValue) { DefaultPointsToValueGenerator = defaultPointsToValueGenerator; } @@ -31,6 +31,9 @@ protected override bool CanSkipNewEntry(AnalysisEntity analysisEntity, PointsToA => value.Kind == PointsToAbstractValueKind.Unknown || value == GetDefaultValue(analysisEntity); + protected override void OnNewMergedValue(PointsToAbstractValue value) + => DefaultPointsToValueGenerator.AddTrackedPointsToValue(value); + protected override void AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, PointsToAbstractValue value) { PointsToAnalysisData.AssertValidPointsToAnalysisKeyValuePair(analysisEntity, value); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs index 508b23584d..2aee648675 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.PointsToDataFlowOperationVisitor.cs @@ -175,7 +175,7 @@ protected override void SetAbstractValue(AnalysisEntity analysisEntity, PointsTo } SetAbstractValueCore(CurrentAnalysisData, analysisEntity, value); - TrackedEntitiesBuilder.AddEntity(analysisEntity); + TrackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs index d8f21bb09a..22f06f8320 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs @@ -99,7 +99,7 @@ protected override PointsToAnalysisResult ToResult(PointsToAnalysisContext analy operationVisitor.GetEscapedLocationsThroughOperationsMap(), operationVisitor.GetEscapedLocationsThroughReturnValuesMap(), operationVisitor.GetEscapedLocationsThroughEntitiesMap(), - operationVisitor.TrackedEntitiesBuilder.ToImmutable()); + operationVisitor.TrackedEntitiesBuilder); } protected override PointsToBlockAnalysisResult ToBlockResult(BasicBlock basicBlock, PointsToAnalysisData blockAnalysisData) => new PointsToBlockAnalysisResult(basicBlock, blockAnalysisData); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs index 4488b0054f..58022caffb 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysisResult.cs @@ -13,19 +13,20 @@ public sealed class PointsToAnalysisResult : DataFlowAnalysisResult> _escapedLocationsThroughReturnValuesMap; private readonly ImmutableDictionary> _escapedLocationsThroughEntitiesMap; private readonly ImmutableHashSet _trackedEntities; + private readonly ImmutableHashSet _trackedPointsToValues; internal PointsToAnalysisResult( DataFlowAnalysisResult corePointsToAnalysisResult, ImmutableDictionary> escapedLocationsThroughOperationsMap, ImmutableDictionary> escapedLocationsThroughReturnValuesMap, ImmutableDictionary> escapedLocationsThroughEntitiesMap, - ImmutableHashSet trackedEntities) + TrackedEntitiesBuilder trackedEntitiesAndPointsToValuesBuilder) : base(corePointsToAnalysisResult) { _escapedLocationsThroughOperationsMap = escapedLocationsThroughOperationsMap; _escapedLocationsThroughReturnValuesMap = escapedLocationsThroughReturnValuesMap; _escapedLocationsThroughEntitiesMap = escapedLocationsThroughEntitiesMap; - _trackedEntities = trackedEntities; + (_trackedEntities, _trackedPointsToValues) = trackedEntitiesAndPointsToValuesBuilder.ToImmutable(); } public ImmutableHashSet GetEscapedAbstractLocations(IOperation operation) @@ -50,5 +51,8 @@ public ImmutableHashSet GetEscapedAbstractLocations(AnalysisEn internal bool IsTrackedEntity(AnalysisEntity analysisEntity) => _trackedEntities.Contains(analysisEntity); + + internal bool IsTrackedPointsToValue(PointsToAbstractValue value) + => _trackedPointsToValues.Contains(value); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs index a51150f9bf..a175d7d3bc 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs @@ -8,7 +8,7 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow.PointsToAnalysis { /// - /// Stores all the entities that got tracked during points to analysis + /// Stores all the s and s that got tracked during points to analysis /// internal sealed class TrackedEntitiesBuilder : IDisposable { @@ -18,23 +18,41 @@ internal sealed class TrackedEntitiesBuilder : IDisposable /// private PooledHashSet _allEntities { get; } + /// + /// Stores all the tracked that some entity from + /// points to during points to analysis. + /// NOTE: Values added to this set should not be removed. + /// + private PooledHashSet _pointsToValues { get; } + public TrackedEntitiesBuilder() { _allEntities = PooledHashSet.GetInstance(); + _pointsToValues = PooledHashSet.GetInstance(); } public void Dispose() { _allEntities.Free(); + _pointsToValues.Free(); } - public void AddEntity(AnalysisEntity analysisEntity) - => _allEntities.Add(analysisEntity); + public void AddEntityAndPointsToValue(AnalysisEntity analysisEntity, PointsToAbstractValue value) + { + _allEntities.Add(analysisEntity); + AddTrackedPointsToValue(value); + } + + public void AddTrackedPointsToValue(PointsToAbstractValue value) + => _pointsToValues.Add(value); public IEnumerable EnumerateEntities() => _allEntities; - public ImmutableHashSet ToImmutable() - => _allEntities.ToImmutableHashSet(); + public bool IsTrackedPointsToValue(PointsToAbstractValue value) + => _pointsToValues.Contains(value); + + public (ImmutableHashSet, ImmutableHashSet) ToImmutable() + => (_allEntities.ToImmutable(), _pointsToValues.ToImmutable()); } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs index be364662c3..9d27b2928f 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs @@ -14,21 +14,50 @@ namespace Microsoft.CodeAnalysis.FlowAnalysis.DataFlow public abstract class AnalysisEntityMapAbstractDomain : MapAbstractDomain { private static readonly Func s_defaultIsTrackedEntity = new Func(_ => true); + private static readonly Func s_defaultIsTrackedPointsToValue = new Func(_ => true); + private readonly Func _isTrackedEntity; + private readonly Func _isTrackedPointsToValue; - private protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomain, Func isTrackedEntity) + private protected AnalysisEntityMapAbstractDomain( + AbstractValueDomain valueDomain, + Func isTrackedEntity, + Func isTrackedPointsToValue) : base(valueDomain) { _isTrackedEntity = isTrackedEntity ?? throw new ArgumentNullException(nameof(isTrackedEntity)); + _isTrackedPointsToValue = isTrackedPointsToValue ?? throw new ArgumentNullException(nameof(isTrackedPointsToValue)); } protected AnalysisEntityMapAbstractDomain(AbstractValueDomain valueDomain, PointsToAnalysisResult pointsToAnalysisResultOpt) - : this(valueDomain, pointsToAnalysisResultOpt != null ? pointsToAnalysisResultOpt.IsTrackedEntity : s_defaultIsTrackedEntity) + : this(valueDomain, + pointsToAnalysisResultOpt != null ? pointsToAnalysisResultOpt.IsTrackedEntity : s_defaultIsTrackedEntity, + pointsToAnalysisResultOpt != null ? pointsToAnalysisResultOpt.IsTrackedPointsToValue : s_defaultIsTrackedPointsToValue) { } protected abstract TValue GetDefaultValue(AnalysisEntity analysisEntity); protected abstract bool CanSkipNewEntry(AnalysisEntity analysisEntity, TValue value); + protected virtual void OnNewMergedValue(TValue value) + { + } + + private bool CanSkipNewEntity(AnalysisEntity analysisEntity) + { + if (_isTrackedEntity(analysisEntity) || + _isTrackedPointsToValue(analysisEntity.InstanceLocation)) + { + return false; + } + + if (analysisEntity.ParentOpt != null && + !CanSkipNewEntity(analysisEntity.ParentOpt)) + { + return false; + } + + return true; + } protected abstract void AssertValidEntryForMergedMap(AnalysisEntity analysisEntity, TValue value); protected virtual void AssertValidAnalysisData(DictionaryAnalysisData map) @@ -48,17 +77,6 @@ protected virtual void AssertValidAnalysisData(DictionaryAnalysisData(); using var newKeys = PooledHashSet.GetInstance(); using var valuesToMergeBuilder = ArrayBuilder.GetInstance(5); @@ -78,8 +96,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) TValue mergedValue = GetMergedValueForEntityPresentInOneMap(key1, value1); Debug.Assert(!map2.ContainsKey(key1)); Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); - AssertValidEntryForMergedMap(key1, mergedValue); - resultMap.Add(key1, mergedValue); + AddNewEntryToResultMap(key1, mergedValue); continue; } @@ -101,8 +118,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) if (key1.InstanceLocation.Equals(key2.InstanceLocation)) { var mergedValue = GetMergedValue(valuesToMergeBuilder); - AssertValidEntryForMergedMap(key1, mergedValue); - resultMap[key1] = mergedValue; + AddNewEntryToResultMap(key1, mergedValue); } else { @@ -135,9 +151,9 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) } var isCandidateToBeSkipped = !isExistingKeyInInput && !isExistingKeyInResult; - if (isCandidateToBeSkipped && !_isTrackedEntity(mergedKey)) + if (isCandidateToBeSkipped && CanSkipNewEntity(mergedKey)) { - // PERF: Do not add a new key-value pair to the resultMap if the key is not a valid tracked entity. + // PERF: Do not add a new key-value pair to the resultMap if the key is not reachable from tracked entities and PointsTo values. continue; } @@ -157,8 +173,8 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) newKeys.Add(mergedKey); } - AssertValidEntryForMergedMap(mergedKey, mergedValue); - resultMap[mergedKey] = mergedValue; + + AddNewEntryToResultMap(mergedKey, mergedValue, isNewKey: !isExistingKeyInInput); } } } @@ -167,7 +183,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) TValue mergedValue = ValueDomain.Merge(value1, value2); Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); Debug.Assert(ValueDomain.Compare(value2, mergedValue) <= 0); - resultMap.Add(key1, mergedValue); + AddNewEntryToResultMap(key1, mergedValue); continue; } @@ -175,8 +191,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) { TValue mergedValue = GetMergedValueForEntityPresentInOneMap(key1, value1); Debug.Assert(ValueDomain.Compare(value1, mergedValue) <= 0); - AssertValidEntryForMergedMap(key1, mergedValue); - resultMap.Add(key1, mergedValue); + AddNewEntryToResultMap(key1, mergedValue); } } @@ -188,8 +203,7 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) { TValue mergedValue = GetMergedValueForEntityPresentInOneMap(key2, value2); Debug.Assert(ValueDomain.Compare(value2, mergedValue) <= 0); - AssertValidEntryForMergedMap(key2, mergedValue); - resultMap.Add(key2, mergedValue); + AddNewEntryToResultMap(key2, mergedValue); } } @@ -197,10 +211,15 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) { Debug.Assert(!map1.ContainsKey(newKey)); Debug.Assert(!map2.ContainsKey(newKey)); - if (ReferenceEquals(resultMap[newKey], GetDefaultValue(newKey))) + var value = resultMap[newKey]; + if (ReferenceEquals(value, GetDefaultValue(newKey))) { resultMap.Remove(newKey); } + else + { + OnNewMergedValue(value); + } } Debug.Assert(Compare(map1, resultMap) <= 0); @@ -211,6 +230,17 @@ TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) static bool IsAnalysisEntityForFieldOrProperty(AnalysisEntity entity) => entity.SymbolOpt?.Kind == SymbolKind.Field || entity.SymbolOpt?.Kind == SymbolKind.Property; + TValue GetMergedValueForEntityPresentInOneMap(AnalysisEntity key, TValue value) + { + if (key.HasConstantValue) + { + return value; + } + + var defaultValue = GetDefaultValue(key); + return ValueDomain.Merge(value, defaultValue); + } + TValue GetMergedValue(ArrayBuilder values) { Debug.Assert(values.Count > 0); @@ -230,6 +260,17 @@ TValue GetMergedValueCore(TValue value1, TValue value2) return mergedValue; } } + + void AddNewEntryToResultMap(AnalysisEntity key, TValue value, bool isNewKey = false) + { + Debug.Assert(isNewKey == (!map1.ContainsKey(key) && !map2.ContainsKey(key))); + AssertValidEntryForMergedMap(key, value); + resultMap.Add(key, value); + if (!isNewKey) + { + OnNewMergedValue(value); + } + } } } } \ No newline at end of file From e6b0441fa87e5a7534f23adf7e9e2d941501ee8d Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Fri, 22 Nov 2019 08:14:40 -0800 Subject: [PATCH 6/6] Use update instead of Add invocation to handle existing keys --- .../Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs index 9d27b2928f..8ed2a0cc65 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityMapAbstractDomain.cs @@ -265,7 +265,7 @@ void AddNewEntryToResultMap(AnalysisEntity key, TValue value, bool isNewKey = fa { Debug.Assert(isNewKey == (!map1.ContainsKey(key) && !map2.ContainsKey(key))); AssertValidEntryForMergedMap(key, value); - resultMap.Add(key, value); + resultMap[key] = value; if (!isNewKey) { OnNewMergedValue(value);