Support NotNullIfNotNull on Task<T?> return type

[ronnygunawan]

Version Used: .NET Core 3.0 preview 8

Steps to Reproduce:

[return: NotNullIfNotNull("value")]
static Task<string?> PassThroughAsync(string? value) {
	return Task.FromResult(value);
}

static void Main(string[] args) {
	string s = PassThroughAsync("Hello World!").Result; // Warning CS8600  Converting null literal or possible null value to non - nullable type.
	Console.WriteLine(s);
}

Expected Behavior:
No warning

Actual Behavior:
Warning CS8600 Converting null literal or possible null value to non - nullable type.

[jaredpar]

Moving to csharplang as this is a design question around the language, not an implementation bug in the compiler.

[jnm2]

Also, Dapper has Task<T> QuerySingleOrDefaultAsync<T>(...). Should there be a way to apply [MaybeNull] to the usage of T in the return type?

Otherwise, you get no warnings if you forget to check a reference type for null:

var maybeNull = await QuerySingleOrDefaultAsync<string>(...);
maybeNull.IndexOf("foo");

In this particular instance, a nice alternative would be if there was a way for QuerySingleOrDefaultAsync to constrain T to allow value types, nullable value types, and nullable reference types, but not non-nullable reference types.

[dotMorten]

I just hit this case too. I'll never return a null Task, but the T will be null if the parameter is null.

This annotation says I won't return a task that's null, but I'm much more interested in saying that T won't be null.

        [return:System.Diagnostics.CodeAnalysis.NotNullIfNotNull("instance")]
        public static Task<MyType2?> ConvertObject(this MyType? instance)
        {
            if (instance == null)
                return Task.FromResult<MyType2?>(null);
            return Task.Result(ConvertObjectHelper(instance)); //Won't return null MyType2
        }

[bennycoomans]

I'm hitting this case as well, using .NET 5.

@jaredpar Do you have any idea whether this is under the attention of the C# language team? Or should this be registered as an Issue to achieve that?

[Logerfo]

Maybe something like this:

public Task<[NotNullIfNotNull("source")] string?> PassThroughAsync(string? value)

[Logerfo]

I tried to look up for it but couldn't find any issue related to this on dotnet/runtime.

[JWilh]

What about doing it like Jetbrains did it with there "ItemNotNull" annotation they had:
https://www.jetbrains.com/help/resharper/Reference__Code_Annotation_Attributes.html#ItemNotNullAttribute

[ItemNotNullIfNotNull("source")]
public Task<string?> PassThroughAsync(string? value)

[ronnygunawan]

For Task<T?> we can have [ResultNotNullIfNotNull("source")] instead

[Logerfo]

I understand that what Jetbrains did covers most scenarios and offers value to their clients, but it's not exhaustive, since it only works for generic types of arity 1. If the compiler is going to add this feature, it makes more sense to support n-arity types be feature-complete. It would indeed require changes to the runtime, which increases costs and we'll inevitably need to work harder and wait longer for it, but on the long term I think it's for the best.

[yaakov-h]

Even #2478 didn't propose adding runtime support.

[RikkiGibson]

I'll consider adding this to #6888 when revising it. It feels like a broader, more coherent take on nullable attributes and tasks would be appropriate.

[gurustron]

@RikkiGibson I would argue that this two are a bit different. Issue is not with Task only but with any generic data holder (IEnumerable, custom wrappers, etc). Ideally we need a way to tell the compiler that closing type is not nullable if some condition, i.e. for example (from this SO question):

string? s1 = GetValueOrDefault<string>(null);
string s2 = GetValueOrDefault<string>("");   // works, C# realizes that the return value can't be null
        
IEnumerable<string?> e1 = GetValuesOrDefault<string>(null);
IEnumerable<string> e2 = GetValuesOrDefault<string>("");   // I want to get rid of the warning here
 
[return: NotNullIfNotNull(nameof(defaultValue))]
public static T? GetValueOrDefault<T>(T? defaultValue)
{ 
    return GetValueFromSomewhere<T>() ?? defaultValue;
}
[return: ???]
public IEnumerable<T?> GetValuesOrDefault<T>(T? defaultValue)
{
    return GetValuesFromSomewhere<T>().Select(x => x ?? defaultValue);
}

[RikkiGibson]

I'm happy to look more closely at this. However, I do want to say that we are trying to be very careful about how much we pull on this thread. There's a whole universe of flow-conditional typing which we don't necessarily want to build out due to the complexity of doing so correctly and usefully.

In the case where GetValuesOrDefault just returns a type parameter, it would make sense to just use method type argument inference here. I think there's usually not a point in using [NotNullIfNotNull] when the values whose nullability we want to modify are of a single type parameter type coming from the enclosing method. SharpLab.

public IEnumerable<T> GetValuesOrDefault<T>(IEnumerable<T?> valuesFromSomewhere, T defaultValue)
{
    return valuesFromSomewhere.Select(x => x ?? defaultValue);
}

[gurustron]

@RikkiGibson thank you!

[ErroneousFatality]

(This is really disappointing that 4.5 years later we don't have support for this)

Could we add a special case for async Task<T?> MethodAsync() usecases for the [return: NotNullIfNotNull("name")] and sibling annotations to work the same as if the method was T? Method() given that this is a really common usecase?

[RiversJohn]

This is still not an insignificant issue when writing any kind of library code, fallback value parameters are a rather common usecase.

The rather obvious solution specifically for Task ValueTask types would be:

[result: NotNullIfNotNull(nameof(fallbackValue))]
public async ValueTask<T?> MyMethodAsync(T? fallbackValue = default(T?)) 
{ 
    /* Code guaranteeing the contract is fulfilled */
}

[colejohnson66]

Is this even a language question? If I recall correctly, nullability flow is handled by analyzers, not the compiler. Essentially, regardless of modifying the existing attributes or adding new ones, this problem would be handled by dotnet/roslyn-analyzers, not dotnet/csharplang.

In fact, I'm sure one could write an analyzer that suppresses nullability warnings based on the attributes.

[RikkiGibson]

It is essentially a language question. LDM needs to buy in on any significant changes to nullability warnings, and nullable analysis is done directly in the compiler.

[LWChris]

So this was moved from roslyn to csharplang in 2019, and now, more than 5 years later, we figure this is something that only roslyn can decide on? 😄

[333fred]

No? This is a proposal to change a language feature, it is in the correct place.

[LWChris]

Then I misunderstood the implications of Rikki's

"It would be a significant change to the runtime, libraries and tooling to actually allow attributes on type arguments"
"There's a whole universe of flow-conditional typing"
"nullable analysis is done directly in the compiler"

I thought all this was evidence that this request goes way beyond the scope of a language feature.

[333fred]

All language features impact the entire ecosystem. That doesn't make them not language features.

[dmillerkeyfive]

Direly needed. No progress yet?

[RikkiGibson]

If anyone feels strongly enough to review the existing design notes and come up with a proposal which addresses the key scenarios and concerns, I will be happy to review/champion it. Thanks!

[ErroneousFatality]

@RikkiGibson So... you, an experienced developer working on the system for 7 years, are telling your customers, who for years are suggesting a logical improvement to the system you are paid handsomely to work on, to do your job for you, which you'll gladly then publish and take the credit for?
Pretty slick, Rikki.

[Suchiman]

@ErroneousFatality while i understand the frustration, because so am i, that's not really fair. There just happens to be higher priority items that just didn't allow this to get sufficient priority yet. Championing it, is the term to describe the person that pushes a topic through the design meetings and means that Rikki is offering to do that here, to represent it in discussions and argue for it to be completed and released. Community contributions are typically called out on release notes so its not even taking the credit.