From dcf4ce71d081adfbbeebac53427c7d06a4727174 Mon Sep 17 00:00:00 2001 From: Julien Couvreur Date: Wed, 2 Jun 2021 12:34:31 -0700 Subject: [PATCH] Fix regression with var pattern nullability (#53691) (#53823) --- .../FlowAnalysis/NullableWalker_Patterns.cs | 17 +- .../CSharp/Portable/Symbols/TypeWithState.cs | 2 +- .../Semantics/NullableReferenceTypesTests.cs | 165 ++++++++++++++++-- .../Collections/SegmentedDictionary`2.cs | 2 +- 4 files changed, 154 insertions(+), 32 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs index 8dfa376a19920..9ac94bcc14106 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs @@ -208,7 +208,6 @@ protected override LocalState VisitSwitchStatementDispatch(BoundSwitchStatement // visit switch header Visit(node.Expression); var expressionState = ResultType; - var initialState = PossiblyConditionalState.Create(this); DeclareLocals(node.InnerLocals); foreach (var section in node.SwitchSections) @@ -217,7 +216,7 @@ protected override LocalState VisitSwitchStatementDispatch(BoundSwitchStatement DeclareLocals(section.Locals); } - var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState, ref initialState); + var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState); foreach (var section in node.SwitchSections) { foreach (var label in section.SwitchLabels) @@ -284,13 +283,11 @@ public PossiblyConditionalState Clone() } } - private PooledDictionary - LearnFromDecisionDag( + private PooledDictionary LearnFromDecisionDag( SyntaxNode node, BoundDecisionDag decisionDag, BoundExpression expression, - TypeWithState expressionType, - ref PossiblyConditionalState initialState) + TypeWithState expressionType) { // We reuse the slot at the beginning of a switch (or is-pattern expression), pretending that we are // not copying the input to evaluate the patterns. In this way we infer non-nullability of the original @@ -319,7 +316,7 @@ public PossiblyConditionalState Clone() tempMap.Add(rootTemp, (originalInputSlot, expressionType.Type)); var nodeStateMap = PooledDictionary.GetInstance(); - nodeStateMap.Add(decisionDag.RootNode, (state: initialState.Clone(), believedReachable: true)); + nodeStateMap.Add(decisionDag.RootNode, (state: PossiblyConditionalState.Create(this), believedReachable: true)); var labelStateMap = PooledDictionary.GetInstance(); @@ -735,8 +732,7 @@ private void VisitSwitchExpressionCore(BoundSwitchExpression node, bool inferTyp Visit(node.Expression); var expressionState = ResultType; - var state = PossiblyConditionalState.Create(this); - var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState, ref state); + var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState); var endState = UnreachableState(); if (!node.ReportedNotExhaustive && node.DefaultLabel != null && @@ -846,8 +842,7 @@ public override BoundNode VisitIsPatternExpression(BoundIsPatternExpression node VisitPatternForRewriting(node.Pattern); Visit(node.Expression); var expressionState = ResultType; - var state = PossiblyConditionalState.Create(this); - var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState, ref state); + var labelStateMap = LearnFromDecisionDag(node.Syntax, node.DecisionDag, node.Expression, expressionState); var trueState = labelStateMap.TryGetValue(node.IsNegated ? node.WhenFalseLabel : node.WhenTrueLabel, out var s1) ? s1.state : UnreachableState(); var falseState = labelStateMap.TryGetValue(node.IsNegated ? node.WhenTrueLabel : node.WhenFalseLabel, out var s2) ? s2.state : UnreachableState(); labelStateMap.Free(); diff --git a/src/Compilers/CSharp/Portable/Symbols/TypeWithState.cs b/src/Compilers/CSharp/Portable/Symbols/TypeWithState.cs index 3ba531b3cadbb..672c03a26d3c2 100644 --- a/src/Compilers/CSharp/Portable/Symbols/TypeWithState.cs +++ b/src/Compilers/CSharp/Portable/Symbols/TypeWithState.cs @@ -87,7 +87,7 @@ public TypeWithAnnotations ToTypeWithAnnotations(CSharpCompilation compilation, if (Type?.IsTypeParameterDisallowingAnnotationInCSharp8() == true) { var type = TypeWithAnnotations.Create(Type, NullableAnnotation.NotAnnotated); - return State == NullableFlowState.MaybeDefault ? type.SetIsAnnotated(compilation) : type; + return (State == NullableFlowState.MaybeDefault || asAnnotatedType) ? type.SetIsAnnotated(compilation) : type; } NullableAnnotation annotation = asAnnotatedType ? (Type?.IsValueType == true ? NullableAnnotation.NotAnnotated : NullableAnnotation.Annotated) : diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index 66a9a5144eb71..17e8084363a95 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -5112,9 +5112,10 @@ void M(T t) var declaration = syntaxTree.GetRoot().DescendantNodes().OfType().Single(); var model = comp.GetSemanticModel(syntaxTree); var local = (ILocalSymbol)model.GetDeclaredSymbol(declaration); - Assert.Equal(CodeAnalysis.NullableAnnotation.NotAnnotated, local.NullableAnnotation); - Assert.Equal(CodeAnalysis.NullableAnnotation.NotAnnotated, local.Type.NullableAnnotation); - Assert.Equal("T", local.Type.ToTestDisplayString(includeNonNullable: true)); + Assert.Equal("T? t2", local.ToTestDisplayString()); + Assert.Equal(CodeAnalysis.NullableAnnotation.Annotated, local.NullableAnnotation); + Assert.Equal(CodeAnalysis.NullableAnnotation.Annotated, local.Type.NullableAnnotation); + Assert.Equal("T?", local.Type.ToTestDisplayString(includeNonNullable: true)); } [Fact] @@ -142785,30 +142786,18 @@ static T F5(U x5) where U : T? }"; var comp = CreateCompilation(source, parseOptions: TestOptions.Regular9); comp.VerifyDiagnostics( - // (7,14): warning CS8600: Converting null literal or possible null value to non-nullable type. - // y1 = default(T); - Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "default(T)").WithLocation(7, 14), // (8,16): warning CS8603: Possible null reference return. // return y1; // 1 Diagnostic(ErrorCode.WRN_NullReferenceReturn, "y1").WithLocation(8, 16), // (14,16): warning CS8603: Possible null reference return. // return y2; // 2 Diagnostic(ErrorCode.WRN_NullReferenceReturn, "y2").WithLocation(14, 16), - // (19,14): warning CS8600: Converting null literal or possible null value to non-nullable type. - // y3 = default(T); - Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "default(T)").WithLocation(19, 14), // (20,16): warning CS8603: Possible null reference return. // return y3; // 3 Diagnostic(ErrorCode.WRN_NullReferenceReturn, "y3").WithLocation(20, 16), - // (25,14): warning CS8600: Converting null literal or possible null value to non-nullable type. - // y4 = default(T); - Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "default(T)").WithLocation(25, 14), // (26,16): warning CS8603: Possible null reference return. // return y4; // 4 Diagnostic(ErrorCode.WRN_NullReferenceReturn, "y4").WithLocation(26, 16), - // (31,14): warning CS8600: Converting null literal or possible null value to non-nullable type. - // y5 = default(U); - Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "default(U)").WithLocation(31, 14), // (32,16): warning CS8603: Possible null reference return. // return y5; // 5 Diagnostic(ErrorCode.WRN_NullReferenceReturn, "y5").WithLocation(32, 16)); @@ -142818,11 +142807,11 @@ static T F5(U x5) where U : T? var locals = tree.GetRoot().DescendantNodes().OfType().ToArray(); Assert.Equal(5, locals.Length); // https://github.com/dotnet/roslyn/issues/46236: Locals should be treated as annotated. - VerifyVariableAnnotation(model, locals[0], "T y1", NullableAnnotation.NotAnnotated); + VerifyVariableAnnotation(model, locals[0], "T? y1", NullableAnnotation.Annotated); VerifyVariableAnnotation(model, locals[1], "T? y2", NullableAnnotation.Annotated); - VerifyVariableAnnotation(model, locals[2], "T y3", NullableAnnotation.NotAnnotated); - VerifyVariableAnnotation(model, locals[3], "T y4", NullableAnnotation.NotAnnotated); - VerifyVariableAnnotation(model, locals[4], "U y5", NullableAnnotation.NotAnnotated); + VerifyVariableAnnotation(model, locals[2], "T? y3", NullableAnnotation.Annotated); + VerifyVariableAnnotation(model, locals[3], "T? y4", NullableAnnotation.Annotated); + VerifyVariableAnnotation(model, locals[4], "U? y5", NullableAnnotation.Annotated); } private static void VerifyVariableAnnotation(SemanticModel model, VariableDeclaratorSyntax syntax, string expectedDisplay, NullableAnnotation expectedAnnotation) @@ -146615,5 +146604,143 @@ void M(I3 i) AssertEx.Equal(new string[] { "I", "I2", "I" }, i3.AllInterfaces().ToTestDisplayStrings(TypeWithAnnotations.TestDisplayFormat)); } + + [Fact] + public void VarPatternDeclaration_TopLevel() + { + var src = @" +#nullable enable +public class C +{ + public void M(string? x) + { + if (Identity(x) is var y) + { + y.ToString(); // 1 + } + + if (Identity(x) is not null and var z) + { + z.ToString(); + } + } + + public T Identity(T t) => t; +} +"; + var comp = CreateCompilation(src); + comp.VerifyDiagnostics( + // (9,13): warning CS8602: Dereference of a possibly null reference. + // y.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "y").WithLocation(9, 13) + ); + } + + [Fact] + public void VarPatternDeclaration_Nested() + { + var src = @" +#nullable enable +public class Container +{ + public T Item { get; set; } = default!; +} +public class C +{ + public void M(Container x) + { + if (Identity(x) is { Item: var y }) + { + y.ToString(); // 1 + } + + if (Identity(x) is { Item: not null and var z }) + { + z.ToString(); + } + } + + public T Identity(T t) => t; +} +"; + var comp = CreateCompilation(src); + comp.VerifyDiagnostics( + // (13,13): warning CS8602: Dereference of a possibly null reference. + // y.ToString(); // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "y").WithLocation(13, 13) + ); + } + + [Theory, WorkItem(52925, "https://github.com/dotnet/roslyn/issues/52925")] + [WorkItem(46236, "https://github.com/dotnet/roslyn/issues/46236")] + [InlineData("")] + [InlineData(" where T : notnull")] + [InlineData(" where T : class")] + [InlineData(" where T : class?")] + public void VarDeclarationWithGenericType(string constraint) + { + var src = $@" +#nullable enable + +class C {constraint} +{{ + void M(T initial, System.Func selector) + {{ + for (var current = initial; current != null; current = selector(current)) + {{ + }} + + var current2 = initial; + current2 = default; + + for (T? current3 = initial; current3 != null; current3 = selector(current3)) + {{ + }} + + T? current4 = initial; + current4 = default; + }} +}} +"; + var comp = CreateCompilation(src); + comp.VerifyDiagnostics(); + + var tree = comp.SyntaxTrees.Single(); + var model = comp.GetSemanticModel(tree, ignoreAccessibility: false); + var declarations = tree.GetRoot().DescendantNodes().OfType(); + foreach (var declaration in declarations) + { + var local = (ILocalSymbol)model.GetDeclaredSymbol(declaration.Variables.Single()); + Assert.Equal("T?", local.Type.ToTestDisplayString()); + Assert.Equal(CodeAnalysis.NullableAnnotation.Annotated, local.Type.NullableAnnotation); + } + } + + [Theory, WorkItem(52925, "https://github.com/dotnet/roslyn/issues/52925")] + [InlineData("")] + [InlineData(" where T : notnull")] + [InlineData(" where T : class")] + [InlineData(" where T : class?")] + public void VarDeclarationWithGenericType_RefValue(string constraint) + { + var src = $@" +#nullable enable + +class C {constraint} +{{ + ref T Passthrough(ref T value) + {{ + ref var value2 = ref value; + return ref value2; + }} +}} +"; + var comp = CreateCompilation(src); + comp.VerifyDiagnostics( + // (9,20): warning CS8619: Nullability of reference types in value of type 'T?' doesn't match target type 'T'. + // return ref value2; + Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "value2").WithArguments("T?", "T").WithLocation(9, 20) + ); + } } } diff --git a/src/Dependencies/Collections/SegmentedDictionary`2.cs b/src/Dependencies/Collections/SegmentedDictionary`2.cs index 271585109af14..123f0e73cc5a8 100644 --- a/src/Dependencies/Collections/SegmentedDictionary`2.cs +++ b/src/Dependencies/Collections/SegmentedDictionary`2.cs @@ -442,7 +442,7 @@ private ref TValue FindValue(TKey key) ConcurrentOperation: ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); ReturnFound: - ref var value = ref entry._value; + ref TValue value = ref entry._value; Return: return ref value; ReturnNotFound: