csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Primary Constructors

Open YairHalberstadt opened this issue 5 years ago • 240 comments

@MadsTorgersen added a proposal for primary constructors yesterday: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-12.0/primary-constructors.md

I wanted to link the proposal to the issue for primary constructors, but I couldn't find any, so I thought I'd create this issue as a dumping ground for discussion.

NOTE:

I will interpret upvotes and downvotes as upvotes/downvotes on the proposal.

Meeting Notes:

  • https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-10-20.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-10-27.md#readonly-modifiers-for-primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-08-31.md#ignored-directives-support
  • 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-17.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-02-15.md#open-questions-in-primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-02-22.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-03.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-08.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-15.md#primary-constructors
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-07-26.md
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-07-31.md
  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-09-25.md#primary-constructors

YairHalberstadt avatar Jul 25 '19 13:07 YairHalberstadt

My general opinion: I don't think it offers any real power over current solutions, and only really benefits brevity, at the cost of significant new rules and unfamiliar syntax. C# is a complex language as it is, and we should reserve additional complexity for where it can make a real difference.

I feel like records however will significantly change the way we write code, by encouraging greater use of small, immutable, data only objects.

As such my vote would go to records over primary constructors.

EDIT:

I partially retracted after some experience with scala. See my comment below

YairHalberstadt avatar Jul 25 '19 13:07 YairHalberstadt

Apologize for brevity, on phone.

I like this, Scala has it. The primary constructor itself is just shorthand for the constructor with properties. You get the rest of the "record" behavior by making it a case class, which provides identity and deconstruction. Wrap that in an "enum" and you have DUs.

HaloFour avatar Jul 25 '19 14:07 HaloFour

@HaloFour

I think if this could be neatly packaged with records in a way that they felt like one unified feature, I might agree with you. At the moment, I'm not seeing that.

YairHalberstadt avatar Jul 25 '19 14:07 YairHalberstadt

@YairHalberstadt

I could see it being helpful for DI, don't need to declare your dependencies three times (fields, parameters and assignments), just once. But aside that I probably agree. I'm ok with primary ctors as being just unopinionated shorthand, as long as it doesn't go down the rabbit hole of feature parity with normal ctors.

HaloFour avatar Jul 25 '19 14:07 HaloFour

I'm fine with PCs being a simple shorthand, with all their limitations, if we get a lightbulb operation that expands them into proper constructors.

@HaloFour the use of PCs for DI could be limited by their visibility. For example, your constructor takes three proper parameters and three injected dependencies. Your proper parameters must be validated and transformed by the constructor. You cannot extract your three dependencies into a primary constructor, because you would want it to be private.

orthoxerox avatar Jul 25 '19 15:07 orthoxerox

@orthoxerox

You cannot extract your three dependencies into a primary constructor, because you would want it to be private.

I don't get why you'd need/want the PC to be private in that case, or why you'd need two constructors at all. I'd just have a single PC of the six parameters.

I'm also not saying that PCs would necessarily solve the issue for all DI-related construction, but they'd likely manage a good 90% or more.

HaloFour avatar Jul 26 '19 01:07 HaloFour

  • This proposal seems completely incompatible with the current record proposal. And I disagree with the statement in the proposal saying this is more widely useful than records. I think this saves a little bit of boilerplate- records (and related features of auto-generating GetHashCode, Equals, ToString) can potentially save a ton of boilerplate in a lot of scenarios.
  • I think the proposal is interesting but I hate all of the proposed extensions. Primary constructor bodies was the worst part of this feature when it was part of C# 6.0, it's still a bad idea today. If you need initialization logic make a real constructor!

MgSam avatar Jul 26 '19 04:07 MgSam

@HaloFour

I think if this could be neatly packaged with records in a way that they felt like one unified feature, I might agree with you. At the moment, I'm not seeing that.

I've said it before in other issues, but I'll say it again here. What do primary constructors have to do with records?

Richiban avatar Jul 26 '19 08:07 Richiban

@Richiban

Nothing intrinsically, but @Halofour was suggesting a link, Mads indicated the syntaxes may clash, and they aim to solve similar pain points.

YairHalberstadt avatar Jul 26 '19 09:07 YairHalberstadt

@Richiban

I've said it before in other issues, but I'll say it again here. What do primary constructors have to do with records?

With the way records have been proposed for C# they include symmetric construction and deconstruction as well as identify based on a specific set of properties. Primary constructors get you all of that in one parameter list given that the parameters are also properties and that list gives you an order in which those properties can be deconstructed.

C# records, as they have been proposed, are more like Scala case classes or F#'s single case unions, and both languages define the construct by how they are constructed.

case class Point(x: Int, y: Int)

val p = Point(1, 2)
val (x, y) = p
type Point = Point of X : int * Y : int

let p = Point(1, 2)
let x y = p
class Point(int x, int x);

var p = new Point(1, 2);
var (x, y) = p;

HaloFour avatar Jul 26 '19 11:07 HaloFour

I really don't see the benefit for this. It lowers readability, and adds complexity.

If a class constructor gets more than 3,4 parameters you will usually want to refactor it to a builder or a group up the parameters into a "configuration" object, so not a lot is saved there. As for simple data classes, you can just have visual studio generate the constructors for you.

brabebhin avatar Jul 26 '19 11:07 brabebhin

@Richiban the syntaxes proposed for these two features are the same:

class Name(string value) {}

so if you use this syntax for primary constructors you need to think how records (who have value semantics) will look like.

orthoxerox avatar Jul 26 '19 12:07 orthoxerox

I really don't see the benefit for this. It lowers readability, and adds complexity.

If a class constructor gets more than 3,4 parameters you will usually want to refactor it to a builder or a group up the parameters into a "configuration" object, so not a lot is saved there. As for simple data classes, you can just have visual studio generate the constructors for you.

You mean it improves readability, surely?

I mean, it may well depend on one's coding style, but our codebase contains literally thousands of classes that look something like this:

internal class CosmosCustomerRepository : ICustomerRepository
{
	private readonly string _connectionString;
	private readonly string _collectionName;

	public CosmosCustomerRepository(string connectionString, string collectionName)
	{
		_connectionString = connectionString;
		_collectionName = collectionName;
	}

	public Customer Retrieve(CustomerId id)
	{
		// Create connection, execute query, return result
	}
}

Being able to reduce the above to:

internal class CosmosCustomerRepository(string connectionString, string collectionName) 
    : ICustomerRepository
{
	public Customer Retrieve(CustomerId id)
	{
		// Create connection, execute query, return result
	}
}

...is a big win for readability, if you ask me.

The constructors in the example above (of which there are many; a quick sample taken from our codebase of 30 classes shows that 22 of them (73%) had an explicit constructor defined and of those 21 (> 95%) did nothing other than set private readonly fields) are dumb, tedious, can be auto generated, are rarely read--normally skipped over--by humans (because they're usually so dumb) and are therefore a surprisingly common source of bugs.

Have you ever had to track down a bug that looked like this?

internal class SomeClass
{
	private readonly string _depA;
	private readonly string _depB;
	private readonly int _depC;

	public SomeClass(string depA, string depB, string depC)
	{
		_depA = depA;
		_depA = depA;
		_depC = depC;
	}

	// Methods here...
}

Or this?

internal class SomeOtherClass
{
	private readonly string _depA;
	private readonly string _depB;

	public SomeOtherClass(string depA, string depB)
	{
		_depA = depB;
		_depB = depA;
	}

	// Methods here...
}

I really truly believe that the majority of constructors look like this and they should not be written by humans.

Leaning on the IDE to generate them for you isn't really a solution because:

a) It won't stay in sync automatically. If you add a field you have to remember to add a corresponding constructor argument and assign it properly. b) The developer still has to look at and read the auto-generated constructor. Sure, you could hide it in a region, but if the constructor is both auto-generated and then hidden then it really should be a language feature.

Richiban avatar Jul 26 '19 12:07 Richiban

