Add std.typecons.Rebindable!struct by ntrel · Pull Request #4363 · dlang/phobos

ntrel · 2016-05-25T11:43:05Z

Rebindable!Struct:
"Models safe reassignment of otherwise constant struct instances.

A constant struct with a field of reference type cannot be assigned to a mutable
struct of the same type. This protects the constant reference field from being
mutably aliased, potentially allowing mutation of immutable data. However, the
assignment could be safe if all reference fields are only exposed as const.

Rebindable!(const S) accepts assignment from
a const S while enforcing only constant access to its fields.
Rebindable!(immutable S) does the same but field access may create a
temporary copy of S in order to enforce true immutability."

Hackerpilot · 2016-05-25T18:23:54Z

Unit tests fail.

ntrel · 2016-05-25T19:10:27Z

@Hackerpilot Thanks. ~~At least one~~ All tests pass now.

JackStouffer · 2016-05-25T21:04:53Z

std/typecons.d

+        this()(S s) @trusted
+        {
+            // we preserve tail immutable guarantees so cast is OK
+            payload = cast(Unqual!S)s;


we preserve tail immutable guarantees so cast is OK

Are you sure? Casting away const is UB in any situation.

Casting away const is UB in any situation.

Just casting is ok. You just can't mutate through the casted reference.

See https://dlang.org/spec/const3.html#removing_with_cast

If the payload has an opAssign, it will be able to mutate through it.

OK, maybe that's why existing Rebindable uses a union to get around the problem.

@schuetzm I've changed the implementation to use a union now, I think that solves the mutable opAssign problem. (I'm not entirely sure about the destructor, I'll try to do some tests).

ntrel · 2016-05-30T14:29:46Z

@Hackerpilot can you remove (or explain) the needs work label please ;-) All unit tests currently pass.

dnadlinger · 2016-05-30T14:34:51Z

std/typecons.d

+            S payload;
+        }
+
+    @trusted:


How can this be true when S can have an arbitrarily unsafe opAssign or dtor?

I think it's OK now, opAssign is never called, and S.~this has its safety inferred. Rebindable.thisalso uses inferrence for safety of S.this.

dnadlinger · 2016-05-30T14:35:45Z

(keeping the "needs work" label up to date is impossible for people without commit access, so removing it for now)

ntrel · 2016-05-30T14:42:06Z

Also there's another problem: unions can't have struct members that themselves define destructors or postblits.

Edit: I'll try to fix this using void[] payload in the next few days.

aG0aep6G · 2016-05-30T15:11:59Z

std/typecons.d

+
+        void opAssign(S s) @trusted
+        {
+            auto rs = Rebindable(s);


Unless I'm missing something, this runs S's postblit which may be unsafe, breaking opAssign's @trusted promise.

Thanks, should be fixed now.

Unions don't allow struct members which have postblit or dtors.

ntrel · 2016-06-06T12:16:18Z

Implemented changing union -> void[].

ntrel · 2016-06-28T10:16:16Z

Ping?

This is part of making Rebindable generic (which should be simple once this is this merged). This will help to implement functions like std.algorithm.searching.extremum correctly - instead of using Unqual for extremeElement we should use Rebindable. I expect this goes for various other places in Phobos too.

Also, should this go in std.experimental first?

wilzbach

A couple of first impressions

wilzbach · 2017-02-19T03:03:55Z

std/typecons.d


+/// ditto
+Rebindable!S rebindable(S)(S s)
+if (is(S == struct) && !isInstanceOf!(Rebindable, S))


The Phobos style is to avoid such type overloads and hide them as implementation details.

wilzbach · 2017-02-19T03:26:11Z

std/typecons.d

+                () @trusted {emplace!S(mutPayload, s);}();
+            else
+                emplace!S(mutPayload, s);
+        }


How about pushing this @safe improvement to std.conv.emplace?

I think it's only safe if S.~this is run when mutPayload goes out of scope, which emplace can't enforce.

wilzbach · 2017-02-19T03:31:15Z

std/typecons.d

+
+        // immutable S cannot be passed to auto ref S above
+        static if (!is(S == immutable))
+        this()(immutable S s)


that's what inout was made for ;-) -> inout auto ref S s

Doesn't work. I'm a bit rusty on this PR but I remember struggling with compile errors - inout triggers them.

wilzbach · 2017-02-19T03:39:53Z

std/typecons.d

+        static if (!is(S == immutable))
+        void opAssign()(immutable S s)
+        {
+            emplace(s);


why is there no movePayload (and thus call to of destructor of the internal payload)?

Added, thanks.

wilzbach · 2017-02-19T03:57:36Z

std/typecons.d

+        }
+
+        static if (!is(S == immutable))
+        ref S Rebindable_getRef() @property


should be private

Can't be because it's used for alias this.

wilzbach · 2017-02-19T03:57:43Z

std/typecons.d

+        }
+
+        static if (is(S == immutable))
+        S Rebindable_get() @property


ditto (private)

wilzbach · 2017-02-19T12:43:06Z

std/typecons.d

+ * temporary copy of `S` in order to enforce _true immutability.
+ */
+template Rebindable(S)
+if (is(S == struct))


It would be awesome if you could put all Rebindable types under this template as sth. like __traits(isSame, TemplateOf!(typeof(ri2)), Rebindable)
would work. Currently Rebindable references to the first symbol - the one for classes etc.

wilzbach · 2017-02-19T14:19:48Z

std/typecons.d

+            emplace(s);
+        }
+
+        void opAssign()(auto ref S s)


for

Rebindable!(immutable S) ri = S(new int); auto ri2 = ri;

this yields conflicting overloads.

Works for me - I added a test for this, let's see if the autotester passes.

wilzbach · 2017-02-19T14:21:19Z

std/typecons.d

+        ~this()
+        {
+            // call destructor with proper constness
+            S s = movePayload;


You can remove the S s = part ;-)

wilzbach · 2017-02-19T14:27:45Z

Also, should this go in std.experimental first?

While usually it's not needed, it seems that your PR got quite stalled and for std.experimental the barrier is lower, so it might be a good option to push your PR forward.

I made a simple test for your code to check the lifetime by counting postblit and destructor calls.
It shows the effect of the temporary copies that get created on every field access and it was also a good test to realize that one of your opAssign was missing the destructor call. Maybe you could include sth. like this to your tests?
Note that atm also you can't do the following auto ri2 = ri; due to overload conflicts.

Last but not least

if you rebase this PR to upstream/master, we would be supplied with coverage annotations.
you now can directly add a changelog entry to your PR (I think that this PR needs one)

unittest
{
    import std.stdio;
    int del;
    int post;
    struct S
    {
        int* ptr;
        int level;
        this(this) {
            writeln("S:this(this)");
            post++;
            level++;
        }
        ~this() {
            writeln("S:~this");
            del++;
        }
    }

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

    // 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);
        assert(post == 2);
        assert(ri.level == 2); // however it's a copy of the payload's level
    }
    assert(post == 3);
    assert(del == 4);
    del = 0, post = 0;

    {
        // the initial value is copied and destructed
        Rebindable!(const S) rc = S(new int);
        assert(post == 1);
        assert(del == 1);
        assert(rc.level == 1);

        // on an assignment to another value is simply post-blitted
        const S cs = rc;
        assert(del == 1);
        assert(post == 2);
        assert(rc.level == 1);
        assert(cs.level == 2);

        // however on an assignment, the old payload gets destructed
        rc = cs;
        assert(del == 2);
        assert(post == 3);
        assert(cs.level == 2);
        assert(rc.level == 3);
    }
    assert(post == 3);
    assert(del == 4);
    del = 0, post = 0;

    {
        // the initial value is copied and destructed
        Rebindable!(immutable S) ri = S(new int);
        assert(post == 1);
        assert(del == 1);
        // creates temporary (gets immediately destroyed), actual level is still 1
        assert(ri.level == 2);
        assert(del == 2);
        assert(post == 2);

        // on an assignment to another value is simply post-blitted
        const S cs = ri;
        assert(del == 2);
        assert(post == 3);
        // creates temporary (gets immediately destroyed), actual level is still 1
        assert(ri.level == 2);
        assert(cs.level == 2);
        assert(del == 3);
        assert(post == 4);

        // however, on an assignment to Rebindable the old value gets destructed
        auto ri2 = ri;
        static assert(is(typeof(ri2) == Rebindable!(immutable S)));
        static assert(TemplateOf!(typeof(ri2)).stringof == "Rebindable(S) if (is(S == struct))");
        assert(del == 3);
        assert(post == 4);
        assert(ri2.level == 2); // should be 3??
        assert(del == 4);

        // similarly an assignment
        //ri = ri2; // fails to compile due to overload conflicts
    }
    assert(post == 5);
    assert(del == 7);
}

ntrel · 2017-02-21T13:30:00Z

@wilzbach Thanks for reviewing this! I'm updating my toolchain so I can work against the latest master. Also I will move the changes to std.experimental, which will deal (for now) with the separate Rebindable template.

wilzbach · 2017-07-09T16:11:25Z

Also I will move the changes to std.experimental, which will deal (for now) with the separate Rebindable template.

@ntrel Did you ever manage to find time for this?

MetaLang · 2017-08-06T05:16:34Z

It's been a year so it looks like this is pretty much dead. I'm going to close this but @ntrel please comment if you want me to re-open it.

wilzbach · 2018-02-07T11:10:01Z

Hmm, I still think this is quite useful (I have had the need for it a few times over the last months), so I have revived this PR to #6136

(I have already addressed all my comments, except for the opAssign overload conflict)

ntrel force-pushed the rebind-struct branch from b77d980 to 95e3072 Compare May 25, 2016 11:46

Hackerpilot added the Review:Needs Work label May 25, 2016

ntrel force-pushed the rebind-struct branch from 95e3072 to 9e6152c Compare May 25, 2016 18:56

Add std.typecons.Rebindable!struct

d5093d1

ntrel force-pushed the rebind-struct branch from 9e6152c to d5093d1 Compare May 25, 2016 19:00

JackStouffer reviewed May 25, 2016
View reviewed changes

ntrel added 4 commits May 29, 2016 19:15

Fix possible tail mutation with mutable S.opAssign

f9599c6

Fix opAssign using swap

d6523f1

Use local @trusted attribute

b828237

Add opAssign(typeof(this)), tests

90a1306

ntrel force-pushed the rebind-struct branch from 33a2877 to 90a1306 Compare May 30, 2016 11:10

dnadlinger reviewed May 30, 2016
View reviewed changes

dnadlinger removed the Review:Needs Work label May 30, 2016

Don't use @trusted where S.this, S.opAssign, S.~this are @System

08a95dc

ntrel force-pushed the rebind-struct branch from 1bffce6 to 08a95dc Compare May 30, 2016 15:04

aG0aep6G reviewed May 30, 2016
View reviewed changes

ntrel force-pushed the rebind-struct branch from ccf51d7 to 20cdf88 Compare May 30, 2016 15:27

Make opAssign infer safety of S.this(this)

49b8f36

ntrel force-pushed the rebind-struct branch from 20cdf88 to 49b8f36 Compare May 30, 2016 15:28

Use void[S.sizeof] mutPayload because union is too inflexible

85ebede

Unions don't allow struct members which have postblit or dtors.

ntrel force-pushed the rebind-struct branch from cf78a67 to 85ebede Compare June 2, 2016 11:56

ntrel added 3 commits June 3, 2016 12:02

Fix swapPayload violating const

3caca47

getPayload cannot be returned as ref S

d128695

Use auto ref for ctor and opAssign

bf3bc5b

ntrel mentioned this pull request Jun 28, 2016

Add std.range.Option #3915

Closed

9il added Review:Needs Review Severity:Enhancement labels Jul 29, 2016

ntrel mentioned this pull request Dec 21, 2016

Issue 8573 - A simpler Phobos function that returns the index of the … #4921

Merged

wilzbach reviewed Feb 19, 2017

View reviewed changes

wilzbach mentioned this pull request Feb 19, 2017

Fix 12564, 14493: add std.range.walkBack to get the last element of an input range #5153

Closed

ntrel added 2 commits February 21, 2017 12:59

Fix destroying existing immutable S on opAssign

45eee71

Test copying Rebindable!immutable

ac5abae

wilzbach added this to the 2.075.0 milestone Mar 4, 2017

dlang-bot added the Merge:stalled label Jun 22, 2017

MartinNowak removed this from the 2.075.0 milestone Jun 25, 2017

JackStouffer added Review:Needs Work and removed Review:Needs Review labels Jul 9, 2017

MetaLang closed this Aug 6, 2017

wilzbach mentioned this pull request Feb 7, 2018

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

Closed

FeepingCreature mentioned this pull request Feb 1, 2023

Feature: Add std.typecons.ManagedLifetime. #8674

Closed

FeepingCreature mentioned this pull request Mar 16, 2023

Feature: Implement Rebindable for non-class/non-array types. #8722

Merged

Uh oh!

Conversation

ntrel commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hackerpilot commented May 25, 2016

Uh oh!

ntrel commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntrel commented May 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger commented May 30, 2016

Uh oh!

ntrel commented May 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntrel commented Jun 6, 2016

Uh oh!

ntrel commented Jun 28, 2016

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 19, 2017

Uh oh!

ntrel commented Feb 21, 2017

Uh oh!

wilzbach commented Jul 9, 2017

Uh oh!

MetaLang commented Aug 6, 2017

Uh oh!

ntrel commented May 25, 2016 •

edited

Loading

ntrel commented May 25, 2016 •

edited

Loading

ntrel commented May 30, 2016 •

edited

Loading