From c82b4255dae7bdcb833728786ad133ec1caa8f11 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 9 Nov 2018 14:56:06 -0800 Subject: [PATCH] Track nullable state in try statements Fixes #30561 --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 96 +++++++++- .../Portable/FlowAnalysis/DataFlowPass.cs | 83 +------- .../Portable/FlowAnalysis/NullableWalker.cs | 14 +- .../FlowAnalysis/PreciseAbstractFlowPass.cs | 5 - .../Semantics/NullableReferenceTypesTests.cs | 179 ++++++++++++++++++ 5 files changed, 284 insertions(+), 93 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index ca1d5883039a1..af1fa09790b77 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -44,6 +44,20 @@ protected AbstractFlowPass( protected abstract void UnionWith(ref TLocalState self, ref TLocalState other); + /// + /// Nontrivial implementation is required for DataFlowsOutWalker or any flow analysis pass that "tracks + /// unassignments" like the nullable walker. The result should be a state, for each variable, that is + /// the strongest result possible (i.e. definitely assigned for the data flow passes, or not null for + /// the nullable analysis). Slightly more formally, this should be a reachable state that won't affect + /// another reachable state when this is intersected with the other state. + /// + protected virtual TLocalState AllBitsSet() + { + return default(TLocalState); + } + + #region TryStatements + public override BoundNode VisitTryStatement(BoundTryStatement node) { var oldPending = SavePending(); // we do not allow branches into a try statement @@ -52,13 +66,13 @@ public override BoundNode VisitTryStatement(BoundTryStatement node) // use this state to resolve all the branches introduced and internal to try/catch var pendingBeforeTry = SavePending(); - VisitTryBlock(node.TryBlock, node, ref initialState); + VisitTryBlock0(node.TryBlock, node, ref initialState); var finallyState = initialState.Clone(); var endState = this.State; foreach (var catchBlock in node.CatchBlocks) { SetState(initialState.Clone()); - VisitCatchBlock(catchBlock, ref finallyState); + VisitCatchBlock0(catchBlock, ref finallyState); IntersectWith(ref endState, ref this.State); } @@ -84,7 +98,7 @@ public override BoundNode VisitTryStatement(BoundTryStatement node) // we will need pending branches as they were before finally later var tryAndCatchPending = SavePending(); var unsetInFinally = AllBitsSet(); - VisitFinallyBlock(node.FinallyBlockOpt, ref unsetInFinally); + VisitFinallyBlock0(node.FinallyBlockOpt, ref unsetInFinally); foreach (var pend in tryAndCatchPending.PendingBranches) { if (pend.Branch == null) continue; // a tracked exception @@ -105,6 +119,80 @@ public override BoundNode VisitTryStatement(BoundTryStatement node) return null; } + protected Optional _tryState; + + private void VisitTryBlock0(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState) + { + if (trackUnassignments) + { + Optional oldTryState = _tryState; + _tryState = AllBitsSet(); + VisitTryBlock(tryBlock, node, ref tryState); + var tts = _tryState.Value; + IntersectWith(ref tryState, ref tts); + if (oldTryState.HasValue) + { + var ots = oldTryState.Value; + IntersectWith(ref ots, ref tts); + oldTryState = ots; + } + + _tryState = oldTryState; + } + else + { + VisitTryBlock(tryBlock, node, ref tryState); + } + } + + private void VisitCatchBlock0(BoundCatchBlock catchBlock, ref TLocalState finallyState) + { + if (trackUnassignments) + { + Optional oldTryState = _tryState; + _tryState = AllBitsSet(); + VisitCatchBlock(catchBlock, ref finallyState); + var tts = _tryState.Value; + IntersectWith(ref finallyState, ref tts); + if (oldTryState.HasValue) + { + var ots = oldTryState.Value; + IntersectWith(ref ots, ref tts); + oldTryState = ots; + } + + _tryState = oldTryState; + } + else + { + VisitCatchBlock(catchBlock, ref finallyState); + } + } + + private void VisitFinallyBlock0(BoundStatement finallyBlock, ref TLocalState unsetInFinally) + { + if (trackUnassignments) + { + Optional oldTryState = _tryState; + _tryState = AllBitsSet(); + VisitFinallyBlock(finallyBlock, ref unsetInFinally); + var tts = _tryState.Value; + IntersectWith(ref unsetInFinally, ref tts); + if (oldTryState.HasValue) + { + var ots = oldTryState.Value; + IntersectWith(ref ots, ref tts); + oldTryState = ots; + } + + _tryState = oldTryState; + } + else + { + VisitFinallyBlock(finallyBlock, ref unsetInFinally); + } + } + protected virtual void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState) { VisitStatement(tryBlock); @@ -130,5 +218,7 @@ protected virtual void VisitFinallyBlock(BoundStatement finallyBlock, ref TLocal { VisitStatement(finallyBlock); // this should generate no pending branches } + + #endregion TryStatements } } diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs index bca81cb3ac975..411e0eaa11690 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs @@ -28,12 +28,6 @@ namespace Microsoft.CodeAnalysis.CSharp { -#if REFERENCE_STATE - using OptionalState = Optional; -#else - using OptionalState = Nullable; -#endif - /// /// Implement C# data flow analysis (definite assignment). /// @@ -1332,7 +1326,7 @@ protected override LocalState ReachableState() protected override LocalState AllBitsSet() { var result = new LocalState(BitVector.AllSet(nextVariableSlot)); - result.Assigned[0] = false; + result.Assigned[0] = false; // make the state reachable return result; } @@ -1952,56 +1946,7 @@ public override BoundNode VisitBaseReference(BoundBaseReference node) return null; } -#region TryStatements - private OptionalState _tryState; - - protected override void VisitTryBlock(BoundStatement tryBlock, BoundTryStatement node, ref LocalState tryState) - { - if (trackUnassignments) - { - OptionalState oldTryState = _tryState; - _tryState = AllBitsSet(); - base.VisitTryBlock(tryBlock, node, ref tryState); - var tts = _tryState.Value; - IntersectWith(ref tryState, ref tts); - if (oldTryState.HasValue) - { - var ots = oldTryState.Value; - IntersectWith(ref ots, ref tts); - oldTryState = ots; - } - _tryState = oldTryState; - } - else - { - base.VisitTryBlock(tryBlock, node, ref tryState); - } - } - protected override void VisitCatchBlock(BoundCatchBlock catchBlock, ref LocalState finallyState) - { - if (trackUnassignments) - { - OptionalState oldTryState = _tryState; - _tryState = AllBitsSet(); - VisitCatchBlockInternal(catchBlock, ref finallyState); - var tts = _tryState.Value; - IntersectWith(ref finallyState, ref tts); - if (oldTryState.HasValue) - { - var ots = oldTryState.Value; - IntersectWith(ref ots, ref tts); - oldTryState = ots; - } - _tryState = oldTryState; - } - else - { - VisitCatchBlockInternal(catchBlock, ref finallyState); - } - } - - private void VisitCatchBlockInternal(BoundCatchBlock catchBlock, ref LocalState finallyState) { DeclareVariables(catchBlock.Locals); @@ -2019,32 +1964,6 @@ private void VisitCatchBlockInternal(BoundCatchBlock catchBlock, ref LocalState } } - protected override void VisitFinallyBlock(BoundStatement finallyBlock, ref LocalState unsetInFinally) - { - if (trackUnassignments) - { - OptionalState oldTryState = _tryState; - _tryState = AllBitsSet(); - base.VisitFinallyBlock(finallyBlock, ref unsetInFinally); - var tts = _tryState.Value; - IntersectWith(ref unsetInFinally, ref tts); - if (oldTryState.HasValue) - { - var ots = oldTryState.Value; - IntersectWith(ref ots, ref tts); - oldTryState = ots; - } - - _tryState = oldTryState; - } - else - { - base.VisitFinallyBlock(finallyBlock, ref unsetInFinally); - } - } - -#endregion TryStatements - public override BoundNode VisitFieldAccess(BoundFieldAccess node) { var result = base.VisitFieldAccess(node); diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index b31afaaa67013..c8f500b1bbf10 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -136,7 +136,7 @@ private NullableWalker( ArrayBuilder<(RefKind, TypeSymbolWithAnnotations)> returnTypes, VariableState initialState, Action callbackOpt) - : base(compilation, method, node, new EmptyStructTypeCache(compilation, dev12CompilerCompatibility: false), trackUnassignments: false) + : base(compilation, method, node, new EmptyStructTypeCache(compilation, dev12CompilerCompatibility: false), trackUnassignments: true) { _sourceAssembly = (method is null) ? null : (SourceAssemblySymbol)method.ContainingAssembly; _callbackOpt = callbackOpt; @@ -629,11 +629,18 @@ private void TrackNullableStateForAssignment(BoundExpression value, TypeSymbolWi if (targetSlot >= this.State.Capacity) Normalize(ref this.State); // https://github.com/dotnet/roslyn/issues/29968 Remove isByRefTarget check? - this.State[targetSlot] = isByRefTarget ? + var newState = isByRefTarget ? // Since reference can point to the heap, we cannot assume the value is not null after this assignment, // regardless of what value is being assigned. (targetType.IsNullable == true) ? (bool?)false : null : !valueType.IsNullable; + this.State[targetSlot] = newState; + if (newState == false && _tryState.HasValue) + { + var state = _tryState.Value; + state[targetSlot] = false; + _tryState = state; + } // https://github.com/dotnet/roslyn/issues/29968 Might this clear state that // should be copied in InheritNullableStateOfTrackableType? @@ -826,7 +833,8 @@ protected override LocalState UnreachableState() protected override LocalState AllBitsSet() { - return new LocalState(BitVector.Create(nextVariableSlot), BitVector.Create(nextVariableSlot)); + // Create a reachable state in which all variables are known to be non-null. + return new LocalState(BitVector.AllSet(nextVariableSlot), BitVector.AllSet(nextVariableSlot)); } private void EnterParameters() diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs index bed20ffa04b78..69b31aa6760c6 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs @@ -1572,11 +1572,6 @@ public override BoundNode VisitTryStatement(BoundTryStatement node) return null; } - protected virtual LocalState AllBitsSet() // required for DataFlowsOutWalker - { - return default(LocalState); - } - public sealed override BoundNode VisitReturnStatement(BoundReturnStatement node) { var result = VisitReturnStatementNoAdjust(node); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index d5d2381a23d92..db4ed71eba0be 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -51742,5 +51742,184 @@ static void Main() var comp = CreateCompilation(source2, new[] { ref1.WithEmbedInteropTypes(true), CSharpRef }, options: WithNonNullTypesTrue(TestOptions.ReleaseExe)); CompileAndVerify(comp, expectedOutput: "4"); } + + [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] + public void SetNullableStateInFinally_01() + { + var source = +@"public static class Program +{ + public static void Main() + { + string? s = string.Empty; + try + { + } + finally + { + s = null; + } + + _ = s.Length; // warning + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (14,13): warning CS8602: Possible dereference of a null reference. + // _ = s.Length; // warning + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(14, 13) + ); + } + + [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] + public void SetNullableStateInFinally_02() + { + var source = +@"public static class Program +{ + public static int Main() + { + string? s = string.Empty; + try + { + s = null; + MayThrow(); + s = string.Empty; + } + catch (System.Exception) + { + } + + return s.Length; // warning: possibly null + } + static void MayThrow() + { + throw null; + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (16,16): warning CS8602: Possible dereference of a null reference. + // return s.Length; // warning: possibly null + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(16, 16) + ); + } + + [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] + public void SetNullableStateInFinally_03() + { + var source = +@"public static class Program +{ + public static int Main() + { + string? s = string.Empty; + try + { + s = null; + MayThrow(); + s = string.Empty; + } + catch (System.Exception) + { + return s.Length; // warning: possibly null + } + + return s.Length; + } + static void MayThrow() + { + throw null; + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (14,20): warning CS8602: Possible dereference of a null reference. + // return s.Length; // warning: possibly null + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(14, 20) + ); + } + + [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] + public void SetNullableStateInFinally_04() + { + var source = +@"public static class Program +{ + public static int Main() + { + string? s = string.Empty; + try + { + s = null; + MayThrow(); + s = string.Empty; + } + catch (System.Exception) + { + _ = s.Length; // warning 1 + } + finally + { + _ = s.Length; // warning 2 + } + + return s.Length; // ok (previously dereferenced) + } + static void MayThrow() + { + throw null; + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (14,17): warning CS8602: Possible dereference of a null reference. + // _ = s.Length; // warning 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(14, 17), + // (18,17): warning CS8602: Possible dereference of a null reference. + // _ = s.Length; // warning 2 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(18, 17) + ); + } + + [Fact, WorkItem(30561, "https://github.com/dotnet/roslyn/issues/30561")] + public void SetNullableStateInFinally_05() + { + var source = +@"public static class Program +{ + public static int Main() + { + string? s = string.Empty; + try + { + s = null; + MayThrow(); + s = string.Empty; + } + finally + { + _ = s.Length; // warning + } + + return s.Length; // ok + } + static void MayThrow() + { + throw null; + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (14,17): warning CS8602: Possible dereference of a null reference. + // _ = s.Length; // warning + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(14, 17) + ); + } } }