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 13 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
183 changes: 144 additions & 39 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,106 @@ 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.
// For example, `a?.b(out x) == true` has a conditional access on the left of the operator,
// but `expr == a?.b(out x) == true` has a conditional access on the right of the operator
if (VisitPossibleConditionalAccess(binary.Left, out var stateWhenNotNull)
&& canLearnFromOperator(binary)
&& isKnownNullOrNotNull(binary.Right))
{
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(binary)
|| !learnFromOperator(binary))
{
Unsplit();
Visit(binary.Right);
}

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

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

static bool canLearnFromOperator(BoundBinaryOperator binary)
{
var kind = binary.OperatorKind;
return kind.Operator() is BinaryOperatorKind.Equal or BinaryOperatorKind.NotEqual
&& (!kind.IsUserDefined() || kind.IsLifted());
}

static bool isKnownNullOrNotNull(BoundExpression expr)
{
return expr.ConstantValue is object
|| (expr is BoundConversion
{
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
ConversionKind: ConversionKind.ExplicitNullable or ConversionKind.ImplicitNullable,
} conv && conv.Operand.Type.IsNonNullableValueType());
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.

&& conv.Operand.Type.IsNonNullableValueType() [](start = 27, length = 45)

When is this part not true? #Resolved

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.

One test where this is not true is EqualsCondAccess_14. Conversion input is an int?, result type is a long?, and the conversion kind is ImplicitNullable. Further discussion in #52425 (comment)
#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)


// Returns true if `binary.Right` was visited by the call.
bool learnFromOperator(BoundBinaryOperator binary)
{
// `true == a?.b(out x)`
if (isKnownNullOrNotNull(binary.Left) && TryVisitConditionalAccess(binary.Right, out var stateWhenNotNull))
{
var isNullConstant = binary.Left.ConstantValue?.IsNull == true;
SetConditionalState(isNullConstant == isEquals(binary)
? (State, stateWhenNotNull)
: (stateWhenNotNull, State));

return true;
}
// `a && b(out x) == 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;
}
// `true == a && b(out x)`
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 +2536,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 @@ -2468,40 +2567,31 @@ public override BoundNode VisitMethodGroup(BoundMethodGroup node)
}

/// <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" />.
/// Visits a node only if it is a conditional access.
/// Returns 'true' if and only if the node was visited.
/// </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
VisitConditionalAccess(access, out stateWhenNotNull);
Debug.Assert(!IsConditionalState);
LeaveRegionIfNeeded(access);
return true;
}

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,15 +2614,32 @@ static bool isAcceptableConversion(BoundConditionalAccess operand, Conversion co
}
}

private bool VisitConditionalAccess(BoundConditionalAccess node, [NotNullWhen(true)] out TLocalState? stateWhenNotNull)
/// <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)
{
if (TryVisitConditionalAccess(node, out stateWhenNotNull))
{
return true;
}
else
{
Visit(node);
return false;
}
}

private void VisitConditionalAccess(BoundConditionalAccess node, out TLocalState stateWhenNotNull)
{
VisitRvalue(node.Receiver);

if (node.Receiver.ConstantValue != null && !IsConstantNull(node.Receiver))
{
VisitRvalue(node.AccessExpression);
stateWhenNotNull = default;
return false;
stateWhenNotNull = this.State.Clone();
}
else
{
Expand Down Expand Up @@ -2566,7 +2673,6 @@ private bool VisitConditionalAccess(BoundConditionalAccess node, [NotNullWhen(tr
stateWhenNotNull = State;
State = savedState;
Join(ref State, ref stateWhenNotNull);
return true;
}
}

Expand Down Expand Up @@ -3310,4 +3416,3 @@ private void VisitMethodBodies(BoundBlock blockBody, BoundBlock expressionBody)
/// </remarks>
internal enum RegionPlace { Before, Inside, After };
}

Loading