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

Factor nullability logic for placeholders #58036

Merged
merged 5 commits into from
Dec 4, 2021
+68 −20
Merged

Factor nullability logic for placeholders #58036

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4596a2
Factor nullability logic for placeholders
jcouv Nov 30, 2021
a7cf49f
Use assertion only
jcouv Dec 1, 2021
8c2c74f
Also in null map case
jcouv Dec 1, 2021
aa96ae6
static
jcouv Dec 2, 2021
f50b6e3
Use assertion method
jcouv Dec 3, 2021
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
103 changes: 82 additions & 21 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 @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

private

static

{
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))
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

{
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))
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

PlaceholderMustBeRegistered

I am not sure if it is worth crashing compiler if the requirement is violated and duplicating the logic to achieve that. It feels like a simple assert within the original else branch would be sufficient. #Closed

{
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