@Richiban the syntaxes proposed for these two features are the same:

class Name(string value) {}

so if you use this syntax for primary constructors you need to think how records (who have value semantics) will look like.

Since records are unlike other classes (in that they have value semantics) I kind of feel that they should be differentiated from other class definitions more anyway (probably with a keyword), never mind that it also frees up the syntax for primary constructors.

Records:

data class Point(int x, int y)

Primary constructor:

class Graph(Point[] points)
{

}

There's a really nice interplay between the two features this way: the constructor syntax makes it clear that the following declarations are parameters that become members of the class, and the data keyword additionally says "they're public properties and participate in equality comparisons.".

Neat, I think

Richiban avatar Jul 26 '19 12:07 Richiban

Simple examples are cool, but you will eventually run into a class where you cannot see the class definition and constructor definition in the same screen, so you will be left to wonder: is this a bug or a feature? And imagine code reviews with this. You see a constructor with no assignments and you nod in agreement and move on, only to see that several lines up somebody forgot to add something to class definition. So it is just as error prone as the usual assignment.

And in your second example, the class obviously lacks readability. It is basically defining members inline with class definition. Just imagine combining that with several interface implementation declarations, several attribute definitions and the usual abstract / sealed keywords.

This feature is just too prone to abuse. I agree writing boilerplate is annoying and ides are not ideal, but this is just a recipe for abuse and cryptic code.

brabebhin avatar Jul 26 '19 12:07 brabebhin

I would like to have the captured values as readonly.

gulshan avatar Jul 26 '19 13:07 gulshan

@mcosmin222

This feature is just too prone to abuse. I agree writing boilerplate is annoying and ides are not ideal, but this is just a recipe for abuse and cryptic code.

I'm sorry, but I simply do not understand your argument. I don't really see how this is open to abuse or cryptic code at all. Can you help me out with an example?

Richiban avatar Jul 26 '19 13:07 Richiban

@Richiban

I kind of feel that they should be differentiated from other class definitions more anyway (probably with a keyword), never mind that it also frees up the syntax for primary constructors.

I agree, and this is exactly how Scala does it. In normal classes the primary constructor only buys you the constructor parameters being in scope for the entire class as fields. But add the case keyword and you also get publicly exposed properties (by default), value semantics, positional deconstruction, string representation and a few other goodies for free:

class Foo(name: String) {
  def greeting: String = s"Hello $name!"
}

val foo1 = new Foo("Richiban")
assert(foo1.greeting == "Hello Richiban!")
val name = f1.name // compiler error, name is not resolved as an accessible member
val foo2 = new Foo("Richiban")
assert(foo1 != foo2)
case class Bar(name: String) {
  def greeting: String = s"Hello $name!"
}

val bar1 = Bar("Richiban")
assert(bar1.greeting == "Hello Richiban!")
assert(bar1.name == "Richiban") // name is an accessible member, by default
val name = bar1 match {
  case Bar(name) => Some(name) // name can be deconstructed/extracted
  case _ => None
}
assert(name.contains("Richiban"))

val bar2 = Bar("Richiban")
assert(bar1 == bar2) // compares equality based on name

HaloFour avatar Jul 26 '19 15:07 HaloFour

@HaloFour

I agree, and this is exactly how Scala does it. In normal classes the primary constructor only buys you the constructor parameters being in scope for the entire class as fields. But add the case keyword and you also get publicly exposed properties (by default), value semantics, positional deconstruction, string representation and a few other goodies for free:

I never actually learned any Scala but, yeah, that's exactly how it should work. case is an odd choice of keyword though.

Richiban avatar Jul 26 '19 18:07 Richiban

@Richiban

case is an odd choice of keyword though.

I believe Scala considers these types much more as ADTs or members of a DU than as just a "data class", which kind of makes sense as they are usually short and sweet, immutable and contain zero business logic. I kind of anticipate that C# records will have similar use cases as opposed to attempting to replace POCOs which are often much larger and mutable.

HaloFour avatar Jul 26 '19 19:07 HaloFour

I've never understood why the C# design team has not considered the syntax TypeScript adopted, where the field is initialized and declared all in the parameter list.

class Foo 
{
    Foo(private string Bar) { }
}

The benefits of this are:

  • Minimal new syntax required
  • No need for jumping through hoops to invent new ways to initialize a class
  • Already familiar to many Microsoft-ecosystem developers

The presence of overloads in C# make this slightly more complex than in TypeScript, but I don't believe that's a blocker.

This accomplishes the same goals as primary constructors with minimal disadvantages.

MgSam avatar Jul 26 '19 20:07 MgSam

I've never understood why the C# design team has not considered the syntax TypeScript adopted

It has been considered.

where the field is initialized and declared all in the parameter list.

This has the negative problem of the parameter and field having inconsistent naming with the naming of the .net ecosystem. The language has not wanted to wade into the space of "how would we name these?" and all the associated baggage (i.e. "how would the user override the naming?").

CyrusNajmabadi avatar Jul 26 '19 20:07 CyrusNajmabadi

I've never understood why the C# design team has not considered the syntax TypeScript adopted

It has been considered.

where the field is initialized and declared all in the parameter list.

This has the negative problem of the parameter and field having inconsistent naming with the naming of the .net ecosystem. The language has not wanted to wade into the space of "how would we name these?" and all the associated baggage (i.e. "how would the user override the naming?").

  • This has been a non-issue in TypeScript. The feature is widely used and I can't recall anyone complaining about it because of style reasons.
  • This is already an issue with tuples and to a lesser degree records. (Do you capitalize the members or not? Should it depend on the usage of the tuple?)
  • The LDT really should have learned the lesson by now to stop discarding features solely on the basis of community bike-shedding over minor style issues. cough private protected cough.

MgSam avatar Jul 26 '19 21:07 MgSam

This has been a non-issue in TypeScript.

The TypeScript ecosystem is not the C# or .Net ecosytem. Patterns and practices are different htere.

The feature is widely used and I can't recall anyone complaining about it because of style reasons.

The style desires of the communities and ecosystems are different. This is greatly mitigated in TS because parameters and properties are normally cased the same for them, where they are not for .net.

This is already an issue with tuples and to a lesser degree records. (Do you capitalize the members or not? Should it depend on the usage of the tuple?)

This has already been a big issue. See the large debate in roslyn/corefx where an API that was tuple-returning was killed because this issue could not be resolved adequately.

The LDT really should have learned the lesson by now to stop discarding features

The feature was not discarded. Where did you get that idea? I responded to your question about why it was not considered by talking about how it has been considered. You jumped from that to it being discarded when that is not the case.

solely

Where did you get 'solely' from?

on the basis of community bike-shedding over minor style issues.

This was not community bike-shedding. The LDM members themselves (including myself) could not come up with an adequate proposal that didn't have significant issues (plural). As such, no forward movement has happened until someone can propose something that will be appropriately championed.

With much more important work getting attention, no one has had the time to invest here. Perhaps that will change in the future.

CyrusNajmabadi avatar Jul 26 '19 23:07 CyrusNajmabadi

public class C(int i, string s) : B(s)
{
    {
        if (i < 0) throw new ArgumentOutOfRangeException(nameof(i));
    }
    int[] a = new int[i];
    public int S => s;
}

I think the example in the proposal is against tens of years of convention. We are not used to see scopes without any header in class. The parameter definitions are just moved from constructor definition to class definition without any benefit and mostly obscuring things.

Also the initialization of the field int[] a = new int[i]; is somewhat controversial too. Today we cannot initialize the fields using members (only statics are allowed). Can we use any field we want? No. So this new field must be a special thing to be able to used like this. This can introduce some confusion (especially to the newcomers).

I am in favor of automatic code generation (#107). It can cover all of the benefits of this proposal (and more) without a new syntax.

public class C
{
    [PrimaryConstructor]
    // You can specify the visibility of the constructor, as we can for some twenty years.
    public C(string someString, int someInt)
    {
    }
    
    // You can specify methods to run before or after member assignments:
    [PrimaryConstructor(beforeAssignmentMethod: nameof(BeforeAssignment), afterAssignmentMethod: nameof(AfterAssignment))]
    public C(string someString, int someInt)
    {
    }

    private void BeforeAssignment(string someString, int someInt)
    {
        if (someInt < 0) throw new ArgumentOutOfRangeException(nameof(someInt));
    }

    public string SomeOtherString { get; private set; }

    // Not necessary, but this method may be inlined in the primary constructor. If so, the private set part of the above property can be omitted.
    private void AfterAssignment(string someString, int someInt) // May or may not have the parameters.
    {
        SomeOtherString = SomeString.ToLower();
    }
    
    // You can specify whether the parameters should be stored in members or in auto properties:
    [PrimaryConstructor(createMembersAs: MemberCreation.AutoProperty)]
    // These are created for you:
    // public string SomeString { get; }
    // public int SomeInt { get; }
    // Or
    [PrimaryConstructor(createMembersAs: MemberCreation.PrivateField or MemberCreation.PrivateReadonlyField)]
    // private (readonly) string _someString;
    // private (readonly) int _someInt;
    public C(string someString, int someInt)
    {
    }
}

Many customizations are available in this way and they are not constrained by the syntax. Also some other attributes that inherit from PrimaryConstructor can simplify things. E.g:

  • PrimaryConstructorWithAutoProperties
  • PrimaryConstructorWithReadonlyFields
  • ...

notanaverageman avatar Jul 28 '19 08:07 notanaverageman

I remember this was ever discussed in length long before, maybe in a design note post rather than its own post. One issue is the primary constructor body syntax, which is consistent with other languages like Python, F#, etc., but looks confusing enough in C#.

qrli avatar Jul 29 '19 01:07 qrli

@yusuf-gunaydin Unfortunately I think the code generation idea is not really going anywhere. I also think that these situations are common enough to warrant their own language feature. As evidence, let's look at some other languages that offer exactly this functionality:

F#:

type Greeter(name : string) =
    member this.SayHello () = printfn "Hello, %s" name

Scala:

class Greeter(name: String) {
    def sayHi() = println("Hello, " + name)
}

Kotlin:

class Greeter(val name: String) {
    fun greet() {
        println("Hello, ${name}");
    }
}

Typescript:

class Greeter {
    constructor(private name: string) {}

    greet() {
        console.log(`Hello, ${this.name}!`)
    }
}

So, you see, there's massive precedence for this feature.

Richiban avatar Jul 30 '19 09:07 Richiban

I have read somewhere that the team will reconsider code generation (possibly reducing some of its scope) after releasing C# 8.0. So I am somewhat optimistic that a result will come out of this.

As I see it, primary constructors will only use code generation (no manipulation of existing code), so it can even be implemented today with existing Roslyn code generator libraries. (Although, it may be in a less consice way than full manipulation support since the constructor body has to be declared in the source code.)

By the way, I am not against the primary constructor concept itself. But, inventing a new, unnatural, and limited syntax should not be the way.

notanaverageman avatar Jul 30 '19 10:07 notanaverageman

@yusuf-gunaydin

By the way, I am not against the primary constructor concept itself. But, inventing a new, unnatural, and limited syntax should not be the way.

It's not unnatural. See my examples of a handful of other very popular languages that use exactly this syntax. There's nothing wrong with it at all.

It could be a somewhat limiting syntax, depending on your viewpoint. For me it's not limiting at all since, as I said before, in our codebases the vast majority of constructors do nothing other than set fields. Even if what you want to do is null checks, there's a new syntax (probably) coming for that too:

class Greeter(string name!)
{
    public string Greet() => $"Hello, {name}!";
}

Richiban avatar Aug 01 '19 10:08 Richiban

Thanks @YairHalberstadt for opening the issue. I went ahead and championed it. There are good arguments for and against primary constructors, but the focus on records in C# 9.0 will force a discussion, so this issue can represent that.

MadsTorgersen avatar Aug 30 '19 17:08 MadsTorgersen