Skip to content
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

Proposal: Expression bodied get and set methods #7881

Closed
MgSam opened this issue Jan 11, 2016 · 30 comments
Closed

Proposal: Expression bodied get and set methods #7881

MgSam opened this issue Jan 11, 2016 · 30 comments
Assignees
Labels
Area-Compilers Area-Language Design Community The pull request was submitted by a contributor who is not a Microsoft employee. New Language Feature - Expression-Bodied Ctor/Dtor/Accessor Expression-Bodied Members Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@MgSam
Copy link

MgSam commented Jan 11, 2016

C# 6.0 allowed get-only properties and all methods to have an expression body. This is a great feature that can help keep the focus on business logic rather than code. However, when you need to fall back to writing a get and set method manually, it can be annoying that you're no longer allowed to use expression bodies.

I propose supporting expression bodies for get and set methods. It makes code less verbose and also would provide more symmetry in the language.

class Foo
{
    int Bar
    {
        get => _bar;
        set => _bar = value;
    }
    private int _bar;
}
@lachbaer
Copy link
Contributor

lachbaer commented Feb 3, 2016

In #8364 I suggested to use another approach for this, because the => operator is mainly for returning values, not for setting them (though you could use them for other things as in a void delegate). My suggestion also supports auto-properties with manually declared backing fields.

@lachbaer
Copy link
Contributor

lachbaer commented Feb 3, 2016

set should always return a value of the appropriate type that can be assigned to an auto-property.

public string AutoProp {  // "standard, old fashioned" auto-property
   get;
   set => "Hello " + value;
}

Together with #8364 (further down) it can be used to populate the backing field:

public string UnfancyProp { get; set; }
public string FancyProp {
    value UnfancyProp;
    get;
    set => "Fance me up " + value;
}

If set is just ordinary and does not return a value one should rely on the standard set { ... } pattern.

UPDATE: #8364 has already evolved and futher defines the wanted behaviour of set =>.

@MgSam
Copy link
Author

MgSam commented Feb 4, 2016

I don't think changing the meaning of => just for the one special case of set is worth the added mental burden.

@lachbaer
Copy link
Contributor

lachbaer commented Feb 4, 2016

@MgSam
what do you mean by "changing the meaning"?

public string Foo {
    get => "Bar";  // using expression body here
    set { xyz = value; }
}

is the same as the already available

public string Foo => "Bar"; // using also expression body here

..., but with an available setter also.

And as => returns a value, we could also use it as an expression body for set to return a value for the default auto backing field. See this:

public int Foo { get; set => (value<0) ? 0 : value; }

or

private object CheckNullObject(object arg) {
    if (arg ! =null) return arg;
    else throw new ArgumentNullException();
}
public string Foo { get; set => (string)CheckNullObject(value); }
public Stream BarStream { get; set => (Stream)CheckNullObject(value); }

thus cleaning the code and still being intuitive (in my eyes).

PS: would be even nicer when allowing throw in the null-coalescing operator

public string Foo { get; set => value ??  throw new ArgumentNullException();  }
public Stream BarStream { get; set => value ??  throw new ArgumentNullException(); }

@HaloFour
Copy link

HaloFour commented Feb 5, 2016

@lachbaer

what do you mean by "changing the meaning"?

He means that in your special syntax set all of a sudden happens to expect a value to be returned for some reason, despite the fact that in every other context a setter accessor returns void. Also, => does not imply that a value will be returned as the following expression-bodied member is perfectly legal:

public void SayHello(string name) => Console.WriteLine($"Hello {name}");

@lachbaer
Copy link
Contributor

lachbaer commented Feb 5, 2016

*@HaloFour *
I now see the point! :-)
set is kind of an internal delegate void set<T>(T value); that is just called when writing to a property.
And as the return value is not part of the method's signature it cannot be overloaded with
delegate T set<T>(T value);. It would probably be a very big style break to 'hack' this into the compiler.
That said, expression syntax can be allowed for set for the sake of consistency (if get allows it i.a.), though it only eases reading and having less curly braces.

public string Foo {
  get => _bar;
  set => _bar = value ?? throw new ArgumentNullException();   // throw: see pattern.md for C# 7
}

In #8364 I'm now suggesting the use of a new let keyword as surrogate for set in auto-properties, that addresses the overload problem with a return value. For that the expression body syntax could fully apply.

