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: Property-scoped fields #850

Closed
bondsbw opened this issue Feb 25, 2015 · 109 comments
Closed

Proposal: Property-scoped fields #850

bondsbw opened this issue Feb 25, 2015 · 109 comments

Comments

@bondsbw
Copy link

bondsbw commented Feb 25, 2015

Here is a typical C# property that wraps a backing field, such that the property includes a little bit of additional behavior:

private string _myField;
public string MyProperty
{
    get { return _myField; }
    set
    {
        _myField = value;
        NotifyOfPropertyChange(nameof(MyProperty));
    }
}

Whenever I set MyProperty from within that class, I set the property instead of the field so that I ensure the extra behavior runs. This is always the intended mechanism for writing this data.

Problem

During maintenance, developers make changes to the class and sometimes manipulate the field directly, instead of setting the property as the class designer intended. The result is that the additional behavior is not run, which leads to subtle bugs. It is a simple mistake, one I would like to prevent if possible.

Solution

One way to fix this situation is to be able to scope fields to their associated property. Within the property, the field can be fully manipulated as expected. Outside of the property, the field is invisible. Methods within that class are only able to interact with the property, which fully encapsulates the field.

Here is an example of what this might look like:

public string MyProperty
{
    string myField;
    get { return myField; }
    set
    {
        myField = value;
        NotifyOfPropertyChange(nameof(MyProperty));
    }
}

In this case, myField is fully encapsulated within MyProperty and nothing outside of MyProperty has access to myField directly.

Another option

Instead of explicitly declaring the field, perhaps a keyword provides access to an implicit field identified by a keyword, such as field. This could look like the following:

public string MyProperty
{
    get { return field; }
    set
    {
        field = value;
        NotifyOfPropertyChange(nameof(MyProperty));
    }
}

Additional Considerations

Fields should probably be initialized in a way that bypasses the property setter, such as the following example. The field should be initialized to null, but any future sets must have a non-null value:

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

Should constructors have special access to the backing field, to be able to set the field directly in the same manner? Dot notation cannot work here. Something different would be necessary, such as

public MyClass(string val)
{
    MyProperty@myField = val;
}

Can the property encapsulate multiple fields? This might be useful in some contexts. It would remove the second option that uses the field keyword.

@KalitaAlexey
Copy link

I think instead of

MyProperty@myField

must be used

MyProperty

but the compiler must ensures no setter method will be called. Just assignment to a field. Like in Swift. Because using of setter in constructor may lead to unexpected side effects

@bondsbw
Copy link
Author

bondsbw commented Mar 18, 2015

Agreed that calling setters in constructors are generally the non-preferred case. I'm just worried that using the MyProperty = someValue; syntax would be confusing, because in non-constructor code, that same syntax does something different.

@yume-chan
Copy link

I think constructors use MyProperty.myField is ok because now myField belongs to MyProperty.
EDIT: it's ambiguous between setting backing field of MyProperty and setting property of the object which `MyProperty returned.

An extra problem is a property must not define a field with same name appeared in the class, compiler should issue an exception indicating field already defined.
EDIT: If the compiler will rename this field in original class or create a mini-class to hold these fields, it's not necessary.

I disagree adding a new keyword, first it will limit one property has only one field, or why this field has a special name when others maybe also affects its return value (int MyProperty { get { return field1 + field2; } }), second adds another keyword will be a breaking change.

Besides I think it's also ok to set the initial value to a custom-setter property with C# 6 syntax (string MyProperty { set { backing_Field = value; } } = "Text";), I don't understand why there is such a limitation.

@bondsbw
Copy link
Author

bondsbw commented Mar 19, 2015

@CnSimonChan:

An extra problem is a property must not define a field with same name appeared in the class, compiler should issue an exception indicating field already defined.

You highlighted the main problem with the MyProperty.myField approach. But another problem is that it appears to be the same as property/field access on the instance returned by the call to MyProperty, which can cause further confusion.

@KalitaAlexey
Copy link

@bondsbw I think about confusion. What if write in constructor like this

MyProperty.initialValue = <someValue>;

This makes no confusion. Access to backed field must be only from property and from constructor. So name of backed field is extra.

@dsaf
Copy link

dsaf commented Mar 25, 2015

#1551 is a slightly different issue focusing on improving auto-properties rather than traditional properties.

@GeirGrusom
Copy link

Why do we need to be able to assign a value to the property scoped field from the constructor? Doesn't that violate the entire point of a property scoped field?

@KalitaAlexey
Copy link

@GeirGrusom Sometimes we want to initialize property with some value, but as said @bondsbw writing something like MyProperty = <someValue>; may confuse someone, because someone expect that in constructor setter will not be called (I prefer this variant) and someone else expect that in constructor setter will be called.
Syntax like MyProperty.initialValue will give up any confusion.

@GeirGrusom
Copy link

@KalitaAlexey @bondsbw
The backing field is, and should be, inaccessible to the constructor. If the constructor needs write access to the property this should be through the setter. If the constructor needs access to the backing field then the field shouldn't be property scoped, but rather be in a scope that is available to the constructor.

There is no need for syntax to except constructors from normal scoping rules. It isn't a useful feature, and it introduces typing constructs on properties. Syntax like MyProperty.initialValue will be ambiguous if the property already is a type (should it resolve to the type member, or should the field shadow it?). Syntax like MyProperty@value will add a new member access operator used to break scoping rules in the case of constructors which will introduce a lot of cruft in the parser, which I think is pointless

My opinion is: just let properties define variables inside their own scope. This feature alone is useful in my opinion. Don't overengineer it.

@KalitaAlexey
Copy link

@GeirGrusom I've thought about any useful case of setting field in constructor without setter and I failed. Indeed if I need change some variable without invoking setter, then my architecture is weak and wrong.

@bondsbw
Copy link
Author

bondsbw commented Mar 27, 2015

Good points. I agree that property scoped fields are worth it even without any direct backing field access from the constructor, whose use case is likely small.

If we did want to handle that case, however, another way to implement it is in reverse. Instead of the constructor having direct access to the backing field, the property can be told that it is being invoked from the constructor. Perhaps something related to CallerMemberNameAttribute can be used... but I don't have any specific ideas on the matter.

All in all, I'd rather have property scoped fields without the ability to distinguish constructor access from others, than to not have them at all.

@HaloFour
Copy link

This creates a new form of scope which seems like it would be inherently problematic:

public class Foo {
    private int _value;

    public int Value {
        private int _value;  // legal?
        get {
            int _value = this._value; // which _value?  legal?
            return _value;
        }
    }
}

As for consideration number 2, field is a legal identifier in C# so this would break existing code.

@bondsbw
Copy link
Author

bondsbw commented Dec 24, 2015

