Recognize annotations on methods that affect nullability analysis

[cston]

Examples, with placeholder attributes:

// ensures arg is true
static void Assert([EnsuresTrue] bool b);

// false if arg is not null
static bool IsNullOrEmpty([NotNullWhenFalse] string? s)

// reference equality for null
[NullEquals] static bool Equals(object? x, object? y)

// result nullability matches arg
static string? NormalizePath([NullInNullOut] string? path)

// ref arg not reset to null
static void InitializeAndAdd<T>([NotNulled] ref List<T>? list, T item)

(jcouv updated): We also need to handle:

• Interlocked.CompareExchange
• a ref parameter which has a non-null inbound value, but possibly null outbound value (see FilterAndAppendAndFreeDiagnostics). This way the caller would be warned if passing a maybe-null argument. (Debug.Assert would not do that)
• TryGetValue on WeakReference
• object.ReferenceEquals
• Would it be possible to hint that AsNode() is safe if IsNode is true?
• Should those attribute affect the inferred type? var x = EnsuresNotNull(something);
• What about M(x = y, y) where EnsuresNotNull is on the second parameter, or M(y, x = y) where it is on the first?

Filed https://github.com/dotnet/corefx/issues/33880 to track work to annotate the BCL and collect ideas of APIs needing annotations.

We should confirm whether the nullable attributes should be added to mono or not.

[cston]

cc @jcouv

[Joe4evr]

Any plans to also include an annotation for marking a method as not returning (dotnet/csharplang#538)/always throwing (dotnet/csharplang#739)? Since throw helpers are still pretty common, it'd be useful if the flow analysis could be informed of those.

public int M(string s)
{
    if (s == null)
        ThrowHelper.ThrowArgNullException(nameof(s));

    //'s' is definitely not null here, but how will the compiler know?
    return s.Length;
}

[Eirenarch]

I second @Joe4evr's request. I use throw helper methods extensively in Web API code with HttpResponseException such as

User user = await UserManager.FindByIdAsync(userId)
ThrowNotFoundIfNull(user);

[Joe4evr]

@Eirenarch Looking at #26656, [EnsuresNotNull] will also be a thing. 🎉

[MrJul]

As a long time user of ReSharper's annotations and nullability analysis, I'm very happy to see nullability annotations coming with the nullable reference types feature.

However, the numbers of attributes used to represent the various true/false/null/not-null/throws state combinations could grow quite large if you want to handle most cases, and I believe that most cases should be handled to ease adoption of this feature. The goal isn't to recreate code contracts, but to allow the nullability analysis to handle 99% of cases while keeping the attributes readable.

ReSharper solves this with ContractAnnotation, having its own syntax. I understand that introducing a completely new syntax for this in Roslyn would be quite out of scope. That's why I propose the following ImpliedNullabilityAttribute:

[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = true)]
public sealed class ImpliedNullabilityAttribute : Attribute
{
    ImpliedNullabilityAttribute(SourceArgumentKind source, TargetResultKind target);
    ImpliedNullabilityAttribute(SourceResultKind source, TargetArgumentKind target);
}

public enum SourceArgumentKind { True, False, Null, NotNull }
public enum TargetResultKind { True, False, NotNull, Throws }

public enum SourceResultKind { True, False, Null, NotNull, Any }
public enum TargetArgumentKind { True, False, NotNull }

Naming is hard, and I'm open to anything.
Let's focus on how the attribute will influence nullability analysis.

• The attribute should only be applied to nullable parameters.
• The first overload is applied to input arguments: if the argument's nullability state matches the SourceArgumentKind value, then the compiler infers that the method return value's nullability matches TargetResultKind.
• The second overload is applied to output arguments: if the method result's nullability state matches the SourceResultKind value, then the compiler infers that the argument's nullability matches TargetArgumentKind.
• Of course, these could also be two different attributes.

Applied to the examples above, the syntax would be:

// ensures arg is true
void Assert(
    [ImpliedNullability(SourceArgumentKind.False, TargetResultKind.Throws)] bool b
)

// false if arg is not null
bool IsNullOrEmpty(
    [ImpliedNullability(SourceArgumentKind.NotNull, TargetResultKind.False)]
    string? s
)

// result nullability matches arg
string? NormalizePath(
    [ImpliedNullability(SourceArgumentKind.NotNull, TargetResultKind.NotNull)]
    string? path
)

// ref arg not reset to null
void InitializeAndAdd<T>(
    [ImpliedNullability(SourceResultKind.Any, TargetResultKind.NotNull)] ref List<T>? list,
    T item
)

Other common use cases:

// ensures arg is not null
void ThrowIfNull(
    [ImpliedNullability(SourceArgumentKind.Null, TargetResultKind.Throws)] object? o
);

// not null if arg is null
string? GetValidationError(
    [ImpliedNullability(SourceArgumentKind.Null, TargetResultKind.NotNull)] object? o
)

Or even more sophisticated cases for existing classes:

bool Version.TryParse(
    [ImpliedNullability(SourceArgumentKind.Null, TargetResultKind.False)] string? input,
    [ImpliedNullability(SourceResultKind.True, TargetArgumentKind.NotNull)] Version? output
)

