csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Proposal: `field` keyword in properties

Open lachbaer opened this issue 8 years ago • 397 comments

Proposal: field keyword in properties

(Ported from dotnet/roslyn#8364)

Specification: https://github.com/dotnet/csharplang/blob/main/proposals/field-keyword.md

LDM history:

  • https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-03-10.md#field-keyword
  • https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-04-14.md#field-keyword
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-01-12.md#open-questions-for-field
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-02-16.md#open-questions-in-field
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-03-02.md#open-questions-in-field
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-03-21.md#open-question-in-semi-auto-properties
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-05-02.md#field-questions
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-26.md#ungrouped
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-10-12.md#keywordness-of-field
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-05-15.md#field-and-value-as-contextual-keywords
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-06-24.md#field-questions
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-07-15.md#field-keyword
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-08-14.md#field-keyword
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-08-21.md#field-keyword-nullability
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-08-26.md#field-keyword-open-questions
  • https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-09-18.md#nullability-in-field

lachbaer avatar Feb 17 '17 21:02 lachbaer

This could also be useful/part of the implementation for https://github.com/dotnet/csharplang/issues/133.

scottdorman avatar Feb 18 '17 01:02 scottdorman

My opinion is that this proposal makes auto-implemented properties more difficult to read by introducing these silent backing fields despite the use of logic. With expression-bodied accessor members a chunk of the boilerplate for standard properties already disappears. The IDE eliminates much of the rest via standard code snippets.

HaloFour avatar Feb 20 '17 23:02 HaloFour

Suggestion:

  • only allow setters to be defined in auto-properties (making them semi-auto)
  • this setter expects the value to be assigned as a return value
  • to set it in the middle of the block, yield return is used.
int ASemiAutoProp
{
    get;
    set
    {
        var pcea = new PropertyChangeEventArgs(this.ASemiAutoProp, value));
        OnPropertyChanging( pcea );
        yield return value;
        OnPropertyChanged( pcea );
    }
}

Then there is no need for a new field keyword/variable.

And the following really looks better than with #133

public T PropertyInitialized
{
    get;
    set => value ?? new ArgumentNullException();
} = default(T);

instead of (with #133) - a bit much of backingField.

public T PropertyInitialized
{
    T backingField = default(T);
    get => backingField;
    set => backingField = value ?? new ArgumentNullException();
}

lachbaer avatar Feb 21 '17 12:02 lachbaer

Instead of using a field keyword, I also suggest following syntax borrowed from C struct:

public string FirstName
{
    get;
    set => _name = value;
} _name;

_name is by definition private to it's type (maybe unless directly prepended by an access modifier?). OR just scoped to the corresponding property, what would even be better.

In case expression-bodied set of semi-auto-properties expect a value being returnd to assign to the backing-field (here with additional initializer):

public string FirstName
{
    get;
    set => value;
} _name = "(not set)";

This completely eradicates the need for a field keyword or yield return as suggested above, because now the backing field's identifier can be used

public string FirstName
{
    get;
    set {
        var pcea = new PropertyChangeEventArgs(_name, value));
        OnPropertyChanging( pcea );
        _name = value;
        OnPropertyChanged( pcea );
    }
} _name = "(not set)";

This would also go hand in hand with #133.

lachbaer avatar Mar 07 '17 21:03 lachbaer

@lachbaer

With the declaration of the field being outside of the property block that would imply that the scope of that field would be for the entire class, not for just that property. That's also consistent with the scoping of that C syntax.

HaloFour avatar Mar 07 '17 21:03 HaloFour

@HaloFour Sure, but in this case we could probably rely on IntelliSense not to spoil the rest of the module. ~~Also without any prepending modifier it would imply internal access.~~ (corrected, of course class members are private by default) 🙄 But nevertheless, I think that this is a neat way to achieve at least some of the goals. Also the costs of implementing it could be reasonable.

lachbaer avatar Mar 07 '17 22:03 lachbaer

Of course specifying a getter only works, too:

public string FirstName
{
    get {
        Debug.Assert( _name != null, "Getting name before setting it?");
        return _name;
    }
    set;
} _name = null;

or combined:

public string FirstName
{
    get {
        Debug.Assert( _name != null, "Getting name before setting it?");
        return _name;
    }
    set => value ?? throw new ArgumentNullException();
} _name;

In the latter example, just specifying the _name identifier qualifies for a semi-auto-property and therefore for set => value .

lachbaer avatar Mar 07 '17 22:03 lachbaer

@HaloFour

My opinion is that this proposal makes auto-implemented properties more difficult to read by introducing these silent backing fields despite the use of logic.

I don't disagree with your whole comment, but I wanted to point out that the difficulty reading is only if you're accustomed to thinking of logic and automatic backing fields as mutually exclusive. I don't think they should be. It's not hard to learn to look for a field keyword. It'll be right where you'll look if you're looking for the backing field anyway.

The reason I really like this proposal is not because it enables me to write less code, but because it allows me to scope a field to a property. A top source of bugs in the past has been inadequate control over direct field access and enforcing consistent property access. Refactoring into multiple types to add that desirable scope is quite often not worth it.

To that end, a field keyword makes perfect sense. I should not care or have to care what the backing field's name is. Ideally, it doesn't have one. This adds the convenience that renaming properties gets a lot easier.

Not infrequently I'm renaming constructs such as this, where Set handles INotifyPropertyChanged.PropertyChanged:

private bool someProperty;
public bool SomeProperty { get { return someProperty; } private set { Set(ref someProperty, value); } }

This kills two birds with one stone: 1) scope safety, 2) more DRY, less maintenence:

public bool SomeProperty { get; private set { Set(ref field, value); } }

jnm2 avatar Mar 07 '17 23:03 jnm2

/cc @CyrusNajmabadi

gafter avatar Mar 24 '17 03:03 gafter

Meanwhile some time went by. I'd like to state my opionion on the questions from the inital post.

Allow both accessors to be defined simultaneously?

Yes, definitely. A nice example is the sample at the end of this comment. A semi-auto-property with an implicit field should be constructed under either or both these circumstances:

  • both, getter and setter are declared, but either is get; or set; the other having a statement/block
  • an initializer is declared and the property is not an auto-property

Assing expression bodied setters? and Assign block body with return?

I'd like to have that, because simply it looks nice and would totally fit into how "assign-to"-return expressions look.

But introducing to much new behaviour and especially having the compiler and reading user to differentiate between return and non-returning bodies can be confusing. Therefore I'd go with "no" on this currently.

Prohibit field keyword if not semi-auto?

No, but it must not be highlighted by the IDE in that case, because it that context it is no keyword anymore. I think it is very unlikely that somebody converts a semi-auto-property to an ordinary property and simultaneously has a 'field' named field in scope.

If property-scoped fields become availble shall that feature be available for semi-auto-properties as well?

Yes, if any possible. SAPs allow both, getter and setter, to be defined. It would make sense to make no difference versus normal properties to restrict that feature.

lachbaer avatar Apr 27 '17 22:04 lachbaer

Taken from @quinmars at https://github.com/dotnet/csharplang/issues/681#issuecomment-308539756, this feature would allow devs to write a 1-liner to implement lazy initialization:

public T Foo => LazyInitializer.EnsureInitialized(ref field, InitializeFoo);

private T InitializeFoo() { ... }

jamesqo avatar Jun 14 '17 20:06 jamesqo

@jamesqo Except using this LazyInitializer method has a downside (unless the initializer method is static) because you'll be creating a delegate instance each time the property is accessed.

What you want to write is:

public T Foo => 
{
   if (field == null)
   {
        System.Threading.Interlocked.CompareExchange(ref field, InitializeFoo(), null);
   }

   return field;
};

And now its not really a one-liner.

mattwar avatar Jun 14 '17 23:06 mattwar

@mattwar Well they said they intend to cache point-free delegates eventually. Also what if people don't care about extra allocations because the initializer is doing something like network I/O which completely dwarfs allocating a few extra objects?

jamesqo avatar Jun 15 '17 00:06 jamesqo

public T Foo => LazyInitializer.EnsureInitialized(ref field, InitializeFoo);

However the field keyword should not be available to every property per se. It then should be decorated with a modifier, like e.g. public field T Foo ....

lachbaer avatar Jun 15 '17 10:06 lachbaer

@lachbaer why?

yaakov-h avatar Jun 15 '17 10:06 yaakov-h

@yaakov-h For two reasons. First, so that you can see from the signature whether a (hidden) backing field is produced by the property. Second, to help the compiler finding out whether an automatic backing field shall be produced or not.

