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

Null inferences do not flow out of a finally block. #35276

Merged
merged 3 commits into from
Apr 29, 2019
Merged

Null inferences do not flow out of a finally block. #35276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e67ddb
Null inferences do not flow out of a finally block.
gafter Apr 25, 2019
a6b2e42
Briefly document that we distinguish assignments from inferences.
gafter Apr 26, 2019
d93a175
Adjust markdown
gafter Apr 29, 2019
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
8 changes: 8 additions & 0 deletions docs/features/nullable-reference-types.md
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1184,9 +1184,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 @@ -2299,7 +2300,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47699,8 +47699,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 @@ -47716,15 +47718,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 @@ -90948,7 +90975,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 @@ -90980,7 +91009,7 @@ static void M2()
if (node is null) {} else {}
}

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

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

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

node.ToString(); // 5
node.ToString();
}
static void M6()
{
Expand All @@ -91048,14 +91077,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 @@ -91069,38 +91098,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