From 02f42615f5f02fe32f2a2cc341b7fb9f1d78b352 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Wed, 18 Jul 2018 13:26:19 -0700 Subject: [PATCH 1/7] `is` operator informs nullability analysis --- docs/features/nullable-reference-types.md | 10 +- .../Portable/FlowAnalysis/NullableWalker.cs | 135 +++++--- .../FlowAnalysis/PreciseAbstractFlowPass.cs | 3 +- .../Semantics/NullableReferenceTypesTests.cs | 312 ++++++++++++++++++ 4 files changed, 408 insertions(+), 52 deletions(-) diff --git a/docs/features/nullable-reference-types.md b/docs/features/nullable-reference-types.md index 61e094383145e..04a43b3eae99b 100644 --- a/docs/features/nullable-reference-types.md +++ b/docs/features/nullable-reference-types.md @@ -75,9 +75,17 @@ Flow analysis is used to infer the nullability of variables within executable co ### Warnings _Describe set of warnings. Differentiate W warnings._ +If the analysis determines that a null check always (or never) passes, a hidden warning is produced. For example: `"string" is null`. ### Null tests -_Describe the set of tests that affect flow state._ +A number of null checks affect the flow state when tested for: +- comparisons to `null`: `x == null` and `x != null` +- `is` operator: `x is null`, `x is K` (where `K` is a constant), `x is string`, `x is string s` + +Invocation to methods annotated with the following attributes will also affect flow analysis: +- `[NotNullWhenTrue]` (e.g. `TryGetValue`) and `[NotNullWhenFalse]` (e.g. `string.IsNullOrEmpty`) +- `[EnsuresNotNull]` (e.g. `ThrowIfNull`) +- `[AssertsTrue]` (e.g. `Debug.Assert`) and `[AssertsFalse]` ## `default` `default(T)` is `T?` if `T` is a reference type. diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 238bf2cabb646..d4cea5390a0f2 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -766,69 +766,78 @@ private void EnterParameter(ParameterSymbol parameter, TypeSymbolWithAnnotations public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node) { - // PROTOTYPE(NullableReferenceTypes): Move these asserts to base class. - Debug.Assert(!IsConditionalState); - - // Create slot when the state is unconditional since EnsureCapacity should be - // called on all fields and that is simpler if state is limited to this.State. - int slot = -1; - if (this.State.Reachable) - { - var pattern = node.Pattern; - // PROTOTYPE(NullableReferenceTypes): Handle patterns that ensure x is not null: - // x is T y // where T is not inferred via var - // x is K // where K is a constant other than null - if (pattern.Kind == BoundKind.ConstantPattern && ((BoundConstantPattern)pattern).ConstantValue?.IsNull == true) - { - slot = MakeSlot(node.Expression); - if (slot > 0) - { - Normalize(ref this.State); - } - } - } - - var result = base.VisitIsPatternExpression(node); + VisitRvalue(node.Expression); + var expressionResultType = this._resultType; - Debug.Assert(IsConditionalState); - if (slot > 0) - { - this.StateWhenTrue[slot] = false; - this.StateWhenFalse[slot] = true; - } + VisitPattern(node.Expression, expressionResultType, node.Pattern); SetResult(node); - return result; + return node; } - public override void VisitPattern(BoundExpression expression, BoundPattern pattern) - { - base.VisitPattern(expression, pattern); - var whenFail = StateWhenFalse; - SetState(StateWhenTrue); - AssignPatternVariables(pattern); - SetConditionalState(this.State, whenFail); - SetUnknownResultNullability(); - } - - private void AssignPatternVariables(BoundPattern pattern) + /// + /// Examples: + /// `x is Point p` + /// `switch (x) ... case Point p:` // PROTOTYPE(NullableReferenceTypes): not yet handled + /// + /// If the expression is trackable, we'll return with different null-states for that expression in the two conditional states. + /// If the patterns is a `var` pattern, we'll also have re-inferred the `var` type with nullability and + /// updated the state for that declared local. + /// + private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations expressionResultType, BoundPattern pattern) { + bool? nullKnowledge = null; switch (pattern.Kind) { - case BoundKind.DeclarationPattern: - // PROTOTYPE(NullableReferenceTypes): Handle. - break; - case BoundKind.WildcardPattern: - break; case BoundKind.ConstantPattern: + // If the constant is null, the pattern tells us the expression is null. + // If the constant is not null, the pattern tells us the expression is not null. + // If there is no constant, we don't know. + nullKnowledge = ((BoundConstantPattern)pattern).ConstantValue?.IsNull; + break; + case BoundKind.DeclarationPattern: + var declarationPattern = (BoundDeclarationPattern)pattern; + if (declarationPattern.IsVar) { - var pat = (BoundConstantPattern)pattern; - this.VisitRvalue(pat.Value); - break; + // The result type and state of the expression carry into the variable declared by var pattern + Symbol variable = declarationPattern.Variable; + _variableTypes[variable] = expressionResultType; + TrackNullableStateForAssignment(expression, expressionResultType, GetOrCreateSlot(variable), expressionResultType); + } + else + { + nullKnowledge = false; // the pattern tells us the expression is not null } - default: break; } + + int slot = -1; + if (nullKnowledge != null) + { + // Create slot when the state is unconditional since EnsureCapacity should be + // called on all fields and that is simpler if state is limited to this.State. + slot = MakeSlot(expression); + if (slot > 0) + { + Normalize(ref this.State); + } + } + + base.VisitPattern(expression, pattern); + Debug.Assert(IsConditionalState); + + // PROTOTYPE(NullableReferenceTypes): We should only report such + // diagnostics for locals that are set or checked explicitly within this method. + if (expressionResultType?.IsNullable == false && nullKnowledge == true) + { + ReportStaticNullCheckingDiagnostics(ErrorCode.HDN_NullCheckIsProbablyAlwaysFalse, pattern.Syntax); + } + + if (slot > 0 && nullKnowledge != null) + { + this.StateWhenTrue[slot] = !nullKnowledge; + this.StateWhenFalse[slot] = nullKnowledge; + } } protected override BoundNode VisitReturnStatementNoAdjust(BoundReturnStatement node) @@ -3663,8 +3672,29 @@ public override BoundNode VisitDefaultExpression(BoundDefaultExpression node) public override BoundNode VisitIsOperator(BoundIsOperator node) { + Debug.Assert(!this.IsConditionalState); + + int slot = -1; + var operand = node.Operand; + if (operand.Type.IsReferenceType) + { + slot = MakeSlot(operand); + if (slot > 0) + { + Normalize(ref this.State); + } + } + var result = base.VisitIsOperator(node); Debug.Assert(node.Type.SpecialType == SpecialType.System_Boolean); + + if (slot > 0) + { + Split(); + this.StateWhenTrue[slot] = true; + this.StateWhenFalse[slot] = false; + } + SetResult(node); return result; } @@ -4185,6 +4215,11 @@ internal void EnsureCapacity(int capacity) _notNull.EnsureCapacity(capacity); } + /// + /// Use `true` value for not-null. + /// Use `false` value for maybe-null. + /// Use `null` value for unknown. + /// internal bool? this[int slot] { get diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs index 2711848153486..cb05df44a796a 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/PreciseAbstractFlowPass.cs @@ -950,9 +950,10 @@ public override BoundNode VisitPassByCopy(BoundPassByCopy node) public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node) { + Debug.Assert(!IsConditionalState); VisitRvalue(node.Expression); VisitPattern(node.Expression, node.Pattern); - return null; + return node; } public virtual void VisitPattern(BoundExpression expression, BoundPattern pattern) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index f29b054c6bccf..c4ec28aa209f0 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -944,6 +944,26 @@ public NonNullTypesAttribute(bool flag = true) { } comp.VerifyDiagnostics(); } + [Fact(Skip = "PROTOTYPE(NullableReferenceTypes): stack overflow")] + public void NonNullTypes_DoNotWarnInsideAttributes() + { + string source = @" +using System.Runtime.CompilerServices; +[NonNullTypes(true)] +class C +{ + [System.Obsolete(D.M1(D.M2()))] + class D + { + static void M1(string x) => throw null; + static string? M2() => throw null; + } +} +"; + var comp = CreateCompilation(source + NonNullTypesAttributesDefinition, parseOptions: TestOptions.Regular8); + comp.VerifyDiagnostics(); + } + [Fact] public void NonNullTypes_AttributeDefinedMultipleTimes() { @@ -11715,6 +11735,298 @@ void Test1(object x1, object? y1) ); } + [Fact] + public void ConditionalBranching_Is_ReferenceType() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x is C) + { + x.ToString(); + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (12,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(12, 13) + ); + } + + [Fact] + public void ConditionalBranching_Is_GenericType() + { + CSharpCompilation c = CreateCompilation(@" +class Base { } +class C : Base +{ + void Test(C? x) where T : Base + { + if (x is T) + { + x.ToString(); + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (13,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsConstantPattern_Null() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x is null) + { + x.ToString(); // warn + } + else + { + x.ToString(); + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (8,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(8, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsConstantPattern_NonNull() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + const string nonNullConstant = ""hello""; + if (x is nonNullConstant) + { + x.ToString(); + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (13,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsConstantPattern_NullConstant() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + const string? nullConstant = null; + if (x is nullConstant) + { + x.ToString(); // warn + } + else + { + x.ToString(); + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (9,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(9, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsConstantPattern_Null_AlreadyTestedAsNonNull() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x != null) + { + if (x is null) // hidden + { + x.ToString(); // warn + } + else + { + x.ToString(); + } + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (8,22): hidden CS8606: Result of the comparison is possibly always false. + // if (x is null) // hidden + Diagnostic(ErrorCode.HDN_NullCheckIsProbablyAlwaysFalse, "null").WithLocation(8, 22), + // (10,17): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(10, 17) + ); + } + + [Fact] + public void ConditionalBranching_IsConstantPattern_Null_AlreadyTestedAsNull() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x == null) + { + if (x is null) + { + x.ToString(); // warn + } + else + { + x.ToString(); + } + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (10,17): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(10, 17) + ); + } + + [Fact] + public void ConditionalBranching_IsDeclarationPattern() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x is C c) + { + x.ToString(); + c.ToString(); + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (13,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsVarDeclarationPattern() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x is var c) + { + x.ToString(); // warn + c /*T:object?*/ .ToString(); // warn + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyTypes(); + c.VerifyDiagnostics( + // (8,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(8, 13), + // (9,13): warning CS8602: Possible dereference of a null reference. + // c /*T:object?*/ .ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(9, 13), + // (13,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13) + ); + } + + [Fact] + public void ConditionalBranching_IsVarDeclarationPattern_AlreadyTestedAsNonNull() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (x != null) + { + if (x is var c) + { + c /*T:object!*/ .ToString(); + c = null; // warn + } + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyTypes(); + c.VerifyDiagnostics( + // (11,21): warning CS8600: Converting null literal or possible null value to non-nullable type. + // c = null; // warn + Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(11, 21) + ); + } + [Fact] public void ConditionalOperator_01() { From 9f0d2d83a2583481531ea41a46512452641283cd Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Thu, 19 Jul 2018 07:20:22 -0700 Subject: [PATCH 2/7] Fixup doc --- docs/features/nullable-reference-types.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/features/nullable-reference-types.md b/docs/features/nullable-reference-types.md index 04a43b3eae99b..7c55d083a78a6 100644 --- a/docs/features/nullable-reference-types.md +++ b/docs/features/nullable-reference-types.md @@ -75,14 +75,14 @@ Flow analysis is used to infer the nullability of variables within executable co ### Warnings _Describe set of warnings. Differentiate W warnings._ -If the analysis determines that a null check always (or never) passes, a hidden warning is produced. For example: `"string" is null`. +If the analysis determines that a null check always (or never) passes, a hidden diagnostic is produced. For example: `"string" is null`. ### Null tests A number of null checks affect the flow state when tested for: - comparisons to `null`: `x == null` and `x != null` - `is` operator: `x is null`, `x is K` (where `K` is a constant), `x is string`, `x is string s` -Invocation to methods annotated with the following attributes will also affect flow analysis: +Invocation of methods annotated with the following attributes will also affect flow analysis: - `[NotNullWhenTrue]` (e.g. `TryGetValue`) and `[NotNullWhenFalse]` (e.g. `string.IsNullOrEmpty`) - `[EnsuresNotNull]` (e.g. `ThrowIfNull`) - `[AssertsTrue]` (e.g. `Debug.Assert`) and `[AssertsFalse]` From bd9c0d5b2c3ec07ee3771911450a36f019f717a1 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 20 Jul 2018 00:16:07 -0700 Subject: [PATCH 3/7] Fix two regressions --- .../CSharp/Portable/FlowAnalysis/NullableWalker.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index d4cea5390a0f2..0acf1d7bd1f50 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -801,8 +801,12 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations { // The result type and state of the expression carry into the variable declared by var pattern Symbol variable = declarationPattern.Variable; - _variableTypes[variable] = expressionResultType; - TrackNullableStateForAssignment(expression, expressionResultType, GetOrCreateSlot(variable), expressionResultType); + // No variable declared for discard (`i is var _`) + if ((object)variable != null) + { + _variableTypes[variable] = expressionResultType; + TrackNullableStateForAssignment(expression, expressionResultType, GetOrCreateSlot(variable), expressionResultType); + } } else { @@ -3676,7 +3680,7 @@ public override BoundNode VisitIsOperator(BoundIsOperator node) int slot = -1; var operand = node.Operand; - if (operand.Type.IsReferenceType) + if (operand.Type?.IsReferenceType == true) { slot = MakeSlot(operand); if (slot > 0) From 607c155f9e325fa34c339e5ca65c37b324b6beb5 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 20 Jul 2018 00:43:57 -0700 Subject: [PATCH 4/7] Address PR feedback --- .../Portable/FlowAnalysis/NullableWalker.cs | 3 +- .../Semantics/NullableReferenceTypesTests.cs | 157 ++++++++++++++++++ 2 files changed, 159 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 0acf1d7bd1f50..38c4b176f026e 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -827,6 +827,7 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations } } + Debug.Assert(!IsConditionalState); base.VisitPattern(expression, pattern); Debug.Assert(IsConditionalState); @@ -3680,7 +3681,7 @@ public override BoundNode VisitIsOperator(BoundIsOperator node) int slot = -1; var operand = node.Operand; - if (operand.Type?.IsReferenceType == true) + if (operand.Type?.IsValueType == false) { slot = MakeSlot(operand); if (slot > 0) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index c4ec28aa209f0..bd50cc3e60151 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -11790,6 +11790,136 @@ void Test(C? x) where T : Base ); } + [Fact] + public void ConditionalBranching_Is_UnconstrainedGenericType() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + static void F(T t, object? o) + { + if (o is T) + { + o.ToString(); + } + else + { + o.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (12,13): warning CS8602: Possible dereference of a null reference. + // o.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "o").WithLocation(12, 13) + ); + } + + [Fact] + public void ConditionalBranching_Is_StructConstrainedGenericType() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + static void F(T t, object? o) where T : struct + { + if (o is T) + { + o.ToString(); + } + else + { + o.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (12,13): warning CS8602: Possible dereference of a null reference. + // o.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "o").WithLocation(12, 13) + ); + } + + [Fact] + public void ConditionalBranching_Is_ClassConstrainedGenericType() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + static void F(T t, object? o) where T : class + { + if (o is T) + { + o.ToString(); + } + else + { + o.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (12,13): warning CS8602: Possible dereference of a null reference. + // o.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "o").WithLocation(12, 13) + ); + } + + [Fact] + public void ConditionalBranching_Is_UnconstrainedGenericOperand() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + static void F(T t, object? o) + { + if (t is string) t.ToString(); + if (t is string s) { t.ToString(); s.ToString(); } + if (t != null) t.ToString(); + } +} +", parseOptions: TestOptions.Regular8); + + // PROTOTYPE(NullableReferenceTypes): even unconstrained generic types can be known to be non-null + c.VerifyDiagnostics( + // (6,26): warning CS8602: Possible dereference of a null reference. + // if (t is string) t.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t").WithLocation(6, 26), + // (7,30): warning CS8602: Possible dereference of a null reference. + // if (t is string s) { t.ToString(); s.ToString(); } + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t").WithLocation(7, 30), + // (8,24): warning CS8602: Possible dereference of a null reference. + // if (t != null) t.ToString(); + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "t").WithLocation(8, 24) + ); + } + + [Fact] + public void ConditionalBranching_Is_NullOperand() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + static void F(object? o) + { + if (null is string) return; + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (6,13): warning CS0184: The given expression is never of the provided ('string') type + // if (null is string) return; + Diagnostic(ErrorCode.WRN_IsAlwaysFalse, "null is string").WithArguments("string").WithLocation(6, 13) + ); + } + [Fact] public void ConditionalBranching_IsConstantPattern_Null() { @@ -11817,6 +11947,33 @@ void Test(object? x) ); } + [Fact] + public void ConditionalBranching_IsConstantPattern_NullInverted() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + if (!(x is null)) + { + x.ToString(); + } + else + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (12,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(12, 13) + ); + } + [Fact] public void ConditionalBranching_IsConstantPattern_NonNull() { From ddda977a97f725c967eed518c709a69b583c2ca5 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 20 Jul 2018 01:28:48 -0700 Subject: [PATCH 5/7] Rename local --- .../Portable/FlowAnalysis/NullableWalker.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 38c4b176f026e..03157e5675258 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -786,14 +786,14 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node /// private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations expressionResultType, BoundPattern pattern) { - bool? nullKnowledge = null; + bool? isNull = null; // the pattern tells us the expression (1) is null, (2) isn't null, or (3) we don't know. switch (pattern.Kind) { case BoundKind.ConstantPattern: // If the constant is null, the pattern tells us the expression is null. // If the constant is not null, the pattern tells us the expression is not null. // If there is no constant, we don't know. - nullKnowledge = ((BoundConstantPattern)pattern).ConstantValue?.IsNull; + isNull = ((BoundConstantPattern)pattern).ConstantValue?.IsNull; break; case BoundKind.DeclarationPattern: var declarationPattern = (BoundDeclarationPattern)pattern; @@ -810,13 +810,13 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations } else { - nullKnowledge = false; // the pattern tells us the expression is not null + isNull = false; // the pattern tells us the expression is not null } break; } int slot = -1; - if (nullKnowledge != null) + if (isNull != null) { // Create slot when the state is unconditional since EnsureCapacity should be // called on all fields and that is simpler if state is limited to this.State. @@ -833,15 +833,15 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations // PROTOTYPE(NullableReferenceTypes): We should only report such // diagnostics for locals that are set or checked explicitly within this method. - if (expressionResultType?.IsNullable == false && nullKnowledge == true) + if (expressionResultType?.IsNullable == false && isNull == true) { ReportStaticNullCheckingDiagnostics(ErrorCode.HDN_NullCheckIsProbablyAlwaysFalse, pattern.Syntax); } - if (slot > 0 && nullKnowledge != null) + if (slot > 0 && isNull != null) { - this.StateWhenTrue[slot] = !nullKnowledge; - this.StateWhenFalse[slot] = nullKnowledge; + this.StateWhenTrue[slot] = !isNull; + this.StateWhenFalse[slot] = isNull; } } From 661e4f1ac28f2878440f544a765e5d1af61462f0 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Fri, 20 Jul 2018 08:52:40 -0700 Subject: [PATCH 6/7] Address PR feedback (2) --- .../Portable/FlowAnalysis/NullableWalker.cs | 2 +- .../Semantics/NullableReferenceTypesTests.cs | 29 ++++++++++++++----- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 03157e5675258..d7636df83da34 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -781,7 +781,7 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node /// `switch (x) ... case Point p:` // PROTOTYPE(NullableReferenceTypes): not yet handled /// /// If the expression is trackable, we'll return with different null-states for that expression in the two conditional states. - /// If the patterns is a `var` pattern, we'll also have re-inferred the `var` type with nullability and + /// If the pattern is a `var` pattern, we'll also have re-inferred the `var` type with nullability and /// updated the state for that declared local. /// private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations expressionResultType, BoundPattern pattern) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index bd50cc3e60151..69231be44d1bf 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -12131,12 +12131,21 @@ void Test(object? x) { if (x is var c) { - x.ToString(); // warn - c /*T:object?*/ .ToString(); // warn + x.ToString(); // warn 1 + c /*T:object?*/ .ToString(); // warn 2 } else { - x.ToString(); // warn + x.ToString(); // warn 3 + } + + if (x is var _) + { + x.ToString(); // warn 4 + } + else + { + x.ToString(); // warn 5 } } } @@ -12145,14 +12154,20 @@ void Test(object? x) c.VerifyTypes(); c.VerifyDiagnostics( // (8,13): warning CS8602: Possible dereference of a null reference. - // x.ToString(); // warn + // x.ToString(); // warn 1 Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(8, 13), // (9,13): warning CS8602: Possible dereference of a null reference. - // c /*T:object?*/ .ToString(); // warn + // c /*T:object?*/ .ToString(); // warn 2 Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c").WithLocation(9, 13), // (13,13): warning CS8602: Possible dereference of a null reference. - // x.ToString(); // warn - Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13) + // x.ToString(); // warn 3 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(13, 13), + // (18,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn 4 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(18, 13), + // (22,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn 5 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(22, 13) ); } From 0503021142e6afd9aa1ae2b570aebb81dbadfa55 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Mon, 23 Jul 2018 11:23:12 -0700 Subject: [PATCH 7/7] Address more PR feedback --- .../Portable/FlowAnalysis/NullableWalker.cs | 2 +- .../Semantics/NullableReferenceTypesTests.cs | 34 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index d7636df83da34..1962f594735e8 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -815,6 +815,7 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations break; } + Debug.Assert(!IsConditionalState); int slot = -1; if (isNull != null) { @@ -827,7 +828,6 @@ private void VisitPattern(BoundExpression expression, TypeSymbolWithAnnotations } } - Debug.Assert(!IsConditionalState); base.VisitPattern(expression, pattern); Debug.Assert(IsConditionalState); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 69231be44d1bf..35c940532964c 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -11796,7 +11796,7 @@ public void ConditionalBranching_Is_UnconstrainedGenericType() CSharpCompilation c = CreateCompilation(@" class C { - static void F(T t, object? o) + static void F(object? o) { if (o is T) { @@ -11823,7 +11823,7 @@ public void ConditionalBranching_Is_StructConstrainedGenericType() CSharpCompilation c = CreateCompilation(@" class C { - static void F(T t, object? o) where T : struct + static void F(object? o) where T : struct { if (o is T) { @@ -11850,7 +11850,7 @@ public void ConditionalBranching_Is_ClassConstrainedGenericType() CSharpCompilation c = CreateCompilation(@" class C { - static void F(T t, object? o) where T : class + static void F(object? o) where T : class { if (o is T) { @@ -12030,9 +12030,37 @@ void Test(object? x) ); } + [Fact] + public void ConditionalBranching_IsConstantPattern_NonConstant() + { + CSharpCompilation c = CreateCompilation(@" +class C +{ + void Test(object? x) + { + string nonConstant = ""hello""; + if (x is nonConstant) + { + x.ToString(); // warn + } + } +} +", parseOptions: TestOptions.Regular8); + + c.VerifyDiagnostics( + // (7,18): error CS0150: A constant value is expected + // if (x is nonConstant) + Diagnostic(ErrorCode.ERR_ConstantExpected, "nonConstant").WithLocation(7, 18), + // (9,13): warning CS8602: Possible dereference of a null reference. + // x.ToString(); // warn + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x").WithLocation(9, 13) + ); + } + [Fact] public void ConditionalBranching_IsConstantPattern_Null_AlreadyTestedAsNonNull() { + // PROTOTYPE(NullableReferenceTypes): confirm that we want such hidden warnings CSharpCompilation c = CreateCompilation(@" class C {