lachbaer avatar Jun 15 '17 10:06 lachbaer

First reason... I can see the value on a getter-only autoproperty like above, but on a get/set one I see little point.

Second reason I don't think is necessary. If the property's syntax tree contains any reference to an undefined variable named field, then you know it needs to emit an automatic backing field.

yaakov-h avatar Jun 15 '17 10:06 yaakov-h

The alternative is to start thinking about properties a bit differently. Every property has an explicit (synthesized) backing field, unless it is opt'ed out, because it is not needed (no get; or set; and no field used). This little change in philosophy is even backwards compatible in (emmited) code.

lachbaer avatar Jun 15 '17 11:06 lachbaer

I think a field property modifier is unnecessary verbosity. The keyword should just be available for use the same place you look at to see the backing field anyway.

jnm2 avatar Jun 15 '17 11:06 jnm2

@jamesqo That delegate caching is for static delegates where they are not being cached in some circumstances today. It is unlikely that the InitializeFoo method is static.

mattwar avatar Jun 15 '17 17:06 mattwar

@mattwar Then we can simply open a corefx proposal to add EnsureInitialized overloads that accept state? e.g.

public class LazyInitializer
{
    public static T EnsureInitialized<T, TState>(ref T value, TState state, Func<TState, T> factory);
}

public T Foo => EnsureInitialized(ref field, this, self => self.InitializeFoo());

And again, the extra allocations may be dwarfed by the initialization logic in some cases.

jamesqo avatar Jun 16 '17 04:06 jamesqo

Instead of introducing a new 'field' keyword you could maybe use the _ (discard) symbol. This would avoid conflicts in old code.

class Employee {

    public string Name {
        get { return _; }
        set { _ = value?.ToUpper(); }
    }

    public decimal Salary {
        get;
        set { _ = value >= 0 ? value : throw new ArgumentOutOfRangeException(); }
    } = 100;
}

I think this could work.

sonnemaf avatar Jun 20 '17 10:06 sonnemaf

@sonnemaf _ = value; already is valid code (it evaluates the right hand expression and discards its result). Therefore it does not work.

lachbaer avatar Jun 20 '17 10:06 lachbaer

Still there are some more questions to answer.

  1. What happens when an [inherited] field with the name field exists?
  2. What if #133 arrives and a property-scoped variable named field is declared?
  3. What if field was a delegate and a member method named field is available?
  4. If a local named field is declared, what about highlighting that keyword within the accessor?

When thinking about it some time, you can come up with quite a long ruleset that must be followed. By always adding the modifier field to the property, this feature is switched on by demand and any occurance of the identifier field within the accessors references the backing-field, shadowing all other members with that name.

lachbaer avatar Jun 20 '17 10:06 lachbaer

@lachbaer: I'd expect an existing variable, field or property named field would take precedence, otherwise backwards compatibility dies. field as a keyword would have to be conditional on nothing else having that name.

yaakov-h avatar Jun 20 '17 11:06 yaakov-h

@yaakov-h It's not that easy. Just as one example imagine you have a class that inherits from a class that has a field member.

class A {
    protected int field;
    [...]
}

class B : A {
    public int Foo {
        get => field;
        set => field = value;
    }
}

Just by the looks of class B (without pure knowledge of A) you don't know what field refers to.

As a second example imagine that class B wasn't derived from class A in the first step and a refactoring made the programmer do so. Before Foo was a semi-auto-property, now no compiling error occurs, but A.field is referenced by the compiler.

As I already said, a mandatory field modifier eliminates all those cases that can break or at least complicate existing code. You can always target the member in a public field int Property by using this.field.

lachbaer avatar Jun 20 '17 12:06 lachbaer

@lachbaer In that case, perhaps the compiler could refuse to make the changes and tell the user (s)he has to rename A.field first.

jamesqo avatar Jun 20 '17 15:06 jamesqo

@lachbaer That is going to be such a rare scenario, it's a shame it has to make the common case worse for everyone.

jnm2 avatar Jun 20 '17 16:06 jnm2

tell the user (s)he has to rename A.field first.

And what if A was in another assembly altogether? 🙃

Joe4evr avatar Jun 20 '17 16:06 Joe4evr

@Joe4evr Then the user wouldn't be able to use this feature in B.

jamesqo avatar Jun 20 '17 16:06 jamesqo