Skip to content

Commit

Permalink
Factor nullability logic for placeholders (#58036)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcouv
jcouv authored Dec 4, 2021
1 parent 6c4f403 commit 1741824
Showing 1 changed file with 68 additions and 20 deletions.
88 changes: 68 additions & 20 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,48 @@ 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);
}

[Conditional("DEBUG")]
private static void AssertPlaceholderAllowedWithoutRegistration(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;

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

default:
// Newer placeholders are expected to follow placeholder discipline, namely that
// they must be added to the map before they are visited, then removed.
throw ExceptionUtilities.UnexpectedValue(placeholder.Kind);
}
}

protected override ImmutableArray<PendingBranch> Scan(ref bool badRegion)
{
if (_returnTypesOpt != null)
Expand Down Expand Up @@ -4177,12 +4219,14 @@ private void LearnFromNonNullTest(BoundExpression expression, ref LocalState sta
{
if (expression is BoundValuePlaceholderBase placeholder)
{
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
if (_resultForPlaceholdersOpt != null &&
_resultForPlaceholdersOpt.TryGetValue(placeholder, out var value))
{
expression = value.Replacement;
}
else
{
AssertPlaceholderAllowedWithoutRegistration(placeholder);
return;
}
}
Expand Down Expand Up @@ -8710,17 +8754,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
AssertPlaceholderAllowedWithoutRegistration(node);
SetNotNullResult(node);
return null;
}
Expand Down Expand Up @@ -8980,11 +9025,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 +9040,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 +9419,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 +9906,16 @@ private static bool IsNullabilityMismatch(TypeSymbol type1, TypeSymbol type2)

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

public override BoundNode? VisitInterpolatedStringArgumentPlaceholder(BoundInterpolatedStringArgumentPlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
AssertPlaceholderAllowedWithoutRegistration(node);
SetNotNullResult(node);
return null;
}
Expand Down Expand Up @@ -9985,37 +10031,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
AssertPlaceholderAllowedWithoutRegistration(node);
SetNotNullResult(node);
return null;
}

public override BoundNode? VisitObjectOrCollectionValuePlaceholder(BoundObjectOrCollectionValuePlaceholder node)
{
// These placeholders don't yet follow proper placeholder discipline
AssertPlaceholderAllowedWithoutRegistration(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
AssertPlaceholderAllowedWithoutRegistration(node);
VisitPlaceholderWithReplacement(node);
return null;
}

private void VisitPlaceholderWithReplacement(BoundValuePlaceholderBase node)
{
if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(node, out var value))
if (_resultForPlaceholdersOpt != null &&
_resultForPlaceholdersOpt.TryGetValue(node, out var value))
{
var result = value.Result;
SetResult(node, result.RValueType, result.LValueType);
}
else
{
AssertPlaceholderAllowedWithoutRegistration(node);
SetNotNullResult(node);
}
}
Expand Down

0 comments on commit 1741824

Please sign in to comment.