Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix regression with var pattern nullability #53691

Merged
merged 4 commits into from
May 28, 2021
Merged

Fix regression with var pattern nullability #53691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ccef3a7
Fix regression with var pattern nullability
jcouv May 26, 2021
12c2f04
Fix nullability on var locals
jcouv May 26, 2021
1ef2e51
Address bootstrapping issue
jcouv May 26, 2021
816ac9f
Verify GetDeclaredSymbol
jcouv May 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Open in desktop
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));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 The issue was that makeDagTempSlot above was recording the type of the var pattern local into State, based on correct state (from expressionType), but we'd drop that by using initialState which captured a prior state.


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
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this all that was needed to fix the 'foreach' scenario?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the for loop starts with a local declaration. That's handled by VisitLocalDeclaration, which correctly calls ToAnnotatedTypeWithAnnotations when the local has an inferred type, but when we get to ToTypeWithAnnotations the annotation would get dropped.

}
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
Open in desktop
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 @@ -142889,30 +142890,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 @@ -142922,11 +142911,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 @@ -146719,5 +146708,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
Open in desktop
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't clear to me why this needed to change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't bootstrap without this change

Return:
return ref value;
ReturnNotFound:
Expand Down