@lachbaer
Copy link
Contributor

lachbaer commented Feb 6, 2016

I had a look into the roslyn source code and it seems to be very easy to make this work.

@lachbaer
Copy link
Contributor

lachbaer commented Feb 9, 2016

I have implemented the expression bodies for getter and setter (for regular properties by now). They are working fine in the test environment. But I don't know how to contribute it to the git archive. Can anybody help?

@alrz
Copy link
Member

alrz commented Feb 9, 2016

Interesting.

@gafter gafter self-assigned this Feb 9, 2016
@gafter gafter added Community The pull request was submitted by a contributor who is not a Microsoft employee. 0 - Backlog labels Feb 9, 2016
@gafter gafter added this to the 2.0 (Preview) milestone Feb 9, 2016
@gafter
Copy link
Member

gafter commented Feb 9, 2016

We will discuss if we want to do this in the next language version based on the work of @lachbaer.

@pawchen
Copy link
Contributor

pawchen commented Feb 11, 2016

Interesting.

@alrz
Copy link
Member

alrz commented Feb 11, 2016

@DiryBoy Don't. 😄

@pawchen
Copy link
Contributor

pawchen commented Feb 11, 2016

@alrz Well, I wonder why this is not in the current version.

@gafter
Copy link
Member

gafter commented Mar 3, 2016

@lachbaer We discussed this at the LDM today, and we are supportive of this, but we'd prefer to support all relevant contexts when we do this. In particular, we'd like to add constructors and destructors to the list of things that can have => bodies. Are you motivated to do that?

@gafter gafter modified the milestones: 2.0 (RC), 2.0 (Preview) Mar 3, 2016
@HaloFour
Copy link

HaloFour commented Mar 3, 2016

@gafter

Should both forms of read-only properties be supported?

public string FirstName => _firstName;
public string LastName { get => _lastName; }

@gafter
Copy link
Member

gafter commented Mar 3, 2016

@HaloFour Yes.

@lachbaer
Copy link
Contributor

lachbaer commented Mar 3, 2016

@gafter I'll have a look at it already on it. What else contexts are missing? using, lock?

For ctor and dtor I don't see that many use cases, but to be consistent over the language it would make sense.

Addendum: If allowing it for using and lock, which might indeed be useful, it should be allowed for for, while, etc. as well. But they will work without => already what might be confusing and also it looks ugly, because it breaks with C-like optics. Whereas using and lock make sense, I'll have a look at that, too.
using and lock support expression syntax, like for. In my eyes no arrow syntax should make this messy.

@lachbaer
Copy link
Contributor

lachbaer commented Mar 3, 2016

I added

<Field Name="ExpressionBody" Type="ArrowExpressionClauseSyntax" Optional="true" />

to BaseMethodDeclarationSyntax, because as far as I could see, all derived nodes now support the ArrowExpressionClauseSyntax.

@gafter gafter self-assigned this Aug 25, 2016
@gafter gafter added 3 - Working Area-Compilers and removed 4 - In Review A fix for the issue is submitted for review. labels Aug 25, 2016
@gafter gafter added 4 - In Review A fix for the issue is submitted for review. and removed 3 - Working 4 - In Review A fix for the issue is submitted for review. labels Sep 1, 2016
@gafter gafter modified the milestones: 2.0 (RC), 2.0 (Preview 5) Sep 7, 2016
@gafter gafter added 3 - Working New Language Feature - Expression-Bodied Ctor/Dtor/Accessor Expression-Bodied Members 4 - In Review A fix for the issue is submitted for review. and removed 3 - Working labels Sep 8, 2016
@ViIvanov
Copy link

ViIvanov commented Oct 1, 2016

What about support expression bodies for events: add/remove methods like get/set methods for properties?

@ViIvanov
Copy link

ViIvanov commented Oct 1, 2016

Sorry, have found #11198 by tag New Language Feature - Expression-Bodied Ctor/Dtor/Accessor

@alrz
Copy link
Member

alrz commented Oct 2, 2016

public event EventHandler Event
{
  add => obj.Event += value;
  remove => obj.Event -= value;
}

@ViIvanov The above syntax is a sensible addition but #11198 talks about a more succinct one,

public event EventHandler Event => obj.Event;

which as my impression, isn't likely to be implemented.

