[revived] Make std.typecons.Rebindable work with structs

[wilzbach]

Revival of #4363

CC @ntrel

I think only the opAssign problem needs to be resolved, apart from that it looks good.

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6136"

[n8sh]

    static if (!is(S == immutable))
    private ref S Rebindable_getRef() @property
    {
        // payload exposed as const ref when S is const
        return trustedPayload;
    }

    static if (is(S == immutable))
    private S Rebindable_get() @property
    {
        // we return a copy for immutable S
        return trustedPayload;
    }

Could you explain why this is necessary?

[wilzbach]

Note that I just revived this PR from the dead because I think it's useful - I'm not the original author (@ntrel is).

However, from what I understand, the problem without the getRef trick, the compiler will complain:

Rebindable!(const S) rs;
rs.ptr = null; // Error: cannot modify const expression rs.Rebindable_get().ptr
s = rs; // Error: cannot implicitly convert expression rs of type Rebindable!(const(S)) to S

However, if the getRef code is used for immutable, no temporary variable would be created and thus resulting in i.e. the postblit not being called:

unittest
{
    int del;
    int post;
    struct S
    {
        int* ptr;
        int level;
        this(this) {
            post++;
            level++;
        }
        ~this() {
            del++;
        }
    }

    // test const
    {
        Rebindable!(const S) rc = S(new int);
        assert(post == 1);
        assert(rc.level == 1);
    }
    assert(post == 1);
    assert(del == 2);
    del = 0, post = 0;

    import std.conv;

    // immutable
    {
        // on every field access a temporary immutable copy gets created
        Rebindable!(immutable S) ri = S(new int);
        assert(post == 1);
        assert(ri.level == 2); // <- would be 1
    }
}

Maybe @ntrel can explain this better?

[MetaLang]

If S is already Rebindable!struct this can just return s.

[wilzbach]

There's already the existing overload Rebindable!T rebindable(T)(Rebindable!T obj) - though they should probably be unified/combined into overload, but that's something that will increase the diff and thus I would prefer to do at the end or in a quick follow-up.

[MetaLang]

By that same token, what if S is already a Rebindable?

[MetaLang]

Why not check S's constructor directly for @safe/@trusted?

[n8sh]

@MetaLang What would that look like?

[MetaLang]

import std.traits;

struct Test
{
    this(Test other) @safe {}
}

void main()
{
    pragma(msg, isSafe!(__traits(getMember, Test, "__ctor")));
}

https://run.dlang.io/is/hgusNS

Note that isSafe returns true if an @safe or @trusted constructor exists, so even if the type in question has an @system constructor, this will work. It will only return false in the case that you can't safely construct an S from another S.

[MetaLang]

What do you think about renaming this emplacePayload, or something similar?

[ntrel]

You can't use private for an alias this symbol, because it won't be accessible outside this module. Ditto below.

[n8sh]

@wilzbach re: #6136 (comment)

However, if the getRef code is used for immutable, no temporary variable would be created and thus resulting in i.e. the postblit not being called

I phrased my question unclearly. Why is this necessary when S == immutable if it's not necessary when S == const?

[jmdavis]

Honestly, my reaction is that this really doesn't make sense. The whole point of Rebindable is to be able to do the equivalent of const(T)* or immutable(T)* with classes. If you have a struct, just put it on the heap, and then you can do const(T)* or immutable(T)* explicitly. And if it's not on the heap, then it fundamentally doesn't make sense for it be used with Rebindable. This just seems like a lot of code for duplicating what the type system can already trivially do for structs by using pointers.

[ntrel]

@jmdavis Here's the motivation for this pull:

T maxElement(R, T = typeof(R.init.front))(R r)
{
    Rebindable!T max = r.front;
    foreach (e; r.dropOne)
        if (e > max)
            max = e;
    return max;
}

Suppose typeof(r.front) is a constant struct with a reference field. You can't declare max as Unqual!T, because a constant reference field is not convertible to a mutable reference. But that is what std.algorithm.searching.maxElement does (the code is in extremum), so that implementation is broken for this kind of struct.

The idea is that with this pull as a prerequisite, we can make Rebindable accept value types for use in generic code as in the above example. This makes for clean and correct generic code. Given this struct use case, there is no reason to artificially restrict Rebindable only to reference types. It would also be unnecessarily restrictive to not allow algorithms to operate on ranges of such structs.

[ntrel]

@wilzbach I made a pull to improve the doc wording & fix private alias this getters: wilzbach#14

[wilzbach]

Suppose typeof(r.front) is a constant struct with a reference field. You can't declare max as Unqual!T, because a constant reference field is not convertible to a mutable reference. But that is what std.algorithm.searching.maxElement does (the code is in extremum), so that implementation is broken for this kind of struct.
The idea is that with this pull as a prerequisite, we can make Rebindable accept value types for use in generic code as in the above example. This makes for clean and correct generic code. [...]

Yep, funnily enough I even have a PR to fix maxElement for const in the pipe that use the RebindableOrUnqual workaround to hack around the problem (#6157).

[ntrel]

@n8sh Escaping a const ref T still allows the T struct data itself to change (shallowly), just not through that ref. Escaping immutable ref T does not, it requires the T data itself to be fully immutable. As the Rebindable data is always head mutable, we must return an immutable T copy. (If we had multiple alias this, we could have both when S == immutable).

[ntrel]

I've discovered an immutable violation:

struct S
{
	immutable int i;
}
Rebindable!(const S) r = const S(1);
immutable int* p = &r.i;
r = S(5);
assert(*p == 1); //fails

I have a fix ready, just need to finish the tests and docs. For structs with head immutable fields, we require alias this to make a copy, like we do for immutable structs.

[ntrel]

I've discovered an immutable violation:

See wilzbach#15

[atilaneves]

I came here to review it and hopefully move this along, but given that the CI was red at the time maybe a new PR?