Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track nullable state in try statements #31082

Merged
merged 5 commits into from
Nov 13, 2018
Merged

Track nullable state in try statements #31082

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c82b425
Track nullable state in try statements
gafter Nov 9, 2018
8aba889
Merge branch 'dev16.0-preview2' of https://github.com/dotnet/roslyn i…
gafter Nov 12, 2018
118f5be
Small changes per code review.
gafter Nov 12, 2018
477cce1
Added a requested test
gafter Nov 12, 2018
7edff3c
Minor changes per code review
gafter Nov 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 93 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Open in desktop
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);
VisitTryBlockWithUnassignments(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);
VisitCatchBlockWithUnassignments(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);
VisitFinallyBlockWithUnassignments(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 VisitTryBlockWithUnassignments(BoundStatement tryBlock, BoundTryStatement node, ref TLocalState tryState)
{
if (trackUnassignments)
{
Optional<TLocalState> oldTryState = _tryState;
_tryState = AllBitsSet();
VisitTryBlock(tryBlock, node, ref tryState);
var tts = _tryState.Value;
Copy link
Member

@jcouv jcouv Nov 13, 2018 •
edited by gafter
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tts [](start = 20, length = 3)

nit: even though this is moving existing code, consider giving tts and ots proper names. #Resolved

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 VisitCatchBlockWithUnassignments(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 VisitFinallyBlockWithUnassignments(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
Open in desktop
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
16 changes: 13 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
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) ? targetType.NullableAnnotation : NullableAnnotation.Unknown :
valueType.NullableAnnotation;
this.State[targetSlot] = newState;
if (newState.IsAnyNullable() && _tryState.HasValue)
{
var state = _tryState.Value;
state[targetSlot] = NullableAnnotation.NullableBasedOnAnalysis;
_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,10 @@ protected override LocalState UnreachableState()

protected override LocalState AllBitsSet()
{
return new LocalState(reachable: true, new ArrayBuilder<NullableAnnotation>(nextVariableSlot));
// Create a reachable state in which all variables are known to be non-null.
var builder = new ArrayBuilder<NullableAnnotation>(nextVariableSlot);
builder.AddMany(NullableAnnotation.NotNullableBasedOnAnalysis, nextVariableSlot);
return new LocalState(reachable: true, builder);
}

private void EnterParameters()
Expand Down
5 changes: 0 additions & 5 deletions src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs
View file
Open in desktop
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