dotnet / csharplang

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

Null-conditional assignment #6045

Open RikkiGibson opened 2 years ago

RikkiGibson commented 2 years ago

Summary

Permits assignment to occur conditionally within a a?.b or a?[b] expression.

using System;

class C
{
    public object obj;
}

void M(C? c)
{
    c?.obj = new object();
}
using System;

class C
{
    public event Action E;
}

void M(C? c)
{
    c?.E += () => { Console.WriteLine("handled event E"); };
}
void M(object[]? arr)
{
    arr?[42] = new object();
}

The full proposal for this feature has moved to https://github.com/dotnet/csharplang/blob/main/proposals/null-conditional-assignment.md.

Design meetings

CyrusNajmabadi commented 2 years ago

We could instead make the ?. syntactically a child of the =. This makes it so any handling of = expressions needs to become aware of the conditionality of the right side in the presence of ?. on the left. It also makes it so the structure of the syntax doesn't correspond as strongly to the semantics.

i would not do this :) Let's be consistent with the way we already do conditionals today.

On the IDE, we'll just need to update some of our conditional helpers we have today, and we'll have to audit all code that handles assignments. Seems doable.

quinmars commented 2 years ago

I'd expect

M(a?.b?.c = d);

be equivalent to:

M(a is null
    ? d
    : (a.b is null
        ? d
        : (a.b.c = d));

so that for any expression, be it a.b or a?.b:

var z = (any_expression = d);

z would always be of the type of d and had the value of d. I know that would go against the lazy evaluation. And probably that is more important in real code, inparticular since the value of an assignment is rarely used. But for a carefree reader, it could cause surprise.

CyrusNajmabadi commented 2 years ago

I would expect that if 'a' was null that no further work was done. That's the prime use case as otherwise we'll execute expensive work that will be thrown away. Having this work differently fro an argument vs an expression-statement would just be enormously weird at that point. If someone ends up writing M(a?.b?.c = d); my expectation woudl be that they're thinking htey can get null as a being null would mean no work should be done at all.

I would also be entirely ok with the expression form of this being disallowed, and only allowing the statement form.

RikkiGibson commented 2 years ago
var z = (any_expression = d);

z would always be of the type of d and had the value of d.

d is converted to the type of any_expression here, and the result type of the = expression is the same as the type of any_expression.

If we were to adopt the semantics you described @quinmars, then in a?.b?.c = d, we would probably want to convert d to the type of a.b.c and use that as the result type of the expression.

a?.SetX(M()) has semantics where M() is only evaluated if a is not null. I think if anyone ended up moving from that form to a?.X = M() due to improved ergonomics here, they'd be disappointed if it ended up having different semantics.

quinmars commented 2 years ago

Nevermind, you are both correct. The lazy evaluation is a very important part of the feature and shouldn't be sacrified for my first impression when seeing this code. I find the symmetry argument (a?.SetX(M()) vs. a?.X = M()) most compelling. Fortunately, the type system will catch wrong assumptions (z has the same type as d) easily - at least for value types. Anyway, the expression form of the assignment is seldom used.

jnm2 commented 2 years ago

I would also be entirely ok with the expression form of this being disallowed, and only allowing the statement form.

This has consequences for lambda return type inference, so it would leave a weird back-compat wart if this restriction was shipped and then loosened later.

CyrusNajmabadi commented 2 years ago

Let me rephrase: i would prefer to disallow one of the forms over having the two forms have different semantics :)

If hte two forms have the same semantics, then there's no problem and i wouldn't disallow either of them.

jnm2 commented 2 years ago

I agree, having the expression mean something different when directly a statement would be frightening.

I also initially expected a = b?.c = d to mean temp = d; b?.c = temp; a = temp;, but that's incompatible with my other strong expectation that d is not evaluated when b is null. So the first expectation has been revised.

michael-hawker commented 2 years ago

Awesome to see this moving forward! 🎉 Just wanted to bubble up our use case from the Windows Community Toolkit where we have to do event hooks all the time (from this original comment here):

We do this all the time in the toolkit for templated controls, where we need to register event handlers:

            if (_tabItemsPresenter != null)
            {
                _tabItemsPresenter.SizeChanged -= TabView_SizeChanged;
            }

            if (_tabScroller != null)
            {
                _tabScroller.Loaded -= ScrollViewer_Loaded;
            }

            _tabContentPresenter = GetTemplateChild(TabContentPresenterName) as ContentPresenter;
            _tabViewContainer = GetTemplateChild(TabViewContainerName) as Grid;
            _tabItemsPresenter = GetTemplateChild(TabsItemsPresenterName) as ItemsPresenter;
            _tabScroller = GetTemplateChild(TabsScrollViewerName) as ScrollViewer;

            if (_tabItemsPresenter != null)
            {
                _tabItemsPresenter.SizeChanged += TabView_SizeChanged;
            }

            if (_tabScroller != null)
            {
                _tabScroller.Loaded += ScrollViewer_Loaded;
            }

It'd be nice to condense this down to this:

            _tabItemsPresenter?.SizeChanged -= TabView_SizeChanged;
            _tabScroller?.Loaded -= ScrollViewer_Loaded;

            _tabContentPresenter = GetTemplateChild(TabContentPresenterName) as ContentPresenter;
            _tabViewContainer = GetTemplateChild(TabViewContainerName) as Grid;
            _tabItemsPresenter = GetTemplateChild(TabsItemsPresenterName) as ItemsPresenter;
            _tabScroller = GetTemplateChild(TabsScrollViewerName) as ScrollViewer;

            _tabItemsPresenter?.SizeChanged += TabView_SizeChanged;
            _tabScroller?.Loaded += ScrollViewer_Loaded;
Duranom commented 2 years ago

Question concerning the P?.A = B; to be equivalent to if (P is not null) { P.A = B; }. Is this the case for class types and structs? Or only for structs? Because for classes I would expect P?.A = B; to be equivalent to if (P is {} __p) { __p.A = B; }

For structs it also feels a bit weird to see P?.A = B; being equivalent to if (P is not null) { P.A = B; }.

Reason I ask is that it is easy enough to get race conditions with this, especially with UI or those that handle parallelism and it can easily assign incorrectly in cases when not aware.

CyrusNajmabadi commented 2 years ago

As the OP says:

P?.A = B is equivalent to if (P is not null) P.A = B;, except that P is only evaluated once.

Duranom commented 2 years ago

My bad, the repeated parts later in example on put on wrong footing, as it feels far from equivalent. But still wondering how it will go with structs, are those magically going to be referenced? Or will there be a need for an analyzer to warn if the struct is not done by reference?

CyrusNajmabadi commented 2 years ago

@RikkiGibson for thoughts on nullable structs.

RikkiGibson commented 2 years ago

I think we probably have to block assignments to members of nullable structs for the same reason we block the following: SharpLab

public struct S
{
    public string Prop { get; set; }
}

public class C {
    public void M(S? s) {
        s.Value.Prop = "a"; // error CS1612: Cannot modify the return value of 'S?.Value' because it is not a variable
    }
}

s?.Prop = "a" would be the same error.

CaseyHofland commented 2 years ago

s?.Prop = "a" would be the same error.

Just as well that's easy enough to include. A simple check to see if you're dealing with a Nullable<> and the condition evaluates to:

s?.Prop = "a"; // equivalent to:
if (s is not Nullable) // pseudo code
{
    // Do nullable reference type check
}
else if (s.HasValue)
{
    s = s.Value with { Prop = "a" };
}

with expression

Of course the real question being "should it be included"? I would say either "yes" or "not completely no"... the with expression makes this tricky. With solves the problem of modifying record and (as of c#10) structure types: cool, we can assign stuff in a single block now. Then, what about this?

S? ns;
S s;
s = ns with { Prop = "a" }; // doesn't compile, should be:

if (ns.HasValue)
{
    s = ns.Value with { Prop = "a" };
}

But that sucks! I want the ?. operator to handle it! So then it should be:

s = ns?.Value with { Prop = "a" }; // won't execute if null.

But if we implement that, we might as well add

s?.Prop = "a"; // evaluates to:
s = s?.Value with { Prop: "a" }; // evaluates to:
if (s.HasValue)
{
    s = s.Value with { Prop: "a" };
}

Well great, we're there. Or are we? Just to throw one more wrinkle in the mix, what happens if we do this?

S? ns;
S? ns2 = new S();
ns2 = ns?.Value with { Prop = "a" }; // Should this return null or leave ns2 unmodified?

I don't know the answer, but I do like the discussion. Single line assignment cases like this aren't going away, the community will keep asking until all of them are eradicated. The same thing happened with events, which after many tantrums turned into the now beautiful standard event?.Invoke();

BillWagner commented 2 years ago

Would this syntax be legal, and would it force the right hand side to be evaluated:

c?.E ?? _ = ExprWithSideEffects();
RikkiGibson commented 2 years ago

I think it is not legal, because we don't define ?? as a conditional operation (i.e. something which is grouped under the right-side of a ? operator, such as .M() in a?.M() or [i] in a?[i]). So you end up back in the case the language says you are trying to assign to something which is not an lvalue.

I think if you wanted semantics like "always do this, and assign it to some target if present" we would want you to do something like

var result = ExprWithSideEffects();
c?.E = result;
RikkiGibson commented 2 years ago

Regarding https://github.com/dotnet/csharplang/issues/6045#issuecomment-1242414606

Thanks for the very interesting discussion. It feels like nullable value types and reference types are maybe experiencing an analogous issue here. SharpLab

C? Method1(C? input)
{
    return input with { field = 43 }; // warning: 'input' may be null
}

S? Method2(S? input)
{
    return input.Value with { field = 43 }; // warning: Nullable value type may be null.
}

record C { public int field; }
struct S { public int field; }

In the original proposal, we are improving the ergonomics of "update this thing if it not null", but we are maybe leaving behind "create an updated version of this thing if it is not null". The closest thing to what we really want is still input == null ? null : input with { ... }.

I'm not certain whether/how to address but I think we should keep an eye on it and see if people keep hitting it.

jnm2 commented 2 years ago

A discussion of with on nullable things has also started here: https://github.com/dotnet/csharplang/discussions/6575

CaseyHofland commented 2 years ago

“The closest thing to what we really want is still input == null ? null : input with { ... }.”

Considering that I recognize it as its separate feature now. Thanks @jnm2 for the link, I’ve followed up! Does null assignment syntax sugar for value types still hold? That just seems like an if statement we would otherwise write ourselves.

BillWagner commented 2 years ago

Inspired by the ECMA committee discussion on dotnet/csharpstandard#664

As we do this, we need to define what happens in a case like this:

(a?.B, c?.D) = (e.M1(), f.M2());

Whatever we do, we should be consistent with the current evaluation order for tuple / deconstruction assignment:

(x, y) = (y, x); // swap

I'll suggest that the order of evaluation (non-spec language) be:

  1. Evaluate the left side expressions in left-to-right order to determine which are variables and which are null.
  2. For all left side expression variables, evaluate the right side expressions in left-to-right order. Note that if all left expressions are null, none of the right side expressions are evaluated.
  3. Perform the necessary assignment in left-to right order.

/cc @MadsTorgersen

CyrusNajmabadi commented 2 years ago

afaict, this is not null-conditional-assignment. It doesn't match teh grammar for that feature. Specifically, the grammar is:

null_conditional_assignment
    : null_conditional_member_access assignment_operator expression
    : null_conditional_element_access assignment_operator expression

Neither of which match the above. As such, this is normal assignment. As such, the LHS doesn't produce LValues to assign into, so it's simply illegal and doesn't compile.

HaloFour commented 1 year ago

@SF-Simon

If you have specific syntax proposals that you would like to suggest based on Swift feel free to open new discussions.

Swift does treat optionals differently than C#, though, in the sense that you are more or less forced to unwrap them into a different identifier. You don't have to do that in C#, and the idiomatic null check x != null or x is not null is generally more concise.

HaloFour commented 1 year ago

@SF-Simon

I hope I can persuade everyone with a little words and change this situation.

I'm suggesting that you represent the change that you would like to see, open discussions around features or syntax that you would like to see C# implement. Guidance like "be like Swift" isn't really actionable, but specific examples of syntax that you think would make development simpler could be considered.

tilkinsc commented 9 months ago

The proposal should clarify this circumstance,

if ((objName?.a = func()) == null)
{

}

In which null-aware assignments in an if statement condition will return:

Assignments still must follow the code of:

Its important to keep the idea that null should be emplaced where (objName?.a = func()) is:

if (null == null)
{

}

Actually, it was pointed out that this is defined in the proposal, https://github.com/dotnet/csharplang/blob/main/proposals/null-conditional-assignment.md#specification

However, I think a welcome change is to add code too in addition to grammar.

Rekkonnect commented 9 months ago

To me this looks implied from the semantics that the expression has when it's not an expression statement; meaning the expression's value is used. The same would apply to all contexts, including boolean variable assignments, if statements, while statements, do-while statements, for statements, etc.

The semantics are pretty clear for this case, even though it's probably best to never write code like that and split out the assignment's result into a variable and then compare it to null.

jamesford42 commented 9 months ago

This is gold, can't tell you guys how many times I've written assign foo to bar if bar isn't null with ?. and had to remember it's not possible.

Rekkonnect commented 4 months ago

Are there any chances this is being included in C# 13? We've been waiting for this improvement for quite a long time and it's a massive QoL. Last time this was touched by the LDM was in 2022.

333fred commented 4 months ago

Very unlikely at this point.