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

Handle nullability attributes in param-nullchecking #59393

Merged
merged 5 commits into from
Feb 16, 2022
Merged

Handle nullability attributes in param-nullchecking #59393

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48e5ecb
Handle nullability attributes in param-nullchecking
RikkiGibson Feb 8, 2022
07e63fc
fix
RikkiGibson Feb 8, 2022
3618a98
Avoid breaking 'TestNullableGenericsImplementingGenericAbstractClass'
RikkiGibson Feb 8, 2022
aa6f565
More tests
RikkiGibson Feb 8, 2022
0d8c58d
Address feedback
RikkiGibson Feb 14, 2022
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
6 changes: 3 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 @@ -2067,7 +2067,7 @@ internal static bool AreParameterAnnotationsCompatible(
overriddenType,
overriddenAnnotations,
// We don't consider '!!' when deciding whether 'overridden' is compatible with 'override'
isNullChecked: false);
applyParameterNullCheck: false);
if (isBadAssignment(valueState, overridingType, overridingAnnotations))
{
return false;
Expand Down Expand Up @@ -2527,9 +2527,9 @@ private void EnterParameter(ParameterSymbol parameter, TypeWithAnnotations param
return null;
}

internal static TypeWithState GetParameterState(TypeWithAnnotations parameterType, FlowAnalysisAnnotations parameterAnnotations, bool isNullChecked)
internal static TypeWithState GetParameterState(TypeWithAnnotations parameterType, FlowAnalysisAnnotations parameterAnnotations, bool applyParameterNullCheck)
{
if (isNullChecked)
if (applyParameterNullCheck)
{
return TypeWithState.Create(parameterType.Type, NullableFlowState.NotNull);
}
Expand Down
10 changes: 7 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,12 +809,16 @@ internal static void ReportParameterNullCheckingErrors(DiagnosticBag diagnostics
{
diagnostics.Add(ErrorCode.ERR_DiscardCannotBeNullChecked, location);
}
if (parameter.TypeWithAnnotations.NullableAnnotation.IsAnnotated()
|| parameter.Type.IsNullableTypeOrTypeParameter())

var annotations = parameter.FlowAnalysisAnnotations;
if ((annotations & FlowAnalysisAnnotations.NotNull) == 0
&& !NullableWalker.GetParameterState(parameter.TypeWithAnnotations, annotations, applyParameterNullCheck: false).IsNotNull
Copy link
Member

@jcouv jcouv Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Consider simplifying the double-negation: && NullableWalker.GetParameterState(....).MayBeNull
Correction: should use the extension method MayBeNull(). Somehow, MaybeNull property and MayBeNull() extension method diverged for the maybe-default case :-/

&& (!parameter.Type.IsTypeParameter() || parameter.Type.IsNullableTypeOrTypeParameter() || parameter.TypeWithAnnotations.NullableAnnotation.IsAnnotated() || (annotations & FlowAnalysisAnnotations.AllowNull) != 0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally, if it's possible for a parameter of a type parameter type to be non-nullable after substitution, we want to avoid giving the warning.

Copy link
Member

Choose a reason for hiding this comment

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

From offline chat:
There's an open question on type parameter constrained to C?.
Is there any existing helper/abstraction we might use to simplify this?
Consider extracting this line to a local function with comments.

{
diagnostics.Add(ErrorCode.WRN_NullCheckingOnNullableType, location, parameter);
}
else if (parameter.Type.IsValueType && !parameter.Type.IsPointerOrFunctionPointer())

if (parameter.Type.IsNonNullableValueType() && !parameter.Type.IsPointerOrFunctionPointer())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to dislike if (...) { warn } else if (...) { error } conditions anyway--can be a bit brittle.

{
diagnostics.Add(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, location, parameter);
}
Expand Down
140 changes: 140 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,9 @@ static void M4<T>([DisallowNull] T x3, [DisallowNull] T y3!!)
// (16,9): warning CS8602: Dereference of a possibly null reference.
// x1.ToString(); // 3
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x1").WithLocation(16, 9),
// (19,55): warning CS8995: Nullable type 'T' is null-checked and will throw if null.
// static void M3<T>([AllowNull] T x2, [AllowNull] T y2!!)
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "y2").WithArguments("T").WithLocation(19, 55),
// (21,9): warning CS8602: Dereference of a possibly null reference.
// x2.ToString(); // 4
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "x2").WithLocation(21, 9));
Expand Down Expand Up @@ -1070,5 +1073,142 @@ public override void M2(string s) { } // 3
Diagnostic(ErrorCode.WRN_TopLevelNullabilityMismatchInParameterTypeOnOverride, "M2").WithArguments("s").WithLocation(13, 26)
);
}

[Fact]
public void TestNullabilityAttributes()
{
var source =
@"
#nullable enable
using System.Diagnostics.CodeAnalysis;

class C
{
void M<T>(
string s1!!,
[NotNull] string s2!!,
[DisallowNull] string s3!!,
[AllowNull] string s4!!, // 1
[AllowNull, DisallowNull] string s5!!, // 2
[AllowNull, NotNull] string s6!!,

string? s7!!, // 3
[NotNull] string? s8!!, // ok: this is a typical signature for an 'AssertNotNull' style method.
[DisallowNull] string? s9!!,
[AllowNull] string? s10!!, // 4
[AllowNull, DisallowNull] string? s11!!, // 5
[AllowNull, NotNull] string? s12!!,

int i1!!, // 6
[NotNull] int i2!!, // 7
[DisallowNull] int i3!!, // 8
[AllowNull] int i4!!, // 9
[AllowNull, DisallowNull] int i5!!, // 10
[AllowNull, NotNull] int i6!!, // 11

int? i7!!, // 12
[NotNull] int? i8!!,
[DisallowNull] int? i9!!,
[AllowNull] int? i10!!, // 13
[AllowNull, DisallowNull] int? i11!!, // 14
[AllowNull, NotNull] int? i12!!
) { }
}";
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
var comp = CreateCompilation(new[] { source, AllowNullAttributeDefinition, DisallowNullAttributeDefinition, NotNullAttributeDefinition }, parseOptions: TestOptions.RegularPreview);
comp.VerifyDiagnostics(
// (11,28): warning CS8995: Nullable type 'string' is null-checked and will throw if null.
// [AllowNull] string s4!!, // 1
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "s4").WithArguments("string").WithLocation(11, 28),
// (12,42): warning CS8995: Nullable type 'string' is null-checked and will throw if null.
// [AllowNull, DisallowNull] string s5!!, // 2
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "s5").WithArguments("string").WithLocation(12, 42),
// (15,17): warning CS8995: Nullable type 'string?' is null-checked and will throw if null.
// string? s7!!, // 3
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "s7").WithArguments("string?").WithLocation(15, 17),
// (18,29): warning CS8995: Nullable type 'string?' is null-checked and will throw if null.
// [AllowNull] string? s10!!, // 4
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "s10").WithArguments("string?").WithLocation(18, 29),
// (19,43): warning CS8995: Nullable type 'string?' is null-checked and will throw if null.
// [AllowNull, DisallowNull] string? s11!!, // 5
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "s11").WithArguments("string?").WithLocation(19, 43),
// (22,13): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// int i1!!, // 6
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i1").WithArguments("int").WithLocation(22, 13),
// (23,23): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// [NotNull] int i2!!, // 7
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i2").WithArguments("int").WithLocation(23, 23),
// (24,28): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// [DisallowNull] int i3!!, // 8
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i3").WithArguments("int").WithLocation(24, 28),
// (25,25): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// [AllowNull] int i4!!, // 9
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i4").WithArguments("int").WithLocation(25, 25),
// (26,39): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// [AllowNull, DisallowNull] int i5!!, // 10
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i5").WithArguments("int").WithLocation(26, 39),
// (27,34): error CS8992: Parameter 'int' is a non-nullable value type and cannot be null-checked.
// [AllowNull, NotNull] int i6!!, // 11
Diagnostic(ErrorCode.ERR_NonNullableValueTypeIsNullChecked, "i6").WithArguments("int").WithLocation(27, 34),
// (29,14): warning CS8995: Nullable type 'int?' is null-checked and will throw if null.
// int? i7!!, // 12
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "i7").WithArguments("int?").WithLocation(29, 14),
// (32,26): warning CS8995: Nullable type 'int?' is null-checked and will throw if null.
// [AllowNull] int? i10!!, // 13
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "i10").WithArguments("int?").WithLocation(32, 26),
// (33,40): warning CS8995: Nullable type 'int?' is null-checked and will throw if null.
// [AllowNull, DisallowNull] int? i11!!, // 14
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "i11").WithArguments("int?").WithLocation(33, 40)
);
}

[Theory]
[InlineData("")]
[InlineData("where T : class")]
[InlineData("where T : class?")]
[InlineData("where T : notnull")]
public void TestNullabilityAttributes_Generic(string constraints)
{
var source =
@"
#nullable enable
using System.Diagnostics.CodeAnalysis;

class C
{
void M<T>(
T t1!!,
[NotNull] T t2!!,
[DisallowNull] T t3!!,
[AllowNull] T t4!!, // 1
[AllowNull, DisallowNull] T t5!!, // 2
[AllowNull, NotNull] T t6!!,

T? t7!!, // 3
[NotNull] T? t8!!,
[DisallowNull] T? t9!!,
[AllowNull] T? t10!!, // 4
[AllowNull, DisallowNull] T? t11!!, // 5
[AllowNull, NotNull] T? t12!!
) " + constraints + @" { }
}";
var comp = CreateCompilation(new[] { source, AllowNullAttributeDefinition, DisallowNullAttributeDefinition, NotNullAttributeDefinition }, parseOptions: TestOptions.RegularPreview);
comp.VerifyDiagnostics(
// (11,23): warning CS8995: Nullable type 'T' is null-checked and will throw if null.
// [AllowNull] T t4!!, // 1
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t4").WithArguments("T").WithLocation(11, 23),
// (12,37): warning CS8995: Nullable type 'T' is null-checked and will throw if null.
// [AllowNull, DisallowNull] T t5!!, // 2
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t5").WithArguments("T").WithLocation(12, 37),
// (15,12): warning CS8995: Nullable type 'T?' is null-checked and will throw if null.
// T? t7!!, // 3
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t7").WithArguments("T?").WithLocation(15, 12),
// (18,24): warning CS8995: Nullable type 'T?' is null-checked and will throw if null.
// [AllowNull] T? t10!!, // 4
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t10").WithArguments("T?").WithLocation(18, 24),
// (19,38): warning CS8995: Nullable type 'T?' is null-checked and will throw if null.
// [AllowNull, DisallowNull] T? t11!! // 5
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t11").WithArguments("T?").WithLocation(19, 38)
);
}
}
}