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

Avoid warning assigning to readonly property marked [AllowNull] #39939

Merged
merged 3 commits into from
Nov 22, 2019
Merged

Avoid warning assigning to readonly property marked [AllowNull] #39939

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e779e32
Avoid warning assigning to readonly property marked [AllowNull]
cston Nov 21, 2019
6d0390d
PR feedback
cston Nov 21, 2019
e05435d
Add WorkItem attribute
cston Nov 22, 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
37 changes: 35 additions & 2 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 @@ -6009,14 +6009,47 @@ private FlowAnalysisAnnotations GetLValueAnnotations(BoundExpression expr)
{
BoundPropertyAccess property => getSetterAnnotations(property.PropertySymbol),
BoundIndexerAccess indexer => getSetterAnnotations(indexer.Indexer),
BoundFieldAccess field => field.FieldSymbol.FlowAnalysisAnnotations,
BoundFieldAccess field => getFieldAnnotations(field.FieldSymbol),
_ => FlowAnalysisAnnotations.None
};

return annotations & (FlowAnalysisAnnotations.DisallowNull | FlowAnalysisAnnotations.AllowNull);

static FlowAnalysisAnnotations getFieldAnnotations(FieldSymbol field)
{
var property = field.AssociatedSymbol as PropertySymbol;
return property is null ?
Copy link
Contributor

@RikkiGibson RikkiGibson Nov 21, 2019 •
edited by cston
Loading

Choose a reason for hiding this comment

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

consider using field.AssociatedSymbol is PropertySymbol property #Resolved

field.FlowAnalysisAnnotations :
getSetterAnnotations(property);
}

static FlowAnalysisAnnotations getSetterAnnotations(PropertySymbol property)
=> property.GetOwnOrInheritedSetMethod()?.Parameters.Last()?.FlowAnalysisAnnotations ?? FlowAnalysisAnnotations.None;
{
var accessor = property.GetOwnOrInheritedSetMethod();
if (accessor is object)
{
return accessor.Parameters.Last().FlowAnalysisAnnotations;
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}
if (property is SourcePropertySymbol sourceProperty)
{
return getPropertyAnnotations(sourceProperty);
}
return FlowAnalysisAnnotations.None;
}

static FlowAnalysisAnnotations getPropertyAnnotations(SourcePropertySymbol property)
{
var annotations = FlowAnalysisAnnotations.None;
if (property.HasAllowNull)
{
annotations |= FlowAnalysisAnnotations.AllowNull;
}
if (property.HasDisallowNull)
{
annotations |= FlowAnalysisAnnotations.DisallowNull;
}
return annotations;
}
}

private static bool UseLegacyWarnings(BoundExpression expr, TypeWithAnnotations exprType)
Expand Down
172 changes: 153 additions & 19 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 @@ -28059,15 +28059,8 @@ void M(C c)
}
void Deconstruct(out C? x, out C? y) => throw null!;
}";
// Note: we warn on the property initializer because it is a direct assignment to the backing field
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
// so the value is not getting normalized if the normalization is handled by the property setter

var comp = CreateNullableCompilation(new[] { source, AllowNullAttributeDefinition });
comp.VerifyDiagnostics(
// (4,44): warning CS8625: Cannot convert null literal to non-nullable reference type.
// [AllowNull] public C P { get; set; } = null;
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(4, 44)
);
comp.VerifyDiagnostics();
}

[Fact]
Expand Down Expand Up @@ -29302,8 +29295,8 @@ static void M5<T>(T? t5) where T : struct
new CStruct<T>().P5 = t5;
}
}";
var expected = new[]
{
var comp = CreateNullableCompilation(source, references: new[] { lib.EmitToImageReference() });
comp.VerifyDiagnostics(
// (13,14): warning CS8600: Converting null literal or possible null value to non-nullable type.
// t2 = null; // 1
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(13, 14),
Expand All @@ -29330,14 +29323,43 @@ static void M5<T>(T? t5) where T : struct
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "t5").WithLocation(35, 30),
// (36,31): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// new CStruct<T>().P5 = t5; // 9
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "t5").WithLocation(36, 31)
};

var comp = CreateNullableCompilation(source, references: new[] { lib.EmitToImageReference() });
comp.VerifyDiagnostics(expected);
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "t5").WithLocation(36, 31));

var comp2 = CreateNullableCompilation(new[] { source, lib_cs, DisallowNullAttributeDefinition });
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
comp2.VerifyDiagnostics(expected);
comp2.VerifyDiagnostics(
// (9,53): warning CS8625: Cannot convert null literal to non-nullable reference type.
// [DisallowNull]public TClass? P3 { get; set; } = null;
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(9, 53),
// (13,14): warning CS8600: Converting null literal or possible null value to non-nullable type.
// t2 = null; // 1
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(13, 14),
// (14,5): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// [DisallowNull]public TStruct? P5 { get; set; }
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "[DisallowNull]public TStruct? P5 { get; set; }").WithLocation(14, 5),
// (14,29): warning CS8601: Possible null reference assignment.
// new COpen<T>().P1 = t2; // 2
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t2").WithLocation(14, 29),
// (15,30): warning CS8601: Possible null reference assignment.
// new CClass<T>().P2 = t2; // 3
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t2").WithLocation(15, 30),
// (16,30): warning CS8601: Possible null reference assignment.
// new CClass<T>().P3 = t2; // 4, [DisallowNull] honored on TClass?
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t2").WithLocation(16, 30),
// (20,29): warning CS8601: Possible null reference assignment.
// new COpen<T>().P1 = t3; // 5
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t3").WithLocation(20, 29),
// (21,30): warning CS8601: Possible null reference assignment.
// new CClass<T>().P2 = t3; // 6
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t3").WithLocation(21, 30),
// (22,30): warning CS8601: Possible null reference assignment.
// new CClass<T>().P3 = t3; // 7, [DisallowNull] honored on TClass?
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t3").WithLocation(22, 30),
// (35,30): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// new COpen<T?>().P1 = t5; // 8
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "t5").WithLocation(35, 30),
// (36,31): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// new CStruct<T>().P5 = t5; // 9
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "t5").WithLocation(36, 31));

CompileAndVerify(comp2, sourceSymbolValidator: symbolValidator, symbolValidator: symbolValidator);

Expand Down Expand Up @@ -29400,10 +29422,11 @@ public class C
[DisallowNull]public string? P1 { get; set; } = null;
}
";

// Missing warning. Honor nullability attributes in method bodies https://github.com/dotnet/roslyn/issues/36039
var comp = CreateNullableCompilation(new[] { DisallowNullAttributeDefinition, source });
comp.VerifyDiagnostics();
comp.VerifyDiagnostics(
// (4,53): warning CS8625: Cannot convert null literal to non-nullable reference type.
// [DisallowNull]public string? P1 { get; set; } = null;
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(4, 53));
}

[Fact]
Expand Down Expand Up @@ -29432,6 +29455,12 @@ void M()

var comp = CreateNullableCompilation(new[] { DisallowNullAttributeDefinition, source });
comp.VerifyDiagnostics(
// (4,5): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// [DisallowNull]public int? P1 { get; set; } = null;
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "[DisallowNull]public int? P1 { get; set; } = null;").WithLocation(4, 5),
// (4,50): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// [DisallowNull]public int? P1 { get; set; } = null;
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "null").WithLocation(4, 50),
// (7,19): warning CS8607: A possible null value may not be assigned to a target marked with the [DisallowNull] attribute
// (P1, _) = (null, 1); // 1
Diagnostic(ErrorCode.WRN_DisallowNullAttributeForbidsMaybeNullAssignment, "(null, 1)").WithLocation(7, 19),
Expand Down Expand Up @@ -119197,5 +119226,110 @@ static T Get<T>()
// static T F6<T>() => true ? default : Get<T>();
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "true ? default : Get<T>()").WithLocation(13, 25));
}

[Fact]
[WorkItem(39926, "https://github.com/dotnet/roslyn/issues/39926")]
public void MaybeNullT_24()
{
var source =
@"#nullable enable
using System.Diagnostics.CodeAnalysis;
class C<T>
{
[MaybeNull]T P1 { get; } = default; // 1
[AllowNull]T P2 { get; } = default;
[MaybeNull, AllowNull]T P3 { get; } = default;
[MaybeNull]T P4 { get; set; } = default; // 2
[AllowNull]T P5 { get; set; } = default;
[MaybeNull, AllowNull]T P6 { get; set; } = default;
C([AllowNull]T t)
{
P1 = t; // 3
P2 = t;
P3 = t;
P4 = t; // 4
P5 = t;
P6 = t;
}
}";
var comp = CreateCompilation(new[] { source, AllowNullAttributeDefinition, MaybeNullAttributeDefinition });
comp.VerifyDiagnostics(
// (5,32): warning CS8601: Possible null reference assignment.
// [MaybeNull]T P1 { get; } = default; // 1
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(5, 32),
// (8,37): warning CS8601: Possible null reference assignment.
// [MaybeNull]T P4 { get; set; } = default; // 2
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(8, 37),
// (13,14): warning CS8601: Possible null reference assignment.
// P1 = t; // 3
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(13, 14),
// (16,14): warning CS8601: Possible null reference assignment.
// P4 = t; // 4
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(16, 14));
}

[Fact]
public void MaybeNullT_25()
{
var source =
@"#nullable enable
using System.Diagnostics.CodeAnalysis;
class C<T>
{
[NotNull]T P1 { get; } = default; // 1
[DisallowNull]T P2 { get; } = default; // 2
[NotNull, DisallowNull]T P3 { get; } = default; // 3
[NotNull]T P4 { get; set; } = default; // 4
[DisallowNull]T P5 { get; set; } = default; // 5
[NotNull, DisallowNull]T P6 { get; set; } = default; // 6
C([AllowNull]T t)
{
P1 = t; // 7
P2 = t; // 8
P3 = t; // 9
P4 = t; // 10
P5 = t; // 11
P6 = t; // 12
}
}";
var comp = CreateCompilation(new[] { source, AllowNullAttributeDefinition, DisallowNullAttributeDefinition, NotNullAttributeDefinition });
comp.VerifyDiagnostics(
// (5,30): warning CS8601: Possible null reference assignment.
// [NotNull]T P1 { get; } = default; // 1
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(5, 30),
// (6,35): warning CS8601: Possible null reference assignment.
// [DisallowNull]T P2 { get; } = default; // 2
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(6, 35),
// (7,44): warning CS8601: Possible null reference assignment.
// [NotNull, DisallowNull]T P3 { get; } = default; // 3
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(7, 44),
// (8,35): warning CS8601: Possible null reference assignment.
// [NotNull]T P4 { get; set; } = default; // 4
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(8, 35),
// (9,40): warning CS8601: Possible null reference assignment.
// [DisallowNull]T P5 { get; set; } = default; // 5
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(9, 40),
// (10,49): warning CS8601: Possible null reference assignment.
// [NotNull, DisallowNull]T P6 { get; set; } = default; // 6
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "default").WithLocation(10, 49),
// (13,14): warning CS8601: Possible null reference assignment.
// P1 = t; // 7
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(13, 14),
// (14,14): warning CS8601: Possible null reference assignment.
// P2 = t; // 8
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(14, 14),
// (15,14): warning CS8601: Possible null reference assignment.
// P3 = t; // 9
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(15, 14),
// (16,14): warning CS8601: Possible null reference assignment.
// P4 = t; // 10
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(16, 14),
// (17,14): warning CS8601: Possible null reference assignment.
// P5 = t; // 11
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(17, 14),
// (18,14): warning CS8601: Possible null reference assignment.
// P6 = t; // 12
Diagnostic(ErrorCode.WRN_NullReferenceAssignment, "t").WithLocation(18, 14));
}
}
}