Skip to content

Commit

Permalink
Factor nullability logic for placeholders
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcouv
jcouv committed Nov 30, 2021
1 parent 03f6adf commit c4596a2
Showing 1 changed file with 82 additions and 21 deletions.
103 changes: 82 additions & 21 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,47 @@ private void AssertNoPlaceholderReplacements()
}
}

private void AddPlaceholderReplacement(BoundValuePlaceholderBase placeholder, BoundExpression expression, VisitResult result)
{
#if DEBUG
Debug.Assert(AreCloseEnough(placeholder.Type, result.RValueType.Type));
#endif

_resultForPlaceholdersOpt ??= PooledDictionary<BoundValuePlaceholderBase, (BoundExpression Replacement, VisitResult Result)>.GetInstance();
_resultForPlaceholdersOpt.Add(placeholder, (expression, result));
}

private void RemovePlaceholderReplacement(BoundValuePlaceholderBase placeholder)
{
Debug.Assert(_resultForPlaceholdersOpt is { });
bool removed = _resultForPlaceholdersOpt.Remove(placeholder);
Debug.Assert(removed);
}

private bool PlaceholderMustBeRegistered(BoundValuePlaceholderBase placeholder)
{
Debug.Assert(placeholder is { });

switch (placeholder.Kind)
{
case BoundKind.DeconstructValuePlaceholder:
case BoundKind.InterpolatedStringHandlerPlaceholder:
case BoundKind.InterpolatedStringArgumentPlaceholder:
case BoundKind.ObjectOrCollectionValuePlaceholder:
case BoundKind.AwaitableValuePlaceholder:
return false;

case BoundKind.ImplicitIndexerValuePlaceholder:
// Since such placeholders are always not-null, we skip adding them to the map.
return false;

default:
// Newer placeholders are expected to follow placeholder discipline, namely that
// they must be added to the map before they are visited, then removed.
return true;
}
}

protected override ImmutableArray<PendingBranch> Scan(ref bool badRegion)
{
if (_returnTypesOpt != null)
Expand Down Expand Up @@ -4177,13 +4218,22 @@ private void LearnFromNonNullTest(BoundExpression expression, ref LocalState sta
{
if (expression is BoundValuePlaceholderBase placeholder)
{
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
if (PlaceholderMustBeRegistered(placeholder))
{
expression = value.Replacement;
Debug.Assert(_resultForPlaceholdersOpt is not null);
expression = _resultForPlaceholdersOpt[placeholder].Replacement;
}
else
{
return;
// We tolerate older placeholders not being in the map
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
{
expression = value.Replacement;
}
else
{
return;
}
}
}

Expand Down Expand Up @@ -8710,17 +8760,18 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter
var receiverResult = _visitResult;
VisitRvalue(node.Argument);

EnsurePlaceholdersToResultMap();
_resultForPlaceholdersOpt.Add(node.ReceiverPlaceholder, (node.Receiver, receiverResult));
AddPlaceholderReplacement(node.ReceiverPlaceholder, node.Receiver, receiverResult);
VisitRvalue(node.IndexerOrSliceAccess);
_resultForPlaceholdersOpt.Remove(node.ReceiverPlaceholder);
RemovePlaceholderReplacement(node.ReceiverPlaceholder);

SetResult(node, ResultType, LvalueResultType);
return null;
}

public override BoundNode? VisitImplicitIndexerValuePlaceholder(BoundImplicitIndexerValuePlaceholder node)
{
// These placeholders don't need to be replaced because we know they are always not-null
Debug.Assert(!PlaceholderMustBeRegistered(node));
SetNotNullResult(node);
return null;
}
Expand Down Expand Up @@ -8980,11 +9031,10 @@ protected override void VisitForEachExpression(BoundForEachStatement node)
{
var moveNextAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, node.EnumeratorInfoOpt.MoveNextInfo.Method);

EnsurePlaceholdersToResultMap();
var result = new VisitResult(GetReturnTypeWithState(moveNextAsyncMethod), moveNextAsyncMethod.ReturnTypeWithAnnotations);
_resultForPlaceholdersOpt.Add(moveNextPlaceholder, (moveNextPlaceholder, result));
AddPlaceholderReplacement(moveNextPlaceholder, moveNextPlaceholder, result);
Visit(awaitMoveNextInfo);
_resultForPlaceholdersOpt.Remove(moveNextPlaceholder);
RemovePlaceholderReplacement(moveNextPlaceholder);
}

// Analyze `await DisposeAsync()`
Expand All @@ -8996,17 +9046,16 @@ protected override void VisitForEachExpression(BoundForEachStatement node)
{
Debug.Assert(disposalPlaceholder is not null);
var disposeAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, originalDisposeMethod);
EnsurePlaceholdersToResultMap();
var result = new VisitResult(GetReturnTypeWithState(disposeAsyncMethod), disposeAsyncMethod.ReturnTypeWithAnnotations);
_resultForPlaceholdersOpt.Add(disposalPlaceholder, (disposalPlaceholder, result));
AddPlaceholderReplacement(disposalPlaceholder, disposalPlaceholder, result);
addedPlaceholder = true;
}

Visit(awaitDisposalInfo);

if (addedPlaceholder)
{
_resultForPlaceholdersOpt!.Remove(disposalPlaceholder!);
RemovePlaceholderReplacement(disposalPlaceholder!);
}
}
}
Expand Down Expand Up @@ -9376,10 +9425,9 @@ private TypeWithState InferResultNullabilityOfBinaryLogicalOperator(BoundExpress
var placeholder = awaitableInfo.AwaitableInstancePlaceholder;
Debug.Assert(placeholder is object);

EnsurePlaceholdersToResultMap();
_resultForPlaceholdersOpt.Add(placeholder, (node.Expression, _visitResult));
AddPlaceholderReplacement(placeholder, node.Expression, _visitResult);
Visit(awaitableInfo);
_resultForPlaceholdersOpt.Remove(placeholder);
RemovePlaceholderReplacement(placeholder);

if (node.Type.IsValueType || node.HasErrors || awaitableInfo.GetResult is null)
{
Expand Down Expand Up @@ -9864,12 +9912,16 @@ private static bool IsNullabilityMismatch(TypeSymbol type1, TypeSymbol type2)

public override BoundNode? VisitInterpolatedStringHandlerPlaceholder(BoundInterpolatedStringHandlerPlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
Debug.Assert(!PlaceholderMustBeRegistered(node));
SetNotNullResult(node);
return null;
}

public override BoundNode? VisitInterpolatedStringArgumentPlaceholder(BoundInterpolatedStringArgumentPlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
Debug.Assert(!PlaceholderMustBeRegistered(node));
SetNotNullResult(node);
return null;
}
Expand Down Expand Up @@ -9985,30 +10037,39 @@ protected override void VisitCatchBlock(BoundCatchBlock node, ref LocalState fin

public override BoundNode? VisitDeconstructValuePlaceholder(BoundDeconstructValuePlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
Debug.Assert(!PlaceholderMustBeRegistered(node));
SetNotNullResult(node);
return null;
}

public override BoundNode? VisitObjectOrCollectionValuePlaceholder(BoundObjectOrCollectionValuePlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
Debug.Assert(!PlaceholderMustBeRegistered(node));
SetNotNullResult(node);
return null;
}

[MemberNotNull(nameof(_resultForPlaceholdersOpt))]
private void EnsurePlaceholdersToResultMap()
{
_resultForPlaceholdersOpt ??= PooledDictionary<BoundValuePlaceholderBase, (BoundExpression Replacement, VisitResult Result)>.GetInstance();
}

public override BoundNode? VisitAwaitableValuePlaceholder(BoundAwaitableValuePlaceholder node)
{
// These placeholders don't always follow proper placeholder discipline yet
Debug.Assert(!PlaceholderMustBeRegistered(node));
VisitPlaceholderWithReplacement(node);
return null;
}

private void VisitPlaceholderWithReplacement(BoundValuePlaceholderBase node)
{
if (PlaceholderMustBeRegistered(node))
{
Debug.Assert(_resultForPlaceholdersOpt is not null);
var result = _resultForPlaceholdersOpt[node].Result;
SetResult(node, result.RValueType, result.LValueType);
return;
}

// We tolerate older placeholders not being in the map
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(node, out var value))
{
var result = value.Result;
Expand Down

0 comments on commit c4596a2

Please sign in to comment.