-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add MemberNotNull/When attributes #33567
Conversation
@stephentoub Do we care about CLS compliance? If not, I'll suppress the diagnostic. |
What is the compiler complaining about here? i.e. what here isn't CLS compliant? |
The compiler is complaining because of the |
I see. I could not have told you arrays as attribute arguments aren't CLS compliant. |
I assume attributing it as [CLSCompliant(false)] makes it go away? If so, let's do that. |
We've been having another conversation (cc: @jkotas, @ericstj, @terrajobst) about whether we care about CLS compliance at all moving forward, but here I wouldn't worry about it at all, regardless. |
src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
Outdated
Show resolved
Hide resolved
@@ -125,4 +125,41 @@ sealed class DoesNotReturnIfAttribute : Attribute | |||
/// <summary>Gets the condition parameter value.</summary> | |||
public bool ParameterValue { get; } | |||
} | |||
|
|||
/// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary> | |||
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, AllowMultiple = false, Inherited = false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AllowMultiple = false is the default and so doesn't need to be specified. Same below.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
Show resolved
Hide resolved
sealed class MemberNotNullAttribute : Attribute | ||
{ | ||
/// <summary>Gets field or property member names.</summary> | ||
public string[] Members { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terrajobst, concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terrajobst Just confirming that it's okay to add the properties to store the arguments to the attribute. This is following what we did for other nullability attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. NullableAttribute
also exposes a raw byte array. /cc @bartonjs
@@ -5616,6 +5616,21 @@ public sealed partial class NotNullWhenAttribute : System.Attribute | |||
public NotNullWhenAttribute(bool returnValue) { } | |||
public bool ReturnValue { get { throw null; } } | |||
} | |||
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false)] | |||
[CLSCompliant(false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add the "System." namespace to these (see existing examples). The tool that generates these files would do so, so without them this will just result in churn the next time the files are auto-gen'd.
{ | ||
/// <summary>Gets field or property member names.</summary> | ||
public string[] Members { get; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please move the properties to below the ctor (here and in the other attribute as well)
/// The list of field and property members that are promised to be not-null. | ||
/// </param> | ||
public MemberNotNullAttribute(params string[] members) | ||
=> this.Members = members; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove the "this."
public MemberNotNullWhenAttribute(bool returnValue, params string[] members) | ||
{ | ||
this.ReturnValue = returnValue; | ||
this.Members = members; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove the "this."
@stephentoub I think this is ready to merge/squash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
@jcouv, do you know when dotnet/runtime will get a compiler that respects this? |
@stephentoub I checked that version |
Shouldn't be 😉 Line 87 in ef71836
|
Fixes #31877
Relates to dotnet/roslyn#41438 (add support for MemberNotNull in C# 9)