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

Jump to bottom

Improved nullable '??' analysis #52646

Merged
merged 7 commits into from
May 4, 2021
Merged

Improved nullable '??' analysis #52646

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa4e198
Improved nullable '??' analysis
RikkiGibson Apr 20, 2021
1e6b5c3
Address some feedback
RikkiGibson Apr 26, 2021
df3ba74
Handle non-null constants on left of ?.
RikkiGibson Apr 27, 2021
c5b9624
Address feedback
RikkiGibson Apr 27, 2021
8c6ccc6
Add tests and remove prototype comment
RikkiGibson Apr 27, 2021
68eb9b1
Use VisitRvalue for null constants
RikkiGibson Apr 27, 2021
b20fa3c
Use strict "acceptable conversion" rules in nullable
RikkiGibson May 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
5 changes: 2 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 @@ -255,7 +255,7 @@ protected string DumpLabels()
}
#endif

private void EnterRegionIfNeeded(BoundNode node)
protected void EnterRegionIfNeeded(BoundNode node)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
if (TrackingRegions && node == this.firstInRegion && this.regionPlace == RegionPlace.Before)
{
Expand All @@ -272,7 +272,7 @@ protected virtual void EnterRegion()
this.regionPlace = RegionPlace.Inside;
}

private void LeaveRegionIfNeeded(BoundNode node)
protected void LeaveRegionIfNeeded(BoundNode node)
{
if (TrackingRegions && node == this.lastInRegion && this.regionPlace == RegionPlace.Inside)
{
Expand Down Expand Up @@ -2783,7 +2783,6 @@ private void VisitConditionalAccess(BoundConditionalAccess node, out TLocalState
VisitRvalue(innerCondAccess.Receiver);
expr = innerCondAccess.AccessExpression;

// PROTOTYPE(improved-definite-assignment): Add NullableWalker implementation and tests which exercises this.
// The savedState here represents the scenario where 0 or more of the access expressions could have been evaluated.
// e.g. after visiting `a?.b(x = null)?.c(x = new object())`, the "state when not null" of `x` is NotNull, but the "state when maybe null" of `x` is MaybeNull.
Join(ref savedState, ref State);
Expand Down
220 changes: 192 additions & 28 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 @@ -3992,6 +3992,7 @@ private static void MarkSlotsAsNotNull(ArrayBuilder<int> slots, ref LocalState s
}
}

// PROTOTYPE(improved-definite-assignment): eventually, GetSlotsToMarkAsNotNullable should be removed
private void LearnFromNonNullTest(BoundExpression expression, ref LocalState state)
{
if (expression.Kind == BoundKind.AwaitableValuePlaceholder)
Expand Down Expand Up @@ -4158,20 +4159,23 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
BoundExpression leftOperand = node.LeftOperand;
BoundExpression rightOperand = node.RightOperand;

TypeWithState leftResult = VisitRvalueWithState(leftOperand);
TypeWithState rightResult;

// PROTOTYPE(improved-definite-assignment):
// Definite assignment has another special case for when the LHS is a non-null constant (a literal string for example).
// Should we replicate that here?
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (IsConstantNull(leftOperand))
{
rightResult = VisitRvalueWithState(rightOperand);
VisitRvalue(leftOperand);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
Visit(rightOperand);
var rightUnconditionalResult = ResultType;
// Should be able to use rightResult for the result of the operator but
// binding may have generated a different result type in the case of errors.
SetResultType(node, TypeWithState.Create(node.Type, rightResult.State));
SetResultType(node, TypeWithState.Create(node.Type, rightUnconditionalResult.State));
return null;
}

var whenNotNull = this.State.Clone();
LearnFromNonNullTest(leftOperand, ref whenNotNull);
VisitPossibleConditionalAccess(leftOperand, out var whenNotNull);
TypeWithState leftResult = ResultType;
Unsplit();
LearnFromNullTest(leftOperand, ref this.State);

bool leftIsConstant = leftOperand.ConstantValue != null;
Expand All @@ -4180,21 +4184,23 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
SetUnreachable();
}

rightResult = VisitRvalueWithState(rightOperand);

Join(ref this.State, ref whenNotNull);
Visit(rightOperand);
TypeWithState rightResult = ResultType;

if (rightOperand.ConstantValue?.IsBoolean ?? false)
if (whenNotNull.IsConditionalState)
{
Split();
if (rightOperand.ConstantValue.BooleanValue)
{
StateWhenFalse = whenNotNull;
}
else
{
StateWhenTrue = whenNotNull;
}
}

if (IsConditionalState)
{
Join(ref StateWhenTrue, ref whenNotNull.IsConditionalState ? ref whenNotNull.StateWhenTrue : ref whenNotNull.State);
Join(ref StateWhenFalse, ref whenNotNull.IsConditionalState ? ref whenNotNull.StateWhenFalse : ref whenNotNull.State);
}
else
{
Debug.Assert(!whenNotNull.IsConditionalState);
Join(ref State, ref whenNotNull.State);
}

var leftResultType = leftResult.Type;
Expand Down Expand Up @@ -4259,12 +4265,112 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
}
}

public override BoundNode? VisitConditionalAccess(BoundConditionalAccess node)
// PROTOTYPE(improved-definite-assignment):
// this is not used outside 'VisitPossibleConditionalAccess' yet, but is expected to be used
// when refactoring 'VisitBinaryOperatorChildren'
private bool TryVisitConditionalAccess(BoundExpression node, out PossiblyConditionalState stateWhenNotNull)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
var (conversion, access) = node switch
{
BoundConditionalAccess ca => (null, ca),
BoundConversion { Conversion: Conversion innerConversion, Operand: BoundConditionalAccess ca } conv when isAcceptableConversion(ca, innerConversion) => (conv, ca),
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
_ => default
};

if (access is object)
{
EnterRegionIfNeeded(access);
Copy link
Member

Choose a reason for hiding this comment

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

Do the Enter/LeaveRegionIfNeeded calls matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When doing the definite assignment side of this feature, I had to add these calls in the analogous position over there, because the usage of VisitConditionalAccess(node, out stateWhenNotNull) caused us to side-step the regular machinery for managing region analysis in AbstractFlowPass.VisitAlways. When this was missing in definite assignment, "extract method" and similar feature tests were broken.

I do not actually know what dependent features could be broken by the absence of this in NullableWalker, or how to test it in a more direct manner, but it seems like something that we should have anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Other nullable analysis methods don't have this, except that we set a value once in Scan (not exactly sure what that achieves either). I think we should remove it unless a scenario warrants it.

In reply to: 620688053 [](ancestors = 620688053)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Unsplit();
VisitConditionalAccess(access, out stateWhenNotNull);
if (conversion is object)
{
var operandType = ResultType;
TypeWithAnnotations explicitType = conversion.ConversionGroupOpt?.ExplicitType ?? default;
bool fromExplicitCast = explicitType.HasType;
TypeWithAnnotations targetType = fromExplicitCast ? explicitType : TypeWithAnnotations.Create(conversion.Type);
Debug.Assert(targetType.HasType);
var result = VisitConversion(conversion,
access,
conversion.Conversion,
targetType,
operandType,
checkConversion: true,
fromExplicitCast,
useLegacyWarnings: true,
assignmentKind: AssignmentKind.Assignment);
SetResultType(conversion, result);
}
Debug.Assert(!IsConditionalState);
LeaveRegionIfNeeded(access);
return true;
}

stateWhenNotNull = default;
return false;

// "State when not null" cannot propagate out of a conditional access if its conversion can return null when the input is non-null.
bool isAcceptableConversion(BoundConditionalAccess operand, Conversion conversion)
{
if (conversion.Kind is not (ConversionKind.ImplicitUserDefined or ConversionKind.ExplicitUserDefined))
{
return true;
}

var method = conversion.Method;
Debug.Assert(method is not null);
Debug.Assert(method.ParameterCount is 1);

// if input is not allowed to be null, then assume "state when not null" can propagate out
var param = method.Parameters[0];
var paramAnnotations = GetParameterAnnotations(param);
var paramType = ApplyLValueAnnotations(param.TypeWithAnnotations, paramAnnotations);
if ((paramAnnotations & FlowAnalysisAnnotations.DisallowNull) != 0
|| paramType.ToTypeWithState().IsNotNull)
{
return true;
}

if (method.ReturnNotNullIfParameterNotNull.Contains(param.Name))
{
return true;
}

// if the return is allowed to be null for a non-null input, we can't propagate out the "state when not null".
var returnState = ApplyUnconditionalAnnotations(method.ReturnTypeWithAnnotations.ToTypeWithState(), method.ReturnTypeFlowAnalysisAnnotations);
return returnState.IsNotNull;
}
}

private void VisitPossibleConditionalAccess(BoundExpression node, out PossiblyConditionalState stateWhenNotNull)
{
if (!TryVisitConditionalAccess(node, out stateWhenNotNull))
{
// in case we *didn't* have a conditional access, the only thing we learn in the "state when not null"
// is that the top-level expression was non-null.
Visit(node);
stateWhenNotNull = PossiblyConditionalState.Create(this);
var slot = MakeSlot(node);
if (slot > -1)
{
if (IsConditionalState)
{
LearnFromNonNullTest(slot, ref stateWhenNotNull.StateWhenTrue);
LearnFromNonNullTest(slot, ref stateWhenNotNull.StateWhenFalse);
}
else
{
LearnFromNonNullTest(slot, ref stateWhenNotNull.State);
}
}
}
}

private void VisitConditionalAccess(BoundConditionalAccess node, out PossiblyConditionalState stateWhenNotNull)
{
Debug.Assert(!IsConditionalState);

var receiver = node.Receiver;
_ = VisitRvalueWithState(receiver);
VisitRvalue(receiver);
_currentConditionalReceiverVisitResult = _visitResult;
var previousConditionalAccessSlot = _lastConditionalAccessSlot;

Expand All @@ -4279,18 +4385,60 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
// In the when-null branch, the receiver is known to be maybe-null.
// In the other branch, the receiver is known to be non-null.
LearnFromNullTest(receiver, ref receiverState);
LearnFromNonNullTest(receiver, ref this.State);
var nextConditionalAccessSlot = MakeSlot(receiver);
if (nextConditionalAccessSlot > 0 && receiver.Type?.IsNullableType() == true)
nextConditionalAccessSlot = GetNullableOfTValueSlot(receiver.Type, nextConditionalAccessSlot, out _);
makeAndAdjustReceiverSlot(receiver);
}

_lastConditionalAccessSlot = nextConditionalAccessSlot;
// We want to preserve stateWhenNotNull from accesses in the same "chain":
// a?.b(out x)?.c(out y); // expected to preserve stateWhenNotNull from both ?.b(out x) and ?.c(out y)
// but not accesses in nested expressions:
// a?.b(out x, c?.d(out y)); // expected to preserve stateWhenNotNull from a?.b(out x, ...) but not from c?.d(out y)
BoundExpression expr = node.AccessExpression;
while (expr is BoundConditionalAccess innerCondAccess)
{
// we assume that non-conditional accesses can never contain conditional accesses from the same "chain".
// that is, we never have to dig through non-conditional accesses to find and handle conditional accesses.
VisitRvalue(innerCondAccess.Receiver);
_currentConditionalReceiverVisitResult = _visitResult;
makeAndAdjustReceiverSlot(innerCondAccess.Receiver);

// The receiverState here represents the scenario where 0 or more of the access expressions could have been evaluated.
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
// e.g. after visiting `a?.b(x = null)?.c(x = new object())`, the "state when not null" of `x` is NotNull, but the "state when maybe null" of `x` is MaybeNull.
Join(ref receiverState, ref State);

expr = innerCondAccess.AccessExpression;
}

var accessTypeWithAnnotations = VisitLvalueWithAnnotations(node.AccessExpression);
TypeSymbol accessType = accessTypeWithAnnotations.Type;
Debug.Assert(expr is BoundExpression);
Visit(expr);

expr = node.AccessExpression;
while (expr is BoundConditionalAccess innerCondAccess)
{
// The resulting nullability of each nested conditional access is the same as the resulting nullability of the rightmost access.
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
SetAnalyzedNullability(innerCondAccess, _visitResult);
expr = innerCondAccess.AccessExpression;
}
Debug.Assert(expr is BoundExpression);
var slot = MakeSlot(expr);
if (slot > -1)
{
if (IsConditionalState)
{
LearnFromNonNullTest(slot, ref StateWhenTrue);
LearnFromNonNullTest(slot, ref StateWhenFalse);
}
else
{
LearnFromNonNullTest(slot, ref State);
}
}

stateWhenNotNull = PossiblyConditionalState.Create(this);
Unsplit();
Join(ref this.State, ref receiverState);

var accessTypeWithAnnotations = LvalueResultType;
TypeSymbol accessType = accessTypeWithAnnotations.Type;
var oldType = node.Type;
var resultType =
oldType.IsVoidType() || oldType.IsErrorType() ? oldType :
Expand All @@ -4302,6 +4450,22 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
SetResultType(node, TypeWithState.Create(resultType, NullableFlowState.MaybeDefault));
_currentConditionalReceiverVisitResult = default;
_lastConditionalAccessSlot = previousConditionalAccessSlot;

void makeAndAdjustReceiverSlot(BoundExpression receiver)
{
var slot = MakeSlot(receiver);
if (slot > 0 && receiver.Type?.IsNullableType() == true)
slot = GetNullableOfTValueSlot(receiver.Type, slot, out _);

_lastConditionalAccessSlot = slot;
if (slot > -1)
LearnFromNonNullTest(slot, ref State);
}
}

public override BoundNode? VisitConditionalAccess(BoundConditionalAccess node)
{
VisitConditionalAccess(node, out _);
return null;
}

Expand Down
Loading