Skip to content

Commit

Permalink
Null inferences do not flow out of a finally block. (#35276)
Browse files Browse the repository at this point in the history 
Fixes #34018
  • Loading branch information
Neal Gafter authored Apr 29, 2019
1 parent 37cd390 commit 3587749
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 38 deletions.
8 changes: 8 additions & 0 deletions docs/features/nullable-reference-types.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ static void F<T>(T x, T y)
The top-level nullability of `x ?? y` is `!` if `x` is `!` and otherwise the top-level nullability of `y`.
A warning is reported if there is a nested nullability mismatch between `x` and `y`.

### Try-finally
We infer the state after a try statement in part by keeping track of which variables may have been assigned a null value in the finally block.
This tracking distinguishes actual assignments from inferences:
only actual assignments (but not inferences) affect the state after the try statement.
See https://github.com/dotnet/roslyn/issues/34018 and https://github.com/dotnet/roslyn/pull/35276.
This is not yet reflected in the language specification for nullable reference types
(as we don't have a specification for how to handle try statements at this time).

## Type parameters
A `class?` constraint is allowed, which, like class, requires the type argument to be a reference type, but allows it to be nullable.
[Nullable strawman](https://github.com/dotnet/csharplang/issues/790)
Expand Down
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1155,9 +1155,10 @@ private void InheritNullableStateOfMember(int targetContainerSlot, int valueCont
}

/// <summary>
/// Whenever setting the state of a variable, and that variable is not declared at the point the state is being set,
/// Whenever assigning a variable, and that variable is not declared at the point the state is being set,
/// and the new state might be <see cref="NullableFlowState.MaybeNull"/>, this method should be called to perform the
/// state setting and to ensure the mutation is visible outside the finally block when the mutation occurs in a finally block.
/// state setting and to ensure the mutation is visible outside the finally block when the mutation occurs in a
/// finally block.
/// </summary>
private void SetStateAndTrackForFinally(ref LocalState state, int slot, NullableFlowState newState)
{
Expand Down Expand Up @@ -2272,7 +2273,7 @@ private int LearnFromNullTest(int slot, TypeSymbol expressionType, ref LocalStat
{
if (slot > 0 && PossiblyNullableType(expressionType))
{
SetStateAndTrackForFinally(ref state, slot, NullableFlowState.MaybeNull);
state[slot] = NullableFlowState.MaybeNull;
}

return slot;
Expand Down
83 changes: 48 additions & 35 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47692,8 +47692,10 @@ void F(C c)
);
}

[Fact, WorkItem(33526, "https://github.com/dotnet/roslyn/issues/33526")]
public void ConditionalAccessIsAPureTest_InFinally()
[Fact]
[WorkItem(33526, "https://github.com/dotnet/roslyn/issues/33526")]
[WorkItem(34018, "https://github.com/dotnet/roslyn/issues/34018")]
public void ConditionalAccessIsAPureTest_InFinally_01()
{
var source = @"
class C
Expand All @@ -47709,15 +47711,40 @@ void F(object o)
o.ToString();
}

o.ToString(); // 1
o.ToString();
}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (15,9): warning CS8602: Dereference of a possibly null reference.
// o.ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "o").WithLocation(15, 9)
);
comp.VerifyDiagnostics();
}

[Fact]
[WorkItem(34018, "https://github.com/dotnet/roslyn/issues/34018")]
public void ConditionalAccessIsAPureTest_InFinally_02()
{
var source = @"
class C
{
void F()
{
C? c = null;
try
{
c = new C();
}
finally
{
if (c != null) c.Cleanup();
}

c.Operation(); // ok
}

void Cleanup() {}
void Operation() {}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics();
}

[Fact, WorkItem(33526, "https://github.com/dotnet/roslyn/issues/33526")]
Expand Down Expand Up @@ -91429,7 +91456,9 @@ public static void Test(bool b, object? o, G<string?> g)
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "g").WithArguments("G<string?>", "G<string>").WithLocation(17, 24));
}

[Fact, WorkItem(33446, "https://github.com/dotnet/roslyn/issues/33446")]
[Fact]
[WorkItem(33446, "https://github.com/dotnet/roslyn/issues/33446")]
[WorkItem(34018, "https://github.com/dotnet/roslyn/issues/34018")]
public void NullInferencesInFinallyClause()
{
var source =
Expand Down Expand Up @@ -91461,7 +91490,7 @@ static void M2()
if (node is null) {} else {}
}

node.ToString(); // 2
node.ToString();
}
static void M3()
{
Expand All @@ -91475,7 +91504,7 @@ static void M3()
node ??= node;
}

node.ToString(); // 3
node.ToString();
}
static void M4()
{
Expand All @@ -91495,7 +91524,7 @@ static void M4()
}
}

node.ToString(); // 4
node.ToString();
}
static void M5()
{
Expand All @@ -91515,7 +91544,7 @@ static void M5()
}
}

node.ToString(); // 5
node.ToString();
}
static void M6()
{
Expand All @@ -91529,14 +91558,14 @@ static void M6()
(node, _) = (null, node);
}

node.ToString(); // 6
node.ToString(); // 2
}
static void MN1()
{
Node? node = null;
Mout(out node);
if (node == null) {} else {}
node.ToString(); // 7
node.ToString(); // 3
}
static void MN2()
{
Expand All @@ -91550,38 +91579,22 @@ static void MN2()
if (node == null) {} else {}
}

node.ToString(); // 8
node.ToString();
}
static void Mout(out Node node) => node = null!;
}
";
var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
// https://github.com/dotnet/roslyn/pull/33929: Once we report one of the (NOT YET) warnings, we should report the other
comp.VerifyDiagnostics(
// (15,9): warning CS8602: Dereference of a possibly null reference.
// node.Next.ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node.Next").WithLocation(15, 9),
// (29,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(29, 9),
// (43,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 3
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(43, 9),
// (63,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 4
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(63, 9),
// (83,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 5
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(83, 9),
// (97,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 6
// node.ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(97, 9),
// (104,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 7
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(104, 9),
// (118,9): warning CS8602: Dereference of a possibly null reference.
// node.ToString(); // 8
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(118, 9));
// node.ToString(); // 3
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "node").WithLocation(104, 9));
}

[Fact, WorkItem(32934, "https://github.com/dotnet/roslyn/issues/32934")]
Expand Down

0 comments on commit 3587749

Please sign in to comment.