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

Walter Bright <bugzilla@digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla@digitalmars.com

--- Comment #11 from Walter Bright <bugzilla@digitalmars.com> ---
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.

--

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

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

--

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

--- Comment #13 from Dlang Bot <dlang-bot@dlang.rocks> ---
@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

--

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

--- Comment #14 from Walter Bright <bugzilla@digitalmars.com> ---
Even smaller test case:

struct S {
    this(ref S) { }
}

struct T {
    S s;
}

void test()
{
    T t;
    T u = t;
}

--

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

--- Comment #15 from Walter Bright <bugzilla@digitalmars.com> ---
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;
}

--

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

--- Comment #16 from Walter Bright <bugzilla@digitalmars.com> ---
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

--

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

Paul Backus <snarwin+bugzilla@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |snarwin+bugzilla@gmail.com

--- Comment #17 from Paul Backus <snarwin+bugzilla@gmail.com> ---
(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.

--

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

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

--