dotnet / csharplang

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

[Proposal]: Avoid synthesizing parameterless struct constructors #5552

Open cston opened 2 years ago

cston commented 2 years ago

Avoid synthesizing parameterless struct constructors

Summary

Avoid synthesizing a parameterless constructor for struct types; run field initializers from declared constructors only; and report an error if the struct declaration includes field initializers but no constructors.

Motivation

With C#10, if a struct type declaration includes field initializers but no constructors, the compiler will synthesize a parameterless constructor that will run the field initializers.

The compiler does not synthesize a parameterless constructor if there are explicit constructors because, with record struct types, the field initializers are bound in the context of the primary constructor so the synthesized parameterless constructor would need to invoke the primary constructor to run the field initializers. But what values should be passed for the primary constructor arguments?

The fact that a parameterless constructor is synthesized in some cases but not all makes it difficult to understand when new() is equivalent to default, and it means the result of new() can change silently when non-default constructors are added or removed.

// With no constructors
struct S
{
    public int Value = 42;
}

WriteLine(new S().Value); // 42
// After adding non-default constructor
struct S
{
    public int Value = 42;
    public S(string message) { }
}

WriteLine(new S().Value); // 0

If instead, struct constructors are never synthesized, then the behavior is simplified:

Detailed design

Never synthesize a parameterless constructor for struct types.

Report an error if a struct type declaration includes field initializers but no explicit constructors.

// With no constructors
struct S
{
    public int Value = 42; // error: containing type 'S' has no declared constructor
}

WriteLine(new S().Value); // error above
// After adding parameterless constructor
struct S
{
    public int Value = 42;
    public S() { }
}

WriteLine(new S().Value); // 42
// After adding non-default constructor instead
struct S
{
    public int Value = 42;
    public S(string message) { }
}

WriteLine(new S().Value); // 0

Drawbacks

It is a breaking change from C#10 to require an explicit constructor if there are field initializers, but it's not a silent break (an error is reported), and the fix is simple (add an empty parameterless constructor).

Alternatives

Unresolved questions

Design meetings

YairHalberstadt commented 2 years ago

This feels like exactly the opposite of what I'd expect. If expect a parameterless constructor to always be synthesized if a field initialiser is declared

cston commented 2 years ago

If we synthesize a parameterless constructor whenever a struct contains field initializers, we'd need to synthesize a parameterless constructor that calls the primary constructor for record struct types because the field initializers in a record can reference the primary constructor arguments. But it's not clear what values to pass for the primary constructor arguments.

I've added that to the proposal description above.

jnm2 commented 2 years ago

This feels like exactly the opposite of what I'd expect. If expect a parameterless constructor to always be synthesized if a field initialiser is declared

I agree with this but only in cases where no other constructor is declared either explicitly or via record parameters. It seems like it would be nice if it stayed consistent with how classes sometimes synthesize a constructor and how they initialize fields from each declared constructor.

jnm2 commented 2 years ago

This heuristic breaks almost immediately, as we have no way to determining whether a struct from metadata has field initializers.

How awful would it be to encode this fact in metadata?

If there was a hypothetical defaultable value types feature, I'd like to be able to have non-defaultable record structs that have field initializers referencing primary parameters, and not have a parameterless constructor, and have call sites get warnings if default(R) or new R() were used.

MauNguyenVan commented 2 years ago

I think the field should be default value if the field does not assign value, and the field has value if the field assigned value.

// After adding non-default constructor instead
struct S
{
    public int Value;    //does not assign value
    public S(string message) { }
}

WriteLine(new S().Value); // 0

--------------------------------------------------------------
// After adding non-default constructor instead
struct S
{
    public int Value = 42;  //assigned value
    public S(string message) { }
}

WriteLine(new S().Value); //42
cston commented 2 years ago

Decision from LDM today: Never synthesize a parameterless constructor for struct types; report an error if a struct type declaration includes field initializers but no explicit constructors. Notes are forthcoming.

oising commented 2 years ago

Having run into this today completely by accident (I upgraded to VS 2022 preview) and my solution exploded, if I could add my disappointment with this change from a productivity point of view. Unlike this proposal, my solution has many, many structs, each of which has 10 to 15 members. Some of these members need default values, so under 6.0.100 I was delighted to have no ctors full of repetitive initialization code, and now with 6.0.200 I am being forced to initialize every property/field again.

This feels like a step backwards in usability.

The simplified example above of a struct with a single field is misleading when coupled with the noted "fix" of just adding an empty parameterless ctor. In fact, I have to add initalizers for every field/property now again. So I went from a nice, readable, compact view of my structs to again structs that have more effectively useless code in the ctor than elsewhere. Surely there's a better way to do this that will allow the previously pithier syntax? This goes against the less-is-more mantra with the new minimal APIs etc.

re:

This heuristic breaks almost immediately, as we have no way to determining whether a struct from metadata has field initializers.

Could an implicit, incremental code generator do the dirty work here instead of relying on metadata? edit: can generators work on technically "incorrect" code this way?

TL;DR in 6.0.100 I had nice, simple code. In 6.0.200 I have to add hundreds of repetitive lines of code again.

CyrusNajmabadi commented 2 years ago

and now with 6.0.200 I am being forced to initialize every property/field again.

This should not be teh case.

compact view of my structs to again structs that have more effectively useless code in the ctor than elsewhere.

You should not need code in your ctor. Can you clarify what you're doing?

RikkiGibson commented 2 years ago

Started poking at this scenario a bit and I observed what I think is a bug. SharpLab

public struct S
{
    int x = 1;
    int y;

    // should be allowed
    // perhaps `: this()` should be 'this = default;'
    // then run field initializers
    public S(int y) : this()
    {
    }
}

Basically, it feels to me like one of the following should occur:

  1. In the presence of field initializers, the requirement to definitely assign struct fields in constructors should disappear completely. We zero-init the fields at the start of the constructor and then run the field initializers.
  2. User should be able to use the : this() initializer to shut up the definite assignment errors and just assign a zero value to every field that didn't have an initializer. I think this could be done by emitting 'initobj' then emitting all the field initializers. I also feel like runtime should be able to do a dead store removal analysis in this scenario without much work and we shouldn't have to e.g. decide which individual fields to assign default to.
    • If this route were taken I think parameterless ctors should also be able to say this, possibly by including a public S() : default constructor initializer, for example.
oising commented 2 years ago

Hi @CyrusNajmabadi - I've been exchanging with @jaredpar on Twitter and he says this is the case. (i.e. that I must initialize all properties in the ctor) and Rosyln seems to agree with him. Here's my struct:

   [ProtoContract]
    public struct LightState : ITwinBaseState
    {
        public LightState()
        {
        }

        [ProtoIgnore]
        public bool IsDirty { get; set; }

        [ProtoMember(1)]
        public DateTime LastUpdate { get; set; }

        [ProtoMember(2)]
        [Obsolete] public string? ParentDSN { get; set; }

        [ProtoMember(3)]
        public string? Model { get; set; }

        [ProtoMember(4)]
        public LightType Type { get; set; }

        [ProtoMember(5)]
        [Obsolete("Use DeviceState instead.")]
        public bool Activated { get; set; }

        [ProtoMember(6)]
        public int Hue { get; set; }

        [ProtoMember(7)]
        public int Saturation { get; set; }

        [ProtoMember(8)]
        public int Level { get; set; }

        [ProtoMember(9)]
        public int Temperature { get; set; }
        [ProtoMember(10)]
        public LightColorMode LightColorMode { get; set; }

        [ProtoMember(11)] public DeviceState DeviceState { get; set; } = DeviceState.Unknown;

        [ProtoMember(100)]
        public int PriorityLatchValue { get; set; }
    }

compiling this with 6.0.200-preview yields:

C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.IsDirty' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.LastUpdate' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.ParentDSN' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Model' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Type' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Activated' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Hue' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Saturation' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Level' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.Temperature' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.LightColorMode' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]
C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\State\LightState.cs(12,16): error CS0843: Auto-implemented property 'LightState.PriorityLatchValue' must be fully assigned before control is returned to the caller. [C:\p\hilo\DigitalTwin\Hilo.Sys.DigitalTwin.Grains\Hilo.Sys.DigitalTwin.Grains.csproj]

Note the initializer on DeviceState. This compiled fine in 6.0.100 - and this issue clearly states that I can add an empty ctor to fix this, but this ain't the case, and I understand why; the previously synthetic ctor probably emitted a prop/field = default for every uninitialized prop/field. So I'm hoping you guys can get around the ambiguity and allow us all to delete the redundant-looking initializers (again)

CyrusNajmabadi commented 2 years ago

I see. Talking to jared, an available option is simply:

 [ProtoContract]
    public struct LightState : ITwinBaseState
    {
        public LightState()
        {
        }

        [ProtoIgnore]
        public bool IsDirty { get; set; } = default;

        [ProtoMember(1)]
        public DateTime LastUpdate { get; set; } = default;

        [ProtoMember(2)]
        [Obsolete] public string? ParentDSN { get; set; } = default;

        [ProtoMember(3)]
        public string? Model { get; set; } = default;

        [ProtoMember(4)]
        public LightType Type { get; set; } = default;

        [ProtoMember(5)]
        [Obsolete("Use DeviceState instead.")]
        public bool Activated { get; set; } = default;

        [ProtoMember(6)]
        public int Hue { get; set; } = default;

        [ProtoMember(7)]
        public int Saturation { get; set; } = default;

        [ProtoMember(8)]
        public int Level { get; set; } = default;

        [ProtoMember(9)]
        public int Temperature { get; set; } = default;
        [ProtoMember(10)]
        public LightColorMode LightColorMode { get; set; } = default;

        [ProtoMember(11)] public DeviceState DeviceState { get; set; } = DeviceState.Unknown;

        [ProtoMember(100)]
        public int PriorityLatchValue { get; set; } = default;
    }

Which should then make it so that the constructors you do provide do this initialization.

HaloFour commented 2 years ago

Is that a change? I thought structs constructors always required all members to be initialized. Or did the synthesized constructor automatically emit default initializers for all members?

oising commented 2 years ago

Thanks @CyrusNajmabadi -- like I said, I understand this change but it is a step backwards in usability.

oising commented 2 years ago

Is that a change? I thought structs constructors always required all members to be initialized. Or did the synthesized constructor automatically emit default initializers for all members?

My assumption is that the synthesized ctor emitted default initializers.

RikkiGibson commented 2 years ago

I believe it was a bug that @oising's scenario worked at the time. dotnet/roslyn#57870

We can either change the language so that @oising's scenario works again with well-defined behavior, or we can decide what users should do instead in this scenario. These are the options I outlined in https://github.com/dotnet/csharplang/issues/5552#issuecomment-1010460568

oising commented 2 years ago

I believe it was a bug that @oising's scenario worked at the time. dotnet/roslyn#57870

We can either change the language so that @oising's scenario works again with well-defined behavior, or we can decide what users should do instead in this scenario. These are the options I outlined in #5552 (comment)

If it was a bug that let people add initializers to structs without adding a ctor, then I'd imagine quite a lot of people are taking advantage of that bug. This change is going to generate a lot of friction, imho.

CyrusNajmabadi commented 2 years ago

@jaredpar and i discussed this offline. This is definitely a surprise for me, and not something i think we communicated well in LDM. I was under the impression that all members would be initialized in the constructor, not just the ones with initializers. Admittedly, that may have been an assumption on my part. I'm do not recall how @cston phrased it at teh time.

I do think we may want to reconsider what's going on here. It's a tough place though as it's unclear waht the logic of the language should be here.

Part of me wants everything initialized (similar to how classes work). However, for structs that would be extra work done at runtime. We could attempt to initialize anything not explicitly initialized (by an initializer, or statement), but that's certainly getting complex and potentially confusing.

I don't have a good answer here.

CyrusNajmabadi commented 2 years ago

If it was a bug that let people add initializers to structs without adding a ctor,

This is teh serious friction concern i have. We certainly understood and wanted it to be nice to not have a constructor, but get the right behavior (so our implicit constructor did the right thing). It seems super unfortunate that if the user explicitly provides the no-arg constructor that they can't get the same behavior that the compiler used to provide.


Importantly @jaredpar @cston, this wasn't clear to me from the LDM meeting. To me, the burden on the user was simply: "ok, just provide public Foo() { } and you're good".

That was a very minor cost in my mind, and got the user into the good state where things were explicit and working as intended.

Now, it's much more like: "ok, provide public Foo() { /* and initialize everything */ }" which is a far larger burden than i think was clear at the time.

Should we bring on wednesday to talk about this again?

RikkiGibson commented 2 years ago

If it was a bug that let people add initializers to structs without adding a ctor, then I'd imagine quite a lot of people are taking advantage of that bug. This change is going to generate a lot of friction, imho.

The bug was not that an explicit constructor was required to use field initializers. The bug was that we didn't issue an error when some fields were not initialized by the synthesized constructor (i.e. not all fields had initializers).

The decision to require an explicit constructor to use field initializers is not related to definite assignment, and was done due to a separate problem: going from zero explicit constructors to one constructor with parameters made it so new() silently changes from running field initializers to just zeroing everything out. This is the problem outlined in the original description of this issue.

CyrusNajmabadi commented 2 years ago

Tagging @MadsTorgersen as well.

CyrusNajmabadi commented 2 years ago

The decision to require an explicit constructor to use field initializers is not related to definite assignment,

I agree that we made the decision without considering definite-assignment. However, the issue is important in terms of friction/ergonomics here. And it's especially relevant given that we're about to start reporting errors for people while pushing them into this situation.

In other words, i don't view these as orthogonal issues. And it's the combination of these issues which is problematic and which we should at least discuss (even if we land on the same conclusion as before).

CyrusNajmabadi commented 2 years ago

Ugh... @RikkiGibson has shed more light on this, including https://github.com/dotnet/roslyn/issues/57870

This is def not at all how i had thought this feature was expected to work (either at launch, or with teh change chuck is making now).

:(

CyrusNajmabadi commented 2 years ago

Talking with rikki, i find the ergonomics here are enormously unpleasant. You are first forced to have the constructor (which is unpleasant... but fine, we decided that being explicit here is good and i can accept that). However, taht constructor then forces all the state assignment to satisfy definite assignment. Even though that just forces the user to do something we coudl already do.

I think i'd far prefer we do the def-assignment check. Then, if we find any members of 'this' aren't assigned, we just assign them default at the end.

Since we already did the work to know there's a problem, forcing teh user to have to shut us up just seems enormously burdensome. It's also a really unpleasant friction point compared to classes :-/

jaredpar commented 2 years ago

Then, if we find any members of 'this' aren't assigned, we just assign them default at the end.

This was discussed at length in previous LDM meetings along with several other strategies on how to deal with implicit assignment for missing initializers. The issues with how we do and don't handle them are very subtle. After lengthy discussions we made the decision to not mess with struct definite assignment due to the troubles with getting it correct. Instead we decided to postpone altering that to a future release.

My feels on the problem have not changed. It's a very tricky area and one I have no desire to rush decisions on.

Since we already did the work to know there's a problem, forcing teh user to have to shut us up just seems enormously burdensome.

I don't see this as anymore burdensome than before. Customers have always had to initialize all members of a struct in constructors. This situation is no different.

The difference here is that we made a change, based on other problems created, to make parameterless constructors explicit. Had this been the decision from the very start it would've been seen as a simple extension of existing rules. It's only seen as a problem now because we are undoing a decision, which caused other bugs, and re-establishing the existing rules.

oising commented 2 years ago

I don't see this as anymore burdensome than before. Customers have always had to initialize all members of a struct in constructors. This situation is no different.

We didn't need constructors at all in 6.0.100, and now we do -- and all the cruft that goes along with it. That's the problem as I see it. We got something nice, and now it's being taken away and I suspect you're going to hear a lot more about it as people get exposed to this change. I think I've said all I can, and I hope you guys can figure out the Right Thing.

CyrusNajmabadi commented 2 years ago

If this route were taken I think parameterless ctors should also be able to say this, possibly by including a public S() : default constructor initializer, for example.

Or doing this = default @RikkiGibson

In the presence of field initializers, the requirement to definitely assign struct fields in constructors should disappear completely. We zero-init the fields at the start of the constructor and then run the field initializers.

Alternatively, we don't zero-init at the start. We could zero-init, at the end, anything the user didn't assign.

Note: partial assign would be an error. So if you did this:

public S(bool y)
{
    if  (y)
        this.field = 1;
}

this should probably error.

CyrusNajmabadi commented 2 years ago

This was discussed at length in previous LDM meetings along with several other strategies on how to deal with implicit assignment for missing initializers. The issues with how we do and don't handle them are very subtle. After lengthy discussions we made the decision to not mess with struct definite assignment due to the troubles with getting it correct. Instead we decided to postpone altering that to a future release.

Note: my position is that i can agree with independent decisions in isolation. However, the combination of those decisions can still lead to a very undesirable situation that wasn't at all clear when the different discussions happened.

For example, in the latest discussion we mentioned that it was very non-burdensome for the user to just write the struct constructor out. I interpretted that with what everyone was saying that it was sufficient to just have the small wart of public Foo() { }, and that that would put you back in a good state.

It wasn't at all clear to me that the prior decisions impacted this and the user would need to write a ton more here. And i think that our perspective on the "friction" of the proposed "user solution" didn't take that into account fully.

CyrusNajmabadi commented 2 years ago

I don't see this as anymore burdensome than before. Customers have always had to initialize all members of a struct in constructors

in the past that wasn't burdensome though. There were trivial mechanisms in place due to the presence of hte runtime-provided behavior around structs.

In other words, prior to default-struct-constructors and/or field-initializers, this was not burdensome. I presumed (clearly incorrectly) that there was little burden when we added this feature. However, while true for the has-param constructor case. It's the has no param constructor case that is particularly burdened. That's deeply problematic for me as that's the case that has generally worked so well since langauge inception.

In other words, that was always low-pain before. Now, it is high-pain. That saddens me. If we can do something about that, i would like to. If we can't, I can accept it, though i woudl find it very unfortunate.

HaloFour commented 2 years ago

@CyrusNajmabadi

Or doing this = default;

Including that in the ctor is already legal and would overwrite any initialized members, would it not?

I kind of like the idea of a pseudo chained constructor to default() to zero-init all members, after which initializers would then run.

public struct S {
    public int X { get; set; } = 1;
    public int Y { get; set; }

    public S() : default { }
}
jaredpar commented 2 years ago

edit removed comments not helping discussion

We didn't need constructors at all in 6.0.100, and now we do -- and all the cruft that goes along with it.

That behavior is considered a bug in the design that we fixed.

We got something nice, and now it's being taken away and I suspect you're going to hear a lot more about it as people get exposed to this change.

The design also caused confusion and unexpected behavior with other customers. Hence the decision to make it more explicit and remove the ambiguity. This is a decision made by the LDM team, not any particular individual.

I'm sorry that it conflicted with the dependencies you took. However the language has always reserved the right to correct issues we feel are bad bugs or design decisions in patch releases following a new language version. Even if those changes cause breaking changes in the customers that took dependencies on. This enables us to correct decision, based on customer feedback, that we realize are incorrect and ultimately detract from the language.

CyrusNajmabadi commented 2 years ago

Including that in the ctor is already legal and would overwrite any initialized members, would it not?

Bleagh. Good point. :(

CyrusNajmabadi commented 2 years ago

I kind of like the idea of a pseudo chained constructor to default() to zero-init all members, after which initializers would then run.

Yes. That seems like a good solution here that helps out the no-arg constructor in the same way that the other constructors can use this() to depend on it.

CyrusNajmabadi commented 2 years ago

Guys. This tangent isn't helping. Let's keep things on topic on the user experience and the desired direction both the language to go in, as well as what we think we want users to have to deal with as the fallout here.

Thanks! :)

CyrusNajmabadi commented 2 years ago

Ok. Having dicussed this heavily with @RikkiGibson @cston and @jaredpar we're of the opinion that this is the expected fallout at this point in time.

We also recognize that there is definitely a friction zone here where a user who mixes non-init and initialized fields/props in a struct doesn't have a good story with the 'no-param constructor'. There are workarounds, but all are somewhat unpleasant. Workarounds include, but are not necessarily limited to:

  1. adding = default to all fields/auto-props.
  2. having the no-param constructor explicitly initialize these (and likely make the other constructor delegate to that).

This is unfortunate, but will be the status-quo for 17.1. In the meantime, i'm going to open a discussion in this repo (which i will link to once i do), about improvements we should consider here to make things more pleasant for the user.

@oising we apologize that this will be unpleasant for you in the short term. We never like putting people through that, and it is one of those pains of back-compat breaks. I personally hope we can make things better here for you in teh future (though obviously i cannot promise anything).

Thanks for the feedback and bringing this more to the forefront of the discussion.

oising commented 2 years ago

Thank you @CyrusNajmabadi @RikkiGibson @cston and @jaredpar for your open feedback. No apology is needed honestly as I'm only trying to ensure this (perceived) regression isn't a bug. From a user experience perspective, it's still going to break a lot of builds in my opinion, but as Jared has explained, this feature in itself was actually a bug - it's usually the other way around, lol. Just make sure this build-breaking change is well publicized ahead of the beta cycle and I'm sure people will deal with it, as I will.

CyrusNajmabadi commented 2 years ago

Discussion opened over at: https://github.com/dotnet/csharplang/discussions/5635

cston commented 2 years ago

@oising, I've updated the breaking change item to clarify that a struct with field initializers must include an instance constructor and that all fields must be definitely assigned from the constructor or from field initializers (see https://github.com/dotnet/roslyn/pull/58812). Thanks for your feedback.

sakno commented 2 years ago

Well.. I'm glad to see (sarcasm) another breaking change in C# compiler that makes my project in .NET Foundation no longer buildable. This first one was #5157. Two changes for last 6 months.

CyrusNajmabadi commented 2 years ago

As mentioned in the linked issue:

Understood. That's why we're wary to make breaks generally. This met our bar though for overall value provided.

The same held here. In this case, this was extremely important as you could literally end up with corrupt data with the prior emitted code.

Two changes for last 6 months.

That seems about right. I would expect around that pace per year. Ideally less, but it does happen.

sq735 commented 2 years ago

The following fact confuses me a little. Constructors with optional parameters are not considered parameterless constructors. However, this example compiles without error.

// Without compile error: A 'struct' with field initializers must include an explicitly declared constructor.
struct Struct0
{
    public int X = 1;

    public Struct0(int i = 10) { }
}
cston commented 2 years ago

@sq735, struct field initializers are only run from explicitly declared constructors, not from the default parameterless constructor. An error is reported if there are field initializers and no explicitly declared constructors, to indicate that the field initializers are never run. In your example, since there is an explicitly declared constructor, no error is reported.

sq735 commented 2 years ago

@cston Thanks for the answer. I didn't read the [proposal] carefully and did not pay attention to this nuance.

Pragmateek commented 1 year ago

If we synthesize a parameterless constructor whenever a struct contains field initializers, we'd need to synthesize a parameterless constructor that calls the primary constructor for record struct types because the field initializers in a record can reference the primary constructor arguments. But it's not clear what values to pass for the primary constructor arguments.

I've added that to the proposal description above.

Sorry but I don't get this point. If I understand well the issue would be that we'd need to generate a default constructor to put inside the initialization code but we can't generate such a default constructor because it would not have any parameter to provide to the primary constructor.

But why do we need this default constructor in the first place? Indeed we already have a place to put initialization code in: the primary constructor, and this is exactly what is done by the compiler. As an example:

record struct A(int N)
{
    public int n = N;
    public int x = 1;
}

No need to generate an additional constructor to initialize x, the primary constructor already does the job:

.method public hidebysig specialname rtspecialname
          instance void  .ctor(int32 N) cil managed
  {
    ...
    IL_000e:  ldarg.0
    IL_000f:  ldc.i4.1
    IL_0010:  stfld      int32 A::x
    IL_0015:  ret
  } // end of method A::.ctor

I'm obviously missing the point, so could you please provide an example where we'd need a default constructor to ensure the initialization of the fields? Thanks in advance :)