From d4c9c7ee82b10fdb92c65995ffe94cbe4b3e9dfb Mon Sep 17 00:00:00 2001 From: Andy Gocke Date: Wed, 11 Mar 2020 13:28:41 -0700 Subject: [PATCH] Fix DataFlowsOut for certain local function situations (#41802) There are two problems here, the first of which is the most important. First, certain data flow analyses use "normalization" to adjust an existing state variable for more variables declared after that state was created. The default behavior is to extend that state to the "top" state, meaning the starting state for that data flow analysis. This is usually the right extension. However, some analysis requires extending the variables the other way, to the bottom state. This PR adds functionality for that customization. The second problem is that data flow for local functions may include variables that are not captured. This is by filtering the transfer function to only include captured variables. Fixes fixes #41600 --- .../AbstractFlowPass_LocalFunctions.cs | 13 +- .../Portable/FlowAnalysis/ControlFlowPass.cs | 2 +- .../DefiniteAssignment.LocalFunctions.cs | 45 ++++--- .../FlowAnalysis/DefiniteAssignment.cs | 20 ++- .../FlowAnalysis/LocalDataFlowPass.cs | 14 ++- .../Portable/FlowAnalysis/NullableWalker.cs | 8 +- .../FlowAnalysis/RegionAnalysisTests.cs | 119 ++++++++++++++++++ .../Core/Portable/Collections/BitVector.cs | 17 ++- 8 files changed, 206 insertions(+), 32 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass_LocalFunctions.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass_LocalFunctions.cs index b8678c7e3adb7..189dcb318e20f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass_LocalFunctions.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass_LocalFunctions.cs @@ -34,10 +34,10 @@ internal abstract class AbstractLocalFunctionState /// public TLocalState StateFromTop; - public AbstractLocalFunctionState(TLocalState unreachableState) + public AbstractLocalFunctionState(TLocalState stateFromBottom, TLocalState stateFromTop) { - StateFromBottom = unreachableState.Clone(); - StateFromTop = unreachableState.Clone(); + StateFromBottom = stateFromBottom; + StateFromTop = stateFromTop; } public bool Visited = false; @@ -155,7 +155,8 @@ private bool RecordStateChange( TLocalFunctionState currentState, ref TLocalState stateAtReturn) { - bool anyChanged = Join(ref currentState.StateFromTop, ref stateAtReturn); + bool anyChanged = LocalFunctionEnd(savedState, currentState, ref stateAtReturn); + anyChanged |= Join(ref currentState.StateFromTop, ref stateAtReturn); if (NonMonotonicState.HasValue) { @@ -166,10 +167,6 @@ private bool RecordStateChange( Meet(ref value, ref stateAtReturn); anyChanged |= Join(ref currentState.StateFromBottom, ref value); } - - // N.B. Do NOT shortcut this operation. LocalFunctionEnd may have important - // side effects to the local function state - anyChanged |= LocalFunctionEnd(savedState, currentState, ref stateAtReturn); return anyChanged; } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/ControlFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/ControlFlowPass.cs index c820425a44ebb..67bef799bcffa 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/ControlFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/ControlFlowPass.cs @@ -67,7 +67,7 @@ public bool Reachable internal sealed class LocalFunctionState : AbstractLocalFunctionState { public LocalFunctionState(LocalState unreachableState) - : base(unreachableState) + : base(unreachableState.Clone(), unreachableState.Clone()) { } } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.LocalFunctions.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.LocalFunctions.cs index 758aa29a66d0e..71c6a113f825b 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.LocalFunctions.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.LocalFunctions.cs @@ -12,14 +12,20 @@ internal partial class DefiniteAssignmentPass internal sealed class LocalFunctionState : AbstractLocalFunctionState { public BitVector ReadVars = BitVector.Empty; - public ref LocalState WrittenVars => ref StateFromTop; - public LocalFunctionState(LocalState unreachableState) - : base(unreachableState) + public BitVector CapturedMask = BitVector.Null; + public BitVector InvertedCapturedMask = BitVector.Null; + + public LocalFunctionState(LocalState stateFromBottom, LocalState stateFromTop) + : base(stateFromBottom, stateFromTop) { } } - protected override LocalFunctionState CreateLocalFunctionState() => new LocalFunctionState(UnreachableState()); + protected override LocalFunctionState CreateLocalFunctionState() + => new LocalFunctionState( + // The bottom state should assume all variables, even new ones, are assigned + new LocalState(BitVector.AllSet(nextVariableSlot), normalizeToBottom: true), + UnreachableState()); protected override void VisitLocalFunctionUse( LocalFunctionSymbol localFunc, @@ -112,15 +118,12 @@ private void RecordReadInLocalFunction(int slot) } } - private BitVector GetCapturedBitmask(ref BitVector state) + private BitVector GetCapturedBitmask() { - BitVector mask = BitVector.Empty; - for (int slot = 1; slot < state.Capacity; slot++) + BitVector mask = BitVector.AllSet(nextVariableSlot); + for (int slot = 1; slot < nextVariableSlot; slot++) { - if (IsCapturedInLocalFunction(slot)) - { - mask[slot] = true; - } + mask[slot] = IsCapturedInLocalFunction(slot); } return mask; @@ -166,7 +169,7 @@ protected override LocalFunctionState LocalFunctionStart(LocalFunctionState star // assignment errors if any of the captured variables is not assigned // on a particular branch. - var savedState = new LocalFunctionState(UnreachableState()); + var savedState = CreateLocalFunctionState(); savedState.ReadVars = startState.ReadVars.Clone(); startState.ReadVars.Clear(); return savedState; @@ -181,10 +184,24 @@ protected override bool LocalFunctionEnd( LocalFunctionState currentState, ref LocalState stateAtReturn) { + if (currentState.CapturedMask.IsNull) + { + currentState.CapturedMask = GetCapturedBitmask(); + currentState.InvertedCapturedMask = currentState.CapturedMask.Clone(); + currentState.InvertedCapturedMask.Invert(); + } + // Filter the modified state variables to only captured variables + stateAtReturn.Assigned.IntersectWith(currentState.CapturedMask); + if (NonMonotonicState.HasValue) + { + var state = NonMonotonicState.Value; + state.Assigned.UnionWith(currentState.InvertedCapturedMask); + NonMonotonicState = state; + } + // Build a list of variables that are both captured and read before assignment - var capturedMask = GetCapturedBitmask(ref currentState.ReadVars); var capturedAndRead = currentState.ReadVars; - capturedAndRead.IntersectWith(capturedMask); + capturedAndRead.IntersectWith(currentState.CapturedMask); // Union and check to see if there are any changes return savedState.ReadVars.UnionWith(capturedAndRead); diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 6bd5e61bbb8b1..67e1bbf5b3106 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -741,9 +741,18 @@ protected override void Normalize(ref LocalState state) { var id = variableBySlot[i]; int slot = id.ContainingSlot; - state.Assigned[i] = (slot > 0) && + + bool assign = (slot > 0) && state.Assigned[slot] && variableBySlot[slot].Symbol.GetTypeOrReturnType().TypeKind == TypeKind.Struct; + + if (state.NormalizeToBottom && slot == 0) + { + // NormalizeToBottom means new variables are assumed to be assigned (bottom state) + assign = true; + } + + state.Assigned[i] = assign; } } @@ -2165,16 +2174,19 @@ protected override bool Join(ref LocalState self, ref LocalState other) } #if REFERENCE_STATE - internal class LocalState : ILocalState + internal class LocalState : ILocalDataFlowState #else - internal struct LocalState : ILocalState + internal struct LocalState : ILocalDataFlowState #endif { internal BitVector Assigned; - internal LocalState(BitVector assigned) + public bool NormalizeToBottom { get; } + + internal LocalState(BitVector assigned, bool normalizeToBottom = false) { this.Assigned = assigned; + NormalizeToBottom = normalizeToBottom; Debug.Assert(!assigned.IsNull); } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs index b9b9ef22e5211..cc88a9c5a2694 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/LocalDataFlowPass.cs @@ -14,9 +14,18 @@ namespace Microsoft.CodeAnalysis.CSharp /// Does a data flow analysis for state attached to local variables and fields of struct locals. /// internal abstract partial class LocalDataFlowPass : AbstractFlowPass - where TLocalState : AbstractFlowPass.ILocalState + where TLocalState : LocalDataFlowPass.ILocalDataFlowState where TLocalFunctionState : AbstractFlowPass.AbstractLocalFunctionState { + internal interface ILocalDataFlowState : ILocalState + { + /// + /// True if new variables introduced in should be set + /// to the bottom state. False if they should be set to the top state. + /// + bool NormalizeToBottom { get; } + } + /// /// A mapping from local variables to the index of their slot in a flow analysis local state. /// @@ -172,6 +181,9 @@ private int GetSlotDepth(int slot) return depth; } + /// + /// Sets the starting state for any newly declared variables in the LocalDataFlowPass. + /// protected abstract void Normalize(ref TLocalState state); /// diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 4c8d65e9193aa..5b9e285bb975f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -8516,9 +8516,9 @@ protected override bool Join(ref LocalState self, ref LocalState other) [DebuggerDisplay("{GetDebuggerDisplay(), nq}")] #if REFERENCE_STATE - internal class LocalState : ILocalState + internal class LocalState : ILocalDataFlowState #else - internal struct LocalState : ILocalState + internal struct LocalState : ILocalDataFlowState #endif { // The representation of a state is a bit vector with two bits per slot: @@ -8530,6 +8530,8 @@ internal struct LocalState : ILocalState public bool Reachable => _state[0]; + public bool NormalizeToBottom => false; + public static LocalState ReachableState(int capacity) { if (capacity < 1) @@ -8612,7 +8614,7 @@ internal sealed class LocalFunctionState : AbstractLocalFunctionState /// public LocalState StartingState; public LocalFunctionState(LocalState unreachableState) - : base(unreachableState) + : base(unreachableState.Clone(), unreachableState.Clone()) { StartingState = unreachableState; } diff --git a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs index d1418dfc28109..bada442325b3d 100644 --- a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs @@ -4131,6 +4131,125 @@ static void Main(string[] args) #region "lambda" + [Fact] + [WorkItem(41600, "https://github.com/dotnet/roslyn/pull/41600")] + public void DataFlowAnalysisLocalFunctions10() + { + var dataFlow = CompileAndAnalyzeDataFlowExpression(@" +class C +{ + public void M() + { + bool Dummy(params object[] x) {return true;} + + try {} + catch when (/**/TakeOutParam(out var x1)/**/ && x1 > 0) + { + Dummy(x1); + } + + var x4 = 11; + Dummy(x4); + + try {} + catch when (TakeOutParam(out var x4) && x4 > 0) + { + Dummy(x4); + } + + try {} + catch when (x6 && TakeOutParam(out var x6)) + { + Dummy(x6); + } + + try {} + catch when (TakeOutParam(out var x7) && x7 > 0) + { + var x7 = 12; + Dummy(x7); + } + + try {} + catch when (TakeOutParam(out var x8) && x8 > 0) + { + Dummy(x8); + } + + System.Console.WriteLine(x8); + + try {} + catch when (TakeOutParam(out var x9) && x9 > 0) + { + Dummy(x9); + try {} + catch when (TakeOutParam(out var x9) && x9 > 0) // 2 + { + Dummy(x9); + } + } + + try {} + catch when (TakeOutParam(y10, out var x10)) + { + var y10 = 12; + Dummy(y10); + } + + // try {} + // catch when (TakeOutParam(y11, out var x11) + // { + // let y11 = 12; + // Dummy(y11); + // } + + try {} + catch when (Dummy(TakeOutParam(out var x14), + TakeOutParam(out var x14), // 2 + x14)) + { + Dummy(x14); + } + + try {} + catch (System.Exception x15) + when (Dummy(TakeOutParam(out var x15), x15)) + { + Dummy(x15); + } + + static bool TakeOutParam(out int x) + { + x = 123; + return true; + } + static bool TakeOutParam(object y, out int x) + { + x = 123; + return true; + } + + } +} +"); + Assert.True(dataFlow.Succeeded); + Assert.Null(GetSymbolNamesJoined(dataFlow.Captured)); + Assert.Null(GetSymbolNamesJoined(dataFlow.CapturedInside)); + Assert.Null(GetSymbolNamesJoined(dataFlow.CapturedOutside)); + Assert.Equal("x1", GetSymbolNamesJoined(dataFlow.VariablesDeclared)); + Assert.Null(GetSymbolNamesJoined(dataFlow.DataFlowsIn)); + Assert.Equal("x1", GetSymbolNamesJoined(dataFlow.DataFlowsOut)); + Assert.Equal("this", GetSymbolNamesJoined(dataFlow.DefinitelyAssignedOnEntry)); + Assert.Equal("this, x1", GetSymbolNamesJoined(dataFlow.DefinitelyAssignedOnExit)); + Assert.Null(GetSymbolNamesJoined(dataFlow.ReadInside)); + Assert.Equal("x1", GetSymbolNamesJoined(dataFlow.WrittenInside)); + Assert.Equal("this, x1, x4, x4, x6, x7, x7, x8, x9, x9, y10, x14, x15, x, x", + GetSymbolNamesJoined(dataFlow.ReadOutside)); + Assert.Equal("this, x, x4, x4, x6, x7, x7, x8, x9, x9, x10, " + + "y10, x14, x14, x15, x15, x, y, x", + GetSymbolNamesJoined(dataFlow.WrittenOutside)); + } + [Fact] [WorkItem(39946, "https://github.com/dotnet/roslyn/issues/39946")] public void DataFlowAnalysisLocalFunctions9() diff --git a/src/Compilers/Core/Portable/Collections/BitVector.cs b/src/Compilers/Core/Portable/Collections/BitVector.cs index 867978b51fd4d..98965db3a12c9 100644 --- a/src/Compilers/Core/Portable/Collections/BitVector.cs +++ b/src/Compilers/Core/Portable/Collections/BitVector.cs @@ -22,7 +22,7 @@ internal struct BitVector : IEquatable // Cannot expose the following two field publicly because this structure is mutable // and might become not null/empty, unless we restrict access to it. - private static readonly Word[] s_emptyArray = Array.Empty(); + private static Word[] s_emptyArray => Array.Empty(); private static readonly BitVector s_nullValue = default; private static readonly BitVector s_emptyValue = new BitVector(0, s_emptyArray, 0); @@ -222,6 +222,21 @@ public BitVector Clone() return new BitVector(_bits0, newBits, _capacity); } + /// + /// Invert all the bits in the vector. + /// + public void Invert() + { + _bits0 = ~_bits0; + if (!(_bits is null)) + { + for (int i = 0; i < _bits.Length; i++) + { + _bits[i] = ~_bits[i]; + } + } + } + /// /// Is the given bit array null? ///