From 3b1d95356a208e6bdd3e6c01ab0f6716aba7306e Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Thu, 31 Oct 2019 17:37:37 -0700 Subject: [PATCH 1/7] Handle dependent slots in pattern-matching null tests. Fixes #39264 --- .../Portable/FlowAnalysis/NullableWalker.cs | 37 +++++++++++++- .../FlowAnalysis/NullableWalker_Patterns.cs | 7 ++- .../NullableReferenceTypesVsPatterns.cs | 51 +++++++++++++++++++ 3 files changed, 91 insertions(+), 4 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 6107b9e9d6477..6475483accc3d 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2717,19 +2717,52 @@ private int LearnFromNullTest(BoundExpression expression, ref LocalState state) // We should not blindly strip conversions here. Tracked by https://github.com/dotnet/roslyn/issues/36164 var expressionWithoutConversion = RemoveConversion(expression, includeExplicitConversions: true).expression; var slot = MakeSlot(expressionWithoutConversion); - return LearnFromNullTest(slot, expressionWithoutConversion.Type, ref state); + + // since we know for sure the slot is null (we just tested it), we know that dependent slots are not + // reachable and therefore can be treated as not null. However, we have not computed the proper + // (inferred) type for the expression, so we cannot compute the correct symbols for the member slots here + // (using the incorrect symbols would result in computing an incorrect default state for them). + // Therefore we do not mark dependent slots not null. See https://github.com/dotnet/roslyn/issues/39624 + return LearnFromNullTest(slot, expressionWithoutConversion.Type, ref state, markDependentSlotsNotNull: false); } - private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalState state) + private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalState state, bool markDependentSlotsNotNull = false) { if (slot > 0 && PossiblyNullableType(expressionType)) { state[slot] = NullableFlowState.MaybeNull; + if (markDependentSlotsNotNull) + { + MarkDependentSlotsNotNull(slot, expressionType, ref state); + } } return slot; } + // If we know for sure that a slot contains a null value, then we know for sure that dependent slots + // are "unreachable" so we might as well treat them as not null. That way when this state is merged + // with another state, those dependent states won't pollute values from the other state. + private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref LocalState state) + { + var variable = variableBySlot[slot]; + foreach (var member in expressionType.GetMembers()) + { + HashSet discardedUseSiteDiagnostics = null; + if ((member.Kind == SymbolKind.Property && !((PropertySymbol)member).IsIndexedProperty || member.Kind == SymbolKind.Field) && + member.RequiresInstanceReceiver() && + AccessCheck.IsSymbolAccessible(member, this._symbol.ContainingType, ref discardedUseSiteDiagnostics)) + { + int childSlot = GetOrCreateSlot(member, slot, true); + if (childSlot > 0) + { + state[childSlot] = NullableFlowState.NotNull; + MarkDependentSlotsNotNull(childSlot, member.GetTypeOrReturnType().Type, ref state); + } + } + } + } + private static BoundExpression SkipReferenceConversions(BoundExpression possiblyConversion) { while (possiblyConversion.Kind == BoundKind.Conversion) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs index 348c29c005095..d9e98b0aea337 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs @@ -98,7 +98,9 @@ private void LearnFromAnyNullPatterns( bool isExplicitNullCheck = cp.Value.ConstantValue == ConstantValue.Null; if (isExplicitNullCheck) { - LearnFromNullTest(inputSlot, inputType, ref this.State); + // Since we're not branching on this null test here, we just infer the top level + // nullability. We'll branch on it later. + LearnFromNullTest(inputSlot, inputType, ref this.State, markDependentSlotsNotNull: false); } break; case BoundDeclarationPattern _: @@ -349,6 +351,7 @@ protected override void VisitSwitchSection(BoundSwitchSection node, bool isLastS case BoundDagNonNullTest t: if (inputSlot > 0) { + MarkDependentSlotsNotNull(inputSlot, inputType, ref this.StateWhenFalse); learnFromNonNullTest(inputSlot, ref this.StateWhenTrue); } gotoNode(p.WhenTrue, this.StateWhenTrue, nodeBelievedReachable); @@ -357,7 +360,7 @@ protected override void VisitSwitchSection(BoundSwitchSection node, bool isLastS case BoundDagExplicitNullTest t: if (inputSlot > 0) { - LearnFromNullTest(inputSlot, inputType, ref this.StateWhenTrue); + LearnFromNullTest(inputSlot, inputType, ref this.StateWhenTrue, markDependentSlotsNotNull: true); learnFromNonNullTest(inputSlot, ref this.StateWhenFalse); } gotoNode(p.WhenTrue, this.StateWhenTrue, nodeBelievedReachable); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs index 1651ad4ff999a..92ab728f54112 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs @@ -1934,5 +1934,56 @@ public interface IOut { } // _ = i switch { 1 => default, _ => x }/*T:T*/; // 6 Diagnostic(ErrorCode.WRN_DefaultExpressionMayIntroduceNullT, "default").WithArguments("T").WithLocation(46, 29)); } + + [Fact] + [WorkItem(39264, "https://github.com/dotnet/roslyn/issues/39264")] + public void IsPatternSplitState_01() + { + CSharpCompilation c = CreateNullableCompilation(@" +#nullable enable +class C +{ + string? field = string.Empty; + + void M1(C c) + { + if (c.field == null) return; + + c.field.ToString(); + } + + void M2(C c) + { + if (c is { field: null }) return; + + c.field.ToString(); + } + + void M3(C c) + { + switch (c) + { + case { field: null }: + break; + default: + c.field.ToString(); + break; + } + } + + void M4(C c) + { + _ = c switch + { + { field: null } => string.Empty, + _ => c.field.ToString(), + }; + } +} +"); + c.VerifyDiagnostics( + ); + } + } } From 704cecde59a3821489ae4c3120c7d0bc704ada86 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 1 Nov 2019 10:52:46 -0700 Subject: [PATCH 2/7] Adjust fix for speculative binding, when we don't have the containing member. --- src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 6475483accc3d..cbeae49a6b6b9 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2749,9 +2749,10 @@ private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref foreach (var member in expressionType.GetMembers()) { HashSet discardedUseSiteDiagnostics = null; + NamedTypeSymbol containingType = this._symbol?.ContainingType; if ((member.Kind == SymbolKind.Property && !((PropertySymbol)member).IsIndexedProperty || member.Kind == SymbolKind.Field) && member.RequiresInstanceReceiver() && - AccessCheck.IsSymbolAccessible(member, this._symbol.ContainingType, ref discardedUseSiteDiagnostics)) + (containingType is null || AccessCheck.IsSymbolAccessible(member, containingType, ref discardedUseSiteDiagnostics))) { int childSlot = GetOrCreateSlot(member, slot, true); if (childSlot > 0) From 81e13dde3e469568125ad85bff79b0a237c9bae4 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 1 Nov 2019 11:39:44 -0700 Subject: [PATCH 3/7] Add a clarifying comment. --- src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 9e909ef36fdf9..7b19326bc0605 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -44,7 +44,7 @@ internal abstract partial class AbstractFlowPass protected readonly Symbol _symbol; From 302959cf1cc0ebea927e7298a789d6955bad398c Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Wed, 4 Dec 2019 13:42:23 -0800 Subject: [PATCH 4/7] Add a test that demonstrates behavior for recursive types. --- .../Portable/FlowAnalysis/NullableWalker.cs | 1 - .../NullableReferenceTypesVsPatterns.cs | 25 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 65b8664dd033c..df03a932a3904 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2730,7 +2730,6 @@ private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalStat // with another state, those dependent states won't pollute values from the other state. private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref LocalState state) { - var variable = variableBySlot[slot]; foreach (var member in expressionType.GetMembers()) { HashSet discardedUseSiteDiagnostics = null; diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs index 12fe4274e51f0..7fdfbf7c93d00 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs @@ -1984,5 +1984,30 @@ void M4(C c) ); } + [Fact] + [WorkItem(39264, "https://github.com/dotnet/roslyn/issues/39264")] + public void IsPatternSplitState_02() + { + CSharpCompilation c = CreateNullableCompilation(@" +#nullable enable +class C +{ + C? c = null; + + void M1(C c, bool b) + { + if (c is { c : null }) + { + if (c.c != null) + { + c.c.c.c.ToString(); + } + } + } +} +"); + c.VerifyDiagnostics( + ); + } } } From 967889149f71a2b2b138317de385cdca867b64db Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 13 Dec 2019 11:24:12 -0800 Subject: [PATCH 5/7] Minor changes per review, and additional tests. --- .../Portable/FlowAnalysis/NullableWalker.cs | 6 +-- .../NullableReferenceTypesVsPatterns.cs | 52 +++++++++++++++++-- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index df03a932a3904..ecb70747e047a 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2703,7 +2703,7 @@ private int LearnFromNullTest(BoundExpression expression, ref LocalState state) var expressionWithoutConversion = RemoveConversion(expression, includeExplicitConversions: true).expression; var slot = MakeSlot(expressionWithoutConversion); - // since we know for sure the slot is null (we just tested it), we know that dependent slots are not + // Since we know for sure the slot is null (we just tested it), we know that dependent slots are not // reachable and therefore can be treated as not null. However, we have not computed the proper // (inferred) type for the expression, so we cannot compute the correct symbols for the member slots here // (using the incorrect symbols would result in computing an incorrect default state for them). @@ -2711,7 +2711,7 @@ private int LearnFromNullTest(BoundExpression expression, ref LocalState state) return LearnFromNullTest(slot, expressionWithoutConversion.Type, ref state, markDependentSlotsNotNull: false); } - private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalState state, bool markDependentSlotsNotNull = false) + private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalState state, bool markDependentSlotsNotNull) { if (slot > 0 && PossiblyNullableType(expressionType)) { @@ -2734,7 +2734,7 @@ private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref { HashSet discardedUseSiteDiagnostics = null; NamedTypeSymbol containingType = this._symbol?.ContainingType; - if ((member.Kind == SymbolKind.Property && !((PropertySymbol)member).IsIndexedProperty || member.Kind == SymbolKind.Field) && + if ((member is PropertySymbol { IsIndexedProperty: false } || member.Kind == SymbolKind.Field) && member.RequiresInstanceReceiver() && (containingType is null || AccessCheck.IsSymbolAccessible(member, containingType, ref discardedUseSiteDiagnostics))) { diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs index 7fdfbf7c93d00..a0e892667cfee 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs @@ -1943,6 +1943,7 @@ public void IsPatternSplitState_01() class C { string? field = string.Empty; + string? otherField = string.Empty; void M1(C c) { @@ -1950,7 +1951,7 @@ void M1(C c) c.field.ToString(); } - + void M2(C c) { if (c is { field: null }) return; @@ -1978,9 +1979,19 @@ void M4(C c) _ => c.field.ToString(), }; } + + void M5(C c) + { + if (c is { field: null }) return; + + c.otherField.ToString(); // W + } } "); c.VerifyDiagnostics( + // (47,9): warning CS8602: Dereference of a possibly null reference. + // c.otherField.ToString(); // W + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c.otherField").WithLocation(47, 9) ); } @@ -1994,9 +2005,9 @@ class C { C? c = null; - void M1(C c, bool b) + void M1(C c) { - if (c is { c : null }) + if (c is { c: null }) { if (c.c != null) { @@ -2009,5 +2020,40 @@ void M1(C c, bool b) c.VerifyDiagnostics( ); } + + [Fact] + [WorkItem(39264, "https://github.com/dotnet/roslyn/issues/39264")] + public void IsPatternSplitState_03() + { + CSharpCompilation c = CreateNullableCompilation(@" +#nullable enable +public class C { + C? c = null; + + public static void Main() + { + C c = new C(); + M1(c, new C()); + } + + static void M1(C c, C c2) + { + if (c is { c : null } && c2 is { c: null }) + { + c.c = c2; + if (c.c != null) + { + c.c.c.ToString(); // warning + } + } + } +} +"); + c.VerifyDiagnostics( + // (19,17): warning CS8602: Dereference of a possibly null reference. + // c.c.c.ToString(); // warning + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c.c.c").WithLocation(19, 17) + ); + } } } From 23ad085fd7f3dc1ba0c510d5ff3d0ef3a0897ef6 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Thu, 2 Jan 2020 17:55:48 -0800 Subject: [PATCH 6/7] Address review comments. --- .../Portable/FlowAnalysis/NullableWalker.cs | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index ecb70747e047a..00454bb1c3941 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2728,9 +2728,12 @@ private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalStat // If we know for sure that a slot contains a null value, then we know for sure that dependent slots // are "unreachable" so we might as well treat them as not null. That way when this state is merged // with another state, those dependent states won't pollute values from the other state. - private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref LocalState state) + private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref LocalState state, int depth = 2) { - foreach (var member in expressionType.GetMembers()) + if (depth <= 0) + return; + + foreach (var member in getMembers(expressionType)) { HashSet discardedUseSiteDiagnostics = null; NamedTypeSymbol containingType = this._symbol?.ContainingType; @@ -2742,10 +2745,42 @@ private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref if (childSlot > 0) { state[childSlot] = NullableFlowState.NotNull; - MarkDependentSlotsNotNull(childSlot, member.GetTypeOrReturnType().Type, ref state); + MarkDependentSlotsNotNull(childSlot, member.GetTypeOrReturnType().Type, ref state, depth - 1); } } } + + IEnumerable getMembers(TypeSymbol type) + { + // First, return the direct members + foreach (var member in type.GetMembers()) + yield return member; + + // All types inherit members from their effective bases + for (NamedTypeSymbol baseType = effectiveBase(type); !(baseType is null); baseType = baseType.BaseTypeNoUseSiteDiagnostics) + foreach (var member in baseType.GetMembers()) + yield return member; + + // Interfaces and type parameters inherit from their effective interfaces + foreach (NamedTypeSymbol interfaceType in inheritedInterfaces(type)) + foreach (var member in interfaceType.GetMembers()) + yield return member; + + yield break; + + NamedTypeSymbol effectiveBase(TypeSymbol type) => type switch + { + TypeParameterSymbol tp => tp.EffectiveBaseClassNoUseSiteDiagnostics, + var t => t.BaseTypeNoUseSiteDiagnostics, + }; + + ImmutableArray inheritedInterfaces(TypeSymbol type) => type switch + { + TypeParameterSymbol tp => tp.AllEffectiveInterfacesNoUseSiteDiagnostics, + { TypeKind: TypeKind.Interface } => type.AllInterfacesNoUseSiteDiagnostics, + _ => ImmutableArray.Empty, + }; + } } private static BoundExpression SkipReferenceConversions(BoundExpression possiblyConversion) From 20cb6d7845b5b37961c0ed2d65c8b956ad31a195 Mon Sep 17 00:00:00 2001 From: Neal Gafter Date: Fri, 3 Jan 2020 11:53:01 -0800 Subject: [PATCH 7/7] Changes per review. --- .../CSharp/Portable/FlowAnalysis/NullableWalker.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 00454bb1c3941..0d04f7ba8fb49 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2750,7 +2750,7 @@ private void MarkDependentSlotsNotNull(int slot, TypeSymbol expressionType, ref } } - IEnumerable getMembers(TypeSymbol type) + static IEnumerable getMembers(TypeSymbol type) { // First, return the direct members foreach (var member in type.GetMembers()) @@ -2768,13 +2768,13 @@ IEnumerable getMembers(TypeSymbol type) yield break; - NamedTypeSymbol effectiveBase(TypeSymbol type) => type switch + static NamedTypeSymbol effectiveBase(TypeSymbol type) => type switch { TypeParameterSymbol tp => tp.EffectiveBaseClassNoUseSiteDiagnostics, var t => t.BaseTypeNoUseSiteDiagnostics, }; - ImmutableArray inheritedInterfaces(TypeSymbol type) => type switch + static ImmutableArray inheritedInterfaces(TypeSymbol type) => type switch { TypeParameterSymbol tp => tp.AllEffectiveInterfacesNoUseSiteDiagnostics, { TypeKind: TypeKind.Interface } => type.AllInterfacesNoUseSiteDiagnostics,