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: Automatic fields and properties setting from the constructor #14853

Closed
sirgru opened this issue Nov 1, 2016 · 29 comments
Closed

Proposal: Automatic fields and properties setting from the constructor #14853

sirgru opened this issue Nov 1, 2016 · 29 comments

Comments

@sirgru
Copy link

sirgru commented Nov 1, 2016

Currently there is a bit of of excess boilerplate around setting fields and properties passed through the constructor.
Just for example:

public class Address {
    public string country { get; private set; }
    public string city { get; private set; }
    public string state { get; private set; }

    public Address(string country, string city, string state) {
        this.country = country;
        this.city = city;
        this.state = state;
    }

  // Some other behaviors.
}

There seems to be a lot of repetition around the 'this' and variable name repeats.
There have also been some solutions for this:
#11909 - Parameter properties like in TypeScript.
The drawbacks I see with this:

  1. The complications with multiple constructors. It can be redundant and awkward to potentially re-specify the same variable with multiple constructors.
  2. Class fields and properties are not clearly separated from the constructor parameters. When looking around the codebase, it can be unexpected to find the variable declaration inside the constructor parameter. This is apparently "more plausible" with the new C# 7 style but still I think most users would find it objectionable and a bad style.
  3. It forbids the different naming conventions for private variables. In some naming conventions private variables start with _, but there are some starting with m_ and there are others out there. In case of assignment of a private variable, this convention is either neglected or if exposed can subtly "give away the internals".
  4. What happens with attributes? What happens with fields with multiple modifiers, e.g. private static readonly string _myString?
    My opinion: no good.

So, I would like to propose another solution to this - just an idea. Let's say it would be allowed to write this code:

public class Address {
    public string country { get; private set; }
    public string city { get; private set; }
    public string state { get; private set; }

    public Address(set string country, set string city, set string state) {    }

  // Some other behaviors.
}

The use of contextual keyword set is allowed only in the constructor of a respective type and means a local field or property is assigned to the passed value. It must appear before the type of the c-tor parameter. The assignment is done as a first operation (before entering the constructor block) and is a syntactic shortcut for this.value = value (see variations below).
If the constructor calls other constructors via :base() or :this(), then the assignment operation is done after those calls.
One or more constructors of a type can contain the setting without conflicts.
The variations proposed below have to do with how the target field / property is chosen.

Variation 1: Strict

The field or property to be set must have the same name as the parameter of the c-tor. If the set keyword is used and the field or property with the same name does not exist, compile time error is thrown.

Variation 2: Loose

The field or property to be set must have the same name as the parameter of the c-tor only if it is public. If it is private or protected, it must end with the name of the variable. Names are compared case-insensitive. If there is any ambiguity around which variable is the target of the set, compile time error is thrown.

Variation 3: More Loose

The field or property to be set must end with the name of the parameter. Names are compared case-insensitive. If there is any ambiguity around which variable is the target of the set, compile time error is thrown.

Upsides relative to above mentioned downsides:

  1. It works correctly with multiple c-tors.
  2. Class fields and properties are defined as before.
  3. The naming convention is influenced depending on the variation decided on from above.
  4. No influence on attributes or other modifiers.
    Additionally:
  5. Less boilerplate than usual.
  6. Easy to understand.
  7. Reuse of existing keyword.
  8. Helps in cases where Records are not a good conceptual fit, for example in cases where you want non-read-only properties, fields, or private variables set;

Addition 1:

This syntax could be compatible even with non-primary constructors of records. For example, in the code:

public class Person(string Name, DateTime DateOfBirth) {
    private DateTime _recordCreationDate;
    public Person(string Name, DateTime DateOfBirth, set DateTime recordCreationDate) 
               : this(Name, DateOfBirth) {}
}

Addition 2:

This syntax could potentially be used in any method, not just the constructor to avoid the setting of variable with the same or "similar" name.

Addition 3:

If the compiler knows the exact variable that's going to be set, then it also knows its type, so repeating its type in the c-tor becomes superfluous. The type can now be shorthanded using var, but only if there is set before it:

public MailingLabelService(ISomething someVariable, ISomething2 someVariable2, 
set var PersonRepository, set var PrintService) 
: base(someVariable, someVariable2, PersonRepository) {
    // do something with someVariable
}

Alternative:

The keyword set is not required and if the type is not provided the "local variable inference" proceeds as before.

public class Address {
    public string country { get; private set; }
    public string city { get; private set; }
    public string state { get; private set; }

    public Address(Country, City, State) {    }
    // Some other behaviors.
}
@DavidArno
Copy link

This sounds like various ideas already covered by the plans for record types?

Also, it's worth remembering that C# 6 already allows some of that boiler plate to be reduced:

public class Address 
{
    public string Country { get; }
    public string City { get; }
    public string State { get; }

    public Address(string country, string city, string state) 
    {
        Country = country;
        City = city;
        State = state;
    }

  // Some other behaviors.
}

but, of course, being able to write:

public class Address(string country, string city, string state)
{
    // Some other behaviors.
}

would be even better.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

Adding class parameters doesn't mesh well, in my opinion, with the declaration of other class properties and fields. Unfortunately it becomes difficult to express this with value types, so I'm going to change some things around to get a slightly more realistic (but still contrived) example.

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { get; }
    IPrinterService PrinterService { get; }

    public MailingLabelService(IPersonRepository personRepo, IPrinterService printService) {
        PersonRepository = personRepo;
        PrinterService = printerService;
    }

    public PrintMailingLabel(int personId) {
        // ... behavior implementation
    }
}

Based on @DavidArno's suggestion, we would get this:

public class MailingLabelService<T>(IPersonRepository PersonRepository, IPrinterService PrinterService) where T : ILabelType
{
    public PrintMailingLabel(int personId) {
        // ... behavior implementation
    }
}

A more complicated implementation for the C# team, but a potentially more elegant solution, would be an inject keyword for auto properties.

The example class would become:

public class MailingLabelService<T> where T : ILabelType
{
    inject IPersonRepository PersonRepository { get; }
    inject IPrinterService PrinterService { get; }

    public PrintMailingLabel(int personId) {
        // ... behavior implementation
    }
}

or another potential implementation could be this, since injection is more of an action rather than a definition:

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { inject; }
    IPrinterService PrinterService { inject; }

    public PrintMailingLabel(int personId) {
        // ... behavior implementation
    }
}

These auto properties would be inherently readonly still.

Usage examples:

Constructor parameters where the parameter order is determined by the order the injected properties are declared. This is how things work today on the calling side, without the syntax issues on the declaration side.

new MailingLabelService<LetterLabel>(PersonRepository, PrinterService);

or

Named parameters as they exist today:

new MailingLabelService<LetterLabel>(PersonRepository: personRepo, PrinterService: printService);

This would still be compiled down to a constructor that accepts parameters in a given order, with fields that are set based on the parameters.

The inject keyword would also make it far more obvious that certain properties are intended to be injected by outside callers rather than managed internally.

I've still left the question unanswered of whether they could still be set through a typical constructor or not.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

This may be outlandish (edit: @HaloFour pointed out below it is in fact outlandish so should be ignored) , which is why I am adding it separately.

To expand on the second usage example, if inject were treated as an action of an auto-property, we could potentially define custom object initialization logic for this readonly injected property. This keeps logical code closer to the declaration of the property, and keeps constructors limited and more maintainable.

Since this would be a part of object initialization, this opens the doors for more appropriate actions to be taken based on injected dependencies without worrying about having a partially initialized object as the case is with constructors.

public class MailingLabelService<T> where T : ILabelType
{
    IPersonRepository PersonRepository { inject; }

    IPrinterService PrinterService {
        inject {
            if ( _some condition_ )
                PrinterIsDotMatrix = true;
            else
                PrinterIsDotMatrix = false;
        }
    }

    // This is readonly
    bool PrinterIsDotMatrix { get; }

    public PrintMailingLabel(int personId) {
        // ... behavior implementation
    }
}

