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.

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

Learn from bool constants and conditional accesses inside ==/!= #52425

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
172 changes: 136 additions & 36 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ internal abstract partial class AbstractFlowPass<TLocalState, TLocalFunctionStat
/// </summary>
private readonly bool _nonMonotonicTransfer;

protected void SetConditionalState((TLocalState whenTrue, TLocalState whenFalse) state)
{
SetConditionalState(state.whenTrue, state.whenFalse);
}

protected void SetConditionalState(TLocalState whenTrue, TLocalState whenFalse)
{
IsConditionalState = true;
Expand Down Expand Up @@ -2235,20 +2240,101 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperator node)
protected virtual void VisitBinaryOperatorChildren(ArrayBuilder<BoundBinaryOperator> stack)
Copy link
Member

@333fred 333fred Apr 9, 2021

Choose a reason for hiding this comment

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

Consider nullable-enabling this method. #Resolved

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

{
var binary = stack.Pop();
VisitRvalue(binary.Left);

while (true)
// Only the leftmost operator of a left-associative binary operator chain can learn from a conditional access on the left
// For simplicity, we just special case it here.
if (VisitPossibleConditionalAccess(binary.Left, out var stateWhenNotNull)
&& canLearnFromOperator()
&& (isNullableValueTypeConversion(binary.Right) || binary.Right.ConstantValue is object))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
VisitRvalue(binary.Right);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
Meet(ref stateWhenNotNull, ref State);
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
var isNullConstant = binary.Right.ConstantValue?.IsNull == true;
SetConditionalState(isNullConstant == isEquals(binary)
? (State, stateWhenNotNull)
: (stateWhenNotNull, State));

if (stack.Count == 0)
{
return;
}

binary = stack.Pop();
}

while (true)
{
if (!canLearnFromOperator()
|| !learnFromOperator())
{
Unsplit();
Visit(binary.Right);
}

if (stack.Count == 0)
{
break;
}

Unsplit(); // VisitRvalue does this
binary = stack.Pop();
}

bool canLearnFromOperator()
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
var kind = binary.OperatorKind;
var op = kind.Operator();
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
return op is BinaryOperatorKind.Equal or BinaryOperatorKind.NotEqual
&& (!kind.IsUserDefined() || kind.IsLifted());
}

static bool isNullableValueTypeConversion(BoundExpression expr)
{
return expr is BoundConversion
{
ConversionKind: ConversionKind.ExplicitNullable or ConversionKind.ImplicitNullable,
} conv && conv.Operand.Type.IsNonNullableValueType();
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}

static bool isEquals(BoundBinaryOperator binary)
=> binary.OperatorKind.Operator() == BinaryOperatorKind.Equal;
Copy link
Member

@333fred 333fred Apr 9, 2021

Choose a reason for hiding this comment

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

It's not in the spec, but should we also consider lifted nullable relational operators? ie, >, >=, etc? #Closed

Copy link
Contributor Author

@RikkiGibson RikkiGibson Apr 9, 2021

Choose a reason for hiding this comment

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

Perhaps, although I think we'll have to reason out the implications of the fact that:

  • (int?)null == (int?)null returns true, but
  • (int?)null >= (int?)null returns false.

https://github.com/dotnet/csharplang/blob/main/spec/expressions.md#lifted-operators

I'd like to file it as a "stretch goal" in dotnet/csharplang#4465. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to file it as a "stretch goal" in dotnet/csharplang#4465.

Fine with me.


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


bool learnFromOperator()
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
if ((isNullableValueTypeConversion(binary.Left) || binary.Left.ConstantValue is object)
&& TryVisitConditionalAccess(binary.Right, out var stateWhenNotNull))
{
var isNullConstant = binary.Left.ConstantValue?.IsNull == true;
SetConditionalState(isNullConstant == isEquals(binary)
? (State, stateWhenNotNull)
: (stateWhenNotNull, State));

return true;
}
else if (IsConditionalState && binary.Right.ConstantValue is { IsBoolean: true } rightConstant)
{
var (stateWhenTrue, stateWhenFalse) = (StateWhenTrue.Clone(), StateWhenFalse.Clone());
Unsplit();
Visit(binary.Right);
SetConditionalState(isEquals(binary) == rightConstant.BooleanValue
? (stateWhenTrue, stateWhenFalse)
: (stateWhenFalse, stateWhenTrue));

return true;
}
else if (binary.Left.ConstantValue is { IsBoolean: true } leftConstant)
{
Unsplit();
Visit(binary.Right);
if (IsConditionalState && isEquals(binary) != leftConstant.BooleanValue)
{
SetConditionalState(StateWhenFalse, StateWhenTrue);
}

return true;
}

return false;
}
}

public override BoundNode VisitUnaryOperator(BoundUnaryOperator node)
Expand Down Expand Up @@ -2445,9 +2531,17 @@ public override BoundNode VisitMethodGroup(BoundMethodGroup node)
}
else
{
TLocalState savedState = VisitPossibleConditionalAccess(node.LeftOperand, out var stateWhenNotNull)
? stateWhenNotNull
: State.Clone();
TLocalState savedState;
if (VisitPossibleConditionalAccess(node.LeftOperand, out var stateWhenNotNull))
{
Debug.Assert(!IsConditionalState);
savedState = stateWhenNotNull;
}
else
{
Unsplit();
savedState = State.Clone();
}

if (node.LeftOperand.ConstantValue != null)
{
Expand All @@ -2467,41 +2561,28 @@ public override BoundNode VisitMethodGroup(BoundMethodGroup node)
return null;
}

/// <summary>
/// Unconditionally visits an expression.
/// If the expression has "state when not null" after visiting,
/// the method returns 'true' and writes the state to <paramref name="stateWhenNotNull" />.
/// </summary>
protected bool VisitPossibleConditionalAccess(BoundExpression node, [NotNullWhen(true)] out TLocalState? stateWhenNotNull)
private bool TryVisitConditionalAccess(BoundExpression node, [NotNullWhen(true)] out TLocalState? stateWhenNotNull)
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
EnterRegionIfNeeded(node);
var hasStateWhenNotNull = visit(out stateWhenNotNull);
Debug.Assert(!IsConditionalState);
LeaveRegionIfNeeded(node);
return hasStateWhenNotNull;

bool visit([NotNullWhen(true)] out TLocalState? stateWhenNotNull)
var access = node switch
{
var access = node switch
{
BoundConditionalAccess ca => ca,
BoundConversion { Conversion: Conversion innerConversion, Operand: BoundConditionalAccess ca } when isAcceptableConversion(ca, innerConversion) => ca,
_ => null
};
BoundConditionalAccess ca => ca,
BoundConversion { Conversion: Conversion innerConversion, Operand: BoundConditionalAccess ca } when isAcceptableConversion(ca, innerConversion) => ca,
_ => null
};

if (access is not null)
{
return VisitConditionalAccess(access, out stateWhenNotNull);
}
else
{
stateWhenNotNull = default;
VisitWithStackGuard(node);
Unsplit();
return false;
}
if (access is not null)
{
EnterRegionIfNeeded(access);
Unsplit();
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
var hasStateWhenNotNull = VisitConditionalAccess(access, out stateWhenNotNull);
Debug.Assert(!IsConditionalState);
LeaveRegionIfNeeded(access);
return hasStateWhenNotNull;
}

stateWhenNotNull = default;
return false;

// "State when not null" can only propagate out of a conditional access if
// it is not subject to a user-defined conversion whose parameter is not of a non-nullable value type.
static bool isAcceptableConversion(BoundConditionalAccess operand, Conversion conversion)
Expand All @@ -2524,6 +2605,25 @@ static bool isAcceptableConversion(BoundConditionalAccess operand, Conversion co
}
}

/// <summary>
/// Unconditionally visits an expression.
/// If the expression has "state when not null" after visiting,
/// the method returns 'true' and writes the state to <paramref name="stateWhenNotNull" />.
/// </summary>
private bool VisitPossibleConditionalAccess(BoundExpression node, [NotNullWhen(true)] out TLocalState? stateWhenNotNull)
{
stateWhenNotNull = default;
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
if (TryVisitConditionalAccess(node, out stateWhenNotNull))
{
return true;
}
else
{
Visit(node);
return false;
}
}

private bool VisitConditionalAccess(BoundConditionalAccess node, [NotNullWhen(true)] out TLocalState? stateWhenNotNull)
{
VisitRvalue(node.Receiver);
Expand Down
Loading