Skip to content

Commit

Permalink
Track nullable state in try statements
Browse files Browse the repository at this point in the history 
Fixes dotnet#30561
  • Loading branch information
@gafter
gafter committed Nov 9, 2018
1 parent 4b724b6 commit c82b425
Show file tree
Hide file tree
Showing 5 changed files with 284 additions and 93 deletions.
96 changes: 93 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,20 @@ protected AbstractFlowPass(

protected abstract void UnionWith(ref TLocalState self, ref TLocalState other);

/// <summary>
/// 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.
/// </summary>
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
Expand All @@ -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);
}

Expand All @@ -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
Expand All @@ -105,6 +119,80 @@ public override BoundNode VisitTryStatement(BoundTryStatement node)
return null;
}

protected Optional<TLocalState> _tryState;

private void VisitTryBlock0(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState)
{
if (trackUnassignments)
{
Optional<TLocalState> 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<TLocalState> 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<TLocalState> 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);
Expand All @@ -130,5 +218,7 @@ protected virtual void VisitFinallyBlock(BoundStatement finallyBlock, ref TLocal
{
VisitStatement(finallyBlock); // this should generate no pending branches
}

#endregion TryStatements
}
}
83 changes: 1 addition & 82 deletions src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@

namespace Microsoft.CodeAnalysis.CSharp
{
#if REFERENCE_STATE
using OptionalState = Optional<DataFlowPass.LocalState>;
#else
using OptionalState = Nullable<DataFlowPass.LocalState>;
#endif

/// <summary>
/// Implement C# data flow analysis (definite assignment).
/// </summary>
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down
14 changes: 11 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ private NullableWalker(
ArrayBuilder<(RefKind, TypeSymbolWithAnnotations)> returnTypes,
VariableState initialState,
Action<BoundExpression, TypeSymbolWithAnnotations> 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;
Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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()
Expand Down
5 changes: 0 additions & 5 deletions src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading

0 comments on commit c82b425

Please sign in to comment.