string? input = ...;

if (Version.TryParse(input, out Version? version)) {
   // input is known to be not null
   // version is known to be not null
}

Various points:

• This is a more general solution, and as such is likely to be more complicated to implement.
• This proposal doesn't handle [NullEquals] since it's quite a different beast.
• It's verbose. Personally I don't think it's a big deal as it's a one-time thing to do when writing the method. Compare it with having every caller having to write code to assert or handle the mismatching nullability. using static can help with the enums here.
• Since the attribute can be specified several times (eg. (True, NotNull) and (True, Throws)), it allows for contradicting contracts. The compiler could either warn or ignore them.
• Same thing with specifying "out" contracts on input parameters, and vice versa.

I believe that this would handle most of the nullability cases. What do you think?

[Eirenarch]

I fail to see how this is better than different attributes. It seems more complex and less readable. With separate attributes which are appropriately named things will be much simpler even if there are a lot of them

[jveselka]

I believe that these Attribute names must be absolutely clear and unambiguous to be useful. Examples above are inprecise:

// false if arg is not null // must be the other way: arg is not null if false
static bool IsNullOrEmpty([NotNullWhenFalse] string? s)

// result nullability matches arg // equivalence?
static string? NormalizePath([NullInNullOut] string? path) // implication?

[alrz]

static void Assert([EnsuresTrue] bool b);

Do these have compile-time guarantees or it's merely used for metadata? i.e. could give error if the control leaves the method while b is not statically proved to be true.

That sounds like method contracts (#119)

static void Assert(bool b) ensures b
static void Fail(string msg) ensures false
static bool IsNullOrEmpty(string? s) returns s != null
static bool Equals(object? x, object? y) returns x == y
static string? NormalizePath(string? path) returns path
static void InitializeAndAdd<T>(ref List<T>? list, T item) ensures list != null

The question is how we're going to formulate contracts in metadata (in a general way?)

[adamfk]

After playing around with the VS 2019 preview, I'm definitely looking forward to this being implemented. It will make null warnings much more intelligent & useful.

Mads mentioned in this video that a mini language of attributes is being developed to allow the compiler to analyze across assemblies.

My question is whether this can be done transparently by the compiler. I love the prospect of intelligent non-nullable analysis, but I don't want to learn a mini language of attributes and I don't want to repeat & maintain my code logic in the mini language.

I really think this needs to be auto generated for the majority of uses. For the 2% of cases where the compiler can't analyze it properly, manually written annotations could be set to override the compiler's.

This auto generation could be implemented by a 3rd party "Analyzer with Code Fix" nuget, but I'd really prefer it was baked in for all users as I'd be concerned with method attributes going stale / rotting and then essentially being untrustworthy.

[gafter]

For methods that do not return, we are considering adding dotnet/csharplang#538 to the scope of C# 8.

[stephentoub]

Here's another case we're hitting with some frequency in coreclr/corefx...

It's a fairly common pattern for methods that do callbacks to take Action<object> and object state arguments, where the object state will be passed into the Action<object> when it's invoked. Thus, the argument to the Action<object> is null if and only if the object state is null.

Consider a method like:

public CancellationTokenRegistration Register(Action<object> callback, object state)

on CancellationToken. This should be annotated as:

public CancellationTokenRegistration Register(Action<object?> callback, object? state)

but that means when I write code like:

ct.Register(thisRef => ((CurrentType)thisRef).Foo(), this);

I'm warned that I may be dereferencing a null thisRef, even though it's obviously not null. If we could annotate the generic argument to Action<object> in some way, e.g.

public CancellationTokenRegistration Register(Action<[NotNullWhenNotNull(nameof(state))]object?> callback, object? state)

then the compiler would be able to see that the argument this passed in as the state argument was non-null, and thus that the Action<object?> was actually Action<object>, and thus it could treat the thisRef as non-null and not warn.

As it stands, most code using such methods (which often know statically that the state argument isn't null) will need to use !, e.g.

ct.Register(thisRef => ((CurrentType)thisRef)!.Foo(), this);

cc: @dotnet/nullablefc

[stephentoub]

Another situation...

There are places where the nullability of the return value is based on a Boolean argument to the method, e.g.
https://github.com/dotnet/coreclr/blob/40dda195590873d18236bfbc7742bb6fe7305730/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs#L198

private ReaderWriterCount GetThreadRWCount(bool dontAllocate)

This can return null if dontAllocate is true, but if it's false it'll never return null.

Related, we have methods like:
https://github.com/dotnet/coreclr/blob/40dda195590873d18236bfbc7742bb6fe7305730/src/System.Private.CoreLib/src/System/Delegate.cs#L387

public static Delegate CreateDelegate(Type type, Type target, string method, bool ignoreCase, bool throwOnBindFailure)

that will never return null if a Boolean argument (in this case, throwOnBindFailure) is true, but that may return null if it's false.

cc: @dotnet/nullablefc

[stephentoub]

Another case, ref arguments guaranteed to be non-null after the method returns, e.g.
https://github.com/dotnet/coreclr/blob/40dda195590873d18236bfbc7742bb6fe7305730/src/System.Private.CoreLib/shared/System/Threading/ReaderWriterLockSlim.cs#L916

private void LazyCreateEvent(ref EventWaitHandle waitEvent, EnterLockType enterLockType)

cc: @dotnet/nullablefc

[svick]

@stephentoub Regarding object state: Those methods currently don't flow anything about the type of state, does it actually make sense to start flowing nullability, but not the rest of the type information?

In other words, if you changed the method to Register<T>(Action<T> callback, T state), then nullability would flow naturally, since it would be part of T. (Of course changing the method would be a binary breaking change, but I think adding the generic version as an overload should be fine.)

[stephentoub]

does it actually make sense to start flowing nullability, but not the rest of the type information?

I think it does. It also might actually be all the type information available.

if you changed the method to Register(Action callback, T state), then nullability would flow naturally, since it would be part of T

As you say, we can't change the signature. Even if we were to add a new one, the existing one would still be there, and if you actually had something typed as object, that overload would continue to be used. Plus, adding a new version of some of these APIs isn't as simple as just adding an overload, at least not to do it "correctly", as it then means the implementation needs to support multiple (or more) kinds of callbacks, needs to type test for them, and so on, which might be fine or might add problematic overhead. (And some are on interfaces, which would require DIM to augment.)

[tannergooding]

Exception Marshal.GetExceptionForHR(int errorCode) only returns null for errorCode >= 0.

The following pattern is common and currently requires a ! after the throw statement:

if (errorCode < 0)
{
    throw Marshal.GetExceptionForHR(errorCode);
}

Providing a way to say: "EnsuresNotNull when errorCode < 0" would be useful.

I also logged #34754, as I'm not sure we should warn for throw <potential null> given that we will be throwing an exception regardless.

[jkotas]

The following pattern is common and currently requires a ! after the throw statement

ThrowExceptionForHR is an API that encapsulates the pattern. Switching to this API is more compact code and does not have this problem. Encapsulating helpers like ThrowExceptionForHR may be a better solution for complex conditions like "EnsuresNotNull when errorCode < 0".

[terrajobst]

I agree with Jan

[stephentoub]

I'm not sure what the attribute would be here, but here's a case that I'm expecting will cause a fair number of spurious warnings...

ArraySegment<T>.Array needs to be T[]?, because the array can actually be null, e.g. default(ArraySegment<T>).Array == null. But in common usage, e.g.

if (buffer.TryGetArray(out ArraySegment<T> arraySegment)
{
    Use(arraySegment.Array);
}

the Array should be considered non-null, with the TryGetArray method guaranteeing that if it returns true it's giving back a validly-backed ArraySegment<T>. This is a variant of the TryXx patterns mentioned earlier, because this is giving back a struct that wraps the reference type rather than giving back the reference type directly.

[stephentoub]

I've come across a few cases now where code does effectively:

if (!ReferenceEquals(something, null)) something.Foo();

and the something.Foo() warns that something could be null. We currently have no way to annotate Object.ReferenceEquals in a way that would allow the compiler to know that something here is non-null.

[jkotas]

!ReferenceEquals(something, null)

Is there a problem with changing this to !(something is null) ?

[stephentoub]

Is there a problem with changing this to !(something is null) ?

No, that's what I've been doing as I encounter them. I'm just calling out examples where code changes are required in order to silence warnings (either using ! or rewriting your code), and where that could be avoided if we could better annotate methods.

[Joe4evr]

@stephentoub Regarding the Action<object> + object state pattern, I've posted an idea (which might still need a lot of work) in dotnet/csharplang#2471. I'll repeat it here for good measure:

On-the-spot idea: [NullablePassthrough] for the object?, indicating that the delegate's argument takes on the nullability of the annotated parameter.

// example using one of System.Threading.Timer's ctors:
public Timer(TimerCallback callback, [NullablePassthrough(0, 0)] object? state, TimeSpan dueTime, TimeSpan period) { }

The (0, 0) arguments are like a navigation to which method parameter is the target delegate is and which delegate parameter is the target argument.

For the consumer, it'd be nice if it simply ended up working like this:

var withNull = new Timer(o => o.GetType(), //warning: potential dereference of a 'null' reference
    null, //because this is the null-literal (or a nullable variable is passed in) 'o' is treated as object?
    TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10));

var withNonNull = new Timer(o => o.GetType(), //no warning
    "Definite non-null object", //because this is a non-null variable, 'o' is treated as object!
    TimeSpan.FromMinutes(10), TimeSpan.FromMinutes(10));

This might be way too complicated to work, but I'm not letting that stop me from putting the idea out there.

[terrajobst]

This might be way too complicated to work, but I'm not letting that stop me from putting the idea out there.

Exactly my thoughts. Seems in line with the other attributes though.

[jcouv]

This issue was superseded by #35816 which describes our plan for C# 8. Also see the language proposal which describes the attributes and their meaning: https://github.com/dotnet/csharplang/blob/master/meetings/2019/LDM-2019-05-15.md

I'll go ahead and close the present issue.