Honor attributes on fields #36704
Honor attributes on fields #36704
Conversation
jcouv
added
Area-Compilers
Feature - Nullable Reference Types
| private const int HasDisallowNullAttribute = 0x1 << 0; | ||
| private const int HasAllowNullAttribute = 0x1 << 1; | ||
| private const int HasMaybeNullAttribute = 0x1 << 2; | ||
| private const int HasNotNullAttribute = 0x1 << 3; |
There was a problem hiding this comment.
Handling individual bits here is not necessary since FlowAnalysisAnnotations is already stored as a set of bits (see PEParameterSymbol.PackedFlags). #ByDesign
There was a problem hiding this comment.
PEParameterSymbol can store all the possible annotations, but here we only need to store only four, which are not even contiguous (see below). So I think we have to deal with individual bits/flags.
[Flags]
internal enum FlowAnalysisAnnotations
{
None = 0,
AllowNull = 1 << 0,
DisallowNull = 1 << 1,
MaybeNullWhenTrue = 1 << 2,
MaybeNullWhenFalse = 1 << 3,
MaybeNull = MaybeNullWhenTrue | MaybeNullWhenFalse,
NotNullWhenTrue = 1 << 4,
NotNullWhenFalse = 1 << 5,
NotNull = NotNullWhenTrue | NotNullWhenFalse,
AssertsTrue = 1 << 6,
AssertsFalse = 1 << 7,
}In reply to: 296767023 [](ancestors = 296767023)
| static PackedFlags() | ||
| { | ||
| // Verify a few things about the values we combine into flags. This way, if they ever | ||
| // change, this will get hit and you will know you have to update this type as well. |
There was a problem hiding this comment.
Consider simplifying or eliminating this comment. In #36152, all similar comments have been changed to "Verify masks are sufficient for values." #Resolved
| var source = @" | ||
| class D | ||
| { | ||
| static void M1<T>(T t1) |
| new COpen<T?>().f1 = t5; | ||
| new CStruct<T>().f5 = t5; | ||
| } | ||
| }"; |
There was a problem hiding this comment.
Consider testing:
static void M6<T>([MaybeNull]T t6) where T : class
{
new CClass<T>().f2 = t6;
}
static void M7<T>([NotNull]T? t7) where T : class
{
new CClass<T>().f2 = t7;
}
...
``` #Resolved
| #if DEBUG | ||
| static PackedFlags() | ||
| { | ||
| Debug.Assert(EnumUtilities.ContainsAllValues<FlowAnalysisAnnotations>(0xFF)); |
There was a problem hiding this comment.
The assert seems unrelated to the number of bits stored. #Resolved
There was a problem hiding this comment.
I'd kept the assertion to alert us if the enum changes, so that we consider adding handling here. That said, that seems superfluous, so I'll remove.
In reply to: 296972213 [](ancestors = 296972213)
| } | ||
| public class CClass<TClass> where TClass : class | ||
| { | ||
| [MaybeNull]public TClass f2 = null!; |
There was a problem hiding this comment.
Out of scope, but I wonder how common it will be for people to misuse the attributes like this.
I think any usage of [MaybeNull] on a non-annotated reference type is probably wrong. The correct attribute usage would probably be to annotate as nullable, right?
There was a problem hiding this comment.
In this specific case, it seems okay to use the [MaybeNull]. That indicates that you could get a null out, but would be disallowed to put a null in.
I don't think that's very useful, but it is not strictly equivalent to saying TClass? f2 = null;
| } | ||
| } | ||
|
|
||
| public abstract FlowAnalysisAnnotations FlowAnalysisAnnotations { get; } |
There was a problem hiding this comment.
This name was pretty confusing for me at first. It doesn't really say anything about nullable or that it's only about the attributes, not ?
There was a problem hiding this comment.
This only represents the annotations from user-specified attributes. I agree this name is a bit confusing (we have annotations like ? and flow analysis annotations like [DisallowNull]). I'll think about this for a subsequent PR.
Thanks
3 participants
Part of #35816 (work items for attributes)