Skip to content

Only enforce Member/NotNullWhen on constants #48425

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

Merged
jcouv merged 7 commits into from
Oct 22, 2020
Merged

Only enforce Member/NotNullWhen on constants #48425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a38e715
Only enforce Member/NotNullWhen on constants
Oct 8, 2020
a92fefb
Don't split unnecessarily on !expr
Oct 8, 2020
93ff57a
Split state for constant boolean expressions
Oct 8, 2020
7c59ff8
Enforce in some non-constant scenarios that are useful
Oct 9, 2020
2acf4dd
Fix sense in diagnostic
Oct 10, 2020
c2602c1
Clarify test
Oct 10, 2020
9ff27bf
Address feedback from Fred
Oct 16, 2020
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
138 changes: 106 additions & 32 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 @@ -611,35 +611,72 @@ void enforceMemberNotNullWhenForPendingReturn(PendingBranch pendingReturn, Bound
{
if (pendingReturn.IsConditionalState)
{
enforceMemberNotNullWhen(returnStatement.Syntax, sense: true, pendingReturn.StateWhenTrue);
enforceMemberNotNullWhen(returnStatement.Syntax, sense: false, pendingReturn.StateWhenFalse);
if (returnStatement.ExpressionOpt is { ConstantValue: { IsBoolean: true, BooleanValue: bool value } })
{
enforceMemberNotNullWhen(returnStatement.Syntax, sense: value, pendingReturn.State);
return;
}

if (!pendingReturn.StateWhenTrue.Reachable || !pendingReturn.StateWhenFalse.Reachable)
{
return;
}

if (_symbol is MethodSymbol method)
{
foreach (var memberName in method.NotNullWhenTrueMembers)
{
enforceMemberNotNullWhenIfAffected(returnStatement.Syntax, sense: true, method.ContainingType.GetMembers(memberName), pendingReturn.StateWhenTrue, pendingReturn.StateWhenFalse);
}

foreach (var memberName in method.NotNullWhenFalseMembers)
{
enforceMemberNotNullWhenIfAffected(returnStatement.Syntax, sense: false, method.ContainingType.GetMembers(memberName), pendingReturn.StateWhenFalse, pendingReturn.StateWhenTrue);
}
}
}
else if (returnStatement.ExpressionOpt is { ConstantValue: { IsBoolean: true, BooleanValue: bool value } })
{
enforceMemberNotNullWhen(returnStatement.Syntax, sense: value, pendingReturn.State);
}
}

void enforceMemberNotNullWhen(SyntaxNode? syntaxOpt, bool sense, LocalState state)
void enforceMemberNotNullWhenIfAffected(SyntaxNode? syntaxOpt, bool sense, ImmutableArray<Symbol> members, LocalState state, LocalState otherState)
{
if (!state.Reachable)
foreach (var member in members)
{
return;
// For non-constant values, only complain if we were able to analyze a difference for this member between two branches
if (memberHasBadState(member, state) != memberHasBadState(member, otherState))
{
reportMemberIfBadConditionalState(syntaxOpt, sense, member, state);
}
}
}

void enforceMemberNotNullWhen(SyntaxNode? syntaxOpt, bool sense, LocalState state)
{
if (_symbol is MethodSymbol method)
{
var notNullMembers = sense ? method.NotNullWhenTrueMembers : method.NotNullWhenFalseMembers;
foreach (var memberName in notNullMembers)
{
foreach (var member in method.ContainingType.GetMembers(memberName))
{
if (memberHasBadState(member, state))
{
// Member '{name}' must have a non-null value when exiting with '{sense}'.
Diagnostics.Add(ErrorCode.WRN_MemberNotNullWhen, syntaxOpt?.GetLocation() ?? methodMainNode.Syntax.GetLastToken().GetLocation(), member.Name, sense ? "true" : "false");
}
reportMemberIfBadConditionalState(syntaxOpt, sense, member, state);
}
}
}
}

void reportMemberIfBadConditionalState(SyntaxNode? syntaxOpt, bool sense, Symbol member, LocalState state)
{
if (memberHasBadState(member, state))
{
// Member '{name}' must have a non-null value when exiting with '{sense}'.
Diagnostics.Add(ErrorCode.WRN_MemberNotNullWhen, syntaxOpt?.GetLocation() ?? methodMainNode.Syntax.GetLastToken().GetLocation(), member.Name, sense ? "true" : "false");
}
}

bool memberHasBadState(Symbol member, LocalState state)
{
switch (member.Kind)
Expand Down Expand Up @@ -767,12 +804,45 @@ void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturn
{
if (pendingReturn.IsConditionalState)
{
enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: true, stateWhen: pendingReturn.StateWhenTrue);
enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: false, stateWhen: pendingReturn.StateWhenFalse);
if (returnStatement.ExpressionOpt is { ConstantValue: { IsBoolean: true, BooleanValue: bool value } })
{
enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: value, stateWhen: pendingReturn.State);
return;
}

if (!pendingReturn.StateWhenTrue.Reachable || !pendingReturn.StateWhenFalse.Reachable)
Copy link
Member

@333fred 333fred Oct 13, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Shouldn't we only suppress when both states are unreachable? We could have just done a test and thrown in the other branch, but accidentally messed up the order of the branches and returned incorrectly. #ByDesign

Copy link
Member

@333fred 333fred Oct 13, 2020

Choose a reason for hiding this comment

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

Same for the other tests of this condition.

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

Copy link
Member Author

@jcouv jcouv Oct 16, 2020

Choose a reason for hiding this comment

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

The only way to have one state being unreachable is using a constant condition (which is case above).

In reply to: 503602558 [](ancestors = 503602558,503602424)

{
return;
}

foreach (var parameter in parameters)
{
// For non-constant values, only complain if we were able to analyze a difference for this parameter between two branches
if (GetOrCreateSlot(parameter) is > 0 and var slot && pendingReturn.StateWhenTrue[slot] != pendingReturn.StateWhenFalse[slot])
Copy link
Member

@RikkiGibson RikkiGibson Oct 9, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

(doesn't need to be addressed in this PR) I have been thinking that it doesn't make sense for a parameter to ever not have a slot--it will never be constant, for example. Perhaps we should use a helper for getting parameter slots that throws if we fail to get a slot for it. #WontFix

{
reportParameterIfBadConditionalState(returnStatement.Syntax, parameter, sense: true, stateWhen: pendingReturn.StateWhenTrue);
reportParameterIfBadConditionalState(returnStatement.Syntax, parameter, sense: false, stateWhen: pendingReturn.StateWhenFalse);
}
}
}
else if (returnStatement.ExpressionOpt is { ConstantValue: { IsBoolean: true, BooleanValue: bool value } })
{
// example: return (bool)true;
enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: value, stateWhen: pendingReturn.State);
return;
}
}
}

void reportParameterIfBadConditionalState(SyntaxNode syntax, ParameterSymbol parameter, bool sense, LocalState stateWhen)
{
if (parameterHasBadConditionalState(parameter, sense, stateWhen))
{
// Parameter '{name}' must have a non-null value when exiting with '{sense}'.
Diagnostics.Add(ErrorCode.WRN_ParameterConditionallyDisallowsNull, syntax.Location, parameter.Name, sense ? "true" : "false");
}
}

void enforceNotNull(SyntaxNode? syntaxOpt, LocalState state)
{
if (!state.Reachable)
Expand Down Expand Up @@ -812,11 +882,7 @@ void enforceParameterNotNullWhen(SyntaxNode syntax, ImmutableArray<ParameterSymb

foreach (var parameter in parameters)
{
if (parameterHasBadConditionalState(parameter, sense, stateWhen))
{
// Parameter '{name}' must have a non-null value when exiting with '{sense}'.
Diagnostics.Add(ErrorCode.WRN_ParameterConditionallyDisallowsNull, syntax.Location, parameter.Name, sense ? "true" : "false");
}
reportParameterIfBadConditionalState(syntax, parameter, sense, stateWhen);
}
}

Expand Down Expand Up @@ -2428,6 +2494,7 @@ private bool TryGetReturnType(out TypeWithAnnotations type, out FlowAnalysisAnno
}

SetResult(node, GetAdjustedResult(type.ToTypeWithState(), slot), type);
SplitIfBooleanConstant(node);
return null;
}

Expand Down Expand Up @@ -8144,9 +8211,27 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter
return null;
}

private void SplitIfBooleanConstant(BoundExpression node)
{
if (node.ConstantValue is { IsBoolean: true, BooleanValue: bool booleanValue })
{
Split();
if (booleanValue)
{
StateWhenFalse = UnreachableState();
}
else
{
StateWhenTrue = UnreachableState();
}
}
}

public override BoundNode? VisitFieldAccess(BoundFieldAccess node)
{
var updatedSymbol = VisitMemberAccess(node, node.ReceiverOpt, node.FieldSymbol);

SplitIfBooleanConstant(node);
SetUpdatedSymbol(node, node.FieldSymbol, updatedSymbol);
return null;
}
Expand Down Expand Up @@ -8606,8 +8691,9 @@ public override void VisitForEachIterationVariables(BoundForEachStatement node)
switch (node.OperatorKind)
{
case UnaryOperatorKind.BoolLogicalNegation:
VisitCondition(node.Operand);
SetConditionalState(StateWhenFalse, StateWhenTrue);
Visit(node.Operand);
if (IsConditionalState)
SetConditionalState(StateWhenFalse, StateWhenTrue);
resultType = adjustForLifting(ResultType);
break;
case UnaryOperatorKind.DynamicTrue:
Expand Down Expand Up @@ -8960,19 +9046,7 @@ private TypeWithState InferResultNullabilityOfBinaryLogicalOperator(BoundExpress
Debug.Assert(!IsConditionalState);
SetResultType(node, TypeWithState.Create(node.Type, node.Type?.CanContainNull() != false && node.ConstantValue?.IsNull == true ? NullableFlowState.MaybeDefault : NullableFlowState.NotNull));

if (node.ConstantValue?.IsBoolean == true)
{
Split();
if (node.ConstantValue.BooleanValue)
{
StateWhenFalse = UnreachableState();
}
else
{
StateWhenTrue = UnreachableState();
}
}

SplitIfBooleanConstant(node);
return result;
}

Expand Down
Loading