public class Foo {
    private int _value;
    public int Value {
        private int _value;  // legal?

Probably not. I can think of few arguments for it and many against.

As for consideration number 2, field is a legal identifier in C# so this would break existing code.

I'm not a huge fan of that one anyway. Explicit fields give you more power and don't require new keywords.

@alrz
Copy link
Member

alrz commented Dec 24, 2015

Some suggestions,

class C 
{
    public int P1
    {
        int field;
        var initialized = false; // (1) multiple, (2) inferred
        get { }
        set { }
    }
    public int P2
    {
        int field; // (3) same name (well, definitely)
        readonly cache = new Dictionary< ... >(); // (4) readonly, inferred
        static let static_local = ...; // (5) accessible to both getter and setter
        get { }
        set { }
    }
}

and (6) Access modifiers should not be permitted.

@bondsbw
Copy link
Author

bondsbw commented Dec 24, 2015

@alrz Did you leave out get/set bodies for brevity? Or is that some kind of implied body?

var, readonly, let, and static are probably fine, but I'm not necessarily interested in bloating this feature if doing so would delay it.

@paulomorgado
Copy link

This creates a new form of scope which seems like it would be inherently problematic:

public class Foo {
    private int _value;

    public int Value {
        private int _value;  // legal?
        get {
            int _value = this._value; // which _value?  legal?
            return _value;
        }
    }
}

As for consideration number 2, field is a legal identifier in C# so this would break existing code.

From a semantic point of view, I would equate the "local" field to a local variable or parameter.

So, value would be the "local" field and "this.value" would be the "global" field.

But then you couldn't have local variables or parameters with the same name.

@bondsbw
Copy link
Author

bondsbw commented Dec 26, 2015

If I could go back in time and if I had my way, I would remove the ability to create fields and local variables/parameters with the same name. I can't think of a single time it helped anything, and "this." is a code smell IMHO.

I certainly don't want to add to the confusion by allowing property-scoped fields to share names with class fields or local variables. I vote to restrict that capability.

@alrz
Copy link
Member

alrz commented Dec 26, 2015

@bondsbw (1) If you use same convention for parameters and fields e.g. camel case, then this would be helpful. The language should not restrict you on this matter. (2) That's actually a good property of blocks and roslyn itself used it in its codebase alot. In my example above, initialized or cache are just the right names for these fields, so why should I use prefixed names (I think this is known as smurf naming convention) for each property when I can figure it out by context and the block it is used in. That's what scopes are good at afterall, as you mentioned in the title

@lachbaer
Copy link
Contributor

@HaloFour Ah, I see. Never used that target before (only return, and assembly oc). Thanks!

@lachbaer
Copy link
Contributor

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, of course. Declaration of the field name could be either after the getter or setter. [This post is also in #8364]

@bondsbw
Copy link
Author

bondsbw commented Feb 20, 2016

@lachbaer That syntax doesn't seem intuitive, and if my interpretation is correct it would limit property scoping to a single field.

I'd prefer

public string FirstName {
    var _firstName = "";
    get => _firstName;
    set => _firstName = value ?? "";
}

or, without something like #7881,

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

@lachbaer
Copy link
Contributor

As you can see you have 3 times written '_firstName'. I want to find a way to avoid that for lightweight properties, as in cases above.

@paulomorgado
Copy link

@lachbaer, what if already have a _firstName defined?

@bondsbw
Copy link
Author

bondsbw commented Feb 21, 2016

@lachbaer I understand that you're looking to make it more concise, which is a goal I prefer as well. But I don't prioritize conciseness over familiarity and consistency with the rest of the language.

If there is only going to be one field allowed, then it should probably just be a keyword. Then you could possibly do something like

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

where get; performs like the auto-property get;, returning the value of field. The term set; would be shorthand for set => field = value;, in case you need custom getter logic. Combining those would give you today's auto-property initializer syntax:

public string FirstName { get; set; } = "";

Another shorthand possibility would be like what you mentioned in #8364, where the setter can be written to return an expression which sets the backing field:

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

That just does the same as the first example, but honestly I'm not sure how much real-world use that has other than the null coalescing in this example. Property change notifications, which are a big reason people write setters, need to occur after the setting of the backing field since synchronous event handlers would otherwise read the wrong (old) value of the property.

Anyway, the use of a single unnamed backing field goes beyond the scope of this issue, and perhaps should be discussed elsewhere. It isn't necessarily incompatible with property-scoped fields, and I can see the value when it is applicable.

@lachbaer
Copy link
Contributor

@paulomorgado No problem. If this proposal comes through the name will be property scoped, otherwise class scoped.

@bondsbw The following I have alread proposed in #8364

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

Where field is like value a compiler supplied parameter. Just the initialization cannot be done (custom setters cannot be initialized anyway). I only put the get _firstName = ""; in this proposal, because it might impact this implementation. For all other discussion about "short porperty notation", or "semi-auto-properties", as I call them, please go to #8364 (where I head over now to share some thoughts ;-)

@lachbaer
Copy link
Contributor

lachbaer commented Jul 7, 2016

An alternative might be #12361. One block indention more, but more flexible.

@lachbaer
Copy link
Contributor

lachbaer commented Jul 9, 2016

In terms of #12361 a property would look like

local uint _age;
public uint Age {
  get {
    local _age;
    return _age;
  }
  set {
    local _age;
    _age = value;
  }
}

Not very elegant. So in terms of this proposal #850 it could be made:

local uint _age;
public uint Age {
  local _age;
  get => _age: // #7881 syntax
  set => _age = value;
}

It is just one line of code more, but more consistent if contemplating on #12361 (wich goes a bit farther).

@alrz
Copy link
Member

alrz commented Sep 5, 2016

Assuming that property blocks maintain their own scopes I think the following will be very common,

string P1
{
  string field;
  get => field;
  set => Set(ref field, value);
}
string P2
{
  string field;
  get => field;
  set => Set(ref field, value);
}

Which certainly is not dry. This is the main use case discussed in the OP where we just need a single field per property.

I think something similar to #7614 or #8364 is a more elegant way of doing this.

string P1
{
  get; 
  set => Set(ref field, value);
}

field will be a contextual keyword that refers to the corresponding field. Since currently get; is not permitted with a setter, there wouldn't be any backward compatibility issue, i.e. field is a keyword in the setter body only when you have get;.

Having said that, I think this would be superior to property-scoped fields because properties rarely require more than one backing field.

EDIT: However, it will come in handy when you don't have a setter.

public object Property => ComputeValue();

public replace object Property
{
  object field;
  get => field ?? (field = original);
}

@bondsbw
Copy link
Author

bondsbw commented Sep 5, 2016

@alrz I don't think it has to be an either-or scenario. I mentioned the field keyword in the OP, but I do like the idea of being able to condense get => field; to simply get;, I think that would be useful.

@pebezo
Copy link

pebezo commented Sep 23, 2016

I love the field keyword. 90+% of the time when I'm introducing a private field is to work around this -- and it always feels dirty. I feel it should be auto-generated (like value) and for more complicated cases we can always fallback to the current "private field" declaration(s). I like @bondsbw proposal the best. A few examples where I'd find it useful:

public string Phone { get => field.Formatted(); set => value.OnlyNumbers(); }
public string Lazy { get => field ?? (field = ReadValue()); }
public string Lazy => field ?? (field = ReadValue());
public int Counter { get; set => field + value; }
public string Log { get { Foo(field); return field; } set { Bar(value); return field = value; }
public string IfDifferent { get; set { if (field != value) { Foo(); } return field = value; }

@lachbaer
Copy link
Contributor

There are some issues with an ever existing field keyword.

  1. Should the compiler always create a backing field, for every property? => No
  2. It could break old code if someone used field as an identifier
  3. Checking all this during analysing time, if there should be a field or not is too heavy.

Though, I absolutely like the idea of not having a seperate backing field.

For the sake of this topic (#850) we should continue the discussion about field on topic #8364.

For this proposal, I think the 'cleanest' solution really is this:

class C {
    public string Prop {
        // property local declarations must come first, like locals in methods (no 'hoisting')
        int _field;    // no access modifiers allowed
        get => _field.ToString();
        set => Int32.TryParse(value, out _field);
    }
}

This would allow for many, many real world scenarios without too much overhead.

Internally this could be nicely converted to:

class C {
    private int <Prop>k__Field__field; // <{PropertyName}>k__Field_{identifier}
    public string Prop {
        get {
                ref int _field = ref this.<Prop>k__Field__field;
                {
                    return _field.ToString();
                }
        }
        set {
                ref int _field = ref this.<Prop>k__Field__field;
                {
                    Int32.TryParse(value, out _field);
                }
        }
    }
}

@paulomorgado
Copy link

Why the ref?

What's wrong with just:

class C
{
    private int <Prop>k__Field__field;
    public string Prop
    {
        get
        {
            return <Prop>k__Field__field.ToString();
        }
        set
        {
            int.TryParse(value, out <Prop>k__Field__field);
        }
    }
}

?

@lachbaer
Copy link
Contributor

@paulomorgado
The field identifier must be available to the user code. It's definition is just a SynthesizedLocal (with name) that does appear in the user code. I think that it will be lowered by the compiler to what you wrote down.

@paulomorgado
Copy link

paulomorgado commented Dec 19, 2016

@lachbaer
Other than explicitly declaring the backing field and using it on the getter and setter, what's the difference to auto-properties?

Auto-properties directly access the backing field without the need for a local ref variable.

@lachbaer
Copy link
Contributor

@paulomorgado
I think your questions rather concern #8364 Semi-auto properties. And as you can see there, they are "semi" 😉

I think we should leave this issue/proposal with the intend of introducing property local fields. They don't conflict with #8364.

@lachbaer
Copy link
Contributor

lachbaer commented Apr 2, 2017

@CyrusNajmabadi
Have you already started on this feature? I would like to commence with development of my proposal of semi-auto-properties, that introduces a field virtual local for the accessors:

string Name {
    ref string field = ref <Name>k__backingField; // as virtual local
    get => field;
    set => field = value;
}

I think that it would be best to have an additional PropertyScopeBinder where this could be placed?

@jcouv
Copy link
Member

jcouv commented May 27, 2017

Issue moved to dotnet/csharplang #626 via ZenHub

@jcouv jcouv closed this as completed May 27, 2017
@jcouv
Copy link
Member

jcouv commented May 27, 2017

Moved the discussion to csharplang repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests