[Issue 24829] New: Rebindable2 does not correctly handle types with copy constructors or postblit constructors which aren't assignable

[d-bugmail at puremagic.com]

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

          Issue ID: 24829
           Summary: Rebindable2 does not correctly handle types with copy
                    constructors or postblit constructors which aren't
                    assignable
           Product: D
           Version: D2
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P1
         Component: phobos
          Assignee: nobody at puremagic.com
          Reporter: issues.dlang at jmdavisProg.com

In working on a fix for https://issues.dlang.org/show_bug.cgi?id=24827, I've
determined that the useQualifierCast == false version of Rebindable2 does not
do anything to handle postblit constructors or copy constructors, meaning that
it will do the wrong thing when given a struct with a postblit constructor or
copy constructor. E.G. this is the test that I have at the moment for it, and
the copy isn't done (which isn't surprising when looking at the code, since
instead of putting the struct directly in the Rebindable2, it's put in an array
of void or ubyte depending on whether it has indirections):

---
unittest
{
    {
        static struct S
        {
            int* ptr;
            static bool copied;

            this(int i)
            {
                ptr = new int(i);
            }

            this(this) @safe
            {
                if(ptr !is null)
                    ptr = new int(*ptr);
                copied = true;
            }

            @disable ref S opAssign()(auto ref S rhs);
        }

        {
            auto foo = rebindable2(S(42));
            assert(!typeof(foo).useQualifierCast);
            S.copied = false;

            auto bar = foo;
            assert(S.copied);
            assert(*(cast(S*)&(foo.data)).ptr == *(cast(S*)&(bar.data)).ptr);
            assert((cast(S*)&(foo.data)).ptr !is (cast(S*)&(bar.data)).ptr);
        }
        {
            auto foo = rebindable2(const S(42));
            S.copied = false;

            auto bar = foo;
            assert(S.copied);
            assert(*(cast(S*)&(foo.data)).ptr == *(cast(S*)&(bar.data)).ptr);
            assert((cast(S*)&(foo.data)).ptr !is (cast(S*)&(bar.data)).ptr);
        }
    }

    // copy constructor without type qualifier cast
    {
        static struct S
        {
            int* ptr;
            static bool copied;

            this(int i)
            {
                ptr = new int(i);
            }

            this(ref inout S rhs) @safe inout
            {
                if(rhs.ptr !is null)
                    ptr = new inout int(*rhs.ptr);
                copied = true;
            }

            @disable ref S opAssign()(auto ref S rhs);
        }

        {
            auto foo = rebindable2(S(42));
            assert(!typeof(foo).useQualifierCast);
            S.copied = false;

            auto bar = foo;
            assert(S.copied);
            assert(*(cast(S*)&(foo.data)).ptr == *(cast(S*)&(bar.data)).ptr);
            assert((cast(S*)&(foo.data)).ptr !is (cast(S*)&(bar.data)).ptr);
        }
        {
            auto foo = rebindable2(const S(42));
            S.copied = false;

            auto bar = foo;
            assert(S.copied);
            assert(*(cast(S*)&(foo.data)).ptr == *(cast(S*)&(bar.data)).ptr);
            assert((cast(S*)&(foo.data)).ptr !is (cast(S*)&(bar.data)).ptr);
        }
    }
}
---

Note that that test would have to be inside of std.typecons alongside
Rebindable2 (or at least somewhere within Phobos), since Rebindable2 is
package(std).

It's not entirely clear to me what circumstances exactly reasonably result in
useQualifierCast being false, since it requires that

    isAssignable!(typeof(cast() T.init))

be false. The test I put together has a disabled opAssign to make that happen,
but I find it very bizarre that we'd even be attempting to make the code work
with a disabled opAssign. So, I don't know if that's what it's trying to solve
or whether it's something else that somehow manages to not work with
qualifier-free assignment.

The affected code would appear to be std.range.repeat,
std.algorithm.searching.minElement, and std.algorithm.maxElement, since those
are the public symbols which ultimately use Rebindable2. So, if any of them are
given a type which fails to be assignable in the fashion that Rebindable2 is
testing for, _and_ that type has either a postblit constructor or a copy
constructor, then that code is going to be wrong.

Fixing this does not look like it will be at all straightforward. Ideally, we'd
make it so that the compiler could generate the postblit constructor or copy
constructor like it would do when useQualiferCast is true, but I don't know how
possible that is. And if we have to generate the postblit or copy constructor
ourselves to get the correct behavior, that gets incredibly messy if we want to
infer the attributes correctly, since we would not only need to use @trusted to
deal with some casting, but we'd probably need to use a string mixin to
generate the appropriate constructor with all of the correct attributes tagged
on.

--