API Proposal: MemberNotNullWhenCompleteAttribute

[Kuinox]

Background and Motivation

MemberNotNullAttribute indicate that when the method return, the given members are not null.
This is perfect when your Initialisation code is fully synchronous.

But when using asynchronous code, the method may return on each await;

Potato? _potato;
[MemberNotNull(  nameof( _potato ))]
async Task FetchPotatoAsync()
{
    await Task.Delay( 100 ); // Member '_potato' must have a non-null value when exiting.
    await Task.Delay( 100 ); // Member '_potato' must have a non-null value when exiting.
    _potato = new Potato();
}

This means we can't use MemberNotNullAttribute to indicate that a member wont be null when an asynchronous operation completes.
I believe this case is fairly common to justify either a change of behavior in MemberNotNull or a new attribute, MemberNotNullWhenCompleteAttribute.

Currently, if we ignore the warnings in FetchPotatoAsync, it will lead to the current behavior:

async Task PreparePotato()
{
   Task task = FetchPotatoAsync();
   _potato.Cook(); // NullReferenceException on runtime, no nullable analysis.
   await task;
   // here _potato wont be null anymore.
}

Proposed API

This would be a verbatim copy of the MemberNotNullAttribute, except it guarantee the members wont be null when the returned value has been awaited/is completed.

Usage Examples

Potato? _potato;

[MemberNotNullWhenComplete(  nameof( _potato ) )]
async Task FetchPotatoAsync()
{
    await Task.Delay( 100 );
    await Task.Delay( 100 );
    _potato = new Potato();
}

async Task PreparePotato()
{
   Task task = FetchPotatoAsync();
   // _potato may be null here.
   await task;
   // _potato is not null here.
}

Alternative Designs

The behavior of MemberNotNullAttribute can be altered:
Stop emitting warning Member '_potato' must have a non-null value when exiting. on awaits, and thats it.
It would be way more error prone to the user, but is way easier to implement.

Risks

I'm not aware of a risk.

[RikkiGibson]

Should there be a version of this attribute analogous to MemberNotNullWhen? That is, something which would result in the following analysis behavior:

async Task<bool> TryFetchPotatoAsync() => ...;

if (await TryFetchPotatoAsync())
{
    _potato.ToString(); // ok: _potato is not null
}
else
{
    _potato.ToString(); // warning: _potato might be null
}

[alrz]

Interestingly, the warning is being suppressed in the first few hits. Which means at the call-site it just gives the wrong result (no warning) if the method is not awaited + not very useful in the declaration itself.

[alrz]

That just proves my point which is: if the method is async the members are most likely set only after awaiting it. Am I wrong in assuming that that's 99% of the cases?

[Kuinox]

That's not wrong, it's the most likely usage.
Having an async variant of the attribute is a bit more complex to the user but cover nicely all the use cases.

[alrz]

This is certainly a safer approach for that 1% usage at the cost of added annotations which in my opinion sounds like overkill.
Just pointing out that if the team is at all interested to do something here, this could just be a bugfix due to the low impact.

[Kuinox]

That what I already proposed in Alternative Designs.

[SoftCircuits]

No response from MS on this? I just figured out why MemberNotNullWhenAttribute doesn't work on the async version of my method. Trying to work around this is going to be a major pain.

[lonix1]

How does the team decide when an issue like this warrants attention? I assume it's not based on upvotes...

[CyrusNajmabadi]

It's when an ldm member decides it warrants it and brings it up to go through the design process.

[SoftCircuits]

I had to write documentation with my SoftCircuits.FixedWidthParser NuGet package that explains how a couple of async methods that populate a property will still produce a warning if you don't first check for null because the MemberNotNullWhenAttribute attribute is not supported for async methods. In other words, I had to document this shortcoming. Hard to understand how this wouldn't been seen as something that should be addressed.

[CyrusNajmabadi]

All things have shortcomings. And there are thousands of different requests from different customers for things they view as needing to be addressed. Invariably, choices will be made that do not align with particular subsets of the user base.

[lonix1]

So what you're saying is, we need a man on the inside... Some bribery then? I can offer expensive high quality cigars. 😉

[RikkiGibson]

The nullability improvements working group discussed this issue yesterday.

[SoftCircuits]

We think we would also like to change the behavior of [NotNullWhen(...)]/[MaybeNullWhen(...)] so that when the return type is Task<bool>, or perhaps any awaitable thing that produces a bool when awaited, those postconditions depend on the awaited value rather than being meaningless/unusable.

That would be great--and intuitive as well. It's not clear to me if this could break existing code. It doesn't really seem like it's usable as it is now, so I don't know what people have been doing with it.

[Kuinox]

I made a regex to find such usage: /Task<bool>\s+[A-Za-z0-9]+\(\[[a-zA-Z]*NullWhen/
I found a total of 3 usages of this pattern, but it looks like a mistake every time:
https://github.com/CommunityToolkit/Maui/blob/main/src/CommunityToolkit.Maui/Behaviors/Validators/TextValidationBehavior.shared.cs#L157
https://github.com/StefanPuia/nadeko/blob/6bc4546c6feb09984118790756c4adec1a90f029/src/NadekoBot/Modules/Searches/Searches.cs#L601
https://github.com/tellurianinteractive/Tellurian.Trains.ModulesRegistryApp/blob/663cfa43ae5f63511fe6e1e9b7d1cf3b0de5d0c7/SourceCode/Services/Extensions/ClaimsPrincipalExtensions.cs#L65

[SoftCircuits]

@Kuinox Nice. So instead of breaking old code, it might fix it! :)

[MisinformedDNA]

Any progress on this?

[RikkiGibson]

Nothing since https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-01-18.md#nullable-post-conditions-and-asyncawait.

Another spitballed proposal gained a bit of traction as we were discussing the above 3 points: adding a new RequireAwait property to our nullability attributes, with some
appropriate defaults, and using that to propagate information on nullability. This has some advantages in being very precise, and likely being easy to guide users towards
(the warning for awaiting without fulfilling a MemberNotNull could mention setting that property, for example, or if a user sets that property and doesn't need to we
could warn), and not requiring significant new amounts of metadata to put on every async method to mark it as such.

[martindybal]

I use this "hack". But I don't like it, and I would be very happy if it got into the language, no matter how.

Potato? _potato;
[MemberNotNull(  nameof( _potato ))]
async Task FetchPotatoAsync()
{
    _potato = null!;
    await Task.Delay( 100 );
    _potato = new Potato();
}

[AartBluestoke]

I am surprised that the concept of "complete" for an async function call is "at first line of code that contains async", not "when the task is complete".

If i used the Result property of an Task that is not completed (or look at any other side-effect of the execution), i'm in undefined behavior; of course prior to the await the _potato could be NULL - i wouldn't expect invariants of the function to be enforced while the Task is incomplete ...

[MemberNotNull(  nameof( _potato ) )]
async Task FetchPotatoAsync()
{
    await Task.Delay( 100 );
    await Task.Delay( 100 );
    _potato = new Potato();
} // <-- this is the end of the scope where i (naively) expect the "_potato is not null" invariant to start applying.

async Task PreparePotato()
{
   Task task = FetchPotatoAsync();
   // _potato may be null here. <-- The task may be complete -- the Result is also possibly not available ..
   await task;
   // _potato is not null here.
}

not the pit of success, as I'd expect the same behavior as the blocking version...

[MemberNotNull(  nameof( _potato ) )]
async Task FetchPotato()
{
    Thread.Sleep( 100 );
    Thread.Sleep( 100 );
    _potato = new Potato();
} // <-- this is the end of the scope where i (naively) expect the "_potato is not null" invariant to start applying.

async Task PreparePotato()
{

  _potato  = FetchPotatoAsync(); 
  //  _potato may be null during the execution of this blocking call
  
  // <-- not async, no opportunity to do other helpful stuff here
  // _potato is not null here.
}

[lonix1]

There's another variant of this problem, in a lazy-loaded scenario:

Potato? _potato;

[MemberNotNull(nameof(_potato))]
public async Task FetchPotatoAsync()
{
    if (_potato != null) return;

    await Task.Delay(100);       // WARNING: Member '_potato' must have a non-null value when exiting. [CS8774]

    _potato = new Potato();
}

In this particular case, I think the analyser should not complain because the object is already non-null.

I added a feature request here.

[colejohnson66]

Is the Thread.Sleep supposed to represent some asynchronous operation? Because what you wrote is fully synchronous.

[lonix1]

Nice catch. Copy-paste error from examples above. Fixed.

[RikkiGibson]

I see there are a good number of 👍s on this discussion. I don't anticipate having time to continue design work on this in the short term. But, if someone wants to draft a proposal, including the specific public API changes, nullable analysis changes, and justification, based on the feedback from the below notes, I would be happy to review it.

https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-01-18.md#nullable-post-conditions-and-asyncawait.