dotnet / csharplang

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

Primary Constructors (VS 17.6, .NET 8) #2691

Open YairHalberstadt opened 5 years ago

YairHalberstadt commented 5 years ago

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

FaustVX commented 1 year ago

@Washi1337 i agree with your points, but the init { } can be useful if it can be used with all other "normal" type, and be executed after constructors/object creation/with like in:

var c = new C()
{
  Prop1 = "a",
}; // init here
var d = c with
{
  Prop1 = "b"
}; // init here
var e = new C(); // init also here for consistency
HaloFour commented 1 year ago

@Washi1337

This feature is directly clashing with how record types are handled by the compiler. While records will generate public properties for each parameter in its constructor, this proposal will make private fields instead.

This feature does not create fields. You get constructor parameters and captures. That capture might be promoted to a field if used in a way that extends beyond the scope of the constructor, but with an unspeakable name like an auto-property. If you capture the parameters as init-only properties then it behaves exactly as a record would, and it's the record modifier which opts into the additional member synthesis.

This also kind of forces a naming convention on developers.

As these parameters are not fields there is no "clash" of conventions. Such a thing already does exist with records today given the parameters, which are typically camelCase, are synthesized into properties, which are typically TitleCase.

Bottom line is that I feel like this feature forces a little too much on how C# should be written according to the C# team.

C# already has primary constructor syntax. It just so happens that they are currently (and unnecessarily) tied to the records feature. This enables them to be used for non-record types while seamlessly working with records, allowing "records" to really be a combination of smaller orthogonal language features, as is already the case with other features like deconstruction and init-only properties.

Washi1337 commented 1 year ago

@HaloFour

You get constructor parameters and captures

While that might be the case on a technical level, to me they look almost exactly like a field, and in the example presented in the last language design meeting notes, they are actually treated as fields:

class C(int i)
{
    protected int i = i; // References parameter
    public int I => i; // References field
}

So this feature can synthesize fields with the wrong naming convention, and to me they look very much exactly like fields even in the parameter case.

EDIT: Apologies, I didn't read the notes properly, this example is about shadowing fields. Nonetheless, given the striking similarity with record types, my comment on them looking very much like fields still stands.

Such a thing already does exist with records today given the parameters, which are typically camelCase, are synthesized into properties, which are typically TitleCase.

I don't think I would agree this is a good argument for introducing more irregularities. Just because the irregularity existed somewhere else, should not mean we should add more (FWIW: I personally wasn't a fan of how records was done either, though that is kind of outside of the scope of this discussion).

C# already has primary constructor syntax. It just so happens that they are currently (and unnecessarily) tied to the records feature. This enables them to be used for non-record types while seamlessly working with records, allowing "records" to really be a combination of smaller orthogonal language features, as is already the case with other features like deconstruction and init-only properties.

I understand this, and that's why I can see where this idea comes from. But my opinion is that we would revise some of these ideas before we actually get them into C# 12, because the way I see it the current proposal doesn't quite fit. That being said, I am not sure how else it should be done (which makes me believe we may not want this to begin with, hence my comment).

CyrusNajmabadi commented 1 year ago

So this feature can synthesize fields with the wrong naming convention

The fields synthesized have no name as far as the language is concerned. It's like how we synthesize stuff for iterators/async with names that include ...<...>... in it. Those names aren't relevant and have no bearing on naming-conventions. We literally would make them entirely unnamed, except that that would likely confuse a heck of a lot of tools that crack open metadata.

From teh perspective of the language (and those examples) they are not not fields. They are parameters, and only that view should affect things like how 'naming conventions' apply.

I don't think I would agree this is a good argument for introducing more irregularities

I think it is. We tried this experiment with records and found the result very acceptable. Taking such an acceptable result and running with it is fine with us.

But my opinion is that we would revise some of these ideas

Sure. What do you think should be revised? Currently there's nothing with records (that i'm aware of, outside of 'required ) that has been problematic at teh ecosystem level that we're considering revising at all. If you feel differently, now's the time to raise it. Though i'll state that given remarkably little in the way of issues so far, it is probably not likely that we will actually revise here.

vgb1993 commented 1 year ago

I understand this, and that's why I can see where this idea comes from. But my opinion is that we would revise some of these ideas before we actually get them into C# 12, because the way I see it the current proposal doesn't quite fit. That being said, I am not sure how else it should be done (which makes me believe we may not want this to begin with, hence my comment).

I disagree, I think many people want this feature, especially people coming from other languages like Typescript. I just think it's incomplete.

In my case, I have some classes with a big amount of constructor parameters I need to assign to public properties. So I have 3 times the number of lines of code I could have written with this feature.

The fields synthesized have no name as far as the language is concerned. It's like how we synthesize stuff for iterators/async with names that include ...<...>... in it. Those names aren't relevant and have no bearing on naming-conventions. We literally would make them entirely unnamed, except that that would likely confuse a heck of a lot of tools that crack open metadata.

I understand, that is why I think the best way would be to have a keyword to allow devs to specify a different behaviour. The same way the feature upgrades a parameter to a field if necessary, couldn't it upgrade to property if required so? Also with this keyword we could mix dependency injection with properties. I agree this would be a pretty good implementation:

public class MyClass(field int foo, protected property string Bar) { }

Any way, I don't know if it's for a technical reason or for an opinion, but if we could get properties I'd be delighted.

CyrusNajmabadi commented 1 year ago

I understand, that is why I think the best way would be to have a keyword to allow devs to specify a different behaviour.

Sure. That's a purely additive idea that we could consider. It doesn't seem to take away or contradict the core basis of hte feature as defined so far :)

HaloFour commented 1 year ago

@vgb1993

I understand, that is why I think the best way would be to have a keyword to allow devs to specify a different behaviour.

@CyrusNajmabadi

Sure. That's a purely additive idea that we could consider. It doesn't seem to take away or contradict the core basis of hte feature as defined so far :)

And something that could be handled by a source generator in the interim:

public partial class MyClass([Field] int foo, [Prop(Protected)] string Bar) {
}

// generated
public partial class MyClass {
    private int foo = foo;
    protected string Bar { get; set; } = Bar;
}
Waleed-KH commented 1 year ago

Why would we need new keywords (field / property) to allow different behavior, we should be able to do this by combining the parameters with access modifiers (Combined parameter and member declarations):

public class MyClass(
    bool b, // parameter that might generate field (not the same name) if captured
    private int foo, // generate field (same name) as expected
    protected string Bar { get; private set; } // not appetizing .. but why not
) { }
MikaelElkiaer commented 1 year ago

Why would we need new keywords (field / property) to allow different behavior, we should be able to do this by combining the parameters with access modifiers

That's what I was thinking. This is similar to how it can be done in TypeScript: https://www.typescriptlang.org/docs/handbook/2/classes.html#parameter-properties

Waleed-KH commented 1 year ago

The problem with Combined parameter and member declarations that I think it should be resolve before shipping this feature is naming conventions, we should have a syntax to allow us declare different names for the parameter and the declared member, maybe something like the following:

public class MyClass(
    b: bool _b,
    foo: private int _foo,
    bar: protected string Bar { get; private set; }
) { }

var x = new MyClass(b: true, foo: 54, bar: "SS");

I understand that the team is focused on implementing the basic feature, and resolving the problems they are facing, but with consideration of the current cases the devs has (e.g. classes with a big amount of constructor parameters that gets assigned to a mix of properties and fields with logic body ...) and one of the main point for this feature (easily declare and assign members in the constructor) this feature could be amazing and really useful for every case

CyrusNajmabadi commented 1 year ago

The problem with Combined parameter and member declarations that I think it should be resolve before shipping this feature is naming conventions

As the current feature only has parameters, you should follow the naming convention you have decided on for parameters.

We are not doing combined parameter/member. So there's no naming convention ambiguity that has to be resolved.

Zastai commented 1 year ago

The problem with Combined parameter and member declarations that I think it should be resolve before shipping this feature is naming conventions, we should have a syntax to allow us declare different names for the parameter and the declared member, maybe something like the following:

public class MyClass(
  b: bool _b,
  foo: private int _foo,
  bar: protected string Bar { get; private set; }
) { }

var x = new MyClass(b: true, foo: 54, bar: "SS");

I understand that the team is focused on implementing the basic feature, and resolving the problems they are facing, but with consideration of the current cases the devs has (e.g. classes with a big amount of constructor parameters that gets assigned to a mix of properties and fields with logic body ...) and one of the main point for this feature (easily declare and assign members in the constructor) this feature could be amazing and really useful for every case

To me that is significantly less readable than

public class MyClass(bool b, int foo, string bar) {
  private int _foo = foo;
  protected string Bar { get; private set; } = bar;
}

which does the same thing and allows you to use whatever name you like for each parameter/field/property.

Waleed-KH commented 1 year ago

@CyrusNajmabadi

As the current feature only has parameters, you should follow the naming convention you have decided on for parameters.

We are not doing combined parameter/member. So there's no naming convention ambiguity that has to be resolved.

I know, and that's what I'm trying to address here. After reading the proposal I found that there is a ton of limitation in the current state of the feature that make it feel half-baked and not usable in a lot of scenarios, some of which has been addressed for a later stage, but still leaves a big gap and disappoints some expectations.

@Zastai

To me that is significantly less readable than

public class MyClass(bool b, int foo, string bar) {
  private int _foo = foo;
  protected string Bar { get; private set; } = bar;
}

which does the same thing and allows you to use whatever name you like for each parameter/field/property.

This example double declare the desired variable and has the potential of accidently capture the parameter

CyrusNajmabadi commented 1 year ago

I found that there is a ton of limitation in the current state of the feature that make it feel half-baked and not usable in a lot of scenarios, some of which has been addressed for a later stage, but still leaves a big gap

This is all too vague to be actionable. I have no idea what you think is half-baked or unusable.

Waleed-KH commented 1 year ago

For example in a case where I need to have a logical body or different access modifier for the constructor ..., I've already faced cases like this with record primary constructor which forced me to switch to normal constructor (more verbose) to overcome some limitation (which made the feature feel half-baked)

CyrusNajmabadi commented 1 year ago

(which made the feature feel half-baked)

To me that's the idea of the feature. They are opinionated, and not meant to be flexible for all scenarios. If you want that full flexibility, use a class. If you are ok with opinionated rails, the new features are intended for that. This is an intentional design choice here.

o overcome some limitation

That's literally how it's supposed to work :) There are already teh full features with full customization. If you need that customization, then that's what they're there for. THe new features don't want or intend to have that same full customization... because... well, then they'd just be the existing construct. The new stuff is for the 90% (often higher) where the defaults suffice just fine, so it's just more pleasant to have the simple form.

ziaulhasanhamim commented 1 year ago

I don't know why there is more focus on features which add very little value and creates so much confusion. This primary constructor would be very confusing given that records are there.

Also it wouldn't solve problems most times. Many times fields will be readonly. We use auto properties with classes. Using primary constructors means you have to add properties for those fields manually. So most times it would be ignored by most developers - because it covers only one case that if you need a non-readonly field then you can use it. If you need property then you are on your own. This is a nish and narrow feature. Also if it is been added it should be similar to records. Because in records the parameters are regarded as properties. If the parameters are regarded as fields in class it would be a huge confusing thing.

HaloFour commented 1 year ago

I don't know why there is more focus on features which add very little value and creates so much confusion.

Every person ascribes a different value to different features.

Also if it is been added it should be similar to records. Because in records the parameters are regarded as properties. If the parameters are regarded as fields in class it would be a huge confusing thing.

The two features build on each other. You can already declare a record without a primary constructor. Now you can declare a primary constructor without it being a record. It's the composition of the two which provides the behavior of exposing the members publicly. Primary constructors by themselves don't. Nor do they generate fields, as mentioned several times on this thread. They offer a lot more flexibility than records, with none of the additional baggage that comes with records. And, as is already mostly the case, records because just a combination of several smaller features.

ziaulhasanhamim commented 1 year ago

It would add some value to code only and only if it has different syntax for adding readonly fields, non-readonly and readonly properties

HaloFour commented 1 year ago

@ziaulhasanhamim

It would add some value to code only and only if it has different syntax for adding readonly fields, non-readonly and readonly properties

I disagree. It already adds a lot of value to cases where you have a class that accepts dependencies via a constructor, as such a class would never expose those dependency publicly. This eliminates a lot of boilerplate that is common in ASP.NET and other server-oriented code.

As for the addition/synthesis of other members, that remains possible through further iteration. As of this proposal, nothing is generated. The parameters are just constructor parameters, unless captured beyond the scope of the constructor, in which case they are promoted to unspeakable fields (just like with auto-properties).

ziaulhasanhamim commented 1 year ago

It already adds a lot of value to cases where you have a class that accepts dependencies via a constructor

Generally dependencies are readonly for me.

Without addition for properties and readonly fields this feature is very nish and targets a very specific situation.

Also there are so many valuable feature than this. Primary constructor is not something people would miss while they are writing code. It doesn't simply code much. Yeah there will be two lines less in code. But efforts are not worth it for it.

HaloFour commented 1 year ago

Generally dependencies are readonly for me.

And when combined with readonly parameters they would become readonly captures. Or primary constructors could be extended further to allow declaring the parameters as members. For a developer so concerned about their mutability in the interim, that could be prevented by an analyzer.

Also there are so many valuable feature than this. Primary constructor is not something people would miss while they are writing code. It doesn't simply code much. Yeah there will be two lines less in code. But efforts are not worth it for it.

I disagree. Primary constructors have long been a requested feature for C#. Everyone has their own opinion as to what features will bring value. I believe that primary constructors will bring value. They already exist in the language, only bundled with a larger and much more opinionated feature.

JeroMiya commented 1 year ago

I don't know why there is more focus on features which add very little value and creates so much confusion. This primary constructor would be very confusing given that records are there.

The way I think about features like these is that while a single feature like this may not seem that valuable, a collection of features that, overall, improve the ergonomics of the language - that is, reduce boilerplate and so on, can be quite valuable for language adoption and retention in the long run.

Also, keep in mind, almost every new feature of a language often has particular use cases that aren't necessarily relevant to everybody. For example, the top level main statements feature was added a while back - it's not something that helps me particularly, but it was a highly requested feature for developers working on tiny single-file micro-services and cloud functions. If all your code for a cloud function fits on one page, and you have tens or hundreds of functions to define, having to type out all the main class, static main function boilerplate, etc... can be pretty tiresome and a detriment to readability.

Eirenarch commented 1 year ago

@HaloFour

I disagree. It already adds a lot of value to cases where you have a class that accepts dependencies via a constructor, as such a class would never expose those dependency publicly. This eliminates a lot of boilerplate that is common in ASP.NET and other server-oriented code.

It does but I disagree that fields is what should be the default here. I have found that dependencies are better written as a get-only (readonly) properties. This makes it easier to change them to protected in case you need to inherit from the class and turn them back to private when it turns out you don't need them to be protected. It also eliminates the annoying argument about when you prefix a field with this. when using a naming scheme where private fields are not _ prefixed. If primary constructors are introduced in a way where they are equivalent to fields I will use them for DI code because they will make things easier but it would be even better if they defaulted to properties which as it happens aligns with what records do.

ziaulhasanhamim commented 1 year ago

Without having full customizability this feature couldn't help much. So if it is to come it should come with properties and read-only fields. Because fields are mostly read-only and many times you will want a public property to expose.

And I think the default should be properties. If by default the parameters are regarded as fields they would be directly opposite to records. The best way for having a less confusing language is Things that look the same must behave the same. You, someone, see primary constructors for the first time after records they will have the expectation that it will create properties. People don't need to know about different behaviors of the same feature of different types.

So why have dual nature and confuse people?

Waleed-KH commented 1 year ago

In practice I don't think there would be a confusion between primary constructors different behavior in records and classes, because records has a specific use-case (encapsulating data) and based on that the different behavior is understandable in order to match their use-case.

classes on the other hand has a lot of use-cases and different scenarios, and that's why I agree with the point that Primary Constructors for classes should have full customizability (Combined parameter and member declarations, logical body, access modifier, different naming conventions ...) before shipping it.

HaloFour commented 1 year ago

@Eirenarch

It does but I disagree that fields is what should be the default here.

Fields aren't the default, captures are. Nothing about this proposal precludes you from declaring the members as properties if you so choose. Yes, you don't benefit from as much brevity out of the box, but it's expected that a feature like this will be opinionated. This feature is not intended to replace every usage of constructors.

@ziaulhasanhamim

So why have dual nature and confuse people?

Like any new feature it's something that will have to be learned once. I disagree that this will be particularly confusing. It's already expected that records take a normal class definition and amend it with additional features, particularly member synthesis. Being able to combine that with other features is expected. It'd probably be a little easier to grok if primary constructors came first and records were to then build upon them, but this is a bit of a retcon in that way. Kotlin already has this exact same behavior where primary constructor parameters are only promoted to public members with the addition of a data modifier to make the class into a record.

@Waleed-KH

and that's why I agree with the point that Primary Constructors for classes should have full customizability (Combined parameter and member declarations, logical body, access modifier, different naming conventions ...) before shipping it.

Better to ship and iterate upon a smaller set of language features that can be combined to provide more value than to porkbarrel everything into a single feature that will likely never ship at all. There's nothing about this proposal that precludes further enhancing the feature to support synthesis of members. Most larger features in C# are made up of smaller features.

OskarKlintrot commented 1 year ago

Better to ship and iterate upon a smaller set of language features that can be combined to provide more value than to porkbarrel everything into a single feature that will likely never ship at all.

This is a bit ot but maybe it would be possible to add to the description of new feature proposals what the feature in turn can enable further down the line? It seems to be a reoccurring critic that feature x doesn't provide much value while in reality feature x enables feature y and z a couple of lang versions down the line. I follow a few issues here and it does get a bit cluttered getting my very personal opinion is that feature x isn't useful and you should ditch it-notifications a couple of times a week. Sorry for adding to the clutter...

Edit: On a whole the discussions is generally interesting to follow, I hope my post didn't come off as too bitter :)

Waleed-KH commented 1 year ago

What I currently find really confusing is that you can declare members with the same name as parameters:

public class C(int i) {
    private int i = i;
    public int I { get; } = i; // i here is the parameter
    public int I2 => i; // i here is the field
}

public class C2(int i) {
    private int _i = i;
    public int I { get; } = i; // i here is the parameter
    public int I2 => i; // i here is the parameter
}

I understand that the compiler can handle it, but with the ability to capture the parameters in the class scope, I think it would be better to not allow declaring members and parameters with the same name to avoid any confusion.

HaloFour commented 1 year ago

@Waleed-KH

I understand that the compiler can handle it, but with the ability to capture the parameters in the class scope, I think it would be better to not allow declaring members and parameters with the same name to avoid any confusion.

Primary constructors already allow this, so it would be a breaking change to disallow, and inconsistent with their existing behavior to differ in the non-record scenario:

public record R(int i) {
    private int i = i;
}
Richiban commented 1 year ago

@ Eirenarch

have found that dependencies are better written as a get-only (readonly) properties. This makes it easier to change them to protected in case you need to inherit from the class and turn them back to private when it turns out you don't need them to be protected

Why can't you use protected (readonly) fields? What is enabled by making them properties?

Robert-M-Lucas commented 1 year ago

For me the biggest quality of life improvement from this feature would be not having to do the tedious

public ClassName(T1 arg1, T2 arg2) {
    this.arg1 = arg1;
    this.arg2 = arg2;
}

However this could be achieved with syntax like

public ClassName(T1 this.arg1, T2 this.arg2);

In either a normal constructor after properties or as a primary constructor

class ClassName(public T1 arg1, private T2 arg2)
Eirenarch commented 1 year ago

@ Eirenarch

have found that dependencies are better written as a get-only (readonly) properties. This makes it easier to change them to protected in case you need to inherit from the class and turn them back to private when it turns out you don't need them to be protected

Why can't you use protected (readonly) fields? What is enabled by making them properties?

In practice it doesn't matter. In theory encapsulation applies to protected members in the same way it applies to public members so we should be using properties. Who knows maybe some day the implementation of the property changes :)

SmikeSix commented 1 year ago

coming from not c# background. it makes most sense that an primary constructor initializes the fields. it should be understandable what it does to the class for all forms. By default it just gives you the fields int age, private string name; if i add an "properties" markup. it also generates the properties to the class.


[ConstructorProperties]
public class MyClass(int age, string name){

}
public class MyClass(public int age = 0, public string name=""){

}

The issue with any complex language is, if it does too much under the hood it will make things worse. Ive seen it with groovy. Keep it simple, self explanatory and short to write. A class is data, that has functional parts on top. If you hide that fact you run into issues, so the feature needs reflect what people know about basic C#.

Even marking it with a optional new keyword like public properties class MyClass()...would be a better since it would explain what this is used for.

And add a:

init {

}

if you want to execute anything after the default constructor.

CyrusNajmabadi commented 1 year ago

The issue with any complex language is, if it does too much under the hood it will make things worse

I agree. Which is why generating fields seems super weird here. You didn't ask for fields. You don't use these pieces of data after construction. So why would fields be created? It's doing too much, and it def is surprising (as well as a potential big perf issue).

SmikeSix commented 1 year ago

The issue with any complex language is, if it does too much under the hood it will make things worse

I agree. Which is why generating fields seems super weird here. You didn't ask for fields. You don't use these pieces of data after construction. So why would fields be created? It's doing too much, and it def is surprising (as well as a potential big perf issue).

hmm yeah maybe. its main purpose is not to have to write the constructor and set the fields everytime. so it can make sense to have it unified.

but i agree it is surprising.

it makes sense for structs though.

vgb1993 commented 1 year ago

Looking at all the coments I'd say the best way would be to let developers decide, if they want fields, properties, or none generated.

CyrusNajmabadi commented 1 year ago

@vgb1993 nothing about the current design precludes that. Indeed, it's very intentional that such things could be added if desirable/necessary in the future.

mrpmorris commented 1 year ago

Observation 1

This document says

Thus, in the following declaration:

class C(int i)
{
    protected int i = i; // references parameter
    public int I => i; // references field
}

The initializer for the field i references the parameter i (as per the first addition), whereas the body of the property I references the field i (since member lookup comes before the second addition).

I'm not keen on the phrase "comes before" here. Does that mean if I were to swap the order of declaration the meaning would change?

class C(int i)
{
    public int I => i; // references parameter
    protected int i = i; // references parameter
}

If this is the case then code will break when someone uses something like CodeMaid and tells it to put public items above protected - I hope that's not the case?

Observation 2

It doesn't seem to give the same flexibility as a normal constructor in how we define and expose the members.

public class Person(Guid id)
{
  public string Salutation { get; set; }
}

What about this as an alternative (or addition)

public class Person(Id)
{
  public Guid Id { get; private set; }
}
Richiban commented 1 year ago

It doesn't seem to give the same flexibility as a normal constructor in how we define and expose the members

The "captures" are available anywhere in the class body, so if you want a public property:


public class Person(Guid id)
{
    public Guid Id { get; set; } = id;
}
HaloFour commented 1 year ago

@mrpmorris

I'm not keen on the phrase "comes before" here. Does that mean if I were to swap the order of declaration the meaning would change?

Based on the implementation currently in the feature branch those two declarations would be identical. SharpLab

I agree that this should be clarified. My understanding is that it depends on when the parameter is referenced in the lifecycle of the class, not the lexical ordering of the members.

It doesn't seem to give the same flexibility as a normal constructor in how we define and expose the members.

It's not supposed to have the same flexibility. It's not intended to replace the existing syntax used to declare constructors or members. It's supposed to be an opinionated form for specific cases. Captures are emitted as fields, but that's an implementation detail, just like captures are with lambdas. However, as you'd notice in your two declarations above, no additional members are emitted to capture the parameter because they don't have to be:

internal class C {
    protected int i;

    public C1(int i) {
        this.i = i;
    }

    public int I { get => i } }
}
Maximys commented 1 year ago

Hello everybody! I agree with @mrpmorris and I think, that primary constructor can create a lot of headache for us. I start working with programming languages about 18 years ago. First programming language, which I had use was Pascal and Object Pascal. Then I start to use C\C++ and already about 13 years I use .NET and C#. Even though Object Pascal and C++ was create a lot of years ago, they does not have primary constructor and I can not remember, that missing primary constructor create some additional work. If anybody want to know feature which I think will be extremely useful for C#, than I can tell, that this feature can be more flexible and rich enums, which we can see in Java.

HaloFour commented 1 year ago

@Maximys

that primary constructor can create a lot of headache for us.

Can you specify why? Primary constructors exist in quite a few languages, including Java. They already exist in C#, except currently tied to positional records.

If anybody want to know feature which I think will be extremely useful for C#, than I can tell, that this feature can be more flexible and rich enums, which we can see in Java.

The C# team is actively exploring that space as well, although more around discriminated unions, which are more powerful.

dotnet/roslyn-analyzers#113 https://github.com/dotnet/csharplang/blob/main/proposals/discriminated-unions.md

7010

7016

The exact flavor of "rich enums" in Java is patented by Oracle.

Maximys commented 1 year ago

@HaloFour , if you want to see list of potential problem, which can be created by primary constructor, then I can provide it:

  1. Collisions. Have you ever fix bug with very large legacy method (2000+ lines of code), which create a lot of local variables with identical names and by mistake your college instead of using the first variable, use the second? I had this bug, really stupid bug and I spend some hours for understand what's wrong;
  2. If I correctly understand, we can access primary constructor variables from any method of our code, that's why we required additional memory for it (for this variables and for property). Just image, you should pass to primary constructor array with 1000000 items of Int32. If my calculation is correct (1000000*32/8=4000000 bytes= 3906,25 kb= 3.8 mb), you will spend 3.8 megabytes additional memory. I think, everybody will agree with me, that it's very high cost.

Additionally, I had just look some popular Java projects on GitHub - A Stylish Music Player and I can not find primary constructor usage in it. My next point is KISS principle and Murphy's law. As I already wrote, primary constructor create collisions and by Murphy's law we will have some bugs, linked with it. Records does not have methods, that's why this problem does not apply to them.

CyrusNajmabadi commented 1 year ago

you will spend 3.8 megabytes additional memory.

You won't spend any more memory. Arrays are not copied. All cost you have is the pointer to the array the field has.

Richiban commented 1 year ago

@Maximys

Records does not have methods, that's why this problem does not apply to them.

Records absolutely can have methods. See https://sharplab.io/#v2:EYLgZgpghgLgrgJwgZwLQAUEEsC2UECeAwgPYB2yMCcAxjCQsgD4ACATAIwCwAULywGYABEhoMAJkPQRG5ABQsOABiEAxLIxgA5KDggAaIYpUBlRGV0QAlLwDevIY6PDjauABt3OvUIC8APiEAEgAiW3VNbwgAXyFbMwQLPWiQgG4HJwzHQSNlIQBxJAgYOSs/QNCACQhPEkMASSFdONUPL0sU9J5ooA, for example.

Collisions. Have you ever fix bug with very large legacy method (2000+ lines of code), which create a lot of local variables with identical names and by mistake your college instead of using the first variable, use the second? I had this bug, really stupid bug and I spend some hours for understand what's wrong;

I don't understand what this has to do with primary constructors. Bad code is bad, and I don't think primary constructors make it any worse (although there might be situations where they make it better; I myself remember fixing a bug in a class with a constructor that only took eight or nine arguments but there was an initialisation mistake that took ages to spot).

I would also add that many other languages (e.g. Kotlin, Scala, F#) have primary constructors and it hasn't proved to be a problem.

If I correctly understand, we can access primary constructor variables from any method of our code, that's why we required additional memory for it (for this variables and for property).

Again, this is a situation that is not made any worse with primary constructors. The constructor argument is only lifted into a field if you actually use it in the body of a method or property; if you only use it to initialise fields / auto-properties then it's just a constructor parameter.

Richiban commented 1 year ago

Devs in a performance-critical domain might wish to invest in a Roslyn Analyzer that allows them to mark a primary constructor parameter with [DoNotLift], or similar, that would give them a compiler error if they try to use it as a field.

I might even argue that this would be a good addition to the BCL, as it would assuage performance concerns users may have when the C# team makes this feature know to the wider community.

cston commented 1 year ago

I'm not keen on the phrase "comes before" here. Does that mean if I were to swap the order of declaration the meaning would change?

Based on the implementation currently in the feature branch those two declarations would be identical.

@mrpmorris, @HaloFour, thanks for catching this. The text of the proposal has been updated in https://github.com/dotnet/csharplang/pull/7036.

idg10 commented 1 year ago

I realise I'm late to this party, since this issue was discussed in LDM-2021-10-27 a year and a half ago, but I only got to looking at this now that it's available in the current VS2022/.NET8 preview 2.0 bits.

I was quite surprised that if you use this as an alternative to a typical 'big-list-o'-field-initializers' style ctor, i.e. replacing this:

public class WithoutPrimaryConstructors
{
    private readonly int id;
    private readonly string displayName;

    public WithoutPrimaryConstructors(int id, string displayName)
    {
        this.id = id;
        this.displayName = displayName;
    }

    public override string ToString() => $"{id}: '{displayName}'";
}```

with this:

```cs
public class WithPrimaryConstructor(int id, string displayName)
{
    public override string ToString() => $"{id++}: '{displayName}'";
}

that the generated fields turn out to be mutable, and there is apparently no way to mark them as readonly.

That LDM notes that this issue "received a large amount of feedback".

In the absence of any feature to make the fields readable I think I'd still use this feature. The reduction in boilerplate and the corresponding removal of opportunities to make certain kinds of mistakes would, for me, outweight the discomfort I feel from knowing that the fields aren't readonly. But I would have preferred use of this feature to be an improvement in all respects, rather than only a net improvement.

So I'd just like to add my voice to that feedback. Within minutes of trying this feature I found myself asking "So how do I make these things readonly?" (And that was having been unaware that this had already been raised a year and a half ago. I just independently had exactly the same response that you got back then.) Given the evidence, it seems likely to be a common reaction.

ViIvanov commented 1 year ago

…the generated fields turn out to be mutable

I would say a little bit more. If a type with a primary constructor is explicitly declared as a readonly struct:

internal readonly struct MyValue(string value) {
  public override string ToString() => value;
}

compiler will add mutable field to this readonly struct. (Not-readonly fields are not allowed for user-defined fields) Not sure, is it was discussed or not.