dotnet / csharplang

The official repo for the design of the C# programming language
11.55k stars 1.02k forks source link

Champion "Null-conditional await" #35

Open MadsTorgersen opened 7 years ago

MadsTorgersen commented 7 years ago

Design Meetings

https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-08-31.md#await

dmitriyse commented 7 years ago

As I am understand it's port from https://github.com/dotnet/roslyn/issues/7171

yaakov-h commented 7 years ago

awaits e if it is non-null, otherwise it results in null

What if I have Task<T> where T is a struct/scalar, or Task without a result type?

alrz commented 7 years ago

awaiting Task yields void so you can't assign it. for a value type I believe it'll be T?.

gafter commented 7 years ago

Confirming @alrz 's response,

If e is of type Task, then await? e; would do nothing if e is null, and await e if it is not null.

If e is of type Task<K> where K is a value type, then await? e would yield a value of type K?.

ljw1004 commented 7 years ago

I think this proposal doesn't quit hit the key scenario, for the same reason as we made the last-minute change to "short circuit ?."

Let's review that short-circuit first. We initially had users put ?. all the way through, because we said every single ?. was evaluated from left to right. If x() returned null, then x()?.y() would return null, and so you had to put ?.z else otherwise it would throw.

  x()?.y()?.z

But this wasn't so nice... (1) it created "question mark pollution" everywhere, (2) it THREW AWAY legitimate null-checking information -- in the case where x() returns non-null, and you want to assert that y() will never return null, so you want to write x()?.y().z. The left-to-right order threw away your ability to distinguish that.

Let's now look at the proposed await? operator. And pretend for a moment that await could be written in a fluent left-to-right syntax, rather than being forced to the left...

  await? x()?.y();
  x()?.y()?.<<AWAIT>>;

The proposal as it stands requires you to write await?. This has the same problems as left-to-write conditional evaluation: it peppers your code with too many question marks, and it throws away the legitimate check you might want to make that y() never returns a null Task.

...

KEY SCENARIO. All of that discussion is theoretical. Let's get concrete. The places where I've wanted this feature (and I've wanted it a lot) have almost all been to do with ?. operators on the right hand side. I reckon this is the key use-case for this scenario:

   await? customers.FirstOrDefault()?.loadedTask;

PROPOSAL2: Let's make the same short-circuit ?. evaluation order apply to await as well. So, if the await operand involved a ?. that short-circuited the remainder of the expression, then it can also short-circuit any await operator that operates on the expression.

Written out like that, the proposal feels partly natural but partly irregular/unpredictable. And to avoid the unpredictability, that's why I end up preferring @bbarry's original suggestion:

PROPOSAL3: Let's say that await always does the null-check, on the grounds that (1) awaiting null feels naturally like a no-op, (2) using the await operator is a kind of goofy way to put in a "non-null" assertion into your code, and if you really wanted a non-null check, then there are much better ways to write it.

Note: I've spent the past two months immersed in the Flow language. I really like how it lets me write clean code with a minimum of annotations and it figures them all out for me. I guess that await? feels extra busy in the light of that experience.

gafter commented 7 years ago

@ljw1004 But isn't your proposal an incompatible change? What if someone has come to depend on the code throwing a NullReferenceException? </joking>

alrz commented 7 years ago

Since return null would still create a Task<T> in an async method I also think making await a no op in case the expression is null is ok. However, it should not always check for null, only when it's being used with null-conditional member access. Though that might not be always the case.

svick commented 7 years ago

@alrz

However, it should not always check for null, only when it's being used with null-conditional member access.

I would find it confusing if await a?.b; gave different result than var task = a?.b; await task;. Something similarly confusing already happens with ?. followed by ., but in that case there is a good reason for it. What is the reason here?

alrz commented 7 years ago

I think I gave the wrong impression. I was just pointing out an issue that would arise if we want to deliberately avoid await? syntax. I think it's not "extra busy" as @ljw1004 claims. And I don't think the compiler should "always" emit null check when we're using await to make awaiting a null a no op.

gafter commented 7 years ago

@ljw1004 I think there is a compatibility issue with your proposal.

If we add the feature that await automatically does the null check (whether or not that depends on the syntax of the operand), then people will write code depending on that. But that code will also compile cleanly with an earlier version of the compiler, where it will produce code that throws an exception.

I think that is a fatal flaw.

bbarry commented 7 years ago

What if said change was tied to a CLR version upgrade (say for default interface methods). A compiler targeting the new CLR could let await null be a no-op. and an older compiler wouldn't be able to target the new framework.

jnm2 commented 7 years ago

@alrz Always checking for null is no worse than what every using statement does today. It seems like the most natural thing in the world to me.

jnm2 commented 7 years ago

@gafter Just brainstorming. To solve the old compiler problem, it could be useful to plan to add the capacity to opt in to new compiler behaviors in the csproj SDK which translate to csc.exe switches, in the same vein as <AllowUnsafeCode> and <CheckForOverflowUnderflow>, but which would cause old compilers to error. Here's what that might look like:

Default template, allows await null

(SDK sets <AllowAwaitNull>true</AllowAwaitNull>)

Csproj, does not override AllowAwaitNull so it stays true:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net462</TargetFramework>
  </PropertyGroup>
</Project>

Class1.cs

class Class1
{
    public async Task Foo(Bar x) => await x?.WhenX;
}

Old compiler gets passed -CompatSwitch:AllowAwaitNull and is smart enough to refuse to build at all (whether or not you await) because it doesn't recognize the AllowAwaitNull switch.

New compiler emits the null check because it recognizes the switch.

In the rare scenario where you need to compile using an older compiler:

(SDK sets <AllowAwaitNull>true</AllowAwaitNull>)

Csproj, overrides AllowAwaitNull so that it can build against older compilers:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net462</TargetFramework>
    <AllowAwaitNull>false</AllowAwaitNull>
  </PropertyGroup>
</Project>

Class1.cs

class Class1
{
    public async Task Foo(Bar x) => await x?.WhenX;
}

Old compiler does not get passed -CompatSwitch:AllowAwaitNull and acts as it always has, and an analyzer sees the await x?.y and warns that you may be awaiting null.

New compiler skips the null check because it was not given the switch, and an analyzer sees the await x?.y and warns that you may be awaiting null.

svick commented 7 years ago

@bbarry @jnm2 I'm not sure this is a problem that can be solved with technical solutions. I think it's an undesirable situation when you find some code e.g. on Stack Overflow, you copy it to your project and it's broken in a subtle way, because you're using a different version of the compiler (compiler error is fine, exception isn't). And neither of your solutions changes that.

ljw1004 commented 7 years ago

@gafter agreed it's a fatal flaw. That's a shame.

jamesqo commented 7 years ago

I wonder if it would make much of a performance difference to return null instead of Task.CompletedTask for tasks that complete synchronously. I'm working on an app right now that awaits a huge (thousands) amount of tasks, most of which are completed. I could check !IsCompleted on each task before awaiting it, but it would feel cleaner to do await? task.

yaakov-h commented 7 years ago

@jamesqo Isn't that what ValueTaskis for?

jamesqo commented 7 years ago

@yaakov-h ValueTask<T> is only available as a counterpart to the generic Task<T> type. There is no non-generic ValueTask.

jamesqo commented 7 years ago

To be fair though, I'm not 100% sure await? x will provide much benefit over awaiting a completed task. From what I can see GetAwaiter(), and IsCompleted and GetResult() on that awaiter reduce to just a few flag checks for a completed task. Maybe it wouldn't make so much of a perf difference to have await?.

Joe4evr commented 7 years ago

I'm not 100% sure await? x will provide much benefit over awaiting a completed task.

It's more for cases like var x = await foo?.SomeTask(); If foo turns out to be null, then the await still attempts to call ((Task<T>)null).GetAwaiter().GetResult() which turns into a cryptic NRE.

jamesqo commented 7 years ago

@Joe4evr Yes, I know; I wasn't criticizing the feature, I meant performance benefit. Sorry for any confusion.

gundermanc commented 7 years ago

I had opened an issue in the Roslyn repo to add a warning for this before seeing this thread.

I don't think that the referenced await? operator addresses the primary risk of the await operator's current behavior. The lack of a null check for the await operator makes async calls using ?. a bit of a special case that the developer has to be cognizant of, and the NRE is difficult to diagnose if you haven't seen it before.

Though a breaking behavioral change is probably not possible, having a warning and changing the exception message for this case would at least aid the developer in prevention/diagnosis.

taori commented 6 years ago

I'm curious about the state of this - as is, it would not be introduced unless someone of the community implements it. Is this correct? At least i didn't see any visible branches named feature(s)/await.

I would really love this feature to exist though.

alrz commented 6 years ago

"Any Time" milestone description:

We probably wouldn't devote resources to implementing this, but we'd likely accept a PR that cleanly implements a solution.

taori commented 6 years ago

@alrz Yes - My question was more about the "I didn't see any visible branches"-part. I've read that description too. I'm just making sure there isn't someone working on this already.

Jopie64 commented 5 years ago

Hi. Just a thought that came up when reading this thread. When the nullable-reference-types feature is turned on, await? should not be necessary anymore. await (without ?) could simply return T in case of awaiting a Task<T> and return a T? in case of awaiting a Task<T>?. The compiler can detect when used erroneously.

Jopie64 commented 5 years ago

Sorry, changed my mind. It should error out when using await on a Task<T>?. Otherwise you cannot express that you don't want to wait on a nullable task when it is a void task or a Task<T?>.

canton7 commented 5 years ago

Since nobody's mentioned it yet:

I think this is relevant for IAsyncDisposable. A common pattern is foo?.Dispose() in your type's Dispose method - it's a bit counter-intuitive that this pattern doesn't carry across to await foo?.DisposeAsync(). I suspect this will catch people out.

yahorsi commented 5 years ago

Any progress on that? It really hurts and initial issue was created back in 2015 (((

ufcpp commented 5 years ago

https://github.com/dotnet/csharplang/milestone/14

We probably wouldn't devote resources to implementing this, but we'd likely accept a PR that cleanly implements a solution.

legistek commented 4 years ago

I've been curious about this for awhile. Count me in the category of await null should simply do nothing similar to the way using (null) does nothing when it comes time to dispose. This way we can do things like await thing?.DoSomethingAsync() without having extra lines of null-checking.

Maybe this proposal is obsolete in the age of nullable reference types though, which I admit I haven't gotten into yet.

paulhickman-a365 commented 4 years ago

In https://github.com/dotnet/csharplang/issues/35#issuecomment-287893347 ljw1004 started off his discussion with a "consider if await was fluent" scenario. Is there a reason why this couldn't actually be implemented?

i.e. make the following valid alternatives to await in front of the expression.

thing.DoSomethingAsync().await;
thing?.DoSomethingAsync().await;
thing?.DoSomethingAsync()?.await;
var t = thing.DoSomethingAsync().await;
var t = thing?.DoSomethingAsync().await;
var t = thing?.DoSomethingAsync()?.await;

This gets around the issue of ? pollution and allows you to express that thing may be null but DoSomethingAsync() shouldn't return null if it is executed. It also means the semantics of the existing syntax of putting await in front of an expression does not change.

It's arguable whether the last example is should be valid or not, as there you do have a method with return type of Task returning null. ?.await could still raise an error whilst accepting ?. operations further to the left in the expression.

It would also have the benefit outside of null coalescing that it makes it cleaner to chain operations on the results of async calls e.g.

var count =thing.DoSomethingAsync().await.Count();

Rather than:

var count =(await thing.DoSomethingAsync()).Count();

If this is considered, I'd also like to see a shortcut for ConfigureAwait(false) by giving an argument to the await - i.e. the following are equivalent:

var count =(await thing.DoSomethingAsync().ConfigureAwait(false)).Count();
var count =thing.DoSomethingAsync().ConfigureAwait(false).await.Count();
var count =thing.DoSomethingAsync().await(false).Count();

Obviously, it would introduce a conflict with properties/methods called "await" but it would be a compile time breaking change, rather than something that causes a runtime exception / change in runtime behaviour, and await has been a keyword long enough now that I can't believe any developers actually still use it as a property/method name in code that is being actively ported into new language versions, or use it in newly written code.

HaloFour commented 4 years ago

@paulhickman-a365

Is there a reason why this couldn't actually be implemented?

The syntax you suggested is already legal in C#, there's nothing stopping some arbitrary expression from having an await property. Sure, the language could consider it as a contextual keyword where such a property doesn't exist, but that would be confusing. And this only serves to add duplicate syntax for an existing feature which doesn't solve any problems except when it comes to stylistic preferences.

gafter commented 4 years ago

@HaloFour No, the suggested syntax is not already legal C#. @paulhickman-a365 's idea is workable.

See https://sharplab.io/#v2:D4AQTAjAsAUCAMACEECsBuWsQGZlkQGFEBvWRC5PAFUQFkAKASkQF4A+RAOwFcAbPphiVE5SrmQAOZADZ6YZmIplhIygH029ZgDoQATiEiAvrFNw84RNVgqREgJZcALsn1tOEIeaA===

paulhickman-a365 commented 4 years ago

I hadn't realised it was already illegal - I thought I was clobbering existing property names.

Thinking about it some more, I think .await() may be preferable to .await. Although it introduces additional punctuation, the act of awaiting is more like invoking a method than accessing a property so I think it is easier to understand what is happening particularly for new developers.

theunrepentantgeek commented 4 years ago

Using the syntax .await() is already valid C# syntax, all you need to do is to write extension methods on Task and Task<T> (Sharplap).

The .await syntax isn't currently legal C# syntax, but would collide with extension properties (#192, #11159).

canton7 commented 4 years ago

@theunrepentantgeek I'm afraid I'm missing your point. It's not valid to call an await() extension method from inside an async method, which is the case that's being discussed here.

Jopie64 commented 4 years ago

@theunrepentantgeek .await() as an extension method would be different behavior from the current await keyword, because the extension method would be a blocking call (it would block the thread). I think there is no way you currently can replicate the behavior of async/await with an extension method.

huoyaoyuan commented 4 years ago

I'm starting an implementation here: https://github.com/huoyaoyuan/roslyn/tree/features/conditional-await

CyrusNajmabadi commented 4 years ago

I'll champion this.

huoyaoyuan commented 3 years ago

@CyrusNajmabadi any update on this?

CyrusNajmabadi commented 3 years ago

There is no update on this.

huoyaoyuan commented 3 years ago

So what does the tag Needs Implementation and milestone Any Time means? Is it welcome for implementation?

CyrusNajmabadi commented 3 years ago

@333fred For info. From my understanding, it means we're not talking this for 10.0, but are not rejecting it. We could take it in some future release.

333fred commented 3 years ago

Any time generally means any time 🙂. We'd accept a community contribution that implements this feature, subject to compiler team review availability. I don't expect that we have time for this in the near term, however, as we're in full swing for bigger C# 10 features at the moment and this is not a priority.

Rekkonnect commented 3 years ago

I'm starting an implementation here: https://github.com/huoyaoyuan/roslyn/tree/features/conditional-await

How's progress on the feature doing?

huoyaoyuan commented 3 years ago

How's progress on the feature doing?

It's laid aside until the team approves it and assign a reviewer.

333fred commented 3 years ago

How's progress on the feature doing?

The feature is in the any time bucket and tagged needs implementation. The team is not going to do anything on it on our own, but we will accept a community contribution.

huoyaoyuan commented 3 years ago

@333fred I had completed a basic implementation. Will you assign someone to review it during C# 11 lifetime?

333fred commented 3 years ago

No, but maybe @jaredpar would.