https://issues.dlang.org/show_bug.cgi?id=20876

moonlightsentinel@disroot.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |moonlightsentinel@disroot.o
                   |                            |rg

--- Comment #1 from moonlightsentinel@disroot.org ---
Digger blames the introduction of copy constructors:

commit f9cbf699c0fa07892137d477fbe1ea197c8d0cbe
Author: Andrei Alexandrescu <andrei@erdani.com>
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

--

https://issues.dlang.org/show_bug.cgi?id=20876

kinke <kinke@gmx.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kinke@gmx.net

--- Comment #2 from kinke <kinke@gmx.net> ---
(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.

--

https://issues.dlang.org/show_bug.cgi?id=20876

--- Comment #3 from moonlightsentinel@disroot.org ---
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;
}

--

https://issues.dlang.org/show_bug.cgi?id=20876

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |razvan.nitu1305@gmail.com

--- Comment #4 from RazvanN <razvan.nitu1305@gmail.com> ---
(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

--

https://issues.dlang.org/show_bug.cgi?id=20876

--- Comment #5 from moonlightsentinel@disroot.org ---
(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...

--

https://issues.dlang.org/show_bug.cgi?id=20876

--- Comment #6 from kinke <kinke@gmx.net> ---
(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.

--

https://issues.dlang.org/show_bug.cgi?id=20876

--- Comment #7 from Puneet Goel <puneet@coverify.org> ---
*** Issue 20927 has been marked as a duplicate of this issue. ***

--

https://issues.dlang.org/show_bug.cgi?id=20876

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alphaglosined@gmail.com

--- Comment #8 from RazvanN <razvan.nitu1305@gmail.com> ---
*** Issue 23681 has been marked as a duplicate of this issue. ***

--

https://issues.dlang.org/show_bug.cgi?id=20876

Steven Schveighoffer <schveiguy@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy@gmail.com
            Summary|DMD gives out not so        |generated constructor
                   |helpful compile error       |always inout regardless of
                   |                            |ability of fields to use
                   |                            |inout

--- Comment #9 from Steven Schveighoffer <schveiguy@gmail.com> ---
Just changing the issue summary to be more specific.

--

https://issues.dlang.org/show_bug.cgi?id=20876

Jonathan M Davis <issues.dlang@jmdavisProg.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |issues.dlang@jmdavisProg.co
                   |                            |m
           Hardware|x86_64                      |All
                 OS|Linux                       |All

--- Comment #10 from Jonathan M Davis <issues.dlang@jmdavisProg.com> ---
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.

--