Bug 21349 – copy and postblit constructors aren't compatible

Status
RESOLVED
Resolution
INVALID
Severity
blocker
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2020-10-29T15:56:10Z
Last change time
2020-11-17T01:52:05Z
Assigned to
No Owner
Creator
Илья Ярошенко

Comments

Comment #0 by ilyayaroshenko — 2020-10-29T15:56:10Z
struct SNew { int s; this(ref return scope inout SNew rhs) inout { this.s = rhs.s + 1; pragma(inline, false); } } struct SOld { int s; this(this) { this.s++; pragma(inline, false); } } struct C { SOld sOld; SNew sNew; this(this) { pragma(inline, false); } } void main() { C c; auto d = c; assert(d.sOld.s); assert(d.sNew.s); }
Comment #1 by ilyayaroshenko — 2020-10-29T16:14:58Z
The last assert fails
Comment #2 by snarwin+bugzilla — 2020-10-29T16:59:20Z
This is an enhancement request, not a bug. Per the language spec: > For backward compatibility reasons, a struct that defines both a copy > constructor and a postblit will only use the postblit for implicit copying. Source: https://dlang.org/spec/struct.html#struct-postblit You can work around this by placing sOld in a union, since unions do not have generated postblits: struct C { union { SOld sOld; } SNew sNew; this(ref C other) { pragma(inline, false); sOld = other.sOld; sNew = other.sNew; } } Of course, if SOld has any other special member functions, such as a destructor or identity opAssign overload, you will have to implement those manually for C as well.
Comment #3 by ilyayaroshenko — 2020-10-29T17:04:51Z
well. The language spec is buggy.
Comment #4 by ilyayaroshenko — 2020-10-29T17:09:06Z
btw, I don't see whare spec says that an aggregate can't hold both old and new style members struct C { SOld sOld; SNew sNew; } void main() { C c; auto d = c; assert(d.sOld.s); assert(d.sNew.s); } This is a buggy definition too and doesn't pass second assert too. The spec doesn't say that this shouldn't work. It just ignores this case. But this is definitely the language design bug.
Comment #5 by snarwin+bugzilla — 2020-10-29T17:16:16Z
When an aggregate has both old and new style members, the compiler generates both a copy constructor and a postblit, and the postblit takes precedence. I agree that this is a bug in the language design: the generated copy constructor will call the members' postblits, so it should be the one that takes precedence.
Comment #6 by ilyayaroshenko — 2020-10-29T17:25:29Z
I have thought think about these lines > For backward compatibility reasons, a struct that defines both a copy > constructor and a postblit will only use the postblit for implicit copying. What they are really about? First, they say about "struct that defines both ", but in the second case the struct doesn't define neither. The compiler generates something, and we can suppose that compiler can generate something good. For example a new style copy constructor. And it should generate it instead of postblit. Why? because the second: Second, "For backward compatibility reasons". The reason is backward compatibility, not something else. I mean that "struct that defines both" should be understood as "struct that _explicilty_ defines both". So, even with the current spec, this can work, and if it can work, then it should work.
Comment #7 by razvan.nitu1305 — 2020-11-11T02:16:54Z
(In reply to Илья Ярошенко from comment #6) > I have thought think about these lines > > > For backward compatibility reasons, a struct that defines both a copy > > constructor and a postblit will only use the postblit for implicit copying. > > What they are really about? > > First, they say about "struct that defines both ", but in the second case > the struct doesn't define neither. The compiler generates something, and we > can suppose that compiler can generate something good. For example a new > style copy constructor. And it should generate it instead of postblit. Why? > because the second: > > Second, "For backward compatibility reasons". The reason is backward > compatibility, not something else. > > I mean that "struct that defines both" should be understood as "struct that > _explicilty_ defines both". > > So, even with the current spec, this can work, and if it can work, then it > should work. I agree that the spec might be misleading, however, the DIP has a more exact formulation [1]: "In order to assure a smooth transition from postblit to copy constructor, the following strategy is employed: if a `struct` defines a postblit (user-defined or generated) all copy constructor definitions will be ignored for that particular `struct` and the postblit will be preferred. Existing code bases that do not use the postblit may start using the copy constructor without any problems, while codebases that rely on the postblit may start writing new code using the copy constructor and remove the deprecated postblit from their code." So from the beginning you must either use one or other; having both in your code will not fly. In this specific example, you have 2 solutions: either you replace the postblit with a copy constructor everywhere (which is the preferred solution) or you simply switch C to a copy constructor and later on update the code base to get rid of postblits. [1] https://github.com/dlang/DIPs/pull/129/files#diff-ecee0474c4314cd47dd8c2656b485c0cfd56e704a85de75839ec2850fb61f0ebR282
Comment #8 by razvan.nitu1305 — 2020-11-11T02:18:37Z
(In reply to Paul Backus from comment #5) > When an aggregate has both old and new style members, the compiler generates > both a copy constructor and a postblit, and the postblit takes precedence. > > I agree that this is a bug in the language design: the generated copy > constructor will call the members' postblits, so it should be the one that > takes precedence. No, it does not generate both. It first looks for user defined postblits in the struct or in its fields and if it finds any, it will ignore user defined copy constructors. So in this case, C will not have a copy constructor because what the user wants takes precedence over generated methods.
Comment #9 by razvan.nitu1305 — 2020-11-11T02:28:26Z
(In reply to Илья Ярошенко from comment #4) > btw, I don't see whare spec says that an aggregate can't hold both old and > new style members > > struct C > { > SOld sOld; > SNew sNew; > } > > void main() > { > C c; > auto d = c; > assert(d.sOld.s); > assert(d.sNew.s); > } > > This is a buggy definition too and doesn't pass second assert too. > > The spec doesn't say that this shouldn't work. > It just ignores this case. But this is definitely the language design bug. It does not say that explicitly, however, the DIP specifies that if you have a postblit (user-defined or generated) then copy constructors will be ignored. In your case, one of the fields has a postblit and therefore struct C will have a postblit generated for it as per the rules in [1]. Now the rules stated in the copy constructor DIP enter the scene and therefore a copy constructor is not going to get generated. It is not a bug, it a language design choice to favor backwards compatibility so that codebases that heavily rely on the postblit will not be affected by the insertion of new structs that define copy constructors. [1] https://dlang.org/spec/struct.html#struct-postblit
Comment #10 by razvan.nitu1305 — 2020-11-11T02:32:57Z
(In reply to Paul Backus from comment #2) > This is an enhancement request, not a bug. Per the language spec: > > > For backward compatibility reasons, a struct that defines both a copy > > constructor and a postblit will only use the postblit for implicit copying. > > Source: https://dlang.org/spec/struct.html#struct-postblit > > You can work around this by placing sOld in a union, since unions do not > have generated postblits: > > struct C > { > union { SOld sOld; } > SNew sNew; > > this(ref C other) { > pragma(inline, false); > sOld = other.sOld; > sNew = other.sNew; > } > } > > Of course, if SOld has any other special member functions, such as a > destructor or identity opAssign overload, you will have to implement those > manually for C as well. A better solution would be to simply disable the postblit in C and everything will work as you expect it.
Comment #11 by razvan.nitu1305 — 2020-11-11T02:36:01Z
I am inclined to close this as INVALID as the issue is easy to fix by disabling the postblit in C, however, I am going to wait for further objections.