From bfcbf205db3a9e5032e1da6c8308e7943bef349a Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Tue, 31 Jan 2023 18:54:24 +0530 Subject: [PATCH] Fix flow analysis to stop tracking entities which have unknown instance location Fixes #6453 While performing flow analysis for loops, we clear out the instance location for analysis entities when processing the back edges. This is primarily done because same operation when processed again might refer to a different runtime object/entity for a different loop iteration, and we have no way to known if any members accessed of this object point to the same runtime instance. Even though we were correctly setting the instance location for such entities to Unknown location on back edges, we were still continuining to track the analysis data for such entities. This led to multiple such distinct entities being considered identical and sharing the same flow analysis values, as seen in the bug repro case. The fix here is to avoid tracking such child or instance member entities which have an unknown instance location for flow analysis. --- .../AvoidDeadConditionalCode_NullAnalysis.cs | 33 +++++++++++++++++++ ...eadConditionalCode_ValueContentAnalysis.cs | 32 ++++++++++++++++++ .../DefaultPointsToValueGenerator.cs | 3 +- .../PointsToAnalysis/PointsToAnalysis.cs | 2 +- .../TrackedEntitiesBuilder.cs | 2 +- ...is.ValueContentDataFlowOperationVisitor.cs | 10 ++++-- .../Framework/DataFlow/AnalysisEntity.cs | 21 +++++++++--- .../AnalysisEntityDataFlowOperationVisitor.cs | 3 +- 8 files changed, 93 insertions(+), 13 deletions(-) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_NullAnalysis.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_NullAnalysis.cs index 7b7108fe5f..2607879017 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_NullAnalysis.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_NullAnalysis.cs @@ -7059,5 +7059,38 @@ public void M(bool flag) LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp8, }.RunAsync(); } + + [Trait(Traits.DataflowAnalysis, Traits.Dataflow.PointsToAnalysis)] + [Trait(Traits.DataflowAnalysis, Traits.Dataflow.NullAnalysis)] + [Trait(Traits.DataflowAnalysis, Traits.Dataflow.CopyAnalysis)] + [Fact, WorkItem(6453, "https://github.com/dotnet/roslyn-analyzers/issues/6453")] + public async Task IndexedNullCompare_NoDiagnosticAsync() + { + await VerifyCSharpAnalyzerAsync(@" +using System.Collections.Generic; + +sealed class Data +{ + public object Value { get; } + public Data(object value) + { + Value = value; + } +} + +static class Test +{ + static void Filter(List list, int j) + { + for (int i = 0; i < list.Count - 1; i++) + { + if (list[i + 1].Value != null) continue; + if (list[i].Value == null) continue; // <-------- CA1508 False positive + list.RemoveAt(i); + } + } +} +"); + } } } diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs index 62e785244b..8c6f889b3a 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/Maintainability/AvoidDeadConditionalCode_ValueContentAnalysis.cs @@ -3152,5 +3152,37 @@ public void M(object o) "; await VerifyCS.VerifyCodeFixAsync(source, source); } + + [Trait(Traits.DataflowAnalysis, Traits.Dataflow.ValueContentAnalysis)] + [Trait(Traits.DataflowAnalysis, Traits.Dataflow.CopyAnalysis)] + [Fact, WorkItem(6453, "https://github.com/dotnet/roslyn-analyzers/issues/6453")] + public async Task IndexedValueCompare_NoDiagnosticAsync() + { + await VerifyCSharpAnalyzerAsync(@" +using System.Collections.Generic; + +sealed class Data +{ + public int Value { get; } + public Data(int value) + { + Value = value; + } +} + +static class Test +{ + static void Filter(List list, int j) + { + for (int i = 0; i < list.Count - 1; i++) + { + if (list[i + 1].Value != 0) continue; + if (list[i].Value == 0) continue; // <-------- CA1508 False positive + list.RemoveAt(i); + } + } +} +"); + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs index eba643dced..6ab03de481 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/DefaultPointsToValueGenerator.cs @@ -43,8 +43,7 @@ public PointsToAbstractValue GetOrCreateDefaultValue(AnalysisEntity analysisEnti value = PointsToAbstractValue.Create(AbstractLocation.CreateAnalysisEntityDefaultLocation(analysisEntity), mayBeNull: true); // PERF: Do not track entity and its points to value for partial analysis for entities requiring complete analysis. - if (PointsToAnalysisKind == PointsToAnalysisKind.Complete || - !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis()) + if (analysisEntity.ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind)) { _trackedEntitiesBuilder.AddEntityAndPointsToValue(analysisEntity, value); _defaultPointsToValueMapBuilder.Add(analysisEntity, value); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs index b0eed39046..7741d3cdfc 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/PointsToAnalysis.cs @@ -103,7 +103,7 @@ internal static bool ShouldBeTracked(AnalysisEntity analysisEntity, PointsToAnal return false; } - return pointsToAnalysisKind == PointsToAnalysisKind.Complete || !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis(); + return analysisEntity.ShouldBeTrackedForPointsToAnalysis(pointsToAnalysisKind); } [Conditional("DEBUG")] diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs index b5a4d574df..47e67c194a 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/PointsToAnalysis/TrackedEntitiesBuilder.cs @@ -45,7 +45,7 @@ public void Dispose() public void AddEntityAndPointsToValue(AnalysisEntity analysisEntity, PointsToAbstractValue value) { - Debug.Assert(PointsToAnalysisKind == PointsToAnalysisKind.Complete || !analysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis()); + Debug.Assert(analysisEntity.ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind)); AllEntities.Add(analysisEntity); AddTrackedPointsToValue(value); diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs index 3ac7c2d212..1e92a464fd 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Analysis/ValueContentAnalysis/ValueContentAnalysis.ValueContentDataFlowOperationVisitor.cs @@ -34,7 +34,10 @@ protected override void ResetAbstractValue(AnalysisEntity analysisEntity) protected override void SetAbstractValue(AnalysisEntity analysisEntity, ValueContentAbstractValue value) => SetAbstractValue(CurrentAnalysisData, analysisEntity, value); - private static void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value) + private void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value) + => SetAbstractValue(analysisData, analysisEntity, value, HasCompletePointsToAnalysisResult); + + private static void SetAbstractValue(ValueContentAnalysisData analysisData, AnalysisEntity analysisEntity, ValueContentAbstractValue value, bool hasCompletePointsToAnalysisResult) { // PERF: Avoid creating an entry if the value is the default unknown value. if (value == ValueContentAbstractValue.MayBeContainsNonLiteralState && @@ -43,7 +46,10 @@ private static void SetAbstractValue(ValueContentAnalysisData analysisData, Anal return; } - analysisData.SetAbstractValue(analysisEntity, value); + if (analysisEntity.ShouldBeTrackedForAnalysis(hasCompletePointsToAnalysisResult)) + { + analysisData.SetAbstractValue(analysisEntity, value); + } } protected override bool HasAbstractValue(AnalysisEntity analysisEntity) diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntity.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntity.cs index d00a52fb71..0eb0bec4a4 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntity.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntity.cs @@ -154,17 +154,28 @@ public bool IsChildOrInstanceMember } } - internal bool IsChildOrInstanceMemberNeedingCompletePointsToAnalysis() + internal bool ShouldBeTrackedForPointsToAnalysis(PointsToAnalysisKind pointsToAnalysisKind) + => ShouldBeTrackedForAnalysis(pointsToAnalysisKind == PointsToAnalysisKind.Complete); + + internal bool ShouldBeTrackedForAnalysis(bool hasCompletePointsToAnalysisResult) { if (!IsChildOrInstanceMember) { - return false; + // We can always perform analysis for entities which are not child or instance members. + return true; + } + + // If we have complete points to analysis result, and this child or instance member has a + // known instance location, we can perform analysis for this entity. + if (hasCompletePointsToAnalysisResult && !HasUnknownInstanceLocation) + { + return true; } - // PERF: This is the core performance optimization for partial PointsToAnalysisKind. - // We avoid tracking PointsToValues for all entities that are child or instance members, + // PERF: This is the core performance optimization when we have partial points to analysis result. + // We avoid tracking analysis values for all entities that are child or instance members, // except when they are fields or members of a value type (for example, tuple elements or struct members). - return Parent == null || !Parent.Type.HasValueCopySemantics(); + return Parent != null && Parent.Type.HasValueCopySemantics(); } public bool HasConstantValue => Symbol switch diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs index aa9c66ff99..9b940a8f51 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/AnalysisEntityDataFlowOperationVisitor.cs @@ -217,8 +217,7 @@ protected override void SetAbstractValueForAssignment(IOperation target, IOperat { if (AnalysisEntityFactory.TryCreate(target, out var targetAnalysisEntity)) { - if (!HasCompletePointsToAnalysisResult && - targetAnalysisEntity.IsChildOrInstanceMemberNeedingCompletePointsToAnalysis()) + if (!targetAnalysisEntity.ShouldBeTrackedForAnalysis(HasCompletePointsToAnalysisResult)) { // We are not tracking points to values for fields and properties. // So, it is not possible to accurately track value changes to target entity which is a member.