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 all 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
34 changes: 31 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,15 +809,43 @@ 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).State.MayBeNull()
&& !isTypeParameterWithPossiblyNonNullableType(parameter.TypeWithAnnotations, annotations))
{
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);
}

// For type parameters, we only want to give the warning if no type argument would result in a non-nullable type.
static bool isTypeParameterWithPossiblyNonNullableType(TypeWithAnnotations typeWithAnnotations, FlowAnalysisAnnotations annotations)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that was much easier to follow :-)

{
if (!typeWithAnnotations.Type.IsTypeParameter())
{
return false;
}

// We avoid checking the nullable annotations, etc. of constraints due to implementation complexity,
// and consider it acceptable to miss "!! on nullable type" warnings in scenarios like `void M<T, U>(U u!!) where U : T?`.
if (typeWithAnnotations.NullableAnnotation.IsAnnotated())
{
return false;
}

// `void M<T>([AllowNull] T t!!)`
if ((annotations & FlowAnalysisAnnotations.AllowNull) != 0)
{
return false;
}

return true;
}
}

internal static ImmutableArray<CustomModifier> ConditionallyCreateInModifiers(RefKind refKind, bool addRefReadOnlyModifier, Binder binder, BindingDiagnosticBag diagnostics, SyntaxNode syntax)
Expand Down
174 changes: 167 additions & 7 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 @@ -725,16 +725,13 @@ internal override void F<U>(U u!! = default) { }
}
class B3 : A<int?>
{
internal override void F<U>(U u!! = default) { }
internal override void F<U>(U u!! = default) { } // note: 'U' is a nullable type here but we don't give a warning due to complexity of accurately searching the constraints.
}";
var compilation = CreateCompilation(source, parseOptions: TestOptions.RegularPreview);
compilation.VerifyDiagnostics(
// (12,35): warning CS8719: Parameter 'u' is null-checked but is null by default.
// internal override void F<U>(U u!! = default) { }
Diagnostic(ErrorCode.WRN_NullCheckedHasDefaultNull, "u").WithArguments("u").WithLocation(12, 35),
// (16,35): warning CS8721: Nullable value type 'U' is null-checked and will throw if null.
// internal override void F<U>(U u!! = default) { }
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "u").WithArguments("U").WithLocation(16, 35));
// (12,35): warning CS8993: Parameter 'u' is null-checked but is null by default.
// internal override void F<U>(U u!! = default) { }
Diagnostic(ErrorCode.WRN_NullCheckedHasDefaultNull, "u").WithArguments("u").WithLocation(12, 35));
}

[Fact]
Expand Down Expand Up @@ -1034,6 +1031,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 +1070,165 @@ 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)
);
}

[Fact]
public void AnnotatedTypeParameter_Indirect()
{
var source = @"
#nullable enable

class C
{
void M<T, U>(
T? t!!, // 1
U u!!) where U : T?
{
}
}";
// note: U is always nullable when a reference type,
// but we don't warn on '!!' for it due to complexity of accurately searching the constraints.
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (7,12): warning CS8995: Nullable type 'T?' is null-checked and will throw if null.
// T? t!!, // 1
Diagnostic(ErrorCode.WRN_NullCheckingOnNullableType, "t").WithArguments("T?").WithLocation(7, 12));
}
}
}