-
Notifications
You must be signed in to change notification settings - Fork 1k
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Champion "Null-conditional await" #35
Comments
As I am understand it's port from dotnet/roslyn#7171 |
What if I have |
awaiting Task yields void so you can't assign it. for a value type I believe it'll be T?. |
Confirming @alrz 's response, If If |
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()?.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 Let's now look at the proposed await? x()?.y();
x()?.y()?.<<AWAIT>>; The proposal as it stands requires you to write ... 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 await? customers.FirstOrDefault()?.loadedTask; PROPOSAL2: Let's make the same short-circuit ?. evaluation order apply to 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 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 |
@ljw1004 But isn't your proposal an incompatible change? What if someone has come to depend on the code throwing a |
Since |
I would find it confusing if |
I think I gave the wrong impression. I was just pointing out an issue that would arise if we want to deliberately avoid |
@ljw1004 I think there is a compatibility issue with your proposal. If we add the feature that I think that is a fatal flaw. |
What if said change was tied to a CLR version upgrade (say for default interface methods). A compiler targeting the new CLR could let |
@alrz Always checking for null is no worse than what every |
@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 Default template, allows
|
@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. |
@gafter agreed it's a fatal flaw. That's a shame. |
I wonder if it would make much of a performance difference to return |
@jamesqo Isn't that what |
@yaakov-h |
To be fair though, I'm not 100% sure |
It's more for cases like |
@Joe4evr Yes, I know; I wasn't criticizing the feature, I meant performance benefit. Sorry for any confusion. |
I appreciate the gesture. But i'd rather the money go to better causes :) |
@CyrusNajmabadi Are there any guidelines on how a feature like this would be implemented or a branch in your memory which adds a feature (so i can learn from file changes of the PR)? I usually only write analyzers/quickfixes, but i suppose with some example on how to add such a feature i would be capable of doing this. I've cloned dotnet/roslyn and jumped back and forth to get an idea, but i guess if there are any recommended docs to read on this, implementing this feature would be a much more feasible task. |
@333fred any thoughts? |
@taori I would suggest coming and talking to us on discord, in one of the communities listed here: https://github.com/dotnet/csharplang/blob/main/Communities.md. This issue isn't likely to be a good place for the type of back and forth real time discussion you'd probably want. |
I agree with those suggesting to simply change The only code that could be negatively impacted, it seems to me, would be code that relied on the throwing of a null ref exception instead of actually checking for null. We shouldn't let past bad practices impede future progress, especially since, as was pointed out, there is much more appetite for breaking changes now than there was several years ago, and language versions are opt-in anyway. FWIW, I would change |
@legistek Changing var someInt = await foo.SomeTaskOfInt;
// someInt is currently `int`, not `int?` Would every await of a value-typed result start returning I don't think replacing throwing with using |
I largely agree to this, you cannot break existing behavior that much. We have already crossed that path. The |
Good point, I hadn't thought of returning non-nullable value types. My first response would have been to go with |
I disagree with this, for the same reason I think that Yes, it will almost always throw when it encounters a bug in the code. But that's a virtue. It tells me when I've done something very wrong. Nulls should not be papered over. They should either be explicitly expected, or they should fail catastrophically. Silently ignoring them is just a way to lead to even worse bugs happening |
Ok but what's the logic for allowing
This is true but it's also worth pointing out that Before But with Regarding the value type question, I assume if we used In other words assume |
That is correct, and as the language has been evolving towards that direction, I believe it's safe to say, such proposals like
Yeah, that would be the most sensible design. You still have the |
Legacy. Doing it again, i would absolutely not do that. |
Sure. But it postdates our belief that null-tolerance is not a virtue, which is why alter versions of the language embraced explicit null-handling as the mechanism to use, not being null-tolerant. |
Sure. But i don't. And i'd much rather just say: write
Correct. |
Yeah ok. Well like I said I can live with I still would let
Interesting! A true believer then. :) |
In the meantime we have .Net 8. Wouldn't that also be an option for this I could imagine something like this: class Foo
{
public async Task DoAsync() { }
public async Task<int> GetValueAsync() => 42;
}
// usage
Foo? foo = null;
await foo?.DoAsync(); // simply does nothing - no exception is thrown
await foo?.GetValueAsync(); // simply does nothing - no exception is thrown
var x = await foo?.GetValueAsync(); // an ArgumentOutOfRange is thrown Same behaviour for |
What you're saying is that the nullability annotation of the type should control whether the task is allowed to be null and thus suppress throwing the null exception. Keep in mind that |
No. My example code should only illustrate how But the compiler "knows" whether the task has a return value or not. If not no exception should be thrown. If the result value is used in any way (assignment to variable, forward to using, ...) an I edited my sample code to be more clear. |
This would silently change the behavior of existing programs and affect the return type of the expression. I don't think that would be palatable. The |
@HaloFour Btw: |
To my surprise there are many hits for await on conditional member access most of which are invalid? https://github.com/search?q=%2Fawait%5Cs%2B%5Cw%2B%5C%3F%5C.%5Cw%2B%2F%20lang%3AC%23&type=code (not a comprehensive query) Would it make sense if the compiler adds a null check in the presence of a null-conditional access with the current syntax? It seems like that's sort of the expected behavior given that many hits. (edit: actually there's already a null check there, it could only await on the success branch, so it still throws on a null task) |
It's interesting this popped up again as I'm in the middle of converting large amounts of APIs from sync to async in one of our largest solutions. I must say this is super dangerous when performing such conversions. I've hit a few cases here where we had "callback/event" properties typed as This caused code to change from this: myClass.SomeCallback?.Invoke(); To this: if (myClass.SomeCallback is not null and var someCallback)
{
await someCallback.Invoke(cancellationToken);
} But that was after I had made the changes and before sending the PR for review that I noticed there could be a problem and switched to the explicit defensive check. It would've been nice if "it just worked" and I could keep it like: await someCallback?.Invoke(cancellationToken); Which is exactly what @alrz is proposing:
I'd even go as far as to suggest this null check should be done regardless of the presence of null coalescing operators in the right-hand side. It would make using ((IDisposable?)null)
{
Console.WriteLine("Hello world");
} If |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
Design Meetings
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-08-31.md#await
The text was updated successfully, but these errors were encountered: