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

[C# feature] Auto-property syntax for custom setter/getter logic. #1551

Closed
dsaf opened this issue Mar 24, 2015 · 16 comments
Closed

[C# feature] Auto-property syntax for custom setter/getter logic. #1551

dsaf opened this issue Mar 24, 2015 · 16 comments

Comments

@dsaf
Copy link

dsaf commented Mar 24, 2015

Problem: having to declare a backing field when adding custom logic to getter and/or setter of a property.

Current situation:

private string _name; //Only used by property.

public string Name
{
    get
    {
        Prepare(_name);
        return _name;
    }
    set
    {
        PreProcess(value);
        _name = value;
        PostProcess(value); //...or should we pass _name? Ambiguity for developers.
    }
}

Suggested change:

public string Name
{
    get
    {
        Prepare(value);
        return value;
    }
    set
    {
        PreProcess(value);
        apply value;
        PostProcess(value);
    }
}

Typical WPF scenario after change:

public string Name
{
    get;
    set
    {
        apply value;
        OnPropertyChanged(); //Uses [CallerMemberName] or something, haven't tried it.
    }
}

Crazy lambda version (separate issue?):

public string Name { get; set OnPropertyChanged; } //Set lambda (reduced to method group here) is always executed last and is always passed the value (is basically an Action<TValue>).

Questions/comments:

  1. Is this worth it, generally? Simpler non-AOP WPF/XAML ViewModel-s? Any other common scenarios?
  2. Is adding a new keyword 'apply' (opposite of 'return') worth it?
  3. Does the suggested change look good enough when used together with auto-property initializers?
  4. Maybe we simply need to keep fields close to properties on syntax level?
public string _name Name
{ ... }
  1. Other ideas in older discussion: https://roslyn.codeplex.com/discussions/550266
@KalitaAlexey
Copy link

I think #850 is better

@dsaf
Copy link
Author

dsaf commented Mar 25, 2015

@KalitaAlexey I disagree:

  1. The issue seems to be regarding specifically non-auto properties, especially given the comments like "I disagree adding a new keyword, first it will limit one property has only one field".

  2. Syntax options discussed there are quite strange and sound like creating mini-classes out of properties, e.g. "MyProperty.initialValue = ;". What's next - adding property-scoped methods and nested types?

  3. It's very subjective but for me the 'field' keyword is focusing on managing the field, while 'apply' is focusing on assigning the value to property, which is more preferable when reasoning about auto-properties.

@dsaf dsaf changed the title [C# feature] [Discussion] Auto-property syntax for custom setter/getter logic. [C# feature] Auto-property syntax for custom setter/getter logic. Mar 25, 2015
@KalitaAlexey
Copy link

@dsaf

  1. If you want to use several fields in property getters and setters, then write it explicit
public string FullName
{
    get
    {
        return lastName + " " + firstName;
    }
}
  1. I think syntax which I suggested is straight and not confusing

@dsaf
Copy link
Author

dsaf commented Mar 25, 2015

@KalitaAlexey Our opinions are getting subjective at this stage. I guess it's up to Roslyn team now to decide. The important thing is that we have pointed out a problem and proposed several different solutions.

@KalitaAlexey
Copy link

@dsaf It's too hard to decide, because there are many solutions.
One of them:

public string Name
{
    get;
    before set
    {
        if (field == value)
            return false;
    }
    set;
    after set
    {
        RaisePropertyChanged(nameof(Name));
    }
}

But problem of addition new keywords also is hard

@dsaf
Copy link
Author

dsaf commented Mar 25, 2015

@bondsbw
Copy link

bondsbw commented Mar 25, 2015

@dsaf Just to be fair, some of the syntax issues you describe about #850 relate to the comments but not to the actual proposal. Also, that proposal's focus is on encapsulating the backing field, which is not explicitly the goal of your issue.

That said, I appreciate what you are proposing. I feel it is an orthogonal concern, adding simple behaviors to automatic property accessors.

Two things I'm not clear on:

  1. How would your "Crazy lambda version" syntax distinguish between Prepare, PreProcess, and PostProcess items?
  2. Is there any way for PreProcess to prevent the set from occurring? For example, only allowing a value to be set once:
public Telephone Phone
{
    get { return _phone; }
    set
    {
        if (_phone == null)
        {
            _phone = value;
        }
    }
}

@dsaf
Copy link
Author

dsaf commented Mar 25, 2015

@bondsbw That would be more fair, agreed. There is an element of orthogonality, true. Not being "forced" to keep backing fields "close" to properties is something that bothers me as well - even from the simple code formatting point of view (to keep all fields at the top of the type or not?).

  1. "Crazy lambda version" would only allow a PostProcess item. This is very controversial, I know. Someone would need to assess the most typical use cases, to say if it's a reasonable simplification. I was coming from the INotifyPropertyChanged scenario.
  2. The only way I could think of is demanding that 'apply value' be a simple statement that has to be present somewhere on the "first level" of the setters body (similar to how returning a value is enforced for getters). Anything more complicated and using a backing field starts to sound more reasonable.

@momijin
Copy link

momijin commented Mar 25, 2015

@dsaf @KalitaAlexey

I also write one of them:

public T PropertyName
{
    get(Func<TCurrent, TResult> prepare);
    set(Func<TOld, TNew, bool> preProcess, Action<TOld, TNew> postProcess);
}

A set accessor is also able to taken as follows:

set(Func<TOld, TNew, bool> preProcess);
set(Action<TOld, TNew> postProcess);

example:

public string Name
{
    get(currentValue => currentValue ?? new string());
    set((oldValue, newValue) => oldValue != newValue, (oldValue, newValue) => OnPropertyChanged());
}

Above is expanded to by compiler:

private string _name;
public string Name
{
    get
    {
        Func<string, string> prepare = currentValue => currentValue ?? new string();

        _name = prepare(_name);
        return _name;
    }
    set
    {
        Func<string, string, bool> preProcess = (oldValue, newValue) => oldValue != newValue;
        Action<string, string> postProcess = (oldValue, newValue) => OnPropertyChanged();

        if (preProcess(_name, value))
        {
            string oldValue = _name;
            _name = value;
            postProcess(oldValue, _name);
        }
    }
}

@bondsbw
I think that Property-scoped fields #850 is a very nice idea, which makes it where fields should be. In simple case I use Auto-property syntax. But when it isn't so, I will use Property-scoped fields. I wish that C# 7 will support it.

@yume-chan
Copy link

  1. The apply keyword: I'm using auto-property so why I still need to set backing field by myself?

    I prefer @KalitaAlexey 's way so get; set; still indicates it's an auto-property and before/after get/set; can add custom logic to it.

  2. set OnPropertyChanged;: I think if it will be added to standard, it should provide a custom method to take control all the setting process, in another words it's just a short-hand for set { SetValue<TValue>(value, nameof(Property)); } (And get { return GetValue<TValue>(nameof(Property)); }) (So it's not an auto-property) and it's easier to understand. (Mentioned in the old discussion, but I think use space is better).

  3. @dsaf said:

    1. Syntax options discussed there are quite strange and sound like creating mini-classes out of properties, e.g. "MyProperty.initialValue = ;". What's next - adding property-scoped methods and nested types?

    I think it's like JavaScript, everything is an object so they can have their own fields, It adds more relevance between the property and fields affects it. And in C# we restrict only properties can have field so it won't cause more mass because we already have classes.

  4. Difference from Proposal: Property-scoped fields #850: We have custom property and auto-property, so we have this issue and Proposal: Property-scoped fields #850, from different starting point.

Problems

  1. If using before set;, can I edit field (the keyword references compiler generated backing field) or value to set fully custom value? OR when using apply keyword, can I apply a custom value?
  2. If we can fully control the logic, Maybe someone will create a custom property with auto generated backing field, is it ok or is it we wanted? (It might be most important).
  3. When using set SomeMethod; (mentioned above, not first post ver, SomeMethod will set value using some other way), what arguments should be passed into the method?

@KalitaAlexey
Copy link

  1. I think no. I mean "before set" must have one purpose - check availability of property assignment.
  2. I am not sure how hard to implement this, but I would prefer this "If backing field is used in getter or in setter, then generate backing field.". And I am not sure but I think backing field must be used in getter and in setter. I have no example of using backing field only in one of them.
  3. I looked at example @momijin and I still think way with "before/after set" is better. Cause lambda syntax cannot be debugged. I can place a breakpoint inside get. In lambda version it is impossible
class Person
{
public string Name
{
    get
    {
        return field ?? (field = new string());
    }
    before set
    {
        if (field == value)
            break;
    }
    set;
    after set
    {
        OnPropertyChanged();
    }
}
}

Also I think will be cool If I'll can write something like this, but I understand it very hard to implement and also it is new keywords, but this is really useful and simple to write and understand.

class Person
{
public string Name
{
    get
    {
        return field ?? (field = new string());
    }
    before set check not equal;
    set;
    after set notify;
}
}

@jbjoshi
Copy link

jbjoshi commented Mar 27, 2015

How about following syntax:

    // Standard property
    public string DefaultProperty { get; set; } using DefaultPropertyProvider<string>;

    // Not-nullable property
    public string NotNullableProperty { get; set; } using NotNullablePropertyProvider<string>;

    // INPC property
    public string InpcProperty { get; set; } using InpcPropertyProvider<string>;

Following is a very rough version of 'would be implementation':

System classes:

    // Property information related to current property
    public struct PropertyProviderContext<T> {
        // Object of type that the property belongs to
        public object Target;

        // Name of current property
        public string PropertyName;

        // Existing value of current property
        public T PropertyValue;
    }

    // A type thjat is used to 'mutate' property code generation
    public interface IPropertyProvider<T>
    {
        // takes the context, returns a value or throws exception
        T GetValue(PropertyProviderContext<T> context);

        // takes the context and new value, returns a value that should actually be set in property
        T SetValue(PropertyProviderContext<T> context, T newValue);
    }

Implementations:

    // Standard implementation
    public class DefaultPropertyProvider<T> : IPropertyProvider<T>
    {
        public T GetValue(PropertyProviderContext<T> context)
        {
            return context.PropertyValue;
        }

        public T SetValue(PropertyProviderContext<T> context, T newValue)
        {
            return newValue;
        }
    }

    // Prevent null values
    public class NotNullablePropertyProvider<T> : IPropertyProvider<T> where T : class
    {
        // If existing value is null, throw exception
        public T GetValue(PropertyProviderContext<T> context)
        {
            if (context.PropertyValue == null)
            {
                throw new InvalidOperationException();
            }

            return context.PropertyValue;
        }

        // If given value is null, throw exception
        public T SetValue(PropertyProviderContext<T> context, T newValue)
        {
            if (newValue == null)
            {
                throw new ArgumentNullException();
            }

            return newValue;
        }
    }

    // A contract that allows other types to invoke property changed event for this type
    public interface ISupportsInpcInvocation
    {
        void RaisePropertyChanged(string propertyName);
    }

    // A property provider that supports INotifyPropertyChanged behavior
    public class InpcPropertyProvider<T> : IPropertyProvider<T> where T : class
    {
        // Nothing special
        public T GetValue(PropertyProviderContext<T> context)
        {
            return context.PropertyValue;
        }

        // Raise property changed signal if values differ
        public T SetValue(PropertyProviderContext<T> context, T newValue)
        {
            if(context.PropertyValue != newValue)
            {
                (context.Target as ISupportsInpcInvocation).RaisePropertyChanged(context.PropertyName);                    
            }

            return newValue;
        }
    }

Something more refined version of this approach should help compiler what code to generate for each property.

Thoughts?

@dsaf
Copy link
Author

dsaf commented Mar 27, 2015

@jbjoshi don't be offended, but IMHO it looks like something from WPF or Java (think BooleanToVisibilityConverter and AbstractSingletonProxyFactoryBean) - Ruby guys call this "ceremony". I would rather use a backing field.

@jbjoshi
Copy link

jbjoshi commented Mar 27, 2015

@dsaf, not at all.

I AM influenced a lot by WPF.

I would rather have these kinds of 'extensions' for 'compiler' to use instead of the compiler deciding on it's own on what code to generate.

Similar to 'type providers' concept in F#, compiler uses our classes to make decisions.

EDIT:

By the way, there is nothing in here that 'assembly rewriter' cannot do. It's just about having language features extensible out of the box. More like AOP out of the box.

@momijin
Copy link

momijin commented Mar 27, 2015

@CnSimonChan

4.Difference from #850: We have custom property and auto-property, so we have this issue and #850, from different starting point.

I agree to that, too. In this issue, 1) apply keyword, 2) before/after set and field keyword, 3) lambda in get/set are appeared. Those made me excited and the suggestion of @CnSimonChan and @KalitaAlexey are very interesting to me. I think that this issue should be useful for future.

@gafter
Copy link
Member

gafter commented Mar 20, 2017

We are now taking language feature discussion on https://github.com/dotnet/csharplang for C# specific issues, https://github.com/dotnet/vblang for VB-specific features, and https://github.com/dotnet/csharplang for features that affect both languages.

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

8 participants