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

Use [NotNullWhenFalse] and [EnsuresNotNull] to drive nullability analysis #26656

Merged
merged 18 commits into from
May 15, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -1863,6 +1863,7 @@ private void VisitArgumentsEvaluateHonoringAnnotations(
else if (notNullWhenFalse)
{
// We'll use the WhenTrue/False states to represent whether the invocation returns true/false
// PROTOTYPE(NullableReferenceTypes): Consider splitting for the entire method, not just once the first annotated argument is encountered
Split();
Copy link
Contributor

@AlekseyTs AlekseyTs May 14, 2018

Choose a reason for hiding this comment

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

Split(); [](start = 20, length = 8)

I think think this might be a wrong place to do the Split, it should be done when we are visiting invocation, not the arguments. #Closed

Copy link
Member Author

@jcouv jcouv May 14, 2018

Choose a reason for hiding this comment

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

I don't see a functional advantage to splitting early. Do you mean that would be better for clarity? #Resolved


// The variable in this slot is not null when the method returns false
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/ParameterSymbol.cs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,11 @@ internal sealed override ObsoleteAttributeData ObsoleteAttributeData

internal abstract AttributeAnnotations FlowAnalysisAnnotations { get; }

/// <summary>
/// If there are any annotations on the member (not just that parameter), then memberHasExtra is true. The purpose is to ensure
/// that if some annotations are present on the member, then annotations win over the attributes on the member in all positions.
/// That could mean removing an attribute.
/// </summary>
protected (bool memberHasExtra, AttributeAnnotations annotations) TryGetExtraAttributeAnnotations()
Copy link
Member

@cston cston May 10, 2018

Choose a reason for hiding this comment

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

Perhaps return AttributeAnnotations?. #ByDesign

Copy link
Contributor

@AlekseyTs AlekseyTs May 14, 2018

Choose a reason for hiding this comment

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

memberHasExtra [](start = 24, length = 14)

The value of AttributeAnnotations member is not obvious to me. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a clear comment might help


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

{
ParameterSymbol originalParameter = this.OriginalDefinition;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,7 @@ internal override bool IsCallerMemberName

internal override AttributeAnnotations FlowAnalysisAnnotations
{
// PROTOTYPE(NullableReferenceTypes): Make sure this is covered by test
get { return _originalParam.FlowAnalysisAnnotations; }
get { return AttributeAnnotations.None; }
}

#endregion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ internal override bool IsCallerMemberName

internal override AttributeAnnotations FlowAnalysisAnnotations
Copy link
Contributor

@AlekseyTs AlekseyTs May 14, 2018

Choose a reason for hiding this comment

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

internal override AttributeAnnotations FlowAnalysisAnnotations [](start = 8, length = 62)

In general, in symbols only leaf classes should provide implementation and each leaf class should be covered by targeted tests. This implementation is fine for the prototype, but, if we decide to go with this approach, we should follow up. For example, I will not be surprised if retargeting symbols would need special handling, etc. Please add a prototype comment. #Closed

{
// PROTOTYPE(NullableReferenceTypes): Ensure this is covered and consider moving to leaf types
// PROTOTYPE(NullableReferenceTypes): Consider moving to leaf types
get { return _underlyingParameter.FlowAnalysisAnnotations; }
}

Expand Down
310 changes: 310 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/StaticNullChecking.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5522,6 +5522,82 @@ public void Main(string? s)
VerifyAnnotationsAndMetadata(c, "C.MyIsNullOrEmpty", NotNullWhenFalse);
}

[Fact]
public void NotNullWhenFalse_ComparedToTrue()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
public void Main(string? s)
{
if (MyIsNullOrEmpty(s) == true)
{
s.ToString(); // warn
}
else
{
s.ToString(); // ok
Copy link
Member

@cston cston May 10, 2018

Choose a reason for hiding this comment

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

// ok [](start = 26, length = 5)

Minor point: comment is misleading. #Resolved

}

s.ToString(); // warn 2
}
public static bool MyIsNullOrEmpty([NotNullWhenFalse] string? s) => throw null;
}
" + NotNullWhenFalseAttributeDefinition, parseOptions: TestOptions.Regular8);

// PROTOTYPE(NullableReferenceTypes): there should only be two diagnostics
c.VerifyDiagnostics(
// (9,13): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 13),
// (13,13): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // ok
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 13),
// (16,9): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(16, 9)
);
}

[Fact]
public void NotNullWhenFalse_ComparedToFalse()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
public void Main(string? s)
{
if (false == MyIsNullOrEmpty(s))
{
s.ToString(); // warn
Copy link
Member

Choose a reason for hiding this comment

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

// ok?

}
else
{
s.ToString(); // ok
}

s.ToString(); // warn 2
}
public static bool MyIsNullOrEmpty([NotNullWhenFalse] string? s) => throw null;
}
" + NotNullWhenFalseAttributeDefinition, parseOptions: TestOptions.Regular8);

// PROTOTYPE(NullableReferenceTypes): there should only be two diagnostics
c.VerifyDiagnostics(
// (9,13): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 13),
// (13,13): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // ok
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(13, 13),
// (16,9): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(16, 9)
);
}

[Fact]
public void NotNullWhenFalse_RequiresBoolReturn()
{
Expand All @@ -5538,6 +5614,35 @@ public class C
VerifyAnnotations(c, "C.MyIsNullOrEmpty", NotNullWhenFalse);
}

[Fact]
public void NotNullWhenFalse_RequiresBoolReturn_OnGenericMethod()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s)
{
if (MyIsNullOrEmpty(s, true))
{
s.ToString(); // warn
}
else
{
s.ToString(); // ok
}
}
public static T MyIsNullOrEmpty<T>([NotNullWhenFalse] string? s, T t) => throw null;
}
" + NotNullWhenFalseAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics(
// (9,13): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(9, 13)
);
}

[Fact]
public void NotNullWhenFalse_OnTwoParameters()
{
Expand Down Expand Up @@ -6367,6 +6472,63 @@ void Main(string? s)
VerifyAnnotationsAndMetadata(c, "C.ThrowIfNull", None, EnsuresNotNull);
}

[Fact]
public void EnsuresNotNull_Generic_WithRefType()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void Main(string? s)
{
ThrowIfNull(s);
Copy link
Member

@cston cston May 10, 2018

Choose a reason for hiding this comment

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

ThrowIfNull(s); [](start = 8, length = 15)

Please add a test for ThrowIfNull(t); t.ToString(); where t is an unconstrained type parameter. #Resolved

s.ToString(); // ok
}
public static void ThrowIfNull<T>([EnsuresNotNull] T s) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics();
}

[Fact]
public void EnsuresNotNull_Generic_WithValueType()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void Main(int s)
{
ThrowIfNull(s);
s.ToString();
}
public static void ThrowIfNull<T>([EnsuresNotNull] T s) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics();
}

[Fact]
public void EnsuresNotNull_Generic_WithUnconstrainedGenericType()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M<U>(U u)
{
ThrowIfNull(u);
u.ToString();
}
public static void ThrowIfNull<T>([EnsuresNotNull] T s) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics();
}

[Fact]
public void EnsuresNotNull_OnInterface()
{
Expand Down Expand Up @@ -6589,6 +6751,154 @@ void M<T>(T t)
VerifyAnnotationsAndMetadata(c, "C.ThrowIfNull", EnsuresNotNull);
}

[Fact]
public void EnsuresNotNull_BeginInvoke()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public delegate void Delegate([EnsuresNotNull] string? s);
public class C
{
void M(Delegate d, string? s)
{
s.ToString(); // warn
d.BeginInvoke(s, null, null);
s.ToString(); // warn 2
}
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics(
// (8,9): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(8, 9),
// (9,26): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
// d.BeginInvoke(s, null, null);
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(9, 26),
// (9,32): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
// d.BeginInvoke(s, null, null);
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(9, 32),
// (10,9): warning CS8602: Possible dereference of a null reference.
// s.ToString(); // ok
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(10, 9)
);
}

[Fact]
public void EnsuresNotNull_BackEffect()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s1, string? s2)
{
ThrowIfNull(s2 = s1, s1);
s2.ToString(); // warn
}
public static void ThrowIfNull(string? x1, [EnsuresNotNull] string? x2) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

// PROTOTYPE(NullableReferenceTypes): Should we be able to trace that s2 was assigned a non-null value?
c.VerifyDiagnostics(
// (9,9): warning CS8602: Possible dereference of a null reference.
// s2.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(9, 9)
);
}

[Fact]
public void EnsuresNotNull_ForwardEffect()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s1, string? s2)
{
ThrowIfNull(s1, s2 = s1);
s2.ToString(); // ok
}
public static void ThrowIfNull([EnsuresNotNull] string? x1, string? x2) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics();
}

[Fact]
public void EnsuresNotNull_ForwardEffect2()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s1)
{
ThrowIfNull(s1, s1 = null);
s1.ToString(); // warn
}
public static void ThrowIfNull([EnsuresNotNull] string? x1, string? x2) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics(
// (8,9): warning CS8602: Possible dereference of a null reference.
// s1.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s1").WithLocation(8, 9)
);
}

[Fact]
public void EnsuresNotNull_ForwardEffect3()
{
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s1, string? s2)
{
ThrowIfNull(s1 = null, s2 = s1, s1 = """", s1);
s2.ToString(); // warn
}
public static void ThrowIfNull(string? x1, string? x2, string? x3, [EnsuresNotNull] string? x4) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics(
// (8,9): warning CS8602: Possible dereference of a null reference.
// s2.ToString(); // warn
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s2").WithLocation(8, 9)
);
}

[Fact]
public void EnsuresNotNull_TypeInference()
{
// PROTOTYPE(NullableReferenceTypes): This test raises the question of flowing information from annotations into the inferred type
// But the issue is currently masked by a broader problem with `out var` and nullability
CSharpCompilation c = CreateCompilation(@"
using System.Runtime.CompilerServices;
public class C
{
void M(string? s1)
{
ThrowIfNull(s1, out var s2);
s2/*T:string!*/.ToString();
}
public static void ThrowIfNull<T>([EnsuresNotNull] T x1, out T x2) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);

c.VerifyTypes();
c.VerifyDiagnostics(
// (7,21): warning CS8604: Possible null reference argument for parameter 'x1' in 'void C.ThrowIfNull<string>(string x1, out string x2)'.
// ThrowIfNull(s1, out var s2);
Diagnostic(ErrorCode.WRN_NullReferenceArgument, "s1").WithArguments("x1", "void C.ThrowIfNull<string>(string x1, out string x2)").WithLocation(7, 21)
);
}

[Fact]
public void EnsuresNotNull_ConditionalMethodInReleaseMode()
{
Expand Down