Fix issue 15645 - Tuple.slice() causes memory corruption

[saurabhdas]

This approach fixes the issue without changing the layout of the tuple.

Changes that need consideration:

1. Return type of Tuple.slice - 'ref' removed. Does this affect how
   Tuple.slice() is expected to behave?

[dlang-bot]

Fix	Bugzilla	Description
✓	15645	Tuple.slice() causes memory corruption.

[tsbockman]

I have no idea how many people would actually care, but this is a breaking change, mostly because:

Tuple!(string) foo = tuple("FOO");
writeln(foo._0);
foo.slice!(0, 1)()._0 = "BAR";
writeln(foo._0);

The last line prints BAR currently. With your PR it will print FOO instead.

Aside from that, I see the following trade-offs:

• Calls to slice() may take longer with your PR, because data has to be re-aligned to match the layout of the return type.
• Code that makes non-trivial use of the return value and/or type of slice() may be slightly faster with your PR, since the slice type is more compact.
• My version introduces some additional template bloat and compile-time logic. Yours does not.

For a clean-sheet design, your way might be a little better. But, I don't think it's worth potentially breaking valid code over. (Mine is not a breaking change, because its runtime behaviour should only differ from the current version in cases where the current version is wrong.)

Nevertheless, I recommend that you leave this PR open until we see what independent reviewers have to say.

EDIT: I should probably link to my earlier PR here: #3973

[tsbockman]

This must be moved to a separate unittest block, because the /// at the head of this one indicates that it is an example for the documentation.

Ideally, your test should be in a unittest block outside the Tuple template entirely, because otherwise many redundant instantiations may be generated.

[saurabhdas]

Thanks @tsbockman

I was not aware. I shall move it into a separate block if there's interest in this approach.

[tsbockman]

Having a const overload of slice() is a good idea, but you still need to have a non-const overload, also:

import std.stdio, std.typecons;

@safe:

class A {
    int x;
    this(int x) {
        this.x = x; }

    override string toString() const {
        import std.conv : to;
        return x.to!string();
    }
}

void main()
{
    Tuple!(A) foo = tuple(new A(1));
    writeln(foo[0]);
    foo.slice!(0, 1)()[0] = new A(2);
    writeln(foo[0]);
}

The above code works as intended with my PR or the current implementation, but with yours it will generate a compiler error at foo.slice.

[quickfur]

Does inout not work in this case, that we need two overloads of slice()?

[tsbockman]

@quickfur

Does inout not work in this case, that we need two overloads of slice()?

inout works. (I have updated my own PR to reflect this: #3973)

EDIT: inout works in my PR (which preserves return-by-ref), but it's not immediately obvious to me how to make it work in this one:

Error: None of the overloads of '__ctor' are callable using a inout object

[saurabhdas]

I've updated the PR to make slice() work with inout.

As suggested by Marco Leise in http://forum.dlang.org/post/20160205201611.14216605@gmx.de have removed @safe - depend on the compiler inferring the attribute.

[tsbockman]

@saurabhdas

I've updated the PR to make slice() work with inout.

Sadly, it's not that simple:

class A
{
    int x;
}

Tuple!(A) foo;
foo.slice!(0, 1);

The above will not compile with your PR.

source/typecons.d(746,49): Error: None of the overloads of '__ctor' are callable using argument types (inout(A)), candidates are:
source/typecons.d(529,13):        typecons.Tuple!(A).Tuple.this(A _param_0)
source/app.d(16,8): Error: template instance typecons.Tuple!(A).Tuple.slice!(0LU, 1LU) error instantiating

[tsbockman]

Thinking about this more, if you want to strip the ref from the return type, you really need to deprecate Tuple.slice and give your version a new name.

Otherwise, this could silently change the behaviour of previously working, valid programs in a most frustrating fashion.

[tsbockman]

I posted a poll on dlang's "General" forum to help us decide which fix to use: http://forum.dlang.org/post/inswmiscuqirkhfqlhtd@forum.dlang.org

[tsbockman]

Since there seem to be people actually using the ref-ness of Tuple.slice, I do not think this PR can be considered a true fix.

[saurabhdas]

I will be withdrawing this PR - given the difference in the function signature, it should be resubmitted with a new function name.

Are there any suggestions for a name? Also some help with the inout problem would really help.

[tsbockman]

Also some help with the inout problem would really help.

Here you go:

// The constructor around line 530 must be modified to accept inout parameters
static if (Types.length > 0)
{
    this(staticMap!(InOut, Types) values) inout @trusted
    {
        field[] = values[];
    }
    private alias InOut(T) = inout(T);
}

// Likewise for slice() :
@property
inout(Tuple!(sliceSpecs!(from, to))) sliceVal(size_t from, size_t to)() inout
    if (from <= to && to <= Types.length)
{
    return typeof(return)(expand[from .. to]);
}

Assuming that my PR is accepted, a little bit of further work will be required to make the two compatible with each other.