Skip to content

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

Allow null conditional (?.) operator on left hand side of assignment #2883

Closed
mrjvdveen opened this issue Oct 14, 2019 · 59 comments
Closed

Allow null conditional (?.) operator on left hand side of assignment #2883

mrjvdveen opened this issue Oct 14, 2019 · 59 comments
Assignees
Milestone

Comments

@mrjvdveen
Copy link

mrjvdveen commented Oct 14, 2019

Consider this code:

if (Foo is { })
{
  Foo.Bar = "value";
}

I suggest introducing the possibility to write this like so:
Foo?.Bar = "value";
Writing it like this, should do exactly the same as the first example. So if Foo is null, then the assignment never happens.

[Update(jcouv):] Detailed proposal write-up: #6045

@svick
Copy link
Contributor

svick commented Oct 14, 2019

There has been a previous discussion about this at #1800, though that issue is now closed.

@Joe4evr
Copy link
Contributor

Joe4evr commented Oct 14, 2019

And that issue was deemed to be a dupe of #34.

@svick
Copy link
Contributor

svick commented Oct 14, 2019

@Joe4evr It was closed that way, but I'm not sure it was actually a duplicate. #34 is part of C# 8.0 and it doesn't let do what's proposed here.

@Tom01098
Copy link

I dislike this because the whole line doesn't happen if the LHS is 'null', it is much clearer to wrap this in an if statement IMO.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Oct 14, 2019

It makes sense to me, when you have expr?.moreexpr, the grammar and language is literally defiend where .moreexpr is simply a lower RHS node of the ? (the conditional access expression). So, if we allowed: expr?.name = whatever then that would just be a node like so:

      ?
     / \
expr     =
        / \
   .name   whatever

So, like any conditional access expression, this would naturally fall out as "evaluate what is before the ?. if it null, do not evaluate what is after".

Feels fine and normal to me. it's just something that didn't fall out with how we did the precedence for everything here.

@mrjvdveen
Copy link
Author

I've had a brief discussion with @KathleenDollard at Techorama in the Netherlands two weeks ago about this feature. It is not a part of C# 8 right now and we both felt it deserved a feature request of it's own.

@Tom01098: I get your point. However I'm also allowed to write:
foo?.Bar();
This line will not execute if foo is null. In my view it would only be consistent to allow
foo?.Prop = 'val';

@YairHalberstadt
Copy link
Contributor

@mrjvdveen
Can I suggest renaming this issue to "allow null conditional (?.) operator on left hand side of assignment"

@mrjvdveen mrjvdveen changed the title Allow nullable reference on the left hand side of an assignment Allow null conditional (?.) operator on left hand side of assignment Oct 15, 2019
@mrjvdveen
Copy link
Author

@mrjvdveen
Can I suggest renaming this issue to "allow null conditional (?.) operator on left hand side of assignment"

Thanks for your input. I renamed this issue.

@DavidArno
Copy link

Having improved your title, would you mind removing the "With the introduction of nullable reference types in C# 8 in mind" line in your OP, please? The code, if (Foo is { }) is a use of pattern matching and the rest of your post relates to the null conditional operator. None of it is related to NRTs.

@mrjvdveen
Copy link
Author

Having improved your title, would you mind removing the "With the introduction of nullable reference types in C# 8 in mind" line in your OP, please? The code, if (Foo is { }) is a use of pattern matching and the rest of your post relates to the null conditional operator. None of it is related to NRTs.

Thanks for your input. I have changed the OP.
The first line of code is a way of doing a check for a non null object. It is relevant to the code and is correct by construction.

@alrz
Copy link
Contributor

alrz commented Oct 15, 2019

I think this has been brought up a couple times before, in short, it comes down to permitting ?. as an lvalue

o?.Event += Handler;

@HaloFour
Copy link
Contributor

#1106

@DavidArno
Copy link

@HaloFour, it's probably best to ignore #1106 though as it was completely derailed by the makeloft troll. Far better to have a new thread that restarts the discussion on the possible merits of allowing ?. in lvalues.

@HaloFour
Copy link
Contributor

@DavidArno

There are other comments there as well. We shouldn't let one bad actor wreck an entire thread. Even if the conversation continues here I think it's good to link the two issues.

@mrjvdveen
Copy link
Author

@HaloFour, I agree on having the link here. It documents the history of the discussion on this subject.

@jnm2
Copy link
Contributor

jnm2 commented Oct 15, 2019

I'd love to be able to write code like this:

(dataSource as BindingList)?.ListChanged -= OnListChanged;
dataSource = value;
(dataSource as BindingList)?.ListChanged += OnListChanged;

@HaloFour
Copy link
Contributor

Would be nicely consistent with using ?. to invoke mutator methods:

// works today
foo?.SetBar(CalculateBar());

// maybe?
foo?.Bar = CalculateBar();

@Leemyy
Copy link

Leemyy commented Oct 16, 2019

I like this idea aswell, so I had a look through the Roslyn source to get a feeling for the ammout of work needed to implement this.
From what I can tell, the syntax already accepts conditional access (?.) as an lvalue, so no changes will be required there. The Value checker is where it gets rejected, and where the error (CS0131) is produced. So some code will have to be added there to properly check and allow for ?. as an lvalue.
Of course that still won't magically make it work. Logic still needs to be added to the assignment lowering step, which will likely ammount to the majority of the work.

I've been looking to implement a compiler feature for a while now. I will have a go at implementing this and get back to you with my results. Even if this proposal gets rejected or my implementation proves inadequate, I can use the exercise.

@YairHalberstadt
Copy link
Contributor

@Leemyy

Have fun!

Be aware that PRs for language features that haven't been accepted are usually closed, so don't make a PR. Just leave it in your branch and add a link here.

If you need any help, check out the Roslyn gitter. (the amazing) @CyrusNajmabadi is usually happy to help , as am I, just less affectively.

Tagging @jaredpar, the Roslyn PM, to see if he thinks there's a chance a PR like this could get accepted.

@mrjvdveen
Copy link
Author

Thanks for all the support.

@Leemyy : that's awesome. I'm curious to see how this pans out.
@YairHalberstadt: thanks for linking in people who could help out.

@Leemyy
Copy link

Leemyy commented Oct 18, 2019

Hmm, while writing the Test cases I stubled upon an interesting corner case: Destructuring Assignments.
I was wondering whether the following should also be possible:

(left?.X, right?.X) = (1, 1);

@Leemyy
Copy link

Leemyy commented Oct 18, 2019

I am thinking that it would be more consistent if it were possible, but maybe there are some aspects of it that I haven't considered?

@YairHalberstadt
Copy link
Contributor

@Leemyy

My advice is keep it simple for now, and forbid it. If this ever gets championed and bought up on a LDM, it will get discussed.

@canton7
Copy link

canton7 commented Oct 18, 2019

I think there's precedent in:

var (a, _) = (new object(), new object())

new object() is evaluated twice, even though one of the instances is discarded. I don't see (left?.X, right?.X) = (new object(), new object()) as being any different if left or right is null.

But, as @YairHalberstadt says, leave this for the LDM.

@Leemyy
Copy link

Leemyy commented Oct 18, 2019

You're right, this would add too much complexity to implement as part of the Prototype.

@Leemyy
Copy link

Leemyy commented Oct 18, 2019

I think there's precedent in:

var (a, _) = (new object(), new object())

new object() is evaluated twice, even though one of the instances is discarded. [...]

This raises another interesting question:
Should the simple conditional assignment evaluate the right side, if the left is null?

@mrjvdveen
Copy link
Author

@Leemyy
I'd not expect the right side to be evaluated. I'd keep it as close too a normal null check as possible.

@canton7
Copy link

canton7 commented Oct 18, 2019

Should the simple conditional assignment evaluate the right side, if the left is null?

I'd argue no. thing?.SetX(new object()) doesn't evaluate new object() if thing is null, so I think thing?.X = new object() should behave the same way.

@Leemyy
Copy link

Leemyy commented Oct 18, 2019

I am thinking it should not, otherwise compound assignments would make no sense:

a?.b += x() * y;

Edit:
My thinking was that the decomposed form

if (a is object)
{
    a.b = a.b + (x() * y)
} 
else
{
    _ = a.b + (x() * y)
}

would try to access a.b even if a is null, but of course you could also decompose it as

if (a is object)
{
    a.b = a.b + (x() * y)
} 
else
{
    _ = x() * y
}

which would be fine.

@333fred 333fred added this to the X.X candidate milestone Jul 13, 2020
@jnyrup
Copy link
Contributor

jnyrup commented Aug 31, 2020

Would this also handle null conditional assignment on set-only properties?

using System;

public class MyClass
{
    private string property;
    public string Property
    { 
        set => property = value;
    }
    
    private void MyMethod()
    {
        // use property
    }
}    

public static class Program 
{
    public static void Main() 
    {
        object obj = new MyClass();
        
        //CS0154: The property or indexer 'MyClass.Property' cannot be used in this context because it lacks the get accessor

        (obj as MyClass)?.Property = ""; 
    }
}

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxAgrgOwD4AEBMAjALABQZuAzAAQHUCyAngMIA20UZA3mdX9QAcEASwBuEeLUIAGQQgD2AxDEYBuXvypTZABQVKEKjXy7Vj/PlDgxqAXgB8cxcsZ3q41ujjrSFgL7m5kJiEnC0ACwMjPTWABbyACYAFACU5jy+FvwA9NnU6FZOBkaZfAGkfhZkFPg03OYE+Gal1BlZ/PLAAFZwAMY2nV1umHAA7lFsHKk+7XzmFkmD1NAT7FBQKQD8AHR6zoauttQARMcz/OV+QA===

@Rekkonnect
Copy link
Contributor

Would this also handle null conditional assignment on set-only properties?

The error in that code sample is only produced because, currently, the compiler expects that with ?. you're only trying to get a value, not assign.

Properties with setters should allow ?. assignment, regardless of whether they contain a getter or not.

@CyrusNajmabadi
Copy link
Member

I'll champion this as well.

@Rekkonnect
Copy link
Contributor

Does this proposal also handle the case of using the ?[ operator for indexing arrays, or other objects with custom indexers in the LHS?

@Xyncgas
Copy link

Xyncgas commented Jan 13, 2022

Is it escaping type safety? something must've gone wrong if you are looking for members in a variable that is null as a consumer. but if you are providing the value you can do this in generic and write once apply to all so it seems to me that it's escaping dealing with the consequences of null in consumer's side

it's like shorthand for try {foo.bar = 5} catch { } or

   #IgnoreTypeSafety
   foo.bar = 5
   #endIgnoreTypeSafety

regardless I still want this feature to be implemented because I want to be lazy and write unsafe code, what can possibly go wrong, at the end of the day I will be blamed for bugs anyways

@Rekkonnect
Copy link
Contributor

It is not a shorthand for either of those cases. The null-conditional operator for any member access operator (., [) should behave like the following:

if (foo is not null)
   foo.bar = 5;

No exceptions involved, because they're too expensive. No type unsafety either, you always account for null when dealing with references.

something must've gone wrong if you are looking for members in a variable that is null as a consumer

Even with your nullability settings and your strong usage of the ! operator, you cannot (at least reliably) get rid of null checks. Whether something's gone wrong or not, it's not the compiler's duty to cleverly eliminate null checks. If this ever happens, it will be during JIT compilation, and I don't even know if it does so, let alone how frequently and reliably.

@Xyncgas
Copy link

Xyncgas commented Jan 14, 2022

Maybe we can get rid of null in c# by opt-in feature, instead of giving nullable to everything, and having variables points to nullptr they should have a pointer to a static object that's the default state. I feel annoyed when I forgot to check for null and things break during testing too, only if things were never null I can be happier, I understand I can also take a walk outside and stop returning null in new codes I write it isn't a big deal for me

@Rekkonnect
Copy link
Contributor

Rekkonnect commented Jan 14, 2022

This is just not happening. null will be part of the language since it's been since day one, and it's also useful in certain scenarios.

You can use the nullability tooling available since C# 8.0/.NET Core 3.1(?) and respect it like you personally desire. Other than that, if you know your values aren't null, you shouldn't care. That's a general development tip, to always properly track what values you're passing. null values aren't the only problematic ones.

@Xyncgas
Copy link

Xyncgas commented Jan 14, 2022

New rules ok
imagine you have the object be default instead of null, which I think microsoft made the decision too soon to say default is null for reference types, because it's gonna break when we change this behavior later and they saw this coming too people have been saying let's get rid of null and RUST did, but anyways I think it would be ideal to let default to actually be type T that lives as an static object in runtime and anytime you try to do something like ObjectA.b it's just not possible to be null because ObjectA by default has a reference to the default state of its type if you've never set it or forgot to

@Rekkonnect
Copy link
Contributor

Such a breaking change is probably never going to happen. I like the idea of having a "default" static object, which from what I assume would have to be constructed by the default instance constructor that will be introduced for that feature. It will have to be triggered in some other way though, like T.default, since default(T) is already valid syntax, despite being simplified to default.

This discussion however should be made in the respective proposal.

@Haukinger
Copy link

Haukinger commented Jan 14, 2022

I'd always prefer seeing that something's going wrong instead of having it going havoc silently. That's what null and NullReferenceException are for - notifying you that something gone awry.

If I have code that accepts optional parameters or values, that code has to check for null and react accordingly.
Also, if I have code that doen't accept optional parameters or values, that code, too, has to check for null just to be on the safe side and then react accordingly.

Also, if (value == null) and much more so value?. is a lot more concise than verbose alternatives like Nullable (i.e. if (value.HasValue) ... value.Value). Conciseness is all what ?. is about, mind you.

@Xyncgas
Copy link

Xyncgas commented Jan 15, 2022

@jnyrup

In your case I would expect it not work (by nothing happens) since the object isn't null but it's the property that wasn't able to be set

@Xyncgas
Copy link

Xyncgas commented Feb 21, 2022

Any Updates?

@CyrusNajmabadi
Copy link
Member

@Xyncgas I've championed this.

@legistek
Copy link

legistek commented Apr 1, 2022

I would love to see this!

@333fred
Copy link
Member

333fred commented Apr 27, 2022

I'm converting this issue to a discussion, as #6045 has the template and proposal.

@dotnet dotnet locked and limited conversation to collaborators Apr 27, 2022
@333fred 333fred converted this issue into discussion #6072 Apr 27, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests