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

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

Open
Tracked by #829
YairHalberstadt opened this issue Jul 25, 2019 · 247 comments
Open
Tracked by #829

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

YairHalberstadt opened this issue Jul 25, 2019 · 247 comments
Assignees
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@YairHalberstadt
Copy link
Contributor

YairHalberstadt commented Jul 25, 2019

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

@YairHalberstadt
Copy link
Contributor Author

YairHalberstadt commented Jul 25, 2019

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

@HaloFour
Copy link
Contributor

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.

@YairHalberstadt
Copy link
Contributor Author

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

@HaloFour
Copy link
Contributor

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

@orthoxerox
Copy link

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.

@HaloFour
Copy link
Contributor

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

@MgSam
Copy link

MgSam commented Jul 26, 2019

  • 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!

@Richiban
Copy link

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

@YairHalberstadt
Copy link
Contributor Author

@Richiban

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

@HaloFour
Copy link
Contributor

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

@brabebhin
Copy link

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.

@orthoxerox
Copy link

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

@Richiban
Copy link

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

@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

@brabebhin
Copy link

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.

@gulshan
Copy link

gulshan commented Jul 26, 2019

I would like to have the captured values as readonly.

@Richiban
Copy link

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

@HaloFour
Copy link
Contributor

HaloFour commented Jul 26, 2019

@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

@Richiban
Copy link

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

@HaloFour
Copy link
Contributor

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

@MgSam
Copy link

MgSam commented Jul 26, 2019

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.

@CyrusNajmabadi
Copy link
Member

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?").

@MgSam
Copy link

MgSam commented Jul 26, 2019

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.

@CyrusNajmabadi
Copy link
Member

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.

@notanaverageman
Copy link

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

@qrli
Copy link

qrli commented Jul 29, 2019

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

@Richiban
Copy link

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

@notanaverageman
Copy link

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.

@idg10
Copy link

idg10 commented Jul 19, 2023

TLDR version: the current Primary Constructors design enables situations where a variable which does not exist yet (according to section §9.2.5 of the C# spec) can be in scope. I don't believe the existing language spec defines what the behaviour is for this scenario because, as far as I know, it has never been possible before. That is the core of the problem I'm trying to describe here.

Full version:

Note: we're not opposed to normative, explanatory, text sections in the spec. Often to just clear up any potential confusion. It's optional, but can be nice to have. We would probably consider taking such a normative spec addition if you wanted to make it.

In the spirit of trying to explain why I think there's a problem with the normative text as it stands, I'm going to show a non-normative explanation that does not contradict either any existing specs, or the new feature spec in its current form, and which I'm confident you will agree is definitely not what you intended.

Furthermore I'd argue that what I'm about to show is more consistent with the existing language behaviour than the actual implementation. In particular, section §9.2.5 states that a value parameter:

comes into existence upon invocation of the function member (method, instance constructor, accessor, or operator)

So according to the existing specification for C#, the input parameters to a primary constructor do not exist before the constructor is invoked. So if the constructor is never invoked, its parameters never exist.

That's the heart of the matter. The current Primary Constructors design creates a situation where a variable can be used before it "comes into existence". This is new, and I believe it's a problem, because nothing describes what this new situation means.

So with that in mind, here's some normative explanatory text explaining the consequences of this fact in the context of a struct with a primary constructor:


Using captured primary constructor arguments when the constructor did not run

MyStruct has a primary constructor and captures one of its constructor arguments:

struct MyStruct(int input)
{
    public int Get() => input;
}

It has always been possible to create a new instance of a struct in such a way that none of the struct's constructors are executed. For example:

var mss = new MyStruct[1];
MyStruct notConstructed = mss[0];

What should the following code do?

Console.WriteLine(notConstructed.Get());

The Get method refers to input, which captures the argument passed to the primary constructor. However, since MyStruct is a struct, there is no guarantee that the primary constructor will have run (and it definitely won't have in this case). Section §9.2.5 tells us that the constructor's input parameter "comes into existence upon invocation of the" constructor. Since the constructor was never invoked, the input parameter never existed.

Executing the Get method in this context constitutes an attempt to use a variable that does not exist. Consequently, this causes a VariableDoesNotExistException to be thrown.

(End of hypothetical non-normative explanation)

I'm claiming that this is more consistent with §9.2.5 than the current Primary Constructor preview implementation, because it has never previously been possible to use a variable before it comes into existence, and the explanation above continues to prevent that. But I would concede that the behaviour here is in fact undefined, making the preview implementation no better or worse than this suggestion when it comes to compliance with the spec. (I think the preview implementation's behaviour is, in practice, a lot better than this suggestion. My issue is not with the implementation, it's with the spec.)

I'm not entirely sure that this shouldn't in fact fall foul of the definite assignment rules, and instead be rejected at compile time with an error of this kind:

Use of unassigned parameter 'input'

That would make more sense: since the variable doesn't necessarily exist yet (despite being in scope) when Get runs, it can't possibly have been assigned. And it's certainly consistent with normal definite assignment handling: if it's possible to reach a line of code without having initialized a variable, the definite assignment rules normally won't let you read from that variable.

However, as far as I can tell, the existing specs around definite assignment make the assumption that if a variable is in scope at a particular point in the code, it must necessarily exist. (That is a valid assumption for C# 11.0, I believe. The primary constructor spec invalidates that assumption.) The rules don't explicitly handle the case where you try to use a variable that is in scope but doesn't exist yet, and I don't see an unambiguous reading of those rules that would resolve this. It does say "An initially assigned variable is always considered definitely assigned" and the constructor arguments of this kind are initially assigned, so you can argue that this doesn't fail definite assignment rules. You could equally argue that an "initially assigned" variable is obviously assigned its value when it comes into existence, and is therefore unassigned upon entry to Get in this example, so an initially assigned variable should be considered unassigned prior to coming into existence.

The feature of Primary Constructors that is novel (and not covered by the current language specs) that enables this to go wrong is that it allows a variable to be in scope in contexts where, according to §9.2.5, the variable does not actually exist. (This is a new problem because this is the first time that a constructor argument—hitherto scoped to the inside of that method—is in scope outside of its defining member. So appeals to existing language behaviour don't help. The proposed Primary Constructors design changes things in a way that creates this problem.)

I think the fundamental problem here is that the current Primary Constructors spec just doesn't even consider the possibility that the primary constructor might not run (but it needs to because this creates a novel situation in which code can execute that attempts to use a variable which, according to §9.2.5, does not exist). It says this:

...it is captured into the state of the enclosing type, so that it remains accessible after the termination of the constructor

That "termination of the constructor" wording presumes the constructor will run. There's nothing saying what happens when the constructor doesn't run, and that's what leaves space for different implementations to have different observable results. (Unless you think §9.2.5 means that the current preview implementation definitely violates the language specification. At any rate, I don't think anything anywhere defines what happens when you use a variable that doesn't exist yet. So I stand by my claim that there is undefined behaviour here.)

As long as it isn't observable, it doesn't matter. So the internal single-field approach would have been possible. But we would then have to emit everything else such that you couldn't know if it was done that way, and so that default versions of the struct still worked.

But it IS observable. Depending on how you interpret the consequences of §9.2.5, we've got 2 behaviours which produce quite different observable behaviour. There has never before been a way to use a variable before it existed, so this is new ground.

You've said that the single-field approach would have to be implemented to produce the same behaviour as the existing multi-field implementation. But since the existing language specs don't define what it means to use a variable before it exists, I could equally say that the multi-field approach should emit everything in such a way that the behaviour is exactly the same as described in the non-normative text above (i.e., throws an exception when you try to use a captured argument when no argument was ever supplied). Since this novel situation that primary constructors create—the ability to use a variable before it exists—does not have behaviour defined by the existing specs, there's no way to say which is right.

@idg10
Copy link

idg10 commented Jul 19, 2023

@Richiban said:

Apologies if I'm telling you something you already know, but in public double X => x; since you've used the parameter as a field it will actually be lifted into a field. Once that happens the normal rules around fields and structs apply.

That's how the preview implementation works. If you take the implementation to be the definition of the feature, then it's all perfectly comprehensible. But the problem is that this is not required by the specs. The specs say that in this situation:

it is captured into the state of the enclosing type, so that it remains accessible after the termination of the constructor

This says nothing at all about how that capture occurs, so we can't just presume that captured parameters will correspond directly to fields. And also, this sentence clearly presumes that the constructor is going to run, and in this case it doesn't.

Now if the spec just said that these things are fields, that would be an end of it. But currently it doesn't say that, and by inspection they are not fields from the perspective of code using these primary ctor parameters. When an instance member refers to a some primary constructor parameter x, It's illegal to write this.x, something which would be allowed if x were a field. If you hover the mouse over x in the body of an instance member, Visual Studio describes it as a parameter, not a field.

And that's why there's a problem. The feature as currently designed has it that these things aren't fields. They might be implemented as fields but they don't have to be. The design says this:

if they are captured by a function member or anonymous function, they are appropriately stored (e.g. as unspeakable private fields of the declared class or struct).

Note the "e.g.". If it just said that they will be stored like that (and, more specifically, if it effectively rules out an alternative in which the captured fields all live in some separate object that the struct has a reference) then the problems I'm describing go away. But this also then imposes constraints on the implementation, which I believe they were trying to avoid. (Back when this feature was first put to the LDM in November 2021, it was in fact designed in terms of fields, as far as I know. There was a note indicating that they were considering changing it to be a capture-based design more like F#. And then in the proposal that cropped up a year later, it was all specified in terms of capture, not fields.)

This is why I was suggesting that the Primary Constructors spec should just define how Primary Constructor parameters are initialized in the case where the constructor is not actually invoked. If it defined that, then it removes the loophole in which you can access a variable before it exists, and the spec could continue to be non-committal about exactly how this is to be achieved. That would, I think, be better than saying it has to be implemented a particular way.

I think a factor contributing to the disagreements here is that the primary constructors design doc does drift a bit from start to finish. It begins with this language that talks about capture and tries to avoid specific implementation details. I've taken that seriously, and it's what informs the position I've taken. But it's true that by the time we get into the examples in the semantics section it has more or less given up on its initial implementation-agnostic stance, and is describing a very specific process of emitting one field per captured parameter. I took that to be a non-normative illustration, but if that's actually meant to be normative, then the earlier text in the spec needs to be made consistent with that. At the moment, the start of the spec tells us that we can't presume any particular relationship between capture of parameters and the storage mechanisms used to achieve this. But if we are in fact required to presume a particular relationship there in order to correctly predict how the feature works, then the initial content of the spec needs to be changed, because right now it strongly suggests that we're not supposed to presume that these things are basically fields.

@idg10
Copy link

idg10 commented Jul 19, 2023

@HaloFour

It's true that any captured parameters would have their default value, but beyond that I don't think it's helpful to think of the primary constructor being called with default values as no initializers that rely on those parameters are going to run,

I'm not committed to a specific resolution of this problem, so I'd be open to alternatives. But the basic problem is that according to Section §9.2.5 of the C# language spec, these parameters do not exist if the constructor has never run.

They come into existence when and only when the constructor runs, because that's how the C# spec says constructor parameters work.

This was my reason for suggesting that the spec should describe the case where the constructor doesn't actually run in terms of what would have happened if it did run.

I agree that this creates potential confusion around initializers. So what is the alternative? My concern is that it might entail significant modifications elsewhere in the C# language spec. The basic problem is that in this struct primary initializer scenario, you end up with parameters being in scope even though they don't actually exist according to §9.2.5. So do we modify the language spec to accommodate this possibility of using non-existent variables?

It seemed less likely to me to cause trouble if the Primary Constructor spec simply asserted that they did exist in this situation, and explained how they were initialized. Currently the spec considers these things to be parameters, and if it wants to continue to do that, then unless it's prepared to change C#'s definition of what a parameter is, it must define behaviour in terms of a constructor being executed because that is, according to §9.2.5, the only way for these things to come into existence.

So I suppose the alternative is for the Primary Constructor spec to define these things as something else. But what? I discuss the pros and cons of using fields in my previous post, but remember that an earlier draft of this feature did exactly that, and they chose to move away from that. I think what you're left with after that is inventing a whole new kind of language element! (I think that would be a bad idea.) I believe the advantage of characterising it as a constructor parameter with extended scope is that they get to reuse all of the existing spec work defining what a constructor parameter is. The one wrinkle is that you're left with this problem that you can use the thing before it exists.

So I still think that the best bet is likely to be some sort of workaround in which the Primary Ctors spec somehow just asserts that the things do come into existence in this "no constructors were invoked in the creation of this value" scenario. But you've got to be careful not to introduce contradictions into the spec. You really want to reuse something that's already in the spec (which is the attraction of trying to define it in terms of actually invoking the constructor).

@CyrusNajmabadi
Copy link
Member

I'm fine simply stating that a struct's primary constructor parameters are always in existence. And for the case of a default instance of the struct they have the default value for their respective types. Seems like a trivial explanatory line for anyone potentially confused here.

@idg10
Copy link

idg10 commented Jul 19, 2023

OK, so if I submitted a PR along those lines you would consider it?

@CyrusNajmabadi
Copy link
Member

Would def consider it. Would leave it to @MadsTorgersen @BillWagner for final determination. But given something so simple, i don't foresee a problem :)

@KathleenDollard
Copy link
Collaborator

ViIvanov

Hi, what the best way to report about typos and other little issues in the documentation?

Make a comment on the bottom of the docs page. They are reviewed. Offer a fix if you have one, they are also reviewed - normal commit/PR path and the buttons take you through the process super easily.

@IanKemp
Copy link

IanKemp commented Jul 21, 2023

@333fred
Copy link
Member

333fred commented Jul 21, 2023

Thanks Ian, updated.

@KathleenDollard
Copy link
Collaborator

@333fred opened an issue on the readonly default here: #7377

@egvijayanand
Copy link

Unable to add a breakpoint while using this Primary Constructors feature. Using the latest preview version of VS2022. Kindly coordinate internally with the IDE team to get it addressed.

https://developercommunity.visualstudio.com/t/Unable-to-add-a-breakpoint-at-the-Type-d/10421141

@CyrusNajmabadi
Copy link
Member

@egvijayanand Please file issue at dotnet/roslyn. Thanks :)

@egvijayanand
Copy link

Please file issue at dotnet/roslyn.

Sure, will do it.

@egvijayanand
Copy link

Seems like there's already an open issue for this.

dotnet/roslyn#67796

@jcouv jcouv modified the milestones: Working Set, 12.0 Aug 8, 2023
@jcouv jcouv added the Proposal label Aug 8, 2023
@egvijayanand
Copy link

egvijayanand commented Nov 10, 2023

How to invoke a method like InitializeComponent() which takes no parameters and returns nothing (this does initialize some UI components) in the context of Primary Constructor?

In general, the question here is how to do something more with a PC.

Similar to partial methods, define a construct that gets hooked up in the PC body during auto code gen.

@HaloFour
Copy link
Contributor

@egvijayanand

How to invoke a method like InitializeComponent() which takes no parameters and returns nothing (this does initialize some UI components) in the context of Primary Constructor?

As of now the primary constructor cannot have a body or execute arbitrary code outside of member initializers.

If that is a use case you'd like to see supported in the future, I'd recommend commenting here: #7667

@egvijayanand
Copy link

If that is a use case you'd like to see supported in the future, I'd recommend commenting here: #7667

Sure will do it.

@jcouv jcouv added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Sep 17, 2024
@jcouv jcouv changed the title Primary Constructors Primary Constructors (VS 17.6, .NET 8) Sep 17, 2024
@github-lernender-corp
Copy link

github-lernender-corp commented Sep 30, 2024

USE CASE:

public class Tag(string name, string value, string? description = null) : Item(name, description), ITag
{
  public required string Value { get; set; } = value;
 .....
}

...

new Tag("Name", "Subscription")
      --> Error occurs "Required member [TAG.Value] must be set in the object initializer..."

Two issues:

(1) No body supplied for the primary constructor!!!! What genius thought this was a good idea!!!
(2) I should be able to both set a required field AND define primary constructor! This code will not compile. What's the point in having a "required" keyword if I am not allowed to satisfy the required field via the primary constructor? Morever, this annotation [SetsRequiredMembers] seems to be an afterthought.

In general, this whole attempt to shoehorn a “primary constructor” into the language has not been thought through and is incomplete at best.

I my opinion, new isn’t always better. In this case introducing a half-baked idea all for the sake of brevity is just plain stupid.

@CyrusNajmabadi
Copy link
Member

@github-lernender-corp

new Tag("Name", "Subscription")

You're missing hte 'value' parameter, which your constructor says is mandatory.

I should be able to both set a required field AND define primary constructor!

You can have a normal constructor in the type which doesn't require the parameter.

What's the point in having a "required" keyword if I am not allowed to satisfy the required field via the primary constructor?

required is stating that the caller must supply it as a named property. If you have it in the constructor, it's already 'required' as you cannot skip non-optional parameters.

@CyrusNajmabadi
Copy link
Member

(1) No body supplied for the primary constructor!!!!

Bodies for primary constructors are tracked with separate issues. We often like to see how important something is before adding to the language. So far, there's been a tiny bit of community interest in this, but not much. So not including this seems like the correct decision. We can revisit based on need as time goes on.

@DavidArno
Copy link

How do you measure that community interest, @CyrusNajmabadi? It's a occasional source of frustration for me, but it happens often enough for me to want that feature. It's pretty much the only thing that I see requested in these discussions for primary constructors. I'm surprised that it's not seen as an important thing to add.

@DavidArno
Copy link

@github-lernender-corp,

(2) I should be able to both set a required field AND define primary constructor!

For what reason? You only have the primary constructor, which sets that field. What use would required be here? If you want two constructors, one of which doesn't set it, then it's the PC that takes that role:

public class Tag(string name)
{
    [SetsRequiredMembers]
    public Tag(string name, string value) : this(name) => Value = value;
    
    public required string Value { get; set; }
}

new Tag("Name", "Subscription"); // works just fine.

What is really required here - independent of PCs - is for the compiler to be able to work out that the non-primary constructor sets Value and therefore meets the required constraint, removing the need for the [SetsRequiredMembers] "hack".

@CyrusNajmabadi
Copy link
Member

How do you measure that community interest

All our signals that come in. That includes things like self selected groups like github, but lots of customer outreach from our product teams, and tons of communications with all the companies, teams and individuals we work with.

It's pretty much the only thing that I see requested in these discussions for primary constructors. I'm surprised that it's not seen as an important thing to add.

I see barely any signals on this topic. And most of it is "yeah... would be nice. but can live without it". Given how paltry the feedback is on this, and how much it is on other topics, it's unsurprisingly not making the cut so far. That may change as time goes on.

@DavidArno
Copy link

That makes sense. It's an irritation at times but is indeed sits in the "yeah... would be nice. but can live without it" space for me. Maybe one day though 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests