dotnet / csharplang

The official repo for the design of the C# programming language
11.61k stars 1.03k forks source link

Proposal: Property-Scoped Fields #133

Closed bondsbw closed 1 week ago

bondsbw commented 7 years ago

Property-Scoped Fields

(Ported from dotnet/roslyn#850)

Summary

Allow fields to be scoped within the accessor body of a property or event declaration.

Motivation

Properties often encapsulate constraints on field access or ensure that certain behaviors are invoked. Sometimes it is appropriate for all field access to be performed through the property, including those from within the same class.

These constraints may be unclear or forgotten, leading to subtle bugs during maintenance which are often difficult to diagnose.

Scoping fields to the body of their property/event would allow the designer to more effectively communicate this intent to encapsulate.

Detailed design

Property-scoped fields may be defined above the accessor list within the body of a property or event:

public string MyProperty
{
    string myField;

    get { return myField; }
    set
    {
        myField = value;
        NotifyOfPropertyChange(nameof(MyProperty));
    }
}
public event EventHandler MyHandler
{
    EventHandler handler;

    add
    {
        handler += value;
        Logger.Log("Added handler");
    } 
    remove
    {
        handler -= value;
        Logger.Log("Removed handler");
    }
}

myField and handler are encapsulated within their respective property/event. In each case, both accessors may directly reference the fields. Nothing outside of the property/event scope has direct access to those fields.

Implementation comments from @CyrusNajmabadi:

First, this is just state that's available to this single property that the value is scoped to. In that regard, it's similar to a 'local'. Just a 'local' that is available to all the accessors. As such, this is how i've been designing things from teh language level:

  1. You can have any number of property locals declared with the property.
  2. The property locals can come before/after/interspersed with the actual property accessors.
  3. The property locals are scoped to that property alone. Other members of the class/struct cannot interact with them.
  4. Subclasses can't interact with them (including overrides of the property).
  5. 'var' can be used for these property locals.
  6. Indexers should also be able to use these sorts of property locals.

Now (while this is out of the scope of the actual language change), here's how i would expect to emit them.

  1. These would simply be fields within the same class/struct that the property/indexer is contained within.
  2. The fields would be private.
  3. The fields would be named in such a way that they could never collide with an actual class member. likely something wonky <>_propname_localname

Alternatives

Automatic backing field

This implementation would be an extension of automatic properties. The backing field could be accessed through a keyword field, but only within the scope of the property/event:

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

This would not allow multiple fields to be encapsulated in the same property.

dotnet/roslyn#7614 has a similar proposal with a different keyword ($state).

Auto-property syntax for custom setter/getter logic

dotnet/roslyn#1551

Semi-auto-properties with setters

dotnet/roslyn#8364

Class-scoped blocks for fields / explicit field usage

dotnet/roslyn#12361

Unresolved questions

A list of questions from @CyrusNajmabadi:

  1. Allow attributes on these locals? It wouldn't be hard to support, and there could be value in putting attributes on the final fields emitted. This does leak through though the implementation of how we deal with property locals.

2. Allow these for events? Yes

  1. How are we going to handle the syntax changes here in our API. It's non trivial to modify the syntactic constructs here in a non-breaking fashion.
  2. We've added 'local functions' to C#7. Should we allow "property local functions" as well?
  3. Should we allow other 'local' constructs? We could have local methods/types/etc. within a property. Technically it would all be possible.

Syntax

It was noted that adding fields above accessors or below accessors (but not interspersed) could be an easier change to the existing compiler design. @lachbaer provided a potential design which would not be a breaking change to the API:

<!-- this Node hase been added, it looks exactly like "FieldDeclarationSyntax" -->
<Node Name="PropertyLocalFieldDeclarationSyntax" Base="BaseFieldDeclarationSyntax">
    <Kind Name="PropertyLocalFieldDeclaration"/>
    <Field Name="AttributeLists" Type="SyntaxList&lt;AttributeListSyntax&gt;" Override="true">
      <PropertyComment>
        <summary>Gets the attribute declaration list.</summary>
      </PropertyComment>
    </Field>
    <Field Name="Modifiers" Type="SyntaxList&lt;SyntaxToken&gt;" Override="true">
      <PropertyComment>
        <summary>Gets the modifier list.</summary>
      </PropertyComment>
    </Field>
    <Field Name="Declaration" Type="VariableDeclarationSyntax" Override="true"/>
    <Field Name="SemicolonToken" Type="SyntaxToken" Override="true">
      <Kind Name="SemicolonToken"/>
    </Field>
  </Node>

  <Node Name="AccessorListSyntax" Base="CSharpSyntaxNode">
    <Kind Name="AccessorList"/>
    <Field Name="OpenBraceToken" Type="SyntaxToken">
      <Kind Name="OpenBraceToken"/>
    </Field>
    <!-- next line has been added -->
    <Field Name="LocalFields" Type="SyntaxList&lt;PropertyLocalFieldDeclarationSyntax&gt;" Optional="true" />
    <Field Name="Accessors" Type="SyntaxList&lt;AccessorDeclarationSyntax&gt;"/>
    <Field Name="CloseBraceToken" Type="SyntaxToken">
      <Kind Name="CloseBraceToken"/>
    </Field>
  </Node>

Field name collisions

Should a property-scoped field be allowed to have the same name as another class member?

Should local identifiers within accessors be allowed to have the same name as a property-scoped field?

Design Meetings

TahirAhmadov commented 3 years ago

@jnm2 Actually, the problem with your solution is this:

bool _active, _dead;
public event EventHandler Foo
{
  EventHandler fooActive, fooInactive;
  add { if(!this._dead) { if(this._active) fooActive += value; else fooInactive += value; } }
  remove { if(!this._dead) { if(this._active) fooActive -= value; else fooInactive -= value; } }
}

What would this.Foo(this, EventArgs.Empty) raise in this case?

HaloFour commented 3 years ago

I'd rather a raise accessor than a get accessor. A get accessor would leak the details of the underlying publisher, which might not even be a delegate of the type of the event. A raise accessor would hide that detail behind a method of a signature matching that delegate.

That assumes that scoped fields make sense in the case of events, and I'm not really sure that they do. It's extremely niche to explicitly implement an event as it is.

CharlesJenkins commented 1 year ago

Is there a way to vote to advance this proposition? So often I have felt guilty about creating private fields that should never be manipulated outside of Property accessors. I know it's wrong to put them in scope to trip up a maintainer, but the language currently gives us no alternative!

CyrusNajmabadi commented 1 year ago

@CharlesJenkins Yes. Vote here:

image

CharlesJenkins commented 1 year ago

@CharlesJenkins Yes. Vote here:

image

Thanks! I hope I got the right one.