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

Check NotNullIfNotNull in implementation #47649

Merged
merged 10 commits into from
Sep 22, 2020
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5600,6 +5600,18 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="WRN_ParameterDisallowsNull_Title" xml:space="preserve">
<value>Parameter must have a non-null value when exiting.</value>
</data>
<data name="WRN_ParameterNotNullIfNotNull" xml:space="preserve">
<value>Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.</value>
</data>
<data name="WRN_ParameterNotNullIfNotNull_Title" xml:space="preserve">
<value>Parameter must have a non-null value when exiting because parameter referenced by NotNullIfNotNull is non-null.</value>
</data>
<data name="WRN_ReturnNotNullIfNotNull" xml:space="preserve">
<value>Return value must be non-null because parameter '{0}' is non-null.</value>
</data>
<data name="WRN_ReturnNotNullIfNotNull_Title" xml:space="preserve">
<value>Return value must be non-null because parameter is non-null.</value>
</data>
<data name="WRN_MemberNotNull" xml:space="preserve">
<value>Member '{0}' must have a non-null value when exiting.</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1835,6 +1835,8 @@ internal enum ErrorCode
ERR_OverrideDefaultConstraintNotSatisfied = 8822,
ERR_DefaultConstraintOverrideOnly = 8823,
WRN_PartialMethodTypeDifference = 8824,
WRN_ParameterNotNullIfNotNull = 8825,
WRN_ReturnNotNullIfNotNull = 8826,

ERR_RuntimeDoesNotSupportCovariantReturnsOfClasses = 8830,
ERR_RuntimeDoesNotSupportCovariantPropertiesOfClasses = 8831,
Expand Down
4 changes: 4 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ static ErrorFacts()
nullableWarnings.Add(getId(ErrorCode.WRN_MemberNotNullBadMember));
nullableWarnings.Add(getId(ErrorCode.WRN_MemberNotNullWhen));
nullableWarnings.Add(getId(ErrorCode.WRN_ParameterDisallowsNull));
nullableWarnings.Add(getId(ErrorCode.WRN_ParameterNotNullIfNotNull));
nullableWarnings.Add(getId(ErrorCode.WRN_ReturnNotNullIfNotNull));

NullableWarnings = nullableWarnings.ToImmutable();

Expand Down Expand Up @@ -480,6 +482,8 @@ internal static int GetWarningLevel(ErrorCode code)
case ErrorCode.WRN_SwitchExpressionNotExhaustiveWithWhen:
case ErrorCode.WRN_SwitchExpressionNotExhaustiveForNullWithWhen:
case ErrorCode.WRN_RecordNamedDisallowed:
case ErrorCode.WRN_ParameterNotNullIfNotNull:
case ErrorCode.WRN_ReturnNotNullIfNotNull:
return 1;
default:
return 0;
Expand Down
68 changes: 51 additions & 17 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -469,10 +469,10 @@ protected override ImmutableArray<PendingBranch> Scan(ref bool badRegion)
{
enforceMemberNotNull(syntaxOpt: pendingReturn.Branch.Syntax, pendingReturn.State);

if (pendingReturn.Branch is BoundReturnStatement { ExpressionOpt: BoundExpression expr } returnStatement)
if (pendingReturn.Branch is BoundReturnStatement returnStatement)
{
enforceNotNull(returnStatement.Syntax, pendingReturn.State);
enforceNotNullWhenForPendingReturn(pendingReturn, expr, returnStatement);
enforceNotNullWhenForPendingReturn(pendingReturn, returnStatement);
enforceMemberNotNullWhenForPendingReturn(pendingReturn, returnStatement);
}
}
Expand Down Expand Up @@ -757,7 +757,7 @@ void makeMemberMaybeNull(MethodSymbol method, string memberName)
}
}

void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundExpression expr, BoundReturnStatement returnStatement)
void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturnStatement returnStatement)
{
var parameters = this.MethodParameters;

Expand All @@ -780,11 +780,24 @@ void enforceNotNull(SyntaxNode? syntaxOpt, LocalState state)

foreach (var parameter in this.MethodParameters)
{
if (parameterHasBadState(parameter, state))
var slot = GetOrCreateSlot(parameter);
if (slot <= 0)
{
continue;
}

var annotations = parameter.FlowAnalysisAnnotations;
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't u use GetFlowAnalysisAnnotations here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was moved from parameterHasBadState below. However GetFlowAnalysisAnnotations(Symbol) here would just dispatch out to ParameterSymbol.FlowAnalysisAnnotations.

It could make sense to change this to GetParameterAnnotations in order to avoid attribute binding cycles. Since I can't come up with a crashing scenario for this off the top of my head I would like to punt changes to it to #47635

var hasNotNull = (annotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
var parameterState = state[slot];
if (hasNotNull && parameterState.MayBeNull())
{
// Parameter '{name}' must have a non-null value when exiting.
Diagnostics.Add(ErrorCode.WRN_ParameterDisallowsNull, syntaxOpt?.GetLocation() ?? methodMainNode.Syntax.GetLastToken().GetLocation(), parameter.Name);
}
else
{
EnforceNotNullIfNotNull(syntaxOpt, state, this.MethodParameters, parameter.NotNullIfParameterNotNull, parameterState, parameter);
}
}
}

Expand All @@ -805,19 +818,6 @@ void enforceParameterNotNullWhen(SyntaxNode syntax, ImmutableArray<ParameterSymb
}
}

bool parameterHasBadState(ParameterSymbol parameter, LocalState state)
{
var slot = GetOrCreateSlot(parameter);
if (slot > 0)
{
var annotations = parameter.FlowAnalysisAnnotations;
bool hasNotNull = (annotations & FlowAnalysisAnnotations.NotNull) == FlowAnalysisAnnotations.NotNull;
return hasNotNull && state[slot].MayBeNull();
}

return false;
}

bool parameterHasBadConditionalState(ParameterSymbol parameter, bool sense, LocalState stateWhen)
{
var refKind = parameter.RefKind;
Expand Down Expand Up @@ -883,6 +883,35 @@ int getSlotForFieldOrPropertyOrEvent(Symbol member)
}
}

private void EnforceNotNullIfNotNull(SyntaxNode? syntaxOpt, LocalState state, ImmutableArray<ParameterSymbol> parameters, ImmutableHashSet<string> inputParamNames, NullableFlowState outputState, ParameterSymbol? outputParam)
{
if (inputParamNames.IsEmpty || outputState.IsNotNull())
{
return;
}

foreach (var inputParam in parameters)
{
if (inputParamNames.Contains(inputParam.Name)
&& GetOrCreateSlot(inputParam) is > 0 and int inputSlot
jaredpar marked this conversation as resolved.
Show resolved Hide resolved
&& state[inputSlot].IsNotNull())
{
var location = syntaxOpt?.GetLocation() ?? methodMainNode.Syntax.GetLastToken().GetLocation();
if (outputParam is object)
{
// Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.
Diagnostics.Add(ErrorCode.WRN_ParameterNotNullIfNotNull, location, outputParam.Name, inputParam.Name);
}
else
{
// Return value must be non-null because parameter '{0}' is non-null.
Diagnostics.Add(ErrorCode.WRN_ReturnNotNullIfNotNull, location, inputParam.Name);
}
break;
}
}
}

private void EnforceDoesNotReturn(SyntaxNode? syntaxOpt)
{
// DoesNotReturn is only supported in member methods
Expand Down Expand Up @@ -2309,6 +2338,11 @@ private static TypeWithState GetParameterState(TypeWithAnnotations parameterType
}

Unsplit();
if (CurrentSymbol is MethodSymbol method)
{
EnforceNotNullIfNotNull(node.Syntax, this.State, method.Parameters, method.ReturnNotNullIfParameterNotNull, ResultType.State, outputParam: null);
}

SetUnreachable();

return null;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ private PEModuleBuilder? EmitModule
}
Debug.Assert(!node.HasErrors, "nodes with errors should not be lowered");

return VisitExpressionImpl(node);
// https://github.com/dotnet/roslyn/issues/47682
return VisitExpressionImpl(node)!;
}

private BoundStatement? VisitStatement(BoundStatement? node)
Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxRewriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ public virtual bool VisitIntoStructuredTrivia
var result = ((CSharpSyntaxNode)node).Accept(this);

_recursionDepth--;
return result;
// https://github.com/dotnet/roslyn/issues/47682
return result!;
}
else
{
Expand Down
32 changes: 26 additions & 6 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -912,14 +912,14 @@
<target state="new">discards</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<trans-unit id="WRN_ParameterNotNullIfNotNull">
<source>Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.</source>
<target state="new">Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<trans-unit id="WRN_ParameterNotNullIfNotNull_Title">
<source>Parameter must have a non-null value when exiting because parameter referenced by NotNullIfNotNull is non-null.</source>
<target state="new">Parameter must have a non-null value when exiting because parameter referenced by NotNullIfNotNull is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_PartialMethodTypeDifference">
Expand All @@ -942,6 +942,16 @@
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ReturnNotNullIfNotNull">
<source>Return value must be non-null because parameter '{0}' is non-null.</source>
<target state="new">Return value must be non-null because parameter '{0}' is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ReturnNotNullIfNotNull_Title">
<source>Return value must be non-null because parameter is non-null.</source>
<target state="new">Return value must be non-null because parameter is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_SwitchExpressionNotExhaustiveWithUnnamedEnumValue">
<source>The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '{0}' is not covered.</source>
<target state="new">The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '{0}' is not covered.</target>
Expand Down Expand Up @@ -2474,6 +2484,16 @@
<target state="translated">Typ odkazu s možnou hodnotou null ve vráceném typu neodpovídá přepsanému členu.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">Typ odkazu s možnou hodnotou null v typu neodpovídá implementovanému členu {0}.</target>
Expand Down
32 changes: 26 additions & 6 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
Original file line number Diff line number Diff line change
Expand Up @@ -912,14 +912,14 @@
<target state="new">discards</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<trans-unit id="WRN_ParameterNotNullIfNotNull">
<source>Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.</source>
<target state="new">Parameter '{0}' must have a non-null value when exiting because parameter '{1}' is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<trans-unit id="WRN_ParameterNotNullIfNotNull_Title">
<source>Parameter must have a non-null value when exiting because parameter referenced by NotNullIfNotNull is non-null.</source>
<target state="new">Parameter must have a non-null value when exiting because parameter referenced by NotNullIfNotNull is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_PartialMethodTypeDifference">
Expand All @@ -942,6 +942,16 @@
<target state="new">Types and aliases should not be named 'record'.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ReturnNotNullIfNotNull">
<source>Return value must be non-null because parameter '{0}' is non-null.</source>
<target state="new">Return value must be non-null because parameter '{0}' is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_ReturnNotNullIfNotNull_Title">
<source>Return value must be non-null because parameter is non-null.</source>
<target state="new">Return value must be non-null because parameter is non-null.</target>
<note />
</trans-unit>
<trans-unit id="WRN_SwitchExpressionNotExhaustiveWithUnnamedEnumValue">
<source>The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '{0}' is not covered.</source>
<target state="new">The switch expression does not handle some values of its input type (it is not exhaustive) involving an unnamed enum value. For example, the pattern '{0}' is not covered.</target>
Expand Down Expand Up @@ -2474,6 +2484,16 @@
<target state="translated">Die NULL-Zulässigkeit von Verweistypen im Rückgabetyp entspricht nicht dem außer Kraft gesetzten Member.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInReturnTypeOnPartial_Title">
<source>Nullability of reference types in return type doesn't match partial method declaration.</source>
<target state="new">Nullability of reference types in return type doesn't match partial method declaration.</target>
<note />
</trans-unit>
<trans-unit id="WRN_NullabilityMismatchInTypeOnExplicitImplementation">
<source>Nullability of reference types in type doesn't match implemented member '{0}'.</source>
<target state="translated">Die NULL-Zulässigkeit von Verweistypen im Typ entspricht nicht dem implementierten Member "{0}".</target>
Expand Down
Loading