Skip to content

Commit

Permalink
Fix regression with var pattern nullability (#53691) (#53823)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jcouv
jcouv authored Jun 2, 2021
1 parent 7791927 commit dcf4ce7
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 32 deletions.
17 changes: 6 additions & 11 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -284,13 +283,11 @@ public PossiblyConditionalState Clone()
}
}

private PooledDictionary<LabelSymbol, (LocalState state, bool believedReachable)>
LearnFromDecisionDag(
private PooledDictionary<LabelSymbol, (LocalState state, bool believedReachable)> 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
Expand Down Expand Up @@ -319,7 +316,7 @@ public PossiblyConditionalState Clone()
tempMap.Add(rootTemp, (originalInputSlot, expressionType.Type));

var nodeStateMap = PooledDictionary<BoundDecisionDagNode, (PossiblyConditionalState state, bool believedReachable)>.GetInstance();
nodeStateMap.Add(decisionDag.RootNode, (state: initialState.Clone(), believedReachable: true));
nodeStateMap.Add(decisionDag.RootNode, (state: PossiblyConditionalState.Create(this), believedReachable: true));

var labelStateMap = PooledDictionary<LabelSymbol, (LocalState state, bool believedReachable)>.GetInstance();

Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/TypeWithState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) :
Expand Down
165 changes: 146 additions & 19 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5112,9 +5112,10 @@ void M<T>(T t)
var declaration = syntaxTree.GetRoot().DescendantNodes().OfType<VariableDeclaratorSyntax>().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]
Expand Down Expand Up @@ -142785,30 +142786,18 @@ static T F5<T, U>(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));
Expand All @@ -142818,11 +142807,11 @@ static T F5<T, U>(U x5) where U : T?
var locals = tree.GetRoot().DescendantNodes().OfType<VariableDeclaratorSyntax>().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)
Expand Down Expand Up @@ -146615,5 +146604,143 @@ void M(I3 i)
AssertEx.Equal(new string[] { "I<object?>", "I2<object!>", "I<object!>" },
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) => 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<T>
{
public T Item { get; set; } = default!;
}
public class C
{
public void M(Container<string?> 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) => 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<T> {constraint}
{{
void M(T initial, System.Func<T, T?> 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<VariableDeclarationSyntax>();
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<T> {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)
);
}
}
}
2 changes: 1 addition & 1 deletion src/Dependencies/Collections/SegmentedDictionary`2.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit dcf4ce7

Please sign in to comment.