Roslyn: Recognize annotations on methods that affect nullability analysis

Created on 10 May 2018 · 28Comments · Source: dotnet/roslyn

Examples, with placeholder attributes:
```c#
// ensures arg is true
static void Assert([EnsuresTrue] bool b);

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

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

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

c# // 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.

Area-Compilers Area-Language Design New Language Feature - Nullable Reference Types

Source

cston

👍9

Most helpful comment

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;
}

Joe4evr on 10 May 2018

👍9

All 28 comments

cc @jcouv

cston on 10 May 2018

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;
}

Joe4evr on 10 May 2018

👍9

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);

Eirenarch on 10 May 2018

👍2

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

Joe4evr on 11 May 2018

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:

```C#
[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:

```C#
// 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:
```C#
// 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:
```C#
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?

MrJul on 11 May 2018

👍2

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

Eirenarch on 11 May 2018

👍2

I believe that these Attribute names must be absolutely clear and unambiguous to be useful. Examples above are inprecise:
```C#
// false if arg is not null // must be the other way: arg is not null if false
static bool IsNullOrEmpty([NotNullWhenFalse] string? s)

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

jveselka on 5 Sep 2018

👎1 👍1

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 (https://github.com/dotnet/roslyn/issues/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?)

alrz on 5 Sep 2018

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.

adamfk on 28 Dec 2018

👍1

sorry if this is not appropriate here, but i think there also needs to be a way to specify nullability of members, parameters & return types in 3rd-party libraries (or even Microsoft libraries).

Spongman on 15 Jan 2019

How would Linq's Where be handled?

IEnumerable<string?> a;
var b = a.Where(x => x != null); // now it's logically IEnumerable<string>

In my code base, I circumvented this with an additional .Cast<string>() which makes the compiler happy.

Tragetaschen on 7 Feb 2019

We also need to handle: Interlocked.CompareExchange

There are other APIs in a similar category, e.g.

Interlocked.Exchange(ref ..., somethingNonNull)
Volatile.Write(ref ..., somethingNonNull)
LazyInitializer.EnsureInitialized(ref ...)

stephentoub on 1 Apr 2019

👍1

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

gafter on 1 Apr 2019

🎉2

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:
```C#
public CancellationTokenRegistration Register(Action

Roslyn: Recognize annotations on methods that affect nullability analysis

Most helpful comment

All 28 comments

Related issues