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.

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

Track additional state for "maybe null even if not nullable" #39778

Merged
merged 14 commits into from
Nov 21, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -1311,7 +1311,7 @@ private void SetStateAndTrackForFinally(ref LocalState state, int slot, Nullable
if (newState != NullableFlowState.NotNull && NonMonotonicState.HasValue)
{
var tryState = NonMonotonicState.Value;
tryState[slot] = newState;
tryState[slot] = newState.Join(tryState[slot]);
NonMonotonicState = tryState;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117638,13 +117638,13 @@ static T F9(bool b)

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
public void MaybeNullT_Meet()
public void MaybeNullT_Meet_01()
{
var source =
@"#nullable enable
class C<T> where T : new()
{
static T F1()
static T F1(T t)
{
T t1;
try { t1 = default(T); }
Expand Down Expand Up @@ -117721,6 +117721,142 @@ static T F9()
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "t7").WithLocation(51, 16));
}

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
public void MaybeNullT_Meet_02()
{
var source =
@"#nullable enable
class C<T> where T : new()
{
static void F0(T t)
{
}
static T F2(T t)
{
T t2 = t;
T r2;
try
{
t2 = default(T);
t2 = t;
}
finally
{
F0(t2); // 1
Copy link
Contributor

@RikkiGibson RikkiGibson Nov 19, 2019

Choose a reason for hiding this comment

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

I'm a little confused by this-- is it expected that we could throw out of the 'try' block at any point and then run the 'finally' block, so we have to be as pessimistic as possible here? #Closed

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, we assume an exception could have been thrown between the assignments above, although perhaps we could do better for those simple assignments.


In reply to: 348225225 [](ancestors = 348225225)

r2 = t2;
}
return r2; // 2
}
static T F3(T t)
{
T t3 = t;
T r3;
try
{
t3 = default(T);
t3 = new T();
}
finally
{
F0(t3); // 3
r3 = t3;
}
return r3; // 4
}
static T F4(T t)
{
T t4 = t;
T r4;
try
{
t4 = t;
t4 = default(T);
}
finally
{
F0(t4); // 5
r4 = t4;
}
return r4; // 6
}
static T F6(T t)
{
T t6 = t;
T r6;
try
{
t6 = t;
t6 = new T();
}
finally
{
F0(t6);
r6 = t6;
}
return r6;
}
static T F7(T t)
{
T t7 = t;
T r7;
try
{
t7 = new T();
t7 = default(T);
}
finally
{
F0(t7); // 7
r7 = t7;
}
return r7; // 8
}
static T F8(T t)
{
T t8 = t;
T r8;
try
{
t8 = new T();
t8 = t;
}
finally
{
F0(t8);
r8 = t8;
}
return r8;
}
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (18,16): warning CS8604: Possible null reference argument for parameter 't' in 'void C<T>.F0(T t)'.
// F0(t2); // 1
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t2").WithArguments("t", "void C<T>.F0(T t)").WithLocation(18, 16),
// (21,16): warning CS8603: Possible null reference return.
// return r2; // 2
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "r2").WithLocation(21, 16),
// (34,16): warning CS8604: Possible null reference argument for parameter 't' in 'void C<T>.F0(T t)'.
// F0(t3); // 3
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t3").WithArguments("t", "void C<T>.F0(T t)").WithLocation(34, 16),
// (37,16): warning CS8603: Possible null reference return.
// return r3; // 4
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "r3").WithLocation(37, 16),
// (50,16): warning CS8604: Possible null reference argument for parameter 't' in 'void C<T>.F0(T t)'.
// F0(t4); // 5
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t4").WithArguments("t", "void C<T>.F0(T t)").WithLocation(50, 16),
// (53,16): warning CS8603: Possible null reference return.
// return r4; // 6
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "r4").WithLocation(53, 16),
// (82,16): warning CS8604: Possible null reference argument for parameter 't' in 'void C<T>.F0(T t)'.
// F0(t7); // 7
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "t7").WithArguments("t", "void C<T>.F0(T t)").WithLocation(82, 16),
// (85,16): warning CS8603: Possible null reference return.
// return r7; // 8
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "r7").WithLocation(85, 16));
}

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
public void MaybeNullT_Conversions_01()
Expand Down Expand Up @@ -118778,6 +118914,60 @@ public void MaybeNullT_18()
var source =
@"#nullable enable
using System.Diagnostics.CodeAnalysis;
class Program
{
[return: MaybeNull] static T F<T>() => default;
static void F1<T>() { _ = F<T>(); }
static void F2<T>() where T : class { _ = F<T>(); }
static void F3<T>() where T : struct { _ = F<T>(); }
static void F4<T>() where T : notnull { _ = F<T>(); }
}";
var comp = CreateCompilation(new[] { source, MaybeNullAttributeDefinition });
comp.VerifyDiagnostics();
}

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
[WorkItem(39888, "https://github.com/dotnet/roslyn/issues/39888")]
public void MaybeNullT_19()
{
var source =
@"#nullable enable
using System.Diagnostics.CodeAnalysis;
class Program
{
[return: MaybeNull]static T F1<T>(bool b, T t) => b switch { false => t, _ => default };
[return: MaybeNull]static T F2<T>(bool b, T t) => b switch { false => default, _ => t };
[return: MaybeNull]static T F3<T>(bool b, T t) where T : class => b switch { false => t, _ => default };
[return: MaybeNull]static T F4<T>(bool b, T t) where T : class => b switch { false => default, _ => t };
[return: MaybeNull]static T F5<T>(bool b, T t) where T : struct => b switch { false => t, _ => default };
[return: MaybeNull]static T F6<T>(bool b, T t) where T : struct => b switch { false => default, _ => t };
[return: MaybeNull]static T F7<T>(bool b, T t) where T : notnull => b switch { false => t, _ => default };
[return: MaybeNull]static T F8<T>(bool b, T t) where T : notnull => b switch { false => default, _ => t };
}";
var comp = CreateCompilation(new[] { source, MaybeNullAttributeDefinition });
comp.VerifyDiagnostics(
// (5,83): warning CS8601: Possible null reference assignment.
// [return: MaybeNull]static T F1<T>(bool b, T t) => b switch { false => t, _ => default };
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(5, 83),
// (6,75): warning CS8601: Possible null reference assignment.
// [return: MaybeNull]static T F2<T>(bool b, T t) => b switch { false => default, _ => t };
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(6, 75),
// (11,101): warning CS8601: Possible null reference assignment.
// [return: MaybeNull]static T F7<T>(bool b, T t) where T : notnull => b switch { false => t, _ => default };
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(11, 101),
// (12,93): warning CS8601: Possible null reference assignment.
// [return: MaybeNull]static T F8<T>(bool b, T t) where T : notnull => b switch { false => default, _ => t };
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(12, 93));
}

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
public void MaybeNullT_20()
{
var source =
@"#nullable enable
using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
class Program
{
Expand All @@ -118804,7 +118994,7 @@ static async Task<T> F<T>(int i, T x, [AllowNull]T y)

[Fact]
[WorkItem(37362, "https://github.com/dotnet/roslyn/issues/37362")]
public void MaybeNullT_19()
public void MaybeNullT_21()
{
var source =
@"#nullable enable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1849,6 +1849,7 @@ static void M(string? s)

[Fact]
[WorkItem(34233, "https://github.com/dotnet/roslyn/issues/34233")]
[WorkItem(39888, "https://github.com/dotnet/roslyn/issues/39888")]
public void SwitchExpressionResultType_01()
{
CSharpCompilation c = CreateNullableCompilation(@"
Expand Down Expand Up @@ -1911,6 +1912,7 @@ public interface IIn<in T> { }
public interface IOut<out T> { }
");
c.VerifyTypes();
// Should not report warnings 5 or 6 (see https://github.com/dotnet/roslyn/issues/39888).
c.VerifyDiagnostics(
// (33,15): error CS8506: No best type was found for the switch expression.
// _ = i switch { 1 => x, _ => y }/*T:!*/; // 1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using System;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Diagnostics;
Expand Down Expand Up @@ -3788,5 +3787,39 @@ void M() {}
var symbolInfo = specModel.GetSymbolInfo(newAttributeUsage.ArgumentList.Arguments[0].Expression);
Assert.Equal(SpecialType.System_String, ((IFieldSymbol)symbolInfo.Symbol).Type.SpecialType);
}

[Fact]
[WorkItem(38638, "https://github.com/dotnet/roslyn/issues/38638")]
public void TypeParameter_Default()
{
var source =
@"#nullable enable
abstract class A<T>
{
internal abstract void F<U>() where U : T;
}
class B1<T> : A<T>
{
internal override void F<U>() { _ = default(U); }
}
class B2 : A<int?>
{
internal override void F<U>() { _ = default(U); }
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics();
var tree = comp.SyntaxTrees[0];
var model = comp.GetSemanticModel(tree);
var exprs = tree.GetRoot().DescendantNodes().OfType<DefaultExpressionSyntax>().ToArray();
verify(exprs[0], PublicNullableAnnotation.NotAnnotated, PublicNullableFlowState.MaybeNull);
verify(exprs[1], PublicNullableAnnotation.Annotated, PublicNullableFlowState.MaybeNull);

void verify(DefaultExpressionSyntax expr, PublicNullableAnnotation expectedAnnotation, PublicNullableFlowState expectedState)
Copy link
Contributor

@RikkiGibson RikkiGibson Nov 19, 2019

Choose a reason for hiding this comment

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

consider putting expected values first or labeling in order to match the conventions of the xunit Assert methods. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave as is since this matches similar local functions in this class.


In reply to: 348226625 [](ancestors = 348226625)

{
var info = model.GetTypeInfo(expr).Nullability;
Assert.Equal(expectedAnnotation, info.Annotation);
Assert.Equal(expectedState, info.FlowState);
}
}
}
}