Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Issue 19532 - chunkBy errors involving non-forward input ranges #6841

Merged
merged 1 commit into from Jan 30, 2019
Merged

Fix Issue 19532 - chunkBy errors involving non-forward input ranges #6841

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
293 changes: 284 additions & 9 deletions std/algorithm/iteration.d
View file
Open in desktop
Expand Up @@ -1790,12 +1790,12 @@ if (isInputRange!Range && !isForwardRange!Range)
{
alias fun = binaryFun!pred;

private Range r;
private Range *r;
private ElementType!Range prev;

this(Range range, ElementType!Range _prev)
this(ref Range range, ElementType!Range _prev)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be return ref cc @WalterBright

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it simply become: this(return ref Range range, ElementType!Range _prev)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked my local unit test build, the iteration.d file is built with -dip1000. My impression was that this flag would catch needed changes. I'll wait additional clarification before making changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WalterBright Can you clarify? Should this be return ref as per @thewilsonator? The PR as written compiles with -dip1000. I searched phobos code and looked through the DIP 1000 document, but didn't see an example using return ref in a constructor function. I'd like to identify the correct way to write this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, the compiler is able to infer parameter attributes like scope and return for templates, lambdas, etc. so in many cases adding the attributes manually is not necessary / has no effect. However adding attributes is needed for cases where the compiler can't infer things on its own.
Which are those cases is highly implementation specific, and it depends on a case by case basis. As rule of thumb, try to go first without scope/return and if the compiler complains, add them as necessary.
Most importantly, have test cases involving static arrays in @safe unittests, as the escape analysis enabled by -dip1000 will emit errors only in @safe functions, and the point of the escape analysis is to disallow escaping a reference to a stack-allocated object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZombineDev Thanks for the explanation. Current chunkBy unit tests are not @safe, they are @system instead. There is a separate issue listed for this, issue 18161. According to that report the problem is that the implementation for the forward range case uses RefCounted.

My personal preference would be to leave the issue of making chunkBy @safe as a separate PR, and have this PR focus on the bug being fixed. That is, leave the implementation as written, without introducing scope / return. However, if reviewers prefer, I could try to make the non-forward input range implementation @safe and introduce unit tests for this purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a closer look at this topic and experimented with creating @safe unit tests. Best I can tell, DIP 1000 is saying that the technique I used is not considered @safe under -dip1000. First time I've really looked at DIP 1000, but my interpretation is that it is saying that the GC can no longer be used to manage lifetime in @safe code, and instead some other technique, such as reference counting, must be used. Implementing this appears be an issue, perhaps temporary, as RefCounted is @system at the moment.

It'd be fantastic if I've missed something basic and there's an easy way satisfy@safe under -dip1000. If so, great. But otherwise I recommend going forward with the PR as written. The bug being addressed is quite significant, enough that it makes chunkBy dangerous to use. It'd be better to fix the bug even if it's @system under -dip1000. The forward input range implementation of chunkBy is @system due to use of RefCounted, so presumably both pieces of the chunkBy implementation could be made @safe together.

{
r = range;
r = ⦥
prev = _prev;
}

Expand All @@ -1804,8 +1804,17 @@ if (isInputRange!Range && !isForwardRange!Range)
return r.empty || !fun(prev, r.front);
}

@property ElementType!Range front() { return r.front; }
void popFront() { r.popFront(); }
@property ElementType!Range front()
{
assert(!empty, "Attempting to fetch the front of an empty chunkBy chunk.");
return r.front;
}

void popFront()
{
assert(!empty, "Attempting to popFront an empty chunkBy chunk.");
r.popFront();
}
}

private template ChunkByImplIsUnary(alias pred, Range)
Expand Down Expand Up @@ -1836,6 +1845,7 @@ if (isInputRange!Range && !isForwardRange!Range)

private Range r;
private ElementType!Range _prev;
private bool openChunk = false;

this(Range _r)
{
Expand All @@ -1857,10 +1867,12 @@ if (isInputRange!Range && !isForwardRange!Range)
_prev = typeof(_prev).init;
}
}
@property bool empty() { return r.empty; }
@property bool empty() { return r.empty && !openChunk; }

@property auto front()
{
assert(!empty, "Attempting to fetch the front of an empty chunkBy.");
openChunk = true;
static if (isUnary)
{
import std.typecons : tuple;
Expand All @@ -1875,6 +1887,8 @@ if (isInputRange!Range && !isForwardRange!Range)

void popFront()
{
assert(!empty, "Attempting to popFront an empty chunkBy.");
openChunk = false;
while (!r.empty)
{
if (!eq(_prev, r.front))
Expand Down Expand Up @@ -2216,11 +2230,22 @@ version (none) // this example requires support for non-equivalence relations
R data;
this(R _data) pure @safe nothrow { data = _data; }
@property bool empty() pure @safe nothrow { return data.empty; }
@property auto front() pure @safe nothrow { return data.front; }
void popFront() pure @safe nothrow { data.popFront(); }
@property auto front() pure @safe nothrow { assert(!empty); return data.front; }
void popFront() pure @safe nothrow { assert(!empty); data.popFront(); }
}
auto refInputRange(R)(R range) { return new RefInputRange!R(range); }

// An input range API with value semantics.
struct ValInputRange(R)
{
R data;
this(R _data) pure @safe nothrow { data = _data; }
@property bool empty() pure @safe nothrow { return data.empty; }
@property auto front() pure @safe nothrow { assert(!empty); return data.front; }
void popFront() pure @safe nothrow { assert(!empty); data.popFront(); }
}
auto valInputRange(R)(R range) { return ValInputRange!R(range); }

{
auto arr = [ Item(1,2), Item(1,3), Item(2,3) ];
static assert(isForwardRange!(typeof(arr)));
Expand Down Expand Up @@ -2251,7 +2276,7 @@ version (none) // this example requires support for non-equivalence relations
assert(byY2.front[1].equal([ Item(1,2) ]));
}

// Test non-forward input ranges.
// Test non-forward input ranges with reference semantics.
{
auto range = refInputRange([ Item(1,1), Item(1,2), Item(2,2) ]);
auto byX = chunkBy!(a => a.x)(range);
Expand All @@ -2275,6 +2300,256 @@ version (none) // this example requires support for non-equivalence relations
assert(byY.empty);
assert(range.empty);
}

// Test non-forward input ranges with value semantics.
{
auto range = valInputRange([ Item(1,1), Item(1,2), Item(2,2) ]);
auto byX = chunkBy!(a => a.x)(range);
assert(byX.front[0] == 1);
assert(byX.front[1].equal([ Item(1,1), Item(1,2) ]));
byX.popFront();
assert(byX.front[0] == 2);
assert(byX.front[1].equal([ Item(2,2) ]));
byX.popFront();
assert(byX.empty);
assert(!range.empty); // Opposite of refInputRange test

range = valInputRange([ Item(1,1), Item(1,2), Item(2,2) ]);
auto byY = chunkBy!(a => a.y)(range);
assert(byY.front[0] == 1);
assert(byY.front[1].equal([ Item(1,1) ]));
byY.popFront();
assert(byY.front[0] == 2);
assert(byY.front[1].equal([ Item(1,2), Item(2,2) ]));
byY.popFront();
assert(byY.empty);
assert(!range.empty); // Opposite of refInputRange test
}

/* Issue 93532 - General behavior of non-forward input ranges.
* - If the same chunk is retrieved multiple times via front, the separate chunk
* instances refer to a shared range segment that advances as a single range.
* - Emptying a chunk via popFront does not implicitly popFront the chunk off
* main range. The chunk is still available via front, it is just empty.
*/
{
import std.algorithm.comparison : equal;
import core.exception : AssertError;
import std.exception : assertThrown;

auto a = [[0, 0], [0, 1],
[1, 2], [1, 3], [1, 4],
[2, 5], [2, 6],
[3, 7],
[4, 8]];

// Value input range
{
auto r = valInputRange(a).chunkBy!((a, b) => a[0] == b[0]);

size_t numChunks = 0;
while (!r.empty)
{
++numChunks;
auto chunk = r.front;
while (!chunk.empty)
{
assert(r.front.front[1] == chunk.front[1]);
chunk.popFront;
}
assert(!r.empty);
assert(r.front.empty);
r.popFront;
}

assert(numChunks == 5);

// Now front and popFront should assert.
bool thrown = false;
try r.front;
catch (AssertError) thrown = true;
assert(thrown);

thrown = false;
try r.popFront;
catch (AssertError) thrown = true;
assert(thrown);
}

// Reference input range
{
auto r = refInputRange(a).chunkBy!((a, b) => a[0] == b[0]);

size_t numChunks = 0;
while (!r.empty)
{
++numChunks;
auto chunk = r.front;
while (!chunk.empty)
{
assert(r.front.front[1] == chunk.front[1]);
chunk.popFront;
}
assert(!r.empty);
assert(r.front.empty);
r.popFront;
}

assert(numChunks == 5);

// Now front and popFront should assert.
bool thrown = false;
try r.front;
catch (AssertError) thrown = true;
assert(thrown);

thrown = false;
try r.popFront;
catch (AssertError) thrown = true;
assert(thrown);
}

// Ensure that starting with an empty range doesn't create an empty chunk.
{
int[] emptyRange = [];

auto r1 = valInputRange(emptyRange).chunkBy!((a, b) => a == b);
auto r2 = refInputRange(emptyRange).chunkBy!((a, b) => a == b);

assert(r1.empty);
assert(r2.empty);

bool thrown = false;
try r1.front;
catch (AssertError) thrown = true;
assert(thrown);

thrown = false;
try r1.popFront;
catch (AssertError) thrown = true;
assert(thrown);

thrown = false;
try r2.front;
catch (AssertError) thrown = true;
assert(thrown);

thrown = false;
try r2.popFront;
catch (AssertError) thrown = true;
assert(thrown);
}
}

// Issue 93532 - Using roundRobin/chunkBy
{
import std.algorithm.comparison : equal;
import std.range : roundRobin;

auto a0 = [0, 1, 3, 6];
auto a1 = [0, 2, 4, 6, 7];
auto a2 = [1, 2, 4, 6, 8, 8, 9];

auto expected =
[[0, 0], [1, 1], [2, 2], [3], [4, 4], [6, 6, 6], [7], [8, 8], [9]];

auto r1 = roundRobin(valInputRange(a0), valInputRange(a1), valInputRange(a2))
.chunkBy!((a, b) => a == b);
assert(r1.equal!equal(expected));

auto r2 = roundRobin(refInputRange(a0), refInputRange(a1), refInputRange(a2))
.chunkBy!((a, b) => a == b);
assert(r2.equal!equal(expected));

auto r3 = roundRobin(a0, a1, a2).chunkBy!((a, b) => a == b);
assert(r3.equal!equal(expected));
}

// Issue 93532 - Using merge/chunkBy
{
import std.algorithm.comparison : equal;
import std.algorithm.sorting : merge;

auto a0 = [2, 3, 5];
auto a1 = [2, 4, 5];
auto a2 = [1, 2, 4, 5];

auto expected = [[1], [2, 2, 2], [3], [4, 4], [5, 5, 5]];

auto r1 = merge(valInputRange(a0), valInputRange(a1), valInputRange(a2))
.chunkBy!((a, b) => a == b);
assert(r1.equal!equal(expected));

auto r2 = merge(refInputRange(a0), refInputRange(a1), refInputRange(a2))
.chunkBy!((a, b) => a == b);
assert(r2.equal!equal(expected));

auto r3 = merge(a0, a1, a2).chunkBy!((a, b) => a == b);
assert(r3.equal!equal(expected));
}

// Issue 93532 - Using chunkBy/map-fold
{
import std.algorithm.comparison : equal;
import std.algorithm.iteration : fold, map;

auto a = [0, 0, 1, 1, 1, 2, 2, 3, 3, 4, 4, 5, 6, 6, 6, 7, 8, 8, 9];
auto expected = [0, 3, 4, 6, 8, 5, 18, 7, 16, 9];

auto r1 = a
.chunkBy!((a, b) => a == b)
.map!(c => c.fold!((a, b) => a + b));
assert(r1.equal(expected));

auto r2 = valInputRange(a)
.chunkBy!((a, b) => a == b)
.map!(c => c.fold!((a, b) => a + b));
assert(r2.equal(expected));

auto r3 = refInputRange(a)
.chunkBy!((a, b) => a == b)
.map!(c => c.fold!((a, b) => a + b));
assert(r3.equal(expected));
}

// Issues 16169, 17966, 93532 - Using multiwayMerge/chunkBy
{
import std.algorithm.comparison : equal;
import std.algorithm.setops : multiwayMerge;

{
auto a0 = [2, 3, 5];
auto a1 = [2, 4, 5];
auto a2 = [1, 2, 4, 5];

auto expected = [[1], [2, 2, 2], [3], [4, 4], [5, 5, 5]];
auto r = multiwayMerge([a0, a1, a2]).chunkBy!((a, b) => a == b);
assert(r.equal!equal(expected));
}
{
auto a0 = [2, 3, 5];
auto a1 = [2, 4, 5];
auto a2 = [1, 2, 4, 5];

auto expected = [[1], [2, 2, 2], [3], [4, 4], [5, 5, 5]];
auto r =
multiwayMerge([valInputRange(a0), valInputRange(a1), valInputRange(a2)])
.chunkBy!((a, b) => a == b);
assert(r.equal!equal(expected));
}
{
auto a0 = [2, 3, 5];
auto a1 = [2, 4, 5];
auto a2 = [1, 2, 4, 5];

auto expected = [[1], [2, 2, 2], [3], [4, 4], [5, 5, 5]];
auto r =
multiwayMerge([refInputRange(a0), refInputRange(a1), refInputRange(a2)])
.chunkBy!((a, b) => a == b);
assert(r.equal!equal(expected));
}
}

}

// Issue 13595
Expand Down