@sirgru
Copy link
Author

sirgru commented Nov 2, 2016

@jyarbro the syntax of style:

public class Person(string Name, DateTime DateOfBirth) {

}

is coming with almost 100% certainty - it's records: Records
thus I feel the discussion about inject would be better taken here: #10154

With that, I think the purpose of records and them looking like a method call more than a class is to make them more explicit with positional matching feature, which would be somewhat obscured by using the inject keyword, and potentially also make them more vulnerable to changing the c-tor by changing the parameter order.

Speaking of records, I am still not convinced they are a good conceptual fit for everything. It seems the properties can be non-read-only if they are re-declared, but then the shorthand of records is lost - the quantity of code is about the same as for non-records. Also, it is unclear if this will apply to fields, will they be able to be re-declared, and even so the shorthand is lost. My intuition is, if the user does not want pattern matching and wants mutable data they will write a class as we know it today and not a record. Then, in those use cases, we are back to C# 6 syntax of setting locals.
Secondly, there is a lot of code today that uses this kind of "setting locals" boilerplate. I think it can be offered by Visual Studio as a code suggestion to simplify the existing code (if this could hypothetically be accepted).
So, this proposal doesn't necessary touch the work on records, although it can, and can help in cases where records are not used.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

I feel that the records feature seems well crafted and useful for value types, but in cases where dependency injection is used heavily, declaring more things in the class definition could make things very convoluted as you're defining many things on one line.

Your solution does solve the direct issue of boiler plate constructor code. It does not solve the syntax issue where many (5 or 6) properties are injected. Your proposed syntax could become even more hairy if non-readonly values are set in the constructor as well.

I'm specifically trying to tackle boiler plate definition exactly as you've described in your issue, specifically with dependency injection in mind.

@sirgru
Copy link
Author

sirgru commented Nov 2, 2016

OK. I thought you were going for more of a syntax alternative to records.

It does not solve the syntax issue where many (5 or 6) properties are injected.

They still have to be re-specified, but the c-tor call is explicitly visible. I can see the benefit in "hiding" the c-tor call, but I can also see how the order of declaration making a difference could be unexpected.

Your proposed syntax could become even more hairy if non-readonly values are set in the constructor as well.

I don't see the issue here. Could you clarify?
If these settings have side-effects like in your last example, or error-checking, these could be handled in the body of the c-tor.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

Yeah, so basically if you look at my example, let's say you want to adhere to naming conventions. Your class-scoped properties would probably be capitalized. This would look weird as parameters but let's say that's what we're doing. So here's my example with your set keyword.

public MailingLabelService(set IPersonRepository PersonRepository, set IPrinterService PrintService) {
}

Now let's say that you want to pass in non-read only parameters as well.

public MailingLabelService(ISomething someVariable1, ISomething2 someVariable2, set IPersonRepository PersonRepository, set IPrinterService PrintService) {
    // do something with someVariable and someVariable2
}

And then you are extending a base class:

public MailingLabelService(ISomething someVariable, ISomething2 someVariable2, set IPersonRepository PersonRepository, set IPrinterService PrintService) : base(someVariable, someVariable2, PersonRepository) {
    // do something with someVariable
}

Your constructor becomes incredibly heavy when it should be one of the lightest parts of your class, short of maybe the disposal logic.

I should probably go more into how inheritance would work in my model, but I need to get going.

@HaloFour
Copy link

HaloFour commented Nov 2, 2016

@jyarbro

Dependency injection is a library concern, the language has no concept of it. Most of what you've described is already supported by assorted IoC containers. You can write setters instead of constructor parameters and have the container call some initializer method after injecting the dependencies. What you lose there is the ability to define the backing fields for the dependencies as readonly as the CLR enforces the hard rule that only constructors may write to initonly fields.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

Dependency injection (inversion) is a principle, as the D in SOLID. The language absolutely has a concept of it because it's just a way of organizing your code.

@HaloFour
Copy link

HaloFour commented Nov 2, 2016

@jyarbro

The language absolutely has a concept of it because it's just a way of organizing your code.

Wrong, the language has no concept of it. You rely on existing general-purpose language constructs in order to enable dependency injection, but the language facilitates absolutely none of it directly. You're relying on libraries to actually carry out the registration, resolution, instantiation and life cycle of the dependencies. Aside from direct instantiation C# doesn't know (nor care) what any of those are.

Either way, you're not "stuck" using constructor parameters for dependencies. Various IoC containers support member injection and post-initialization. If that's the strategy that you'd prefer you don't need any new language features to support it.

@DavidArno
Copy link

@HaloFour & @jyarbro

Take 100 devs and you'll get at least 100 opinions on what dependency injection, IoC and dependency inversion mean. Arguing semantics with each other, for such misunderstood and misused terms, is pointless.

@HaloFour
Copy link

HaloFour commented Nov 2, 2016

@DavidArno

Take 100 devs and you'll get at least 100 opinions on what dependency injection, IoC and dependency inversion mean. Arguing semantics with each other, for such misunderstood and misused terms, is pointless.

Indeed, which, if anything, is all the more reason to not try to bake more complicated forms of dependency injection directly into the language. At the crux of whatever that might be would have to be some kind of easing on the CLR enforcement of initonly fields anyway, otherwise you could never have the concept of a post-constructor initialization phase, and I don't think that's a good idea.

@jyarbro
Copy link

jyarbro commented Nov 2, 2016

It's hard to overcome your antagonism just to talk about what is potentially a naive understanding on my own part to see where I am wrong / mistaken / misled. But I'll try.

I find it hard to set aside the seemingly necessary pedantry of discussing what DI is in order to explain why I find my proposal to be functional. I clearly don't have the level of expertise you do simply by you mentioning CLR level code, however I would like to still carry on with this.

Can you try to explain, as non-antagonistically as you can muster, how you feel that what I'm describing is either more complicated, or how it would negate the concept of post-constructor initialization? Maybe then I could ask better questions more directly related to the proposal.

@HaloFour
Copy link

HaloFour commented Nov 2, 2016

@jyarbro

To expand on the second usage example, if inject were treated as an action of an auto-property, we could potentially define custom object initialization logic for this readonly injected property.

Effectively, this is not possible with the CLR. The CLR enforces that only a constructor may change an initonly field (initonly is just what the CLR calls readonly). Anything else is non-verifiable which means that the CLR will forbid it to run at all except when granted full trust privileges.

The other option, which you have described, is some kind of auto-generation of said constructor. But that just leads to messy and disparate syntax in my opinion, especially since any/all of that inject logic would have to be in said constructor. You aren't making the constructor lighter, you're just hiding it's heaviness by splitting the body up and spreading it around the class.

Maybe then I could ask better questions more directly related to the proposal.

As an aside, might I suggest opening a separate proposal? This conversation has hijacked @sirgru 's proposal.

@sirgru
Copy link
Author

sirgru commented Nov 2, 2016

Here is another idea.
We know we want less biolerplate in the constructor and some type of constructor is preferable to most people as opposed to flagging locals as arguments (I think I've seen this argument somewhere in records discussion).

Presumably the set thing is used, and compiler can conclude the name of the variable from the name of the c-tor paramter (added case insensitivity to the proposal as I forgot to write it and @jyarbro mentioned it). If the compiler knows the exact variable that's going to be set, then it also knows its type, so repeating its type in the c-tor becomes superfluous.

So, the bad case scenario becomes:

public MailingLabelService(ISomething someVariable, ISomething2 someVariable2, 
set var PersonRepository, set var PrintService) 
: base(someVariable, someVariable2, PersonRepository) {
    // do something with someVariable
}

This probably wouldn't work though, as both set and var are context keywords so you can name your class set or var... In case somebody actually has a type called var then the compiler can treat it like a type.

@ghost
Copy link

ghost commented Nov 3, 2016

In the case of your last suggestion, why not simply let the "set" keyword and property name without the type?

public class Address
{
    public string Country { get; private set; }
    public string City { get; private set; }
    public string State { get; private set; }

    public Address(set Country, set City, set State) {    }
}

It would be very clear and sweet!

@jyarbro
Copy link

jyarbro commented Nov 3, 2016

@HaloFour thanks for the effort. I understand more of your statement now. I don't really find this as hijacking @sirgru's proposal as he stated his goal is reducing boilerplate constructor code. With that said, it's good to have someone like you who understands the underlying CLR limitations to provide boundaries of reality.

@sirgru I think boilerplate constructor code may be a symptom of a bigger issue. I propose we define the issue as how we deal with externally defined readonly properties.

If defining a constructor is required, I do find @nmarcel's idea elegant.

I would prefer to not need a constructor at all though when we're simply passing in some references for private class properties, and perhaps clarifying that point explicitly may help explain my position a bit better.

The location of a constructor in defined code is one of those things that is often left up to the whims of the developer. Some people define constructors first, some define them under the properties and fields, some put it down with statics at the bottom of the class, and some just don't care and put it where ever they happen to be on the scroll bar.

A large portion of those constructors in my organization are simply passing in parameters into private property setters that couldn't be initialized with object initialization. My desire is to simply remove the constructor altogether from the code if this is the case. This would make it a bit easier to get a better grasp of what is happening, when looking at the section of your class where properties are defined, as opposed to needing to split your code window to view the constructor while going through your properties. Perhaps this really only becomes a problem in team development environments that don't have rigid coding standards.

@sirgru
Copy link
Author

sirgru commented Nov 3, 2016

@nmarcel That was my original idea but... since it's perfectly legal to have a type called set then some ambiguity can arise as to whether set is used as a type or a keyword. A similar thing can also be said about var, but I have an intuition that compiler could see set var and figure out we're dealing with concluding the type as opposed to using an existing type a bit easier, because var's ambiguity is a solved problem, yet there may be people out there with a class named set.

So, for me, set would be preferable but set var is not that bad either. I would think this proposal would be a nice little saver from ceremony but I guess we all like our own opinions. For what I wanted to do with it, I am satisfied with it's shape now.

@jyarbro I'm also new here, but seeing how these proposals work I think your ideas would be better taken to a separate one. I think it's important to have it well defined in the first post and not in the comments. I think you need to figure out what happens in all cases.. for example, what about other c-tors? They must chain to the main one, but chaining to the main one means typing the variables again. So, in your case it is not helping unless you want multiple c-tors automatically generated, but than there are more issues, such as do you want all combinations and which ones? I would look into syntax they did with records and not clash with it, otherwise I don't think there's any chance - my opinion.

Also, seeing your problem, I think you're trying to use a cannon to kill a fly, so to speak. I think every coding standard needs a specification where the c-tors go, and the fact that you're running into this issue means that you definitely need one. I would not call that "rigid", but rather a "standardization of practices", a good thing.

@jyarbro
Copy link

jyarbro commented Nov 3, 2016

Alright, I suppose I'm viewing this from a StackExchange perspective where there's a common problem and the proposed solutions all go into the same thread. Sorry about that.

@ghost
Copy link

ghost commented Nov 3, 2016

@sirgru in that case (of 'set' used as a type name) then maybe just using the property name would suffice.

    public Address(Country, City, State) {    }

@sirgru
Copy link
Author

sirgru commented Nov 4, 2016

I like it! Not sure if it would be acceptable to a larger audience tho, but I've added it above. I have found this comment to be very true about syntax:
#5292 (comment)

Speaking of this, wouldn't it be nice if the records allowed for something similar? For example, if I wanted to re-create the syntax from my first example in records, as they are defined today, it would look like this:

public class Address(string country, string city, string state) {
    // Re-initialization is required for read-write properties.
    public string country { get; private set; } = country;
    public string city { get; private set; } = city;
    public string state { get; private set; } = state;
}

This would continue to work for read-only:

public class Address(string country, string city, string state)
{
    // Some other behaviors.
}

but for read-write something like this:

public class Address(country, city, state) {
    // concluded the locals and assigned them implicitly!
    public string Country { get; private set; }
    public string City { get; private set; }
    public string State { get; private set; }
}

@binki
Copy link

binki commented Sep 20, 2018

I don’t like the Strict/Loose/More Loose algorithms suggested in the proposal. I think that if the compiler is doing case insensitive matches with member variable names or postfix matching, it is quickly very confusing which member variable a parameter would be matched with.

The Strict algorithm would conflict with the popular existing property/parameter naming conventions. For example, the most widely seen convention is to spell property names in PascalCase and parameter names in camelCase. If your syntax allowed specifying the target name, then you would still be slightly reducing boilerplate:

public class Address(country sets Country, city sets City, state sets State) {
    public string Country { get; }
    public string City { get; }
    public string State { get; }
}

However, this is still more verbose than TypeScript-style initialization. For that reason, I prefer proposal dotnet/csharplang#804 over this.

@sirgru
Copy link
Author

sirgru commented Oct 2, 2018

How does the TypeScript-style initialization not have the same problem as the Strict version of this proposal? It will force the property/field name into the same name as argument. It just puts it in the unexpected place (the c-tor).

I've also seen this syntax in the dotnet/csharplang#804 propsal:

Something that your proposal might have to take into consideration is that the C# naming guidelines stipulate that property names are PascalCase while parameter names are camelCase. The record proposal solves this through having a second identifier declared on the primary parameter, e.g.:

public class Person(string firstName FirstName, string lastName LastName);

which I will not comment on.

For the record, I don't support this proposal any more. I don't think it is idiomatic C# and so it will not fly in the C# world. However, I find the alternatives (records) syntactically horrible. So yes, this one can be closed.

@CyrusNajmabadi
Copy link
Member

How does the TypeScript-style initialization not have the same problem as the Strict version of this proposal?

Because, in JavaScript, it's common for those names to match. So the common pattern is supported by the simple code snippet. That's different from .Net where the 99.9% naming case for the two ends up making them not match.

@sirgru
Copy link
Author

sirgru commented Oct 2, 2018

How does the TypeScript-style initialization not have the same problem as the Strict version of this proposal?

Because, in JavaScript, it's common for those names to match. So the common pattern is supported by the simple code snippet. That's different from .Net where the 99.9% naming case for the two ends up making them not match.

I know, I was referring to implementing TypeScript-style initialization in C#.

However, this is still more verbose than TypeScript-style initialization. For that reason, I prefer proposal dotnet/csharplang#804 over this.

@qwertie
Copy link

qwertie commented Aug 7, 2019

While I like @ghost's version of this...

public class Address
{
    public string Country { get; }
    public string City { get; }
    public string State { get; }

    public Address(set Country, set City, set State) { }
}

...there is a widespread convention that parameter names start with a lowercase letter. Could C# use one of these two rules?

  1. The compiler selects a parameter name automatically (i.e. set Country implies a string country parameter, and set _foo implies a foo parameter)
  2. The constructor accepts the actual parameter name, but has a rule to detect the corresponding property or field name (i.e. you write set country and the compiler cannot find a field or property by that name, so it matches Country or _country instead)

@Nagendracv
Copy link

Nagendracv commented Dec 24, 2020

How about

public class Address
{
    public Address(private string Country, private string City, private string State) { }
}

So that there is no need for an explicit declaration of the member variables

@CyrusNajmabadi
Copy link
Member

Closing this out. We're doing all language design now at dotnet/csharplang. If you're still interested in this idea let us know and we can migrate this over to a discussion in that repo. Thanks!

@CyrusNajmabadi CyrusNajmabadi closed this as not planned Won't fix, can't repro, duplicate, stale Nov 8, 2022
@Mik4sa
Copy link

Mik4sa commented Nov 8, 2022

@CyrusNajmabadi Yes, atleast I'm still interested in this one. This would be a very neat and timesaving feature :)

@dotnet dotnet locked and limited conversation to collaborators Nov 8, 2022
@CyrusNajmabadi CyrusNajmabadi converted this issue into a discussion Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants