dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.03k forks source link

Proposal: Semi-auto-properties with setters [topic has evolved] #8364

Closed lachbaer closed 7 years ago

lachbaer commented 8 years ago

Status 2016-03-02

This topic has evolve/changed from providing a name for the backing field to extending the auto properties of C# with setters, thus providing semi-auto-properties.

The initial proposal was droped on that way.


Original topic start

According to an Expression Body Definition of a property with the lambda operator it would be consistent to allow the => operator also in the get section of a property

        private string _customHelloMessage = null;

        public string HelloMessage
        {
            get => _customHelloMessage ?? "Hello World";
            set
            {
                _customHelloMessage = value;
            }
        }

This would be consistent as the lambda operator introduces a return expression.

Having this said, it would also be nice to shorten the set section of the property. Here I have some ideas.

For the following samples I assume that there is a field private string _customHelloMessage = null;

First we could also use the => operator, but I don't think that is a good idea as set is not a return expression. Just for an optical impression:

        public string HelloMessage
        {
            get => _customHelloMessage ?? "Hello World";
            set => _customHelloMessage = value;
        }

Looks better, but the only thing that makes a real differnce is the letter s or r in get and set. Also, as said, it is not consistent to the current use of =>.

The next idea is to extent the contextual keyword value to stand with set:

        public string HelloMessage
        {
            get => _customHelloMessage ?? "Hello World";
            set value _customHelloMessage;
        }

It looks a bit odd, but should be intuitive.

This idea could be extended for the case that you want to specify the backup field by yourself:

        public string HelloMessage
        {
            get value _customHelloMessage;
            private set value _customHelloMessage;
        } = "Hello";

Also note the implicit initialization, that sould be further possible for auto-implemented properties;

The above sample could be abbrevated even more with the value keyword declaring the backup field within the property definition:

        public string HelloMessage
        {
            value _customHelloMessage;
            get; 
            set;
        } = "Hello";

what we could even more shorten with

        public string HelloMessage { value _customHelloMessage; } = "Hello";

In case there should be modifiers, get and set must be explicitly stated.

        public string HelloMessage { value _customHelloMessage; get; private set; } = "Hello";

The get and set methods can be overwritten, but at least one must be 'default' (get; or set;) to make sense for the value being there, if it's there.

A backup field must never be implied automatically by the compiler, meaning the _customHelloMessage needs an explicit declaration. Otherwise it will be error-prone.

I hope this is a worthy idea.

Regards, Ike

dsaf commented 8 years ago

7881 #850

lachbaer commented 8 years ago

I like my suggestion more ;-) No, really! :) Think, that it's a neat re-use of the value keyword.

Also I like the idea of Property-scoped fields #850 ! But it stands in no contrast to this issue.

gwenzek commented 8 years ago
public string HelloMessage { value _customHelloMessage; } = "Hello";

Really neat !

lachbaer commented 8 years ago

Addition:

...
value _backingField;
...

should only be present 1 time, either after set or get or standalone. Specifying it twice (like in a sample above) is either redundant or again error-prone:

public string HelloMessage
        {
            get value _backing_ONE;
            private set value _backing_TWO;  // WTF, why 'two'?
        }

There might be cases where you want this intentionally, but I recommend using the good old get {...} set {...} for that scenario.

Having value either after get or set or standalone supports reading code in case you have something like this:

public string HelloMessage
        {
            get value _backingField;
            set {
                if (value == null) throw new ArgumentNullException();
                set value; // tells compiler to write to assign to declared _backingField
            }
        }

Please notice the set value; expression. Just in case you rewrite your code it ensures that the property value is always written to the correct, specified backing field. This is in direct accordance with the set value; outside the code block but inside the property.

HaloFour commented 8 years ago

The lambda (=>) operator is already valid in void methods so I don't see why that would be an issue:

private int x;
public void set_X(int value) => this.x = value;

The value keyword already refers to the proposed value in the setter accessor method. Having it do double duty as the backing field and the proposed value would be very confusing. set value in particular, as what are you setting, the backing field or the proposed value, the latter of which is referenced by the keyword value.

I really don't understand the point of specifying the backing fields at the scope of the accessors. get value _backingField doesn't come close to resembling a declaration and doesn't describe anything about the field. Scope-wise it doesn't make sense for something declared in the getter accessor to be accessible in the setter accessor, and it makes even less sense for the setter accessor to declare yet another backing field.

This proposal just seems like a syntactic jumble. I certainly prefer the syntax proposed in #850, at least where fields can be declared within the scope of the property.

lachbaer commented 8 years ago

@HaloFour you probably got me wrong. get value _backingField is an abbrevation of value _backingField; get;.

The _backingField is not a declaration, but the name of the backing variable. This could be a normal instance variable or according to #850 a property-scoped field.

value variable_name; in the scope of the first property block gives the compiler order to use _variablename as the (self declared) backing field instead of generating an own. This seems very consistent to me as e.g. string myStringVariable; declares myStringVariable for holding a string and value backingVariable; declares backingVariable holding the property value.

For set value; within a code block I agree with you (partly ;-) after giving it some thoughts. :-)

Maybe a single set; would suffice to instruct the compiler to put value in the backing field? See this:

  public string AnotherMessage {
    get; // implicitly created getter, we don't have access to the backing field!
         // unless of course no 'value ...' instruction is given before 'get;'
         // This short 'get;' allone tells the compiler to create an auto-property.
    set { 
        TestForNullValue(); // e.g.
        set; // put the value in the backing field, though we don't know it.
    } 
  } = "Hello again";

value is a contextual keyword, valid only within the set accessor of a property declaration. So imho there is nothing against making set the same!

I also agree using the => lambda operator for set operations (#7881) , though primarily intended for returning values, syntactically it is already correct for void delegates().

So, after all for me it get's down to

   public string HelloMessage { value _customHelloMessage; get; private set; } = "Hello";
   // or having 'get' and 'set' implicity implemented when ommited:
   public string HelloMessage { value _customHelloMessage; } = "Hello";

   public string AnotherMessage { get; set { doSomething(); set; } }

dropping off all of this get value variable; set value; thingy.

Should I open a new issue for this revised idea?

HaloFour commented 8 years ago

I think it's fine for a proposal to evolve in place.

In effect it is a declaration if the result is an instance field. But I don't see a real use for having it if you don't intend to use it directly as an identifier.

I'm personally not a big fan of new syntax that straddles normal properties and auto-properties. I think it confuses the reason as to why you'd use one or the other. But let's break this down into the use cases. Namely, it's about being able to add custom validation or logic to the accessor, most likely the setter accessor. Currently that requires going from:

public string Foo { get; set; }

to

private string foo;
public string Foo {
    get { return foo; }
    set {
        if (value == null) throw new ArgumentNullException("value");
        foo = value;
    }
}

That is big jump in verbosity and it is understandable to want to find some kind of short-hand. Perhaps something like the following:

public string Foo {
    get;
    set {
        if (value == null) throw new ArgumentNullException("value");
        set;
    }
}

Although does that honestly buy you much? You eliminate the getter boilerplate, the instance field declaration and the manual assignment, but the property is still almost as long. And is it really clear what the property is going to do? I'm not completely sold, but I can see value in going down that path.

As for syntax for declaring the backing field, I don't think that has any value unless you actually use that field directly. I don't see proposals like #850 as being about verbosity but rather about visibility; trying to prevent accidental direct assignment of the backing field. I wouldn't expect a field declared within the body of a property to be accessible anywhere else in that class. So the two examples where you define the backing field and never refer to it directly in the accessors has no value to me.

leppie commented 8 years ago

How about just:

string bar; // a field
public string Foo => bar, x => bar = x;

Or more extreme:

string bar; // a field
public string Foo => bar, bar = value; // as if value => bar = value
HaloFour commented 8 years ago

Extreme is right. Definitely on the wrong side of the succinct/terse spectrum in my opinion. Do we really need a syntax shorter than auto-properties? What is bar supposed to mean?

leppie commented 8 years ago

@HaloFour: the backing field (in that example)

HaloFour commented 8 years ago

Declared elsewhere or entirely implicit? If the latter what is its visibility? Type?

leppie commented 8 years ago

@HaloFour backing field was a bit wrong terminology :D I meant just any field (but you one you use to back the property with)

Updated example

HaloFour commented 8 years ago

@leppie I think that the term is appropriate, it just didn't clear up the confusion I had as to where it was declared. :smile:

I think that it's a pretty far departure to property syntax today. I think that I'd prefer something more akin to:

private string bar;
public string Foo { get => bar; set => bar = value; }

Which is effectively just normal properties combined with expression members.

lachbaer commented 8 years ago

In your last sample you wrote 3 times 'bar'. This is slightly shorter, especially if you want many properties with own backing variables:

private string bar1;
private string bar2;
private string bar3;
//...
private string bar19;
private string bar20;

public string Foo1 { value bar1; }
public string Foo2 { value bar2; }
public string Foo3 { value bar3; }
// ...
public string Foo19 { value bar3; /* uses field bar3 for some reason */ }
public string Foo20 { value bar20; }

One could also use value in the getter

private int _counter;
public int Counter {
    value _counter;
    get {
        value = CalculateSomething(value);
        return value;
    }
    private set;
}

When you change your mind and you want to switch from _counter to base.counter then guess where and what you only have to change.

I think it even gets more intersting when we reference other properties instead of fields with the value keyword.

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

Writing this brings me back to the lambda operator => for the set part: It should be used for a return value, i.e. the value that is writte to the backing field:

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

If one doesn't want to set a value to the backing or wants to do some other important stuff, she should use the common set { ... }.

HaloFour commented 8 years ago

@lachbaer

Why would you ever want to use that syntax? It makes no sense to declare a backing field and a property that does nothing more than wraps that backing field. We already have auto-properties.

Using value to alias fields/properties/whatever doesn't sound like a good idea. You save a few keystrokes but now you have to look in multiple places to find out what that property is supposed to be doing and the semantics could change depending on what was being aliased, e.g. passing value to a function as a ref argument takes on a very different meaning if value is a field vs. a property. I'd rather type the extra keystrokes and declare my intent more apparently.

I don't see the use cases here.

lachbaer commented 8 years ago

You're giving me headaches :-D ;-) What I want to achieve is to specify the backing field of (semi-)-auto-properties. By now you cannot secify an auto-property with even some lightweight code, as there is definitely use for it! Let's get rid of the 'value-definition' for the moment.

public string Foo {
    get;
    set => { if (value==null) throw new ArgumentNullException(); return value; }
}

here we have an auto property where we are not really interested in the backing field, just in the argument checking. Without auto-property it is slightly more cumbersome to write. But where should set put the returned value? get; gives the hint of an semiauto-property. But what if this hint (could be set; also) is not given?

Let's try another syntax, still c-sharpy, because the property kind of 'inherits' the backing field:

private string _backingField;
public string Foo {
    get => "Foo is " + value ; // oops, no more hints for an auto-prop!!!
    set => { if (value==null) throw new ArgumentNullException(); return value; }
} : _backingField = "Initial string";

Now it is obvious what value refers to, without the value keyword before the get. ;-) @HaloFour look at this code snippet.

private T CheckNull<T> (T myvalue) {
    if (myvalue==null) throw new ArgumentNullException();
    return myvalue;
}
/* complete auto-property, just with a bit of setter logic */
public string FirstName{ get; set => CheckNull<string>(value) } = "";

/* here no auto-property can be created, because getter and setter are defined,
   therefore we reference the backing field */
private string _lastName ;
public string LastName{
    get {
        Console.WriteLine("Another get on family " + value);
        return value;
    }
    set => CheckNull<string>(value);
} : _lastName = "";
public string FullName { get => FirstName + " " + _lastName ; }

And if this is too much code for a sample, here's an auto-property with explicit backing field:

private int _bar;
public int Foo { get; set; } : _bar;
public var Foo2 { get; set; } : _bar; // type inference, looks ugly...
HaloFour commented 8 years ago

@lachbaer

I get what you want. What I don't understand is why. If you're going to the trouble of defining a backing field then you have no reason to use auto-implemented properties. Trying to hammer them together makes little sense and solves no problems.

As for trying to add logic into auto-implemented properties, I must note that the charter for auto-implemented properties is to provide a concise syntax when no additional logic is required in the property accessors. I'm largely of the opinion that trying to add logic back into auto-implemented properties doesn't make a lot of sense particularly since it's quite easy to define a full-fledged property.

That said I think that there may be some compelling cases to trying to squeak validation into an auto-implemented property setter accessor but only if the syntax is concise enough and clear enough and I haven't seen anything that I think fits the bill.

lachbaer commented 8 years ago

A little summary by now:

/* this is still an auto-property */
public int Foo { get; set => (value<0) : 0 : value; } 

For self-defined properties (not auto), if you want to use the expression syntax on set or want to use the value keyword in a get section, then you must specify the name of the backing field after the property's closing brace with a :.

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

Assigning a backing field manually to an auto-property could be done, but actually makes no sense. But maybe it might be easier to implement it in the complier that to 'explement' it ;-)

protected int _foo;
public int Foo { get; protected set; } : _foo;
lachbaer commented 8 years ago

@HaloFour You ask, why I want to do that. Well, many things in C# are syntactic sugar, since the first versions. Image following scenario:

private T CheckNull<T> (T myvalue) {
    if (myvalue==null) throw new ArgumentNullException();
    return myvalue;
}

public string MrMrs { get; set => CheckNull<string>(value) } = "";
public string FirstName{ get; set => CheckNull<string>(value) } = "";
public string MiddleName{ get; set => CheckNull<string>(value) } = "";
public string LastName { get; set => CheckNull<string>(value) } = "";
public string Address1{ get; set => CheckNull<string>(value) } = "";
public string Address2{ get; set => CheckNull<string>(value) } = "";
public string ZipCode{ get; set => CheckNull<string>(value) } = "";
public string City { get; set => CheckNull<string>(value) } = "";
public string Country { get; set => CheckNull<string>(value) } = "";
public int Age { get; set => (value<0) ? 0 : value; };

This was written really fast with copy-paste.

Specifying the backing field and using value instead in the code reduces typos and eases code refacturing.

HaloFour commented 8 years ago

@lachbaer

Having the setter accessor expression require a returned value doesn't make a lot of sense given that setter accessors return void. I understand that you're trying to define the expression as the assignment to the backing field itself but given that the behavior is inherently different I think it would be quite confusing. Not to mention it makes it absolutely impossible to simply not assign the backing field via return. Assigning the backing field to its own value isn't the same thing and could cause unexpected thread-safety issues.

lachbaer commented 8 years ago

@HaloFour I don't want to replace the old syntax where set has a { ... } block. Here no return value is necessary. But why using an => expression instead of { ... } when they are identical in the outcome? That's why I thought it would be more consistent to use => with a return value. As with the new with on pattern matching.

HaloFour commented 8 years ago

It's not consistent. To be consistent it should behave as a property setter is supposed to, including having a void return parameter. It should be consistent with using member expressions on any other void method. Making it an expression that returns a value makes it quite inconsistent.

lachbaer commented 8 years ago

Can you rewrite the above addess sample without set => ... for assinging the value by return? Maybe I oversee something?

HaloFour commented 8 years ago

@lachbaer

Not with auto-implemented property syntax given that there is no way to directly access the backing field. You could potentially add a new contextual keyword to reference it but that doesn't seem terribly useful, not to mention confusing if that keyword is in use in any other way.

With normal properties, you could potentially do the following:

private void CheckNull<T>(T proposed, ref T field) {
    if (proposed == null) throw new ArgumentNullException();
    field = proposed;
}

private string name;
public string Name { get => name; set => CheckNull(value, ref name); }

That said, I figure that expression member syntax would apply more to the getter than the setter.

lachbaer commented 8 years ago

I still stick with the basic idea of my proposal. Auto-properties were introduced, because they were shortening code a lot. Expression body methods and properties were introduced with C# 6 to shorten this. Of course this could all be done with the old language features, but what's the idea behind evolving a language? There are already some easements, like Expression Body Definitions [(https://msdn.microsoft.com/en-us/library/x9fsa0sw.aspx#Anchor_0)]. We can go some steps further.

You might be right that declaring an external backing field and still using coding shortcuts is nothing that would be done often in real code, probably never seriously. So I drop this idea for the moment.

Let's focus on having more control over auto-props, thus reducing the need for an instanciated backing variable:

private string _customHelloMessage = null;
public string HelloMessage
{
    get { return _customHelloMessage ?? "Hello World"; }
    set { _customHelloMessage = value; }
}

This is already using new C# features, but nevertheless it is quite long for achieving a small thing. Actually I don't really need the _customHelloMessage field and it annoys me seeing it in my Intellisense list.

With my idea we could shorten this long code a lot!

public string HelloMessage
{
    get => value ?? "Hello World";
    set;
} = null;

Next I want to check for null in the setter

public string HelloMessage
{
    get;
    set => value ?? "Hello world";
} = null;  // initial value goes through setter for integrity!

I think we agree that set => only makes sense on auto-properties or when we declare a backing field (what we don't want to do any more outside of the property scope ;-).

Getting back to the idea of #850, my proposal for the use of the value keyword eliminates the need for a property-scoped variable as backing purpose.

Now there might be scenarios, where you still want some more control over your code, but you also want kind of an auto-prop. This might be the case if your code grows. Here I introduce the new contextual keyword let, that instructs the compiler to generate an (unnamed) backing field, like for auto-props.

public virtual int Foo
{
    let backingField = 0; // creates a backing field with the appropriate type, optionally initializes it.
    get;
    set => backingField + value;
} = 12; // initializes again, but using the setter this time

// one useless sample to make it clear ;)
public int Counter {
    let mycounter = 0;
    get => ++mycounter;
    set => 0;  // any set call resets the counter
}

let now creates a backing field that will not list anywhere in my code (again, see #850).

Derived classes don't have access to the property internal backing field of the base class. It would not be intuitive in the derived classes and mess things up.

Résumé: This could make code significantly shorter for many all day routines as well improving stability, because there is no more need to declare an instance backing field that could be accessed from anywhere within the class or even worse the whole code (when accidentially being public).

Any occurence of get;, set;, set => or let ... indicates to the compiler to create an 'auto-property', that can be customized.

HaloFour commented 8 years ago

Shorter code is not always better code. Clarity is significantly more important. Code will be read (by humans) significantly more often than it will be written, so the language should always optimize for that case.

I disagree with the premise of trying to create a spectrum of syntax between normal properties and auto-implemented properties. I don't think that normal properties are so verbose as to be a burden on a developer, particularly when you need additional logic.

lachbaer commented 8 years ago

Sorry, but I disagree. I had a night to sleep over it and I still think that my latest draft is really fine in the sense and feel of C#.

In C# quite a lot is about encapsulation and clearance of code. Creating a private instance backing field if you just want to alter small code pieces of a standard property does not stand in this line. You could be tempted to access the private field in your code instead of using the setter, what could lead to semantic errors if your code grows.

C# alread has an approach to easier writing properties with the expression body syntax. It is just consequent go a step further.

In these expression body syntax => returns a value. Expression body syntax for get returns a value. Most used lambda expressions return a value, likewise the expression body syntax for methods.

So in my eyes it is only consequent for set => to accept return values. And these return values go to the standard backing field. As in probably most, most cases even a customized property has a standard getter or setter and this syntax enhancement could not only save writing code, but also clear the code of unnecessary side effects, like actually 'stale' private fields for the rest of the code.

And for the sake of readability...

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

... is in my eyes absoluetly clear to understand, even to novices that have just heard about properties.

The new let *variable*; keyword extends the idea of #850 to having a standard backing field that can be used with set => and the value-keyword in the getter. It also avoids declaring an internal backing field of a different type other as the properties own type and (like in #850) helps hiding this field from the rest of the class implementation.

So, the more I write about this approach, the more I like it and I would really be pleased to see it in C# 7.

lachbaer commented 8 years ago

Next idea, just for people who really, really need it:

protected int internalBackingFieldForFoo = 1;
private int Foo {
    let _backing : internalBackingFieldForFoo;
    get;
    set => calculateSometing(value);
}

makes _backing the standard backing field of this auto-implemented property, but saves its value in the protected internalBackingFieldForFoo. So this 'inheritance' of _backing references the actual backing field - whereever it is. This approach is the syntactically most consistent within this proposal.

Just to make this clear: this extension is only for people who really need it. Others could just keep their fingers away from : internalBackingField; ;-)

lachbaer commented 8 years ago

Suggestion: what if the backing field is not of the type of the property? See this:

public string PrintOrderDate {
    default DateTime orderDate;
    set => DateTime.Parse(value);
    get => /* format the date output depending on its age */;
}

Maybe the default keyword is more verbose than let, and it comes as kind of a modifier. For type inference of the backing field, we could still use var

/* this makes no sense, but it's a just a presentation */
public string PrintOrderDate {
    default var orderDate;
    get; set;
}

This gives space for defining more property-scoped variables (though I am not a friend of latter).

lachbaer commented 8 years ago

As HaloFour stated above and what became clear to me in #7881 set should really not expect a return value, as this would be a breaking change to C# internals.

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.

Therefore instead of set we could use let for semi-auto-properties to achieve the above discussed. The use of let would always instruct the compiler to automatically create an auto backing field.

In terms of C# internal representation to method signatures we then would have

delegate T get<T>();    // called for 'normal' getters
delegate void set<T>(T value);  // called for 'normal' setters
delegate T get<T>(T value);  // called for getters of '(semi-)auto-properties'
delegate T let<T>(T value); // called for getters of '(semi-)auto-properties'

Now expression body syntax (#7881) could be applied to all of them.

So, next question is when to create an auto-property? That's even more easy now with let:

The use of set { ... } must be forbidden in semi-auto-properties! get; set; and get; let; should be synonyms, for backwards compatibility. But for semi-auto-properties the use of the let keyword is mandatory.

/* this makes no sense, but should point out the importance of 'let' for the compiler */
public string Foo {
   get => "I always return this";  // is this 'normal' or 'semi-auto-property' ???
   let;  // ah, it's 'semi-auto-property', now I know what to do...
}

I had a look in the current roslyn C# 6 code and think that this suggestion is the easiest to implement.

BTW, I let drop the suggested use of the default keyword. HaloFour is absolutely right that when anything too fancy is to be done, one should rely on the old scheme, maybe together with #850.

lachbaer commented 8 years ago

Can anyone think of a practical use case of modifying the getter of an auto-property?

An auto-property must have a getter, otherwise what is the purpose of having a backing field if you can only set it, but never get it? Using a standard method is what you normally do for this.

So the only cases I can think of by now is either to log something or to set kind of a counter (or timeout timer) when a value is accessed. But again this would probably be already too special, so that a standard manual implementation would be better, maybe with #850.

I.e. that an auto-property must always have a get; statement! (so no body)

On the other side, having set analyse the value and manipulating it, so that the backing field already contains the processed value makes a lot of sense to me. Just see the samples above that already legalize this idea!

So I get back a step and consider allowing set { ... } or set => for auto-properties. BUT this time the set routine must - like get always - return a value, that is then stored in the automated backing-field.

This shouldn't be too hard, as the current Roslyn source code already brings a lot for implementing this.

DavidArno commented 8 years ago

My take on this is very simple: a property with a setter should be a last-resort option when invariance simply cannot be avoided. In these situations, you are likely to want more than one line of code in the setter, so the expression body syntax wouldn't apply.

Syntax changes that lower the pain of writing "bad code" should be rejected. This proposal is one such change.

lachbaer commented 8 years ago

@DavidArno You probably refer to #7881. But I don't think so! Checking and modifying the submitted value is exactly the thing that setters were made for. And they exist since C# 1.0.

lachbaer commented 8 years ago

I still stick with my description from a few days ago, before gafter added the 0 - Backlog label and start to like it more and more.

But I made up my mind concerning the initialization of a semi-auto-property by adding = "initValue"; to it. In short: initializers must not be allowed for this kind of auto-property!

First of all, semi-auto-properties (with setter) should only be allowed if "not so fancy stuff" is to be done, like with the current auto-properties. I.e. that specifying a setter defines kind of a constraint that the property must meet, like never being null or having only positive values.

And even an initializer must meet this constraint! Otherwise it would already be somewhat special and a semi-auto-property is not what should be used in such a case. Therefore C# should not (easily) allow writing that kind of code. It's all about unambiguous code :-)

Another reason is the point where this constraint should be checked. Initializers are inspected at compile time. But the compiler cannot examine the semantic complexity of self-written setter's algorithm. So this initializer must be called before the constructor, and now an exception could be thrown in the setter. (I admit, this already is possible when constructors are called as initializing value...)

Last, initializers are currently not allowed for regular properties - probably for a good reason!

DavidArno commented 8 years ago

@lachbaer,

goto has been in C# since v1 too. Just because a feature exists in a language, doesn't mean it's a good feature. The same applies to setters in my view. Coming across set in code should be a rare event: it should only occur in edge cases. The majority of the time, the field behind a get should be readonly.

So this proposal is the opposite of moves to improve the language to encourage "falling in to the pit of success" and thus I think it a bad idea. The same applies to #7881.

I found this proposal whilst looking to see if anyone had proposed an "initialiser setter", ie one that can only be called as part of object construction. That would be a good proposal in my view, this one isn't.

HaloFour commented 8 years ago

@DavidArno

I found this proposal whilst looking to see if anyone had proposed an "initialiser setter", ie one that can only be called as part of object construction. That would be a good proposal in my view, this one isn't.

That whole concept was tossed about during the design of read-only properties. The issue is that readonly fields cannot be assigned from any method outside of the constructors so it would not be possible to have true initialize-only setter accessor methods. At best the compiler would have to copy the setter code into the constructor but that would be kind of messy. That's why read-only auto-properties assign directly to the backing field from a constructor or initializer.

DavidArno commented 8 years ago

@HaloFour

I disagree with those assumptions. Let me give you an example:

class C<T>
{
    public T P { get; let; }  // "let" is used to say it's a "construction-time only setter

    public SomeMethod(T v) =>  P = v;  // this is a compilation error as "P" is read-only
}

...

var x = new C<int> { P = 1 };  // this is valid
x.P = 2; // this is a compilation error; P is now read-only.

I suspect this would have to be "syntactic sugar", as I doubt the CLR supports it and, someone trying hard enough, could work around it via reflection. However, the benefits of overcoming the current conflict between wanting a property to be read-only and avoiding too many constructor parameters, far outweigh such minor limitations.

HaloFour commented 8 years ago

@DavidArno

What assumptions? The CLR does not permit an initonly/readonly field to be assigned to from anywhere but a constructor. Doing so produces an unverifiable assembly.

There have been proposals tossed around to better support object initialization with immutable types, e.g. #229. However, whatever that syntax is must map to constructor arguments, otherwise the fields simply cannot be initonly/readonly.

DavidArno commented 8 years ago

@HaloFour,

The assumption that it cannot be achieved because it can only be achieved via the CLR and the CLR doesn't support it. As I showed though, it can be achieved without modifications to the CLR.

Whilst #229 is indeed such a proposal, it all too quickly gets bogged down in suggestions that records will solve this. As far as I know, the current proposal for records is that they result in classes that do not override == and != and thus are flawed for two reasons:

  1. They can be null
  2. They are equality compared by reference, rather than value

As such, it seems sensible to have a plan B for creating immutable structs and classes that support object initializers.

HaloFour commented 8 years ago

@DavidArno

Until you can demonstrate how your syntax can be translated into proper C# (or IL) which retains the behaviors and restrictions that you described you haven't really shown anything. Inventing syntax is the easy part.

You have property setters and getters. A setter has no idea how its used and frankly doesn't care. Object initializers are just shorthand for calling the constructor and then calling the individual setters accessors serially. The logic is disjointed and there is nothing the declaring class can really do to enforce that the consumer is following the expected behavior, nor anything the CLR can do to actually enforce any form of immutability. At best the class could ensure that the setter isn't called twice assuming that the class maintains some kind of mutable state internally, but it couldn't do anything about how and when the setters are called in relation to the constructor.

I'm all about improved syntax for working with immutable types and structs, though. I don't find it that problematic with readonly auto-properties and optional constructor arguments, though:

public struct Point {
    public int X { get; }
    public int Y { get; }

    public Point(int X = 0, int Y = 0) {
        this.X = X;
        this.Y = Y;
    }
}

var point = new Point(Y: 2);

I do agree that integrating object initialization syntax there might be nice, although I'm not really sure what it buys you. The #206 proposal does mention record structs which might be able to decrease the verbosity. Unfortunately the link to the record spec is a 404 at the moment.

DavidArno commented 8 years ago

@HaloFour ,

Until you can demonstrate how your syntax can be translated into proper C# (or IL) which retains the behaviors and restrictions that you described you haven't really shown anything. Inventing syntax is the easy part.

Good point. Thinking it through further, slightly reworking the example I gave:

class C<T>
{
    private readonly int _field1;

    public C(int field1) { _field1 = field1; }

    public T Prop1 { get; let; }
}

could be converted to:

class C<T>
{
    private readonly int _field1;

    public C(int field1, T Prop1 = default(T)) 
    { 
        _field1 = field1; }
        this.Prop1 = Prop1;
    }

    public T Prop1 { get; }
}

And then the rules around object construction/initialisation could be changed to allow C<T> to be constructed in one of the following two ways:

var x = new C<int>(1, 2);
var x = new C<int> { field1 = 1, Prop1 = 2 };

but this isn't really what I'm after. So I'll accept at this point that to make it work as I want, there'd need to be a change to the CLR and so it's likely not a good proposal for C# 7.

DavidArno commented 8 years ago

What I therefore need to do is to campaign more for records to be structs rather than classes. That would fix both of my concerns around records and would affectively achieve the let idea (assuming record construction must use named parameters:

struct SomeRecord<T>(T Prop1)

var x = new SomeRecord<int>(Prop1: 1);
lachbaer commented 8 years ago

Intermediate State

[Let's go back to the initial topic]

Negatives

  1. Having automatic backing fields in properties, just to shorten the accessors occasionally to get; or set; is needless
  2. Having a backing field created automatically, that can be accessed within the code by a self-chosen name is also needless
  3. Reuse of such cases of the value keyword and restructuring property syntax is misleading and ambiguous
  4. Many initial proposes of this issue will be better off with #850 (Property-scoped fields) and #7881 (Expression bodied accessors)

Positives

  1. Semi-auto-properties do not create a syntax-change (though semantic, see below)
  2. There is often a practial need for setters to do additional value-rewriting, checking, triggering side-effects and events.
  3. May scenarios could be done with auto-properties, if it weren't for item 2.

Conclusion

Example 1:

public int FooSemiProp
{
    get;
    internal set
    {
        OnFooSemiPropChanging(/* old value */ this.FooSemiProp, /* new */ value);
        return value;
    }
}

Example 2 (with possible upcoming syntax changes)

public int FooSemiProp
{
    get;
    set => (value > 0) ? value : throw new ArgumentOutOfRangeException();
}

Semantic assumptions

Remarks

On first sight it may seem strange for set to await a return value from the defined method. But as there is no direct way to access the backing field this is the only way to assign a value to it. The internal method and binding is nevertheless based on what the accessor should do in respect to the usage (whether it is a getter, a setter, or a regular auto-property).

Questions

    internal set
    {
        var oldValue = this.FooSemiProp;
        yield return value;
        OnFooSemiPropChanged(oldValue, /* new */ value);
    }
lachbaer commented 8 years ago

How can a OnFooSemiPropChanged event be made possible without defining a regular property the long way? Maybe by yielding the value and then immediately returning to the setter method.

After a night of sleep I have to say that yield does not feel right. yield should be for iterators only!

But for semi-auto-properties the signature for set could be changed to void set(T value, ref T field); Then the backing field can be accessed by reference through the field local parameter.

  internal set
    {
        var oldValue = field;
        OnFooSemiPropChanging(oldValue, /* new */ value);
        field = value;
        OnFooSemiPropChanged(oldValue, /* new */ value);
    }

field is a (reserved) keyword, but like let, await, yield, etc. they are allowed as identifier names for locals, so there would be no conflict here.

What still bothers me is, that I'd like to have a the set routine return a value for assignment

  set => (value > 0) ? value : throw new ArgumentOutOfRangeException();

What about an additional put keyword, that can be used instead of set and requires the return value. Signature is T put(T value, ref T field);. put makes it clear that the signature is different and the return value is awaited.

lachbaer commented 8 years ago

The longer I think about it, I have to admit that having a put over set does probably not the justify the effort of changes to Roslyn. Though I still like the "look" of it very much, it might also be a bit confusing to programmers, what actually the difference is between the two. And besides it is only for the purpose of semi-auto-properties.

/* "field" is a reference to the underlying synthesized auto-property backing field */
public int Foo {
    get;
    set { field = (value > 0) ? value : throw new ArgumentOutOfRangeException(); }
    /* or */
    set => field = (value > 0) ? value : throw new ArgumentOutOfRangeException();
}

could be rewritten to

public int Foo {
    get;
    put { return (value > 0) ? value : throw new ArgumentOutOfRangeException(); }
    /* or */
    put => (value > 0) ? value : throw new ArgumentOutOfRangeException();
}

That last put => has optically something nice to it. But when you compare

    set { field = (value > 0) ? value : throw new ArgumentOutOfRangeException(); }
    put { return (value > 0) ? value : throw new ArgumentOutOfRangeException(); }

Here there is really not much of a gain - if not even none at all... But this:

    set => field = (value > 0) ? value : throw new ArgumentOutOfRangeException();
    put => (value > 0) ? value : throw new ArgumentOutOfRangeException();

Of course the 2nd with put looks much cleaner, but does just that look justify introducing a new keyword that might cause ambiguity between set and put? I can't tell and leave it to the C# language design team.

The observant reader might have noticed the use of a local called field. That idea, though, I think is very, very good!

In case of a semi-auto-propery the signature of set changes from

    void set(T value);

to

    void set (T value, ref T field);  /* field is a reference to the synthesized backing field */

This would keep syntax changes nearly not recognizable, and so stays in line with the existing C#. But it empowers C# to define setters for lightweight auto-properties, semi-auto-properties.

[ADDENDUM] The public signature must not change, because otherwise you cannot convert an auto-property to a semi-auto-property without recompiling all referencing binaries. So, this must be internally converted to

    private void set (T value, ref T field);  /* field is a reference to the synthesized backing field */
    public void set(T value) => set(value, ref <Foo>k__BackingField);

For private or assembly internal calls the call should go to the extended set method directly.

lachbaer commented 8 years ago

@gafter I have implemented that previously described behaviour as a first feature preview in Roslyn and would like to submit a pull request, after I have prepared the branch for it. The changes were minimal, understanding the Roslyn code was the hard work ;-)

[UPDATE:] There are still some issues that must be fixed (see addendum above). E.g. now a set_XXX method is generated with the call breaking non-optional ref field. Apart from that it works fine :-)

Can anybody shed a light for me on how to achieve the wanted rewriting? Any hints where I could look such things up in the code?

lachbaer commented 8 years ago

What do you think about following syntax for quickly defining a backing field?

public string FirstName {
    get _firstName = "";    // _firstName is 'private string _firstName = "";'
    set { _firstName = value ?? "";  }
 }

Scope of the field is property scoped (#850), or class scope as long as #850 is not implemented. Declaration of the field name could be either after the getter or setter, otherwise the property is probably more complex and a shortcut is not needed.

Advantages over the previous suggestion:

Disadvantages:

lachbaer commented 8 years ago

In #850 @bondsbw complained that the last syntax is not concise. I agree somehow - it was just a thought.

I'll try to point out my goal with this proposal and why it seems important to me.

In the beginning the only way to define a lightweight, no intelligence property like this: (for those, who don't remeber the old times or never got to know them ;-)

private string _firstName = "";
public string FirstName {
  get { return _firstName; }
  set { _firstName = value; }
}

You will notice that _firstName is written down 3 whole times, just to achieve to have simple property. Quite a lot of code for light goal...

That probably lead to code that relies on public class fields ('variables') more than properties as 'public interface'. It was just too much to write, even with code snippets.

Then, probably for that very reason, in C# 3.0 auto-properties were invented. Now the above code shortens to

public string FirstName { get; set; };

Much better, but there was no way to initialize it. The first access to the above property returns null. Also this doesn't allow for quick read-only properties in immutable classes. If you want a property like the one in the first example, still a whole property must be programmed - leading lazy programmes to accept the constraints of auto-properties, and so possible 'dangerous' code.

C# 6 corrects this

public string FirstName { get; } = "";
public string LastName { get; } = "(none)";
public string FullName => $"{FirstName} {LastName}";

public int Age { get; set; } = 0;

Much better, immutable and with initialization.

Now to the reasons of this proposal: There is no possibility to influence the supplied value. If somebody wants to set a negative age on the sample above neither an exception is thrown nor a correction to a minimum of 0 is done. This lightweight procedure demands a full property like in C# 1.0 . Again it is very probable for our lazy programmer that she implies that nobody will ever probably do so. As it does not make any sense it will (maybe) never happen ;-)

So, the reason why auto-properties were invented and extended with further possibilities were in the first place was to encourage programmers to use them to make their code more stable and reliable, preventing weak spots and points of failure in the applications.

I want to try to find a syntax that fits into the style of C# and that allows writing of lightweight properties. Those are properties that have no explicitly backing field and only a custom setter or custom getter. [There might be reasons to recalculate the value the moment it is accessed].

The following quote from MSDN should still stay true for semi-auto-properties:

Attributes are permitted on auto-implemented properties but obviously not on the backing fields since those are not accessible from your source code. If you must use an attribute on the backing field of a property, just create a regular property.

I cannot emphasize enough that this proposal is mainly about verification setters. If it emits a syntax feature that is short, precise and could do more, I'm fine :-)

lachbaer commented 8 years ago

As local functions will come with C# 7 very likely the following solution might be the currently best approach.

public string FirstName {
   get;
   set { field = value ?? ""; }
} = "none";

will rewrite to ('k__' = compiler generated symbols)

private string k__BackingField = "none";
public string FirstName {
  get { return k__BackingField; }
  set { 
    void k__set_value(string value, ref string field) { field = value ?? ""; }
    k__set_value(value, ref k__BackingField);
  }
}

likewise for get {...}, if only set; is stated. Either get; or set; introduce a semi-auto-property.

Rewriting takes place later in the compiler, so this might be a good way?

lachbaer commented 8 years ago

One sample for a semi-auto-property with getter:

public string FirstName {
   get => field ?? "";
   set;
} // = null is implicitly initialized

This would avoid accidentially initializing the property with null and returning this invalid value.

Maybe it is a wise idea to make an initialisation mandatory for semi-auto-properties! It ensures that the programmer choses a suitable starting value for the property, that complies with his setter rule, instead on relying on the compilers chosen init value, that might violate it.