@ViIvanov
Copy link

ViIvanov commented Oct 2, 2016

@alrz, Thank you for the clarification. Now I see the difference.

@gafter
Copy link
Member

gafter commented Oct 6, 2016

This was completed in #13543.

@gafter gafter closed this as completed Oct 6, 2016
@gafter gafter added Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented and removed 4 - In Review A fix for the issue is submitted for review. labels Oct 6, 2016
@fedeazzato
Copy link

This is a really nice feature. I would like to propose an extension to this one.

I find now and then a common scenario, where I have a class Foo that exposes a property Bar, and then a second class Blah that is composed with an instance of Foo, and has to expose the Bar property in Foo unchanged. eg

class Foo {
    int Bar { get; set; }
}

class Blah {
    Foo f = new Foo();
    int Bar {
        get { return f.Bar; }
        set { f.Bar = value; }
    }
}

With this proposal, Blah could be written like:

class Blah {
    Foo f = new Foo();
    int Bar {
        get => f.Bar;
        set => f.Bar = value;
    }
}

but that doesn't look to much of an improvement.
If I where interested in only exposing Bar in Blah as a readonly property, I could write Blah like:

class Blah {
    Foo f = new Foo();
    int Bar => f.Bar;
}

What I would like to be able to do, is say the property Bar in Blah is binded bidirectionally to the property Bar in the instance f of Foo. Maybe something like:

class Blah {
    Foo f = new Foo();
    int Bar <=> f.Bar;
}

Hope I'm not too late for this one.

@Ziflin
Copy link

Ziflin commented Dec 16, 2016

I'm not sure if this is the right place to bug this, but the following should be legal in C# and it's currently not:

[MethodImpl( MethodImplOptions.AggressiveInlining )] // <-- Error
public float Length => Math.Sqrt( X * X + Y * Y + Z * Z );

This results in:

Error CS0592 Attribute 'MethodImpl' is not valid on this declaration type. It is only valid on 'constructor, method' declarations.

Method attributes should be allowed on getter-only expression-bodied properties as the following is allowed:

public float Length
{
    [MethodImpl( MethodImplOptions.AggressiveInlining )]
    get => Math.Sqrt( X * X + Y * Y + Z * Z );
}

@gafter
Copy link
Member

gafter commented Dec 16, 2016

@Ziflin That is not a bug. If you need to place an attribute on the getter (but not on the property itself), you can't use the shortest form.

@lachbaer
Copy link
Contributor

lachbaer commented Dec 16, 2016

@fedeazzato
I understand what you mean, but a new topic for that would be more appropriate in my eyes.
Nevertheless, you suggest to delegate a property to an internal field, like you could do with methods:

class Foo {
    private OtherClass oc = new OtherClass();  // has int BarMethod();
    public int BarMethod() => oc.BarMethod();
}

For fields it would be something like:

class Foo {
    private OtherClass oc = new OtherClass();  // has int BarProperty { get; set; }
    public int FoosProperty <=> oc.BarProperty;
    // I'd rather use a syntax like:
    public int FoosProperty { get; private set; } => oc.BarProperty
   // allows arrow operator after get; set;
}

But, I am not a friend of this! Properties are somehow like fields, just with get/set/access field control. In terms of clean programming it wouldn't be a great idea if you could delegate a field:

/* discouraged example */
class Foo {
    private OtherClass oc = new OtherClass();  // has int BarField = 0;
    public int FoosField <=> oc.BarField;
    /* <=> operator delegates to oc.BarField; */
}

Well, it looks okay, but is really, really messy!

@Ziflin (also)
Having played around with Java in the last time (mea culpa ;-) I have to admit that strictly seperating properties into getter and setter methods - what internally they are - is a really good idea from the viewpoint of readability and understandability. You can clearly use access restrictions and attributes on either of the methods, setter or getter. Never combine getter and setter for reasons of abbrevation in code!

My conclusion: there sould be no <=> operator at all, that delegates or links properties or fields. Hiding fields by properties is an established practice and properties consist of getter and setter methods, which should not be hided further.

PS: #15708 is okay, but that is Visual Basic ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Area-Language Design Community The pull request was submitted by a contributor who is not a Microsoft employee. New Language Feature - Expression-Bodied Ctor/Dtor/Accessor Expression-Bodied Members Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests