-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal]: Avoid synthesizing parameterless struct constructors #5552
Comments
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 |
If we synthesize a parameterless constructor whenever a I've added that to the proposal description above. |
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. |
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 |
I think the field should be default value if the field does not assign value,
|
Decision from LDM today: Never synthesize a parameterless constructor for |
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:
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. |
This should not be teh case.
You should not need code in your ctor. Can you clarify what you're doing? |
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:
|
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:
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) |
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. |
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? |
Thanks @CyrusNajmabadi -- like I said, I understand this change but it is a step backwards in usability. |
My assumption is that the synthesized ctor emitted default initializers. |
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. |
@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. |
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 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 Should we bring on wednesday to talk about this again? |
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 |
Tagging @MadsTorgersen as well. |
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). |
Ugh... @RikkiGibson has shed more light on this, including dotnet/roslyn#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). :( |
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 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 :-/ |
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.
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. |
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. |
Or doing
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:
this should probably error. |
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 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. |
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. |
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 public struct S {
public int X { get; set; } = 1;
public int Y { get; set; }
public S() : default { }
} |
edit removed comments not helping discussion
That behavior is considered a bug in the design that we fixed.
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. |
Bleagh. Good point. :( |
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 |
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! :) |
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:
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. |
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. |
Discussion opened over at: #5635 |
@oising, I've updated the breaking change item to clarify that a |
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. |
As mentioned in the linked issue:
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.
That seems about right. I would expect around that pace per year. Ideally less, but it does happen. |
The following fact confuses me a little. Constructors with optional parameters are not considered parameterless constructors. However, this example compiles without error.
|
@sq735, |
@cston Thanks for the answer. I didn't read the [proposal] carefully and did not pay attention to this nuance. |
Update volatile process program code to avoid CS8983 (A 'struct' with field initializers must include an explicitly declared constructor) After the changes in C# discussed at dotnet/csharplang#5552 For the upstream changes, see dotnet/roslyn#58581 For discussion of the change, also see dotnet/sdk#23971
Sorry but I don't get this point. But why do we need this default constructor in the first place?
No need to generate an additional constructor to initialize
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? |
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 todefault
, and it means the result ofnew()
can change silently when non-default constructors are added or removed.If instead,
struct
constructors are never synthesized, then the behavior is simplified:struct
type declares a parameterless constructor,new()
is equivalent todefault
, consistent with C#9 and before.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.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
The text was updated successfully, but these errors were encountered: