Bug 20876 – generated constructor always inout regardless of ability of fields to use inout

Created attachment 1793 test.d Compiles without error with dmd-2.085. With the dmd nightly build I get: $ dmd -c test.d /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(208): Error: none of the overloads of opSliceAssign are callable using argument types (Foo, ulong, ulong), candidates are: /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(724): std.container.array.Array!(Foo).Array.opSliceAssign(Foo value) /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(731): std.container.array.Array!(Foo).Array.opSliceAssign(Foo value, ulong i, ulong j) /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(215): Error: none of the overloads of opSliceAssign are callable using argument types (Foo, ulong, ulong), candidates are: /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(724): std.container.array.Array!(Foo).Array.opSliceAssign(Foo value) /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(731): std.container.array.Array!(Foo).Array.opSliceAssign(Foo value, ulong i, ulong j) /tmp/dmd2/linux/bin64/../../src/phobos/std/container/array.d(529): Error: template instance std.container.array.RangeT!(Array!(Foo)) error instantiating test.d(3): instantiated from here: Array!(Foo)

Digger blames the introduction of copy constructors: commit f9cbf699c0fa07892137d477fbe1ea197c8d0cbe Author: Andrei Alexandrescu <[email protected]> Date: Wed Mar 27 21:21:33 2019 -0400 dmd: Merge pull request #8688 from RazvanN7/CpCtor https://github.com/dlang/dmd/pull/8688 Implement copy constructor

(In reply to moonlightsentinel from comment #1) > Digger blames the introduction of copy constructors: Yes; it works with postblit and a by-value copy ctor, but doesn't with the used by-ref copy ctor.

Reduced test case: struct Array { void opSliceAssign(Foo) {} void opSliceAssign(Foo, size_t, size_t) {} } struct Foo { Bar _bar; } struct Bar { version (Bug) this(ref Bar) { } else this(Bar) { } } void main() { Foo foo; Array arr; arr[] = foo; }

(In reply to moonlightsentinel from comment #3) > Reduced test case: > > struct Array > { > void opSliceAssign(Foo) {} > void opSliceAssign(Foo, size_t, size_t) {} > } > > struct Foo { > Bar _bar; > } > > struct Bar { > version (Bug) > this(ref Bar) { } > else > this(Bar) { } > } > > void main() > { > Foo foo; > Array arr; > arr[] = foo; > } The fundamental problem is that when the compiler sees a struct that has a field that defines a copy constructor it generates an inout copy construct that has the form: this(ref inout(T)) inout; In this specific situation it creates such a copy constructor for Foo: this(ref inout(Foo) p) inout { this._bar = p._bar; } This generated copy constructor fails to compile because `this._bar = p._bar` gets rewritten to `this._bar.__copyCtor(p._bar)` and this failst because `this(ref Bar)` cannot be called with argument types `(ref inout(Bar)) inout`. Therefore it will be annotated with disable. When I implemented the copy constructor I initially proposed this scheme having in mind that if you want to upgrade your postblits to copy constructors, then you can simply replace `this(this)` with `this(ref inout typeof(this) p) inout`. That way the generation scheme for postblits would work kind of the same and the code would not be affected. However, pondering upon it some more I thought of a superior generation scheme [1]. The latter would make this error go away and the code would compile, however Walter opposed it so now we are stuck with inout constructors. So, to conclude, by the current language rules the error is correct. The fix in this situation would be to annotate the copy constructor of Bar with `inout`: this(ref inout(Bar)) inout . That will make the code to compile. But I agree that the error could be improved. [1] https://github.com/dlang/DIPs/pull/129/commits/7815bc3fc49dd9f1a2195f871f3f6afe6c9d6c8b#diff-7f9578c76cc7367ecc56acfce679fc4bR675

(In reply to RazvanN from comment #4) > The fix in this situation would be to annotate the copy constructor of Bar > with `inout`: this(ref inout(Bar)) inout . That will make the code to > compile. That doesn't work if the struct contains indirections, e.g. when you add a mutable pointer to Bar...

(In reply to RazvanN from comment #4) > So, to conclude, by the current language rules the error is correct. > > The fix in this situation would be to annotate the copy constructor of Bar > with `inout`: this(ref inout(Bar)) inout . That will make the code to > compile. > > But I agree that the error could be improved. Thx for the explanation. - I think this really needs some work. So currently, this fails for both struct Bar { this(ref Bar) {} } struct Bar { this(const ref Bar) {} } because the compiler tries to generate an inout copy ctor for struct Foo { Bar _bar; } Instead of disabling it completely if that fails, I think it should try to fall back to a mutable or const copy ctor, propagating the limitations of its fields.

*** Issue 20927 has been marked as a duplicate of this issue. ***

*** Issue 23681 has been marked as a duplicate of this issue. ***

Just changing the issue summary to be more specific.

This really needs to be improved so that we're not just creating an inout copy constructor when an implicit copy constructor needs to be generated. It simply does not work when the member variable with a copy constructor has a different signature, and with all of the situations where the programmer has _zero_ control over the type with the implicit constructor (e.g. it's a Voldemort range type from a function in Phobos or some other library), we really need to be able to rely on implicit copy constructors actually working with other signatures. At this point, it's turning out to be shockingly difficult to add a copy constructor to a type that's core to the code base that I work on at work, and the fact that implicit constructor generation is so simplistic (and therefore fails so easily) is a major contributing factor. And right now, I'm forced to use an inout constructor even though that's _really_ not what I should be using in this case, because the implicit constructor generation doesn't work with anything else. IMHO, we really need to look at having the compiler generate the full set of copy constructors that are necessary to work with the member variables and not just a single one that tries to work in general but really doesn't. There are just too many situations where you don't control the code with the implicit constructor and therefore can't make it do the right thing, whereas in almost all cases like that it should be very possible for the compiler to generate a set of copy constructors that actually work - but instead, it just punts on the issue and tries a single inout copy constructor. This isn't the only issue that causes major problems with copy constructors, but it's a significant one.

What it could do is examine each field's copy constructor, and pick the most restrictive annotation that is inclusive to all of them. If that doesn't work, then fail.

(In reply to Walter Bright from comment #11) > What it could do is examine each field's copy constructor, and pick the most > restrictive annotation that is inclusive to all of them. If that doesn't > work, then fail. That would be better, but the ideal situation would involve generating multiple copy constructors if the types being wrapped have multiple, since it is possible to overload copy constructors on qualifiers. Obviously, it could only generate the ones which would work with all of the member variables, so you could easily end up in a situation where only a subset could be generated, but in many cases, it should work to be able to forward all of the overloads of the copy constructor - especially when only one of the member variables has a copy constructor, which will likely be the case a lot of the time. But either way, right now, as soon as you declare a copy constructor that doesn't work with the default signature, you're just screwed with any implicit copy constructors, since the compiler can't generate one that works.

@RazvanN7 created dlang/dmd pull request #16429 "Implement the generation of multiple copy constructors for fields" mentioning this issue: - Implement the generation of multiple copy constructors for fields + Fix Bugzilla Issue 20876 https://github.com/dlang/dmd/pull/16429

Even smaller test case: struct S { this(ref S) { } } struct T { S s; } void test() { T t; T u = t; }

An illustration of the generated copy constructor error: struct S { this(ref S) { } } struct T { S s; this(inout ref T t) { this.s = t.s; // copy constructor `test.S.this(ref S __param_0)` is not callable using argument types `(inout(S))` } } void test() { T t; T u = t; }

If we add `const` to the first copy constructor: struct S { this(const ref S) { } } it compiles without error. It seems like a simple solution would be: 1. if all fields take a const argument, then generate a const copy constructor 2. if all fields take an immutable argument, then generate an immutable copy constructor 3. otherwise, generate a mutable copy constructor

(In reply to Walter Bright from comment #16) > It seems like a simple solution would be: > > 1. if all fields take a const argument, then generate a const copy > constructor > > 2. if all fields take an immutable argument, then generate an immutable copy > constructor > > 3. otherwise, generate a mutable copy constructor The problem with this approach is that it is not always possible to find a single copy constructor signature that's compatible with all field copy constructors. For example: --- struct S { this(const ref S) {} this(shared ref S) {} } struct T { S s; // If all fields take a const argument, then // generate a const copy constructor. this(const ref T other) { this.s = other.s; } } void main() { { shared S original; S copy = original; // ok } { shared T original; T copy = original; // fails } } --- In this case, T needs to have two copy constructor overloads (a const one and a shared one) in order to correctly preserve the behavior of S.

(In reply to Walter Bright from comment #16) > If we add `const` to the first copy constructor: > > struct S { > this(const ref S) { } > } > > it compiles without error. > > It seems like a simple solution would be: > > 1. if all fields take a const argument, then generate a const copy > constructor > > 2. if all fields take an immutable argument, then generate an immutable copy > constructor > > 3. otherwise, generate a mutable copy constructor Why are we imposing random rules of copy constructor generation? As I see it, each copy constructor from an overload set of copy constructors defines how that object performs a copy for a given qualifier pair. Whenever that object is copied it should respect the rules defined in its copy constructors. Why don't we propagate that when the object is a field of another struct? It seems like the most natural thing to do and it is what the majority of users would expect. Moreover, it leads to natural composability. Trying to elide the generation of copy constructors only leads to confusing situations (see the inout(inout) copy constructor that is currently being generated). When I wrote the copy constructor DIP the copy constructor generation was done according to what https://github.com/dlang/dmd/pull/16429 implements. However, you rejected it not for any objective reason, but because you thought this is too complex and proposed the inout(inout) alternative. Now people have been complaining about inout(inout) copy constructor generation, which is completely useless, and asked for the natural generation of copy constructors and yet without a single argument to show why this is not a good strategy (simply stated "I am not sure this PR is the best solution") you propose a different solution which also doesn't actually fix the issue. I think that you should come up with a concrete example to showcase why generating multiple copy constructors is a bad strategy. If it's compile time performance that you are worried about, I don't think that that is something we should actually fear - I expect that that's not going to be a problem. However, even if we a pay a small compile time price, we strike a major win on the front of user experience.

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/19710 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB