From a38e7156ad09f943decfcec3134ead2a86746290 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Wed, 7 Oct 2020 18:22:23 -0700 Subject: [PATCH 1/7] Only enforce Member/NotNullWhen on constants --- .../Portable/FlowAnalysis/NullableWalker.cs | 19 +- .../Semantics/NullableReferenceTypesTests.cs | 287 ++++++++++++++---- 2 files changed, 248 insertions(+), 58 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 0c56ef19f9d59..3996a61b382c3 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -609,10 +609,17 @@ void enforceMemberNotNullOnMember(SyntaxNode? syntaxOpt, LocalState state, Metho void enforceMemberNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturnStatement returnStatement) { - if (pendingReturn.IsConditionalState) + if (returnStatement.ExpressionOpt is BoundExpression { ConstantValue: { IsBoolean: true, BooleanValue: bool value } }) { - enforceMemberNotNullWhen(returnStatement.Syntax, sense: true, pendingReturn.StateWhenTrue); - enforceMemberNotNullWhen(returnStatement.Syntax, sense: false, pendingReturn.StateWhenFalse); + if (pendingReturn.IsConditionalState) + { + enforceMemberNotNullWhen(returnStatement.Syntax, sense: true, pendingReturn.StateWhenTrue); + enforceMemberNotNullWhen(returnStatement.Syntax, sense: false, pendingReturn.StateWhenFalse); + } + else + { + enforceMemberNotNullWhen(returnStatement.Syntax, sense: value, pendingReturn.State); + } } } @@ -763,13 +770,17 @@ void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturn { var parameters = this.MethodParameters; - if (!parameters.IsEmpty) + if (!parameters.IsEmpty && returnStatement.ExpressionOpt is BoundExpression { ConstantValue: { IsBoolean: true, BooleanValue: bool value } }) { if (pendingReturn.IsConditionalState) { enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: true, stateWhen: pendingReturn.StateWhenTrue); enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: false, stateWhen: pendingReturn.StateWhenFalse); } + else + { + enforceParameterNotNullWhen(returnStatement.Syntax, parameters, sense: value, stateWhen: pendingReturn.State); + } } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 1c0a98a67a16f..4529a396ec043 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -24075,6 +24075,49 @@ bool Init() ); } + [Fact] + public void MemberNotNullWhenTrue_EnforcedInMethodBody_NonConstant() + { + var c = CreateNullableCompilation(new[] { @" +using System.Diagnostics.CodeAnalysis; +public class C +{ + public string field1; + public string? field2; + public string field3; + public string? field4; + + C() // 1 + { + if (!Init()) throw null!; + } + + [MemberNotNullWhen(true, nameof(field1), nameof(field2))] + bool Init() + { + bool b = true; + if (b) + { + return b; + } + if (b) + { + return !b; + } + return M(out _); + } + + bool M([MaybeNullWhen(true)]out string s) => throw null!; +} +", MemberNotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); + + c.VerifyDiagnostics( + // (10,5): warning CS8618: Non-nullable field 'field3' is uninitialized. Consider declaring the field as nullable. + // C() // 1 + Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C").WithArguments("field", "field3").WithLocation(10, 5) + ); + } + [Fact] public void MemberNotNullWhenTrue_WithMemberNotNull() { @@ -24307,17 +24350,13 @@ bool IsInit2 { get { - return field2 == null; // 1 + return field2 == null; } } } ", MemberNotNullWhenAttributeDefinition }, parseOptions: TestOptions.Regular9); - c.VerifyDiagnostics( - // (21,13): error CS8765: Member 'field2' must have a non-null value when exiting with 'true'. - // return field2 == null; // 1 - Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field2 == null;").WithArguments("field2", "true").WithLocation(21, 13) - ); + c.VerifyDiagnostics(); } [Fact] @@ -25491,13 +25530,13 @@ public class C public static bool TryGetValue(C? c, [NotNullWhen(true)] out string? s) { s = null; - return c; // 1 + return c; } public static bool TryGetValue2(C c, [NotNullWhen(false)] out string? s) { s = null; - return c; // 2 + return c; } public static implicit operator bool(C? c) => throw null!; @@ -25505,18 +25544,22 @@ public static bool TryGetValue2(C c, [NotNullWhen(false)] out string? s) static bool TryGetValue3([MaybeNullWhen(false)]out string s) { s = null; - return (bool)true; // 3 + return (bool)true; // 1 } static bool TryGetValue4([MaybeNullWhen(false)]out string s) { s = null; - return (bool)false; // 4 + return (bool)false; } } "; var comp = CreateNullableCompilation(new[] { source, NotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); - comp.VerifyDiagnostics(); + comp.VerifyDiagnostics( + // (22,9): warning CS8762: Parameter 's' must have a non-null value when exiting with 'true'. + // return (bool)true; + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return (bool)true;").WithArguments("s", "true").WithLocation(22, 9) + ); } [Fact] @@ -36978,13 +37021,13 @@ static bool TryGetValue(TYPE x, [MaybeNullWhen(true)] out TYPE y) static bool TryGetValue2(TYPE x, [MaybeNullWhen(true)] out TYPE y) { y = x; - return y != null; // 1 + return y != null; } static bool TryGetValue3(TYPE x, [MaybeNullWhen(false)] out TYPE y) { y = x; - return y == null; // 2 + return y == null; } static bool TryGetValue4(TYPE x, [MaybeNullWhen(false)] out TYPE y) @@ -36995,14 +37038,7 @@ static bool TryGetValue4(TYPE x, [MaybeNullWhen(false)] out TYPE y) } "; var comp = CreateNullableCompilation(new[] { MaybeNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics( - // (18,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return y != null; // 1 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null;").WithArguments("y", "false").WithLocation(18, 9), - // (24,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. - // return y == null; // 2 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y == null;").WithArguments("y", "true").WithLocation(24, 9) - ); + comp.VerifyDiagnostics(); } [Theory] @@ -37035,13 +37071,13 @@ static bool TryGetValue([AllowNull] TYPE x, [MaybeNullWhen(true)] out TYPE y) static bool TryGetValue2([AllowNull] TYPE x, [MaybeNullWhen(true)] out TYPE y) { y = x; - return y != null; // 1 + return y != null; } static bool TryGetValue3([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) { y = x; - return y == null; // 2 + return y == null; } static bool TryGetValue4([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) @@ -37052,14 +37088,7 @@ static bool TryGetValue4([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) } "; var comp = CreateNullableCompilation(new[] { AllowNullAttributeDefinition, MaybeNullAttributeDefinition, MaybeNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics( - // (24,9): error CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return y != null; // 1 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null;").WithArguments("y", "false").WithLocation(24, 9), - // (30,9): error CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. - // return y == null; // 2 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y == null;").WithArguments("y", "true").WithLocation(30, 9) - ); + comp.VerifyDiagnostics(); } [Theory] @@ -37115,7 +37144,7 @@ bool TryGetValue([MaybeNullWhen(true)] out string y) bool TryGetValue2([MaybeNullWhen(true)] out string y) { - return !TryGetValueCore(out y); // 1 + return !TryGetValueCore(out y); } bool TryGetValueCore([NotNullWhen(false)] out string? y) @@ -37125,11 +37154,7 @@ bool TryGetValueCore([NotNullWhen(false)] out string? y) } "; var comp = CreateNullableCompilation(new[] { NotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition, source }); - comp.VerifyDiagnostics( - // (15,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return !TryGetValueCore(out y); // 1 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return !TryGetValueCore(out y);").WithArguments("y", "false").WithLocation(15, 9) - ); + comp.VerifyDiagnostics(); } [Fact, WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")] @@ -37146,7 +37171,7 @@ static bool TryGetValue([AllowNull]T x, [MaybeNullWhen(true)]out T y, [MaybeN { y = x; z = x; - return y != null || z != null; // 1, 2 + return y != null || z != null; } static bool TryGetValue2([AllowNull]T x, [MaybeNullWhen(false)]out T y, [MaybeNullWhen(false)]out T z) @@ -37158,14 +37183,7 @@ static bool TryGetValue2([AllowNull]T x, [MaybeNullWhen(false)]out T y, [Mayb } "; var comp = CreateNullableCompilation(new[] { AllowNullAttributeDefinition, MaybeNullWhenAttributeDefinition, source }); - comp.VerifyDiagnostics( - // (12,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return y != null || z != null; // 1, 2 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null || z != null;").WithArguments("y", "false").WithLocation(12, 9), - // (12,9): warning CS8762: Parameter 'z' must have a non-null value when exiting with 'false'. - // return y != null || z != null; // 1, 2 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null || z != null;").WithArguments("z", "false").WithLocation(12, 9) - ); + comp.VerifyDiagnostics(); } [Theory] @@ -37325,14 +37343,7 @@ static bool TryGetValueString2(string key, [NotNullWhen(true)] out string? value } "; var comp = CreateNullableCompilation(new[] { MaybeNullWhenAttributeDefinition, NotNullAttributeDefinition, NotNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics( - // (17,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. - // return TryGetValue3(out y); // 1 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return TryGetValue3(out y);").WithArguments("y", "true").WithLocation(17, 9), - // (27,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return TryGetValue2(out y); // 2 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return TryGetValue2(out y);").WithArguments("y", "false").WithLocation(27, 9) - ); + comp.VerifyDiagnostics(); } [Fact, WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")] @@ -139749,6 +139760,174 @@ void M() Diagnostic(ErrorCode.ERR_DiscardTypeInferenceFailed, "_").WithLocation(10, 13)); } + [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] + public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126() + { + var source = @" +using System.Diagnostics.CodeAnalysis; + +#nullable enable + +class C +{ + static bool NullWhenFalseA(bool a, [MaybeNullWhen(false)] out string s1) + { + s1 = null; + return a; + } + + static bool NullWhenFalseNotA(bool a, [MaybeNullWhen(false)] out string s1) + { + s1 = null; + return !a; + } + + static bool NullWhenTrueA(bool a, [MaybeNullWhen(true)] out string s1) + { + s1 = null; + return a; + } + + static bool NullWhenTrueNotA(bool a, [MaybeNullWhen(true)] out string s1) + { + s1 = null; + return !a; + } +}"; + var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); + comp.VerifyEmitDiagnostics(); + } + + [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] + public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_2() + { + var source = @" +using System.Diagnostics.CodeAnalysis; + +#nullable enable + +class C +{ + static bool M1([MaybeNullWhen(false)] out string s1) + { + const bool b = true; + s1 = null; + return b; // 1 + } + + static bool M2([MaybeNullWhen(false)] out string s1) + { + s1 = null; + return (bool)true; // 2 + } +}"; + var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); + comp.VerifyEmitDiagnostics( + // (12,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. + // return b; // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return b;").WithArguments("s1", "true").WithLocation(12, 9), + // (18,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. + // return (bool)true; // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return (bool)true;").WithArguments("s1", "true").WithLocation(18, 9) + ); + } + + [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] + public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_3() + { + var source = @" +using System.Diagnostics.CodeAnalysis; + +#nullable enable + +class C +{ + static bool M1([MaybeNullWhen(false)] out string s1) + { + const bool b = true; + s1 = null; + return b; // 1 + } + + static bool M2([MaybeNullWhen(false)] out string s1) + { + const bool b = true; + s1 = null; + return b && true; // 2 + } + + static bool M3([MaybeNullWhen(false)] out string s1) + { + const bool b = false; + s1 = null; + return b; + } +}"; + var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); + comp.VerifyEmitDiagnostics( + // (12,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. + // return b; // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return b;").WithArguments("s1", "true").WithLocation(12, 9), + // (19,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. + // return b && true; // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return b && true;").WithArguments("s1", "true").WithLocation(19, 9) + ); + } + + [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] + public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_4() + { + var source = @" +using System.Diagnostics.CodeAnalysis; + +#nullable enable + +class C +{ + static bool M1([MaybeNullWhen(false)] out string s1) + { + return M1(out s1); + } + + static bool M2([MaybeNullWhen(false)] out string s1) + { + return !M1(out s1); + } +}"; + var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); + comp.VerifyEmitDiagnostics(); + } + + [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] + public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_5() + { + var source = @" +using System.Diagnostics.CodeAnalysis; + +#nullable enable + +internal static class Program +{ + public static bool M1([MaybeNullWhen(true)] out string s) + { + s = null; + return HasAnnotation(out _); + } + + public static bool M2([MaybeNullWhen(true)] out string s) + { + s = null; + return NoAnnotations(); + } + + private static bool HasAnnotation([MaybeNullWhen(true)] out string s) { s = null; return true; } + + private static bool NoAnnotations() => true; +}"; + var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); + comp.VerifyEmitDiagnostics(); + } + [Fact] [WorkItem(47221, "https://github.com/dotnet/roslyn/issues/47221")] public void PropertyAccessorWithNullableContextAttribute_01() From a92fefb42662102815c79eb5b65325b197653503 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Thu, 8 Oct 2020 16:04:19 -0700 Subject: [PATCH 2/7] Don't split unnecessarily on !expr --- src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 3996a61b382c3..bc73e7274dc24 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -8617,8 +8617,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: From 93ff57a1aef8afde257c54ffeaae80040e0ff6f7 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Thu, 8 Oct 2020 16:26:28 -0700 Subject: [PATCH 3/7] Split state for constant boolean expressions --- .../Portable/FlowAnalysis/NullableWalker.cs | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index bc73e7274dc24..e48a74592ed16 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2439,6 +2439,7 @@ private bool TryGetReturnType(out TypeWithAnnotations type, out FlowAnalysisAnno } SetResult(node, GetAdjustedResult(type.ToTypeWithState(), slot), type); + SplitIfBooleanConstant(node); return null; } @@ -8155,9 +8156,27 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter return null; } + private void SplitIfBooleanConstant(BoundExpression node) + { + if (node.ConstantValue is { IsBoolean: true }) + { + Split(); + if (node.ConstantValue.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; } @@ -8972,19 +8991,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; } From 7c59ff8d15606cfbf861eaead58e4f0ce68100d7 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 9 Oct 2020 12:42:38 -0700 Subject: [PATCH 4/7] Enforce in some non-constant scenarios that are useful --- .../Portable/FlowAnalysis/NullableWalker.cs | 101 ++++++-- .../Semantics/NullableReferenceTypesTests.cs | 221 +++++++++++++++--- 2 files changed, 265 insertions(+), 57 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index e48a74592ed16..3c56b804bf002 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -609,27 +609,52 @@ void enforceMemberNotNullOnMember(SyntaxNode? syntaxOpt, LocalState state, Metho void enforceMemberNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturnStatement returnStatement) { - if (returnStatement.ExpressionOpt is BoundExpression { ConstantValue: { IsBoolean: true, BooleanValue: bool value } }) + if (pendingReturn.IsConditionalState) { - if (pendingReturn.IsConditionalState) + if (returnStatement.ExpressionOpt is { ConstantValue: { IsBoolean: true, BooleanValue: bool value } }) { - enforceMemberNotNullWhen(returnStatement.Syntax, sense: true, pendingReturn.StateWhenTrue); - enforceMemberNotNullWhen(returnStatement.Syntax, sense: false, pendingReturn.StateWhenFalse); + enforceMemberNotNullWhen(returnStatement.Syntax, sense: value, pendingReturn.State); + return; } - else + + if (!pendingReturn.StateWhenTrue.Reachable || !pendingReturn.StateWhenFalse.Reachable) { - enforceMemberNotNullWhen(returnStatement.Syntax, sense: value, pendingReturn.State); + 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: true, 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 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; @@ -637,16 +662,21 @@ void enforceMemberNotNullWhen(SyntaxNode? syntaxOpt, bool sense, LocalState stat { 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) @@ -770,20 +800,49 @@ void enforceNotNullWhenForPendingReturn(PendingBranch pendingReturn, BoundReturn { var parameters = this.MethodParameters; - if (!parameters.IsEmpty && returnStatement.ExpressionOpt is BoundExpression { ConstantValue: { IsBoolean: true, BooleanValue: bool value } }) + if (!parameters.IsEmpty) { 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) + { + 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]) + { + reportParameterIfBadConditionalState(returnStatement.Syntax, parameter, sense: true, stateWhen: pendingReturn.StateWhenTrue); + reportParameterIfBadConditionalState(returnStatement.Syntax, parameter, sense: false, stateWhen: pendingReturn.StateWhenFalse); + } + } } - else + 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) @@ -823,11 +882,7 @@ void enforceParameterNotNullWhen(SyntaxNode syntax, ImmutableArray throw null!; +} +", MemberNotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); + + c.VerifyDiagnostics( + // (19,13): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + // return field1 == null; // 1 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field1 == null;").WithArguments("field1", "true").WithLocation(19, 13), + // (29,9): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + // return !Init(); // 2, 3 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field1", "true").WithLocation(29, 9), + // (29,9): warning CS8775: Member 'field3' must have a non-null value when exiting with 'true'. + // return !Init(); // 2, 3 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field3", "true").WithLocation(29, 9) + ); + } + + [Fact] + public void MemberNotNullWhenFalse_EnforcedInMethodBody_NonConstant_ButAnalyzable() + { + var c = CreateNullableCompilation(new[] { @" +using System.Diagnostics.CodeAnalysis; +public class C +{ + public string field1; + public string field3; + + C() + { + if (Init()) throw null!; + } + + [MemberNotNullWhen(false, nameof(field1), nameof(field3))] + bool Init() + { + bool b = true; + if (b) + { + return field1 == null; + } + if (b) + { + return field1 != null; // 1 + } + if (b) + { + return Init(); + } + return !Init(); // 2, 3 + } + + bool M([MaybeNullWhen(true)]out string s) => throw null!; +} +", MemberNotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); + + c.VerifyDiagnostics( + // (23,13): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + // return field1 != null; // 1 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field1 != null;").WithArguments("field1", "true").WithLocation(23, 13), + // (29,9): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + // return !Init(); // 2, 3 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field1", "true").WithLocation(29, 9), + // (29,9): warning CS8775: Member 'field3' must have a non-null value when exiting with 'true'. + // return !Init(); // 2, 3 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field3", "true").WithLocation(29, 9) + ); + } + [Fact] public void MemberNotNullWhenTrue_WithMemberNotNull() { @@ -24350,13 +24452,17 @@ bool IsInit2 { get { - return field2 == null; + return field2 == null; // 1 } } } ", MemberNotNullWhenAttributeDefinition }, parseOptions: TestOptions.Regular9); - c.VerifyDiagnostics(); + c.VerifyDiagnostics( + // (21,13): warning CS8775: Member 'field2' must have a non-null value when exiting with 'true'. + // return field2 == null; // 1 + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field2 == null;").WithArguments("field2", "true").WithLocation(21, 13) + ); } [Fact] @@ -37021,13 +37127,13 @@ static bool TryGetValue(TYPE x, [MaybeNullWhen(true)] out TYPE y) static bool TryGetValue2(TYPE x, [MaybeNullWhen(true)] out TYPE y) { y = x; - return y != null; + return y != null; // 1 } static bool TryGetValue3(TYPE x, [MaybeNullWhen(false)] out TYPE y) { y = x; - return y == null; + return y == null; // 2 } static bool TryGetValue4(TYPE x, [MaybeNullWhen(false)] out TYPE y) @@ -37038,7 +37144,14 @@ static bool TryGetValue4(TYPE x, [MaybeNullWhen(false)] out TYPE y) } "; var comp = CreateNullableCompilation(new[] { MaybeNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics(); + comp.VerifyDiagnostics( + // (18,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return y != null; // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null;").WithArguments("y", "false").WithLocation(18, 9), + // (24,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. + // return y == null; // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y == null;").WithArguments("y", "true").WithLocation(24, 9) + ); } [Theory] @@ -37071,13 +37184,13 @@ static bool TryGetValue([AllowNull] TYPE x, [MaybeNullWhen(true)] out TYPE y) static bool TryGetValue2([AllowNull] TYPE x, [MaybeNullWhen(true)] out TYPE y) { y = x; - return y != null; + return y != null; // 1 } static bool TryGetValue3([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) { y = x; - return y == null; + return y == null; // 2 } static bool TryGetValue4([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) @@ -37088,7 +37201,14 @@ static bool TryGetValue4([AllowNull] TYPE x, [MaybeNullWhen(false)] out TYPE y) } "; var comp = CreateNullableCompilation(new[] { AllowNullAttributeDefinition, MaybeNullAttributeDefinition, MaybeNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics(); + comp.VerifyDiagnostics( + // (24,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return y != null; // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null;").WithArguments("y", "false").WithLocation(24, 9), + // (30,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. + // return y == null; // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y == null;").WithArguments("y", "true").WithLocation(30, 9) + ); } [Theory] @@ -37144,7 +37264,7 @@ bool TryGetValue([MaybeNullWhen(true)] out string y) bool TryGetValue2([MaybeNullWhen(true)] out string y) { - return !TryGetValueCore(out y); + return !TryGetValueCore(out y); // 1 } bool TryGetValueCore([NotNullWhen(false)] out string? y) @@ -37154,7 +37274,11 @@ bool TryGetValueCore([NotNullWhen(false)] out string? y) } "; var comp = CreateNullableCompilation(new[] { NotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition, source }); - comp.VerifyDiagnostics(); + comp.VerifyDiagnostics( + // (15,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return !TryGetValueCore(out y); // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return !TryGetValueCore(out y);").WithArguments("y", "false").WithLocation(15, 9) + ); } [Fact, WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")] @@ -37217,20 +37341,32 @@ static bool TryGetValue2([NotNullWhen(true)] out TYPE y) return y != null; } + static bool TryGetValue2B([NotNullWhen(true)] out TYPE y) + { + y = null; + return y == null; // 2 + } + static bool TryGetValue3([NotNullWhen(false)] out TYPE y) { y = null; return y == null; } + static bool TryGetValue3B([NotNullWhen(false)] out TYPE y) + { + y = null; + return y != null; // 3 + } + static bool TryGetValue4([NotNull] TYPE x, [NotNullWhen(false)] out TYPE y) { y = null; if (y != null) { - return true; // 2 + return true; // 4 } - return false; // 3, 4 + return false; // 5, 6 } } "; @@ -37239,15 +37375,21 @@ static bool TryGetValue4([NotNull] TYPE x, [NotNullWhen(false)] out TYPE y) // (15,13): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. // return true; // 1 Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return true;").WithArguments("y", "true").WithLocation(15, 13), - // (37,13): warning CS8777: Parameter 'x' must have a non-null value when exiting. - // return true; // 2 - Diagnostic(ErrorCode.WRN_ParameterDisallowsNull, "return true;").WithArguments("x").WithLocation(37, 13), - // (39,9): warning CS8777: Parameter 'x' must have a non-null value when exiting. - // return false; // 3, 4 - Diagnostic(ErrorCode.WRN_ParameterDisallowsNull, "return false;").WithArguments("x").WithLocation(39, 9), - // (39,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. - // return false; // 3, 4 - Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return false;").WithArguments("y", "false").WithLocation(39, 9) + // (29,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. + // return y == null; // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y == null;").WithArguments("y", "true").WithLocation(29, 9), + // (41,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return y != null; // 3 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return y != null;").WithArguments("y", "false").WithLocation(41, 9), + // (49,13): warning CS8777: Parameter 'x' must have a non-null value when exiting. + // return true; // 4 + Diagnostic(ErrorCode.WRN_ParameterDisallowsNull, "return true;").WithArguments("x").WithLocation(49, 13), + // (51,9): warning CS8777: Parameter 'x' must have a non-null value when exiting. + // return false; // 5, 6 + Diagnostic(ErrorCode.WRN_ParameterDisallowsNull, "return false;").WithArguments("x").WithLocation(51, 9), + // (51,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return false; // 5, 6 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return false;").WithArguments("y", "false").WithLocation(51, 9) ); } @@ -37343,7 +37485,14 @@ static bool TryGetValueString2(string key, [NotNullWhen(true)] out string? value } "; var comp = CreateNullableCompilation(new[] { MaybeNullWhenAttributeDefinition, NotNullAttributeDefinition, NotNullWhenAttributeDefinition, source.Replace("TYPE", type) }); - comp.VerifyDiagnostics(); + comp.VerifyDiagnostics( + // (17,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'true'. + // return TryGetValue3(out y); // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return TryGetValue3(out y);").WithArguments("y", "true").WithLocation(17, 9), + // (27,9): warning CS8762: Parameter 'y' must have a non-null value when exiting with 'false'. + // return TryGetValue2(out y); // 2 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return TryGetValue2(out y);").WithArguments("y", "false").WithLocation(27, 9) + ); } [Fact, WorkItem(39922, "https://github.com/dotnet/roslyn/issues/39922")] @@ -139769,7 +139918,7 @@ public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126() #nullable enable class C -{ +{ static bool NullWhenFalseA(bool a, [MaybeNullWhen(false)] out string s1) { s1 = null; @@ -139807,14 +139956,14 @@ public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_2() #nullable enable class C -{ +{ static bool M1([MaybeNullWhen(false)] out string s1) { const bool b = true; s1 = null; return b; // 1 } - + static bool M2([MaybeNullWhen(false)] out string s1) { s1 = null; @@ -139841,14 +139990,14 @@ public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_3() #nullable enable class C -{ +{ static bool M1([MaybeNullWhen(false)] out string s1) { const bool b = true; s1 = null; - return b; // 1 + return b; // 1 } - + static bool M2([MaybeNullWhen(false)] out string s1) { const bool b = true; @@ -139866,7 +140015,7 @@ static bool M3([MaybeNullWhen(false)] out string s1) var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); comp.VerifyEmitDiagnostics( // (12,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. - // return b; // 1 + // return b; // 1 Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return b;").WithArguments("s1", "true").WithLocation(12, 9), // (19,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. // return b && true; // 2 @@ -139883,19 +140032,23 @@ public void EnforcedInMethodBody_MaybeNullWhen_Issue_48126_4() #nullable enable class C -{ +{ static bool M1([MaybeNullWhen(false)] out string s1) { return M1(out s1); } - + static bool M2([MaybeNullWhen(false)] out string s1) { - return !M1(out s1); + return !M1(out s1); // 1 } }"; var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); - comp.VerifyEmitDiagnostics(); + comp.VerifyEmitDiagnostics( + // (15,9): warning CS8762: Parameter 's1' must have a non-null value when exiting with 'true'. + // return !M1(out s1); // 1 + Diagnostic(ErrorCode.WRN_ParameterConditionallyDisallowsNull, "return !M1(out s1);").WithArguments("s1", "true").WithLocation(15, 9) + ); } [Fact, WorkItem(48126, "https://github.com/dotnet/roslyn/issues/48126")] From 2acf4ddafe289d8b2497a33b58ab082f08360466 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 9 Oct 2020 17:08:46 -0700 Subject: [PATCH 5/7] Fix sense in diagnostic --- .../Portable/FlowAnalysis/NullableWalker.cs | 2 +- .../Semantics/NullableReferenceTypesTests.cs | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 3c56b804bf002..e5c79cd20278f 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -631,7 +631,7 @@ void enforceMemberNotNullWhenForPendingReturn(PendingBranch pendingReturn, Bound foreach (var memberName in method.NotNullWhenFalseMembers) { - enforceMemberNotNullWhenIfAffected(returnStatement.Syntax, sense: true, method.ContainingType.GetMembers(memberName), pendingReturn.StateWhenFalse, pendingReturn.StateWhenTrue); + enforceMemberNotNullWhenIfAffected(returnStatement.Syntax, sense: false, method.ContainingType.GetMembers(memberName), pendingReturn.StateWhenFalse, pendingReturn.StateWhenTrue); } } } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 939124a1944bd..d8afb42c4ae79 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -24208,15 +24208,15 @@ bool Init() ", MemberNotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); c.VerifyDiagnostics( - // (23,13): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + // (23,13): warning CS8775: Member 'field1' must have a non-null value when exiting with 'false'. // return field1 != null; // 1 - Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field1 != null;").WithArguments("field1", "true").WithLocation(23, 13), - // (29,9): warning CS8775: Member 'field1' must have a non-null value when exiting with 'true'. + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return field1 != null;").WithArguments("field1", "false").WithLocation(23, 13), + // (29,9): warning CS8775: Member 'field1' must have a non-null value when exiting with 'false'. // return !Init(); // 2, 3 - Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field1", "true").WithLocation(29, 9), - // (29,9): warning CS8775: Member 'field3' must have a non-null value when exiting with 'true'. + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field1", "false").WithLocation(29, 9), + // (29,9): warning CS8775: Member 'field3' must have a non-null value when exiting with 'false'. // return !Init(); // 2, 3 - Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field3", "true").WithLocation(29, 9) + Diagnostic(ErrorCode.WRN_MemberNotNullWhen, "return !Init();").WithArguments("field3", "false").WithLocation(29, 9) ); } @@ -140042,6 +140042,11 @@ static bool M2([MaybeNullWhen(false)] out string s1) { return !M1(out s1); // 1 } + + static bool M3([MaybeNullWhen(true)] out string s1) + { + return !M1(out s1); + } }"; var comp = CreateCompilation(new[] { source, MaybeNullWhenAttributeDefinition }); comp.VerifyEmitDiagnostics( From c2602c15406a8d533fde5b6d53eebef1a5199510 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 9 Oct 2020 17:10:50 -0700 Subject: [PATCH 6/7] Clarify test --- .../Test/Semantic/Semantics/NullableReferenceTypesTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index d8afb42c4ae79..d63ae18f29634 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -25633,6 +25633,8 @@ public void NotNullWhenTrue_EnforceInMethodBody_OnConversionToBool() using System.Diagnostics.CodeAnalysis; public class C { + public static implicit operator bool(C? c) => throw null!; + public static bool TryGetValue(C? c, [NotNullWhen(true)] out string? s) { s = null; @@ -25645,8 +25647,6 @@ public static bool TryGetValue2(C c, [NotNullWhen(false)] out string? s) return c; } - public static implicit operator bool(C? c) => throw null!; - static bool TryGetValue3([MaybeNullWhen(false)]out string s) { s = null; From 9ff27bfa8f2cd87c30712bcc15846136d5ff6802 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 16 Oct 2020 15:35:04 -0700 Subject: [PATCH 7/7] Address feedback from Fred --- .../Portable/FlowAnalysis/NullableWalker.cs | 4 ++-- .../Semantics/NullableReferenceTypesTests.cs | 19 +++++++++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index e5c79cd20278f..cfed56447c2ab 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -8213,10 +8213,10 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter private void SplitIfBooleanConstant(BoundExpression node) { - if (node.ConstantValue is { IsBoolean: true }) + if (node.ConstantValue is { IsBoolean: true, BooleanValue: bool booleanValue }) { Split(); - if (node.ConstantValue.BooleanValue) + if (booleanValue) { StateWhenFalse = UnreachableState(); } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index d63ae18f29634..67559081ed77b 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -24151,8 +24151,6 @@ bool Init() } return !Init(); // 2, 3 } - - bool M([MaybeNullWhen(true)]out string s) => throw null!; } ", MemberNotNullWhenAttributeDefinition, MaybeNullWhenAttributeDefinition }); @@ -25228,6 +25226,23 @@ public static bool TryGetValue([NotNullWhen(true)] out string? s) comp.VerifyDiagnostics(); } + [Fact] + public void NotNullWhenTrue_EnforceInMethodBody_ConditionalWithThrow() + { + var source = @" +using System.Diagnostics.CodeAnalysis; +public class C +{ + static bool M([NotNullWhen(true)] object? o) + { + return o == null ? true : throw null!; + } +} +"; + var comp = CreateNullableCompilation(new[] { source, NotNullWhenAttributeDefinition }); + comp.VerifyDiagnostics(); + } + [Fact] public void NotNullWhenTrue_EnforceInMethodBody_WithMaybeNull_CallingObliviousAPI() {