diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index a9b73cd60c6c3..35f79128369a8 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; diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 986369928f2a7..0d04f7ba8fb49 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -2702,19 +2702,87 @@ 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) { 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, int depth = 2) + { + if (depth <= 0) + return; + + foreach (var member in getMembers(expressionType)) + { + HashSet discardedUseSiteDiagnostics = null; + NamedTypeSymbol containingType = this._symbol?.ContainingType; + if ((member is PropertySymbol { IsIndexedProperty: false } || member.Kind == SymbolKind.Field) && + member.RequiresInstanceReceiver() && + (containingType is null || AccessCheck.IsSymbolAccessible(member, containingType, ref discardedUseSiteDiagnostics))) + { + int childSlot = GetOrCreateSlot(member, slot, true); + if (childSlot > 0) + { + state[childSlot] = NullableFlowState.NotNull; + MarkDependentSlotsNotNull(childSlot, member.GetTypeOrReturnType().Type, ref state, depth - 1); + } + } + } + + static 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; + + static NamedTypeSymbol effectiveBase(TypeSymbol type) => type switch + { + TypeParameterSymbol tp => tp.EffectiveBaseClassNoUseSiteDiagnostics, + var t => t.BaseTypeNoUseSiteDiagnostics, + }; + + static ImmutableArray inheritedInterfaces(TypeSymbol type) => type switch + { + TypeParameterSymbol tp => tp.AllEffectiveInterfacesNoUseSiteDiagnostics, + { TypeKind: TypeKind.Interface } => type.AllInterfacesNoUseSiteDiagnostics, + _ => ImmutableArray.Empty, + }; + } + } + 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 9f26199125abf..a0e892667cfee 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs @@ -1933,5 +1933,127 @@ public interface IOut { } // _ = i switch { 1 => default, _ => x }/*T:T*/; // 6 Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").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; + string? otherField = 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(), + }; + } + + 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) + ); + } + + [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) + { + if (c is { c: null }) + { + if (c.c != null) + { + c.c.c.c.ToString(); + } + } + } +} +"); + 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) + ); + } } }