Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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: field keyword in properties #140

Closed
lachbaer opened this issue Feb 17, 2017 · 425 comments
Closed

Proposal: field keyword in properties #140

lachbaer opened this issue Feb 17, 2017 · 425 comments
Assignees
Labels
Feature Request Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion
Milestone

Comments

@lachbaer
Copy link
Contributor

lachbaer commented Feb 17, 2017

Proposal: field keyword in properties

(Ported from dotnet/roslyn#8364)

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

LDM history:

@scottdorman
Copy link
Contributor

This could also be useful/part of the implementation for #133.

@HaloFour
Copy link
Contributor

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.

@lachbaer
Copy link
Contributor Author

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
Copy link
Contributor Author

lachbaer commented Mar 7, 2017

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.

@HaloFour
Copy link
Contributor

HaloFour commented Mar 7, 2017

@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.

@lachbaer
Copy link
Contributor Author

lachbaer commented Mar 7, 2017

@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
Copy link
Contributor Author

lachbaer commented Mar 7, 2017

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 .

@jnm2
Copy link
Contributor

jnm2 commented Mar 7, 2017

@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); } }

@gafter
Copy link
Member

gafter commented Mar 24, 2017

/cc @CyrusNajmabadi

@lachbaer
Copy link
Contributor Author

lachbaer commented Apr 27, 2017

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.

@jamesqo
Copy link
Contributor

jamesqo commented Jun 14, 2017

Taken from @quinmars at #681 (comment), 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() { ... }

@mattwar
Copy link
Contributor

mattwar commented Jun 14, 2017

@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.

@jamesqo
Copy link
Contributor

jamesqo commented Jun 15, 2017

@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?

@lachbaer
Copy link
Contributor Author

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 ....

@yaakov-h
Copy link
Member

@lachbaer why?

@lachbaer
Copy link
Contributor Author

@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.

@yaakov-h
Copy link
Member

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.

@lachbaer
Copy link
Contributor Author

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.

@jnm2
Copy link
Contributor

jnm2 commented Jun 15, 2017

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.

@mattwar
Copy link
Contributor

mattwar commented Jun 15, 2017

@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.

@jamesqo
Copy link
Contributor

jamesqo commented Jun 16, 2017

@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.

@sonnemaf
Copy link

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.

@lachbaer
Copy link
Contributor Author

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

@lachbaer
Copy link
Contributor Author

Still there are some more questions to answer.

  1. What happens when an [inherited] field with the name field exists?
  2. What if Proposal: Property-Scoped Fields #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.

@yaakov-h
Copy link
Member

@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.

@jnm2
Copy link
Contributor

jnm2 commented Nov 12, 2024

The .NET SDK 9.0 can now be downloaded, and it supports building with field at the command line with dotnet build under <LangVersion>preview</LangVersion>!

@chucker
Copy link

chucker commented Nov 12, 2024

@WeihanLi,

Get an error when using the field in the interpolated string

Thanks for reporting this issue. This should be addressed in dotnet/roslyn#75566.

Does the 17.13 P1 milestone effectively mean .NET SDK 9.0.200? Or will it be back ported to 9.0.1xx?

@WeihanLi
Copy link
Contributor

Get an error when using the field in the interpolated string

Still get error when building with .NET 9 SDK, any plan for the fix? and which nuget package version could be expected to include the fix?

@umlx5h
Copy link

umlx5h commented Nov 13, 2024

I installed latest Visual Studio 17.12.0 to try .NET9

I can compile properties with field with preview, but not with 13.0 or unspecified.

# OK
<LangVersion>preview</LangVersion>.
# NG
<LangVersion>13.0</LangVersion> 
or unspecified

Will this be available in C#14? or in C#13?

@WeihanLi
Copy link
Contributor

I can compile properties with field with preview, but not with 13.0 or unspecified.
Will this be available in C#14? or in C#13?

This would be GA in C# 14, preview feature for now

@UniqeId6542
Copy link

    private string? foo = null;
    public string Foo
    {
        get
        {
            if (this.foo is null)
            {
                FieldInfo field = typeof(String).GetField(nameof(string.Length), BindingFlags.Public | BindingFlags.Instance)!;
                this.foo = field.GetValue("hi");
            }

            return this.foo;
        }
    }

The "FieldInfo field" does not have an error. (bad)
The "field.GetValue("hi")" does have an error. (good)

I would expect an error on the declaration of a local variable named 'field'.
(tested with VS 17.12.0 with language preview enabled.)

@mikernet
Copy link

mikernet commented Nov 13, 2024

@UniqeId6542 FieldInfo.GetValue() returns object not string, so you can't assign it to foo. You would need to do .ToString() on it.

@UniqeId6542
Copy link

@UniqeId6542 FieldInfo.GetValue() returns object not string, so you can't assign it to foo.

Yes. But that does NOT produce the error. The NEXT line produces the error - try it yourself.
The first line SHOULD produce an error, but it does not.

@chucker
Copy link

chucker commented Nov 13, 2024

@UniqeId6542 but the warning on field tells you what to do:

In language version preview, the 'field' keyword binds to a synthesized backing field for the property. To avoid generating a synthesized backing field, and to refer to the existing member, use 'this.field' or '@field' instead.

If I change the line to:

this.foo = @field.GetValue("hi");

Then the error, as expected, is:

Cannot implicitly convert type 'object' to 'string'. An explicit conversion exists (are you missing a cast?)

@UniqeId6542

This comment has been minimized.

@UniqeId6542
Copy link

@UniqeId6542 but the warning on field tells you what to do:

In language version preview, the 'field' keyword binds to a synthesized backing field for the property. To avoid generating a synthesized backing field, and to refer to the existing member, use 'this.field' or '@field' instead.

If I change the line to:

this.foo = @field.GetValue("hi");
Then the error, as expected, is:

Cannot implicitly convert type 'object' to 'string'. An explicit conversion exists (are you missing a cast?)

Consider:

String field = "hello"; // no error
String this = "world"; // error

I can fix the error on the second line by changing this to @this.
I expect the first line to work the same way.

@jnm2
Copy link
Contributor

jnm2 commented Nov 13, 2024

@UniqeId6542 Actually, many keywords can be used as identifier names. It's only no -contextual keywords that can't. field is a contextual keyword.

@HaloFour
Copy link
Contributor

I can fix the error on the second line by changing this to @this.
I expect the first line to work the same way.

This was explored and found to have way too much of an impact to existing code, so the team decided to take a step back and treat it as a contextual keyword instead that is treated as a keyword only when an identifier of the same name is not in scope. This is how C# has approached adding most new identifiers, to reduce breaking changes, and while the team was open to taking a harder stance here the data showed that it would not be a good idea.

@CyrusNajmabadi
Copy link
Member

@UniqeId6542 please file issue at dotnet/roslyn. This issue is for discussion around the design of this feature.

@UniqeId6542
Copy link

@jnm2 @UniqeId6542 Actually, many keywords can be used as identifier names. It's only no -contextual keywords that can't. field is a contextual keyword.

Can you name one contextual keyword where declaring it does not require the '@' prefix but using it does require the '@' prefix?
I went through the list in a quick basic attempt, but found nothing.
(Also, give a code example.)

@HaloFour This was explored and found to have way too much of an impact to existing code, so the team decided to take a step back and treat it as a contextual keyword instead that is treated as a keyword only when an identifier of the same name is not in scope. This is how C# has approached adding most new identifiers, to reduce breaking changes, and while the team was open to taking a harder stance here the data showed that it would not be a good idea.

Perhaps it was considered. But I disagree with the decision. Since using a local variable named 'field' requires the '@' prefix, I don't see how it is more of a breaking change to also require the declaration to use the '@' prefix. (I don't expect people to declare a local variable but not use it.)

@CyrusNajmabadi
Copy link
Member

I don't see how it is more of a breaking change to also require the declaration to use the '@' prefix

You can add taht restriction if you want in your own code through the use of custom analyzer. The language doesn't require it, so we didn't add such a restriction.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 13, 2024

Can you name one contextual keyword where declaring it does not require the '@' prefix but using it does require the '@' prefix?

Yes. Here's an example with from. But there are numerous others:

Image

        // Declared without @
        int from = 0;
        var v = from x in numbers
                // Requires @ here
                where x == @from
                select x.ToString();

@CyrusNajmabadi
Copy link
Member

I don't expect people to declare a local variable but not use it

You can declare it. We don't require @ in the declaration as there's no ambiguity as to what it means. Similar to the int from case above. It's completely unambiguous as to what it means. However, at the consumption point, you need to use @ as we need a way to know if you're referencing a variable or starting a new query construct.

The same is true for field. At the consumption point, we need to know if this is the special built-in auto-property backing field reference. Or if it's referencing one of our own variables named "field".

@jnm2
Copy link
Contributor

jnm2 commented Nov 13, 2024

I don't see how it is more of a breaking change to also require the declaration to use the '@' prefix. (I don't expect people to declare a local variable but not use it.)

The data showed it was tremendously more breaking. See https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-07-15.md#field-keyword for the meeting decision on this.

@jnm2
Copy link
Contributor

jnm2 commented Nov 14, 2024

@UniqeId6542 That's what we tried first, but GitHub data showed it broke too much code, so we had to dial it down.

@colejohnson66
Copy link

A bit of a tangent, but has the team considered creating auto-fixers like cargo fix for Rust?

@DanFTRX
Copy link

DanFTRX commented Nov 14, 2024

@UniqeId6542 That's what we tried first, but GitHub data showed it broke too much code, so we had to dial it down.

I believe you, but I am curious what cases are people declaring a local variable "field" in a property body then not accessing it? Or were people already using a @ in uses of the field but not the declaration?

@Boomkop3
Copy link

Perhaps I'm not understanding the idea wrong, but it looks like there's more than just a backing field being synthesized.

Image

@CyrusNajmabadi
Copy link
Member

@Boomkop3 what else do you see synthesized there? All i see is a synthesized backing List<string> field created there.

@Boomkop3
Copy link

@CyrusNajmabadi I am seeing =null!; but I didn't write set; or set => field = value;.

@CleanCodeX
Copy link

the property initalizer " = null!" is not part of the setter, it will be set as the initial value of the backing field.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 14, 2024

@CyrusNajmabadi I am seeing =null!; but I didn't write set; or set => field = value;.

Yes. That assigns to the backing field (just like a normal auto prop today). For example:

Image

That didn't change with field.

@Boomkop3
Copy link

@CyrusNajmabadi ah, you're right, that's just for initialization. It's quite obvious now that I look at it again. Thank you!

@dotnet dotnet locked and limited conversation to collaborators Nov 19, 2024
@CyrusNajmabadi CyrusNajmabadi converted this issue into discussion #8634 Nov 19, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
Feature Request Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion
Projects
None yet
Development

No branches or pull requests