Fix issue 18751: chunkBy predicate cannot access local variable. #6441
Conversation
quickfur
requested review from
andralex,
JackStouffer,
wilzbach and
PetarKirov
as code owners
|
Thanks for your pull request, @quickfur! Bugzilla references
Testing this PR locallyIf 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#6441" |
std/algorithm/iteration.d
Outdated
| @@ -1705,6 +1705,63 @@ if (isInputRange!Range && !isForwardRange!Range) | |||
| } | |||
| } | |||
|
|
|||
| // Inner range | |||
| struct ChunkByGroupImpl(alias eq, Range, Impl) | |||
std/algorithm/iteration.d
Outdated
| @@ -1883,6 +1887,19 @@ if (isForwardRange!Range) | |||
| assert(grp1.save.equal([1, 3, 5])); | |||
| } | |||
|
|
|||
| // Issue 18751 | |||
| unittest | |||
There was a problem hiding this comment.
Ouch. I added @safe and suddenly dmd is complaining:
std/algorithm/iteration.d(1899): Error: @safe function std.algorithm.iteration.__unittest_L1891_C7 cannot call @system function std.algorithm.iteration.__unittest_L1891_C7.chunkBy!((i, j) => data[i] == data[j], int[]).chunkBy
std/algorithm/iteration.d(1899): Error: @safe function std.algorithm.iteration.__unittest_L1891_C7 cannot call @system destructor std.algorithm.iteration.__unittest_L1891_C7.ChunkByImpl!(__lambda1, int[]).ChunkByImpl.~this
std/algorithm/iteration.d(1900): Error: @safe function std.algorithm.iteration.__unittest_L1891_C7 cannot call @system function std.algorithm.comparison.equal!(equal).equal!(ChunkByImpl!(__lambda1, int[]), int[][]).equal
std/algorithm/iteration.d(1900): Error: @safe function std.algorithm.iteration.__unittest_L1891_C7 cannot call @system generated function std.algorithm.iteration.__unittest_L1891_C7.ChunkByImpl!(__lambda1, int[]).ChunkByImpl.__fieldPostblit
I've no idea what happened; I literally just copy-n-pasted the implementation from the original code pretty much untouched, except to add template arguments so that it will compile.
There was a problem hiding this comment.
(this is due to its use of RefCounted under the hood which isn't @safe)
|
I can't figure it out. What is it about |
|
I give up. I've no idea how to coax the compiler to accept the code in all cases. It seems that fixing one issue leads to another, and fixing the other leads back to the first problem. Any help would be appreciated. This issue is making |
|
This looks like something that needs to be fixed in the compiler rather than the library. Voldemort types shouldn't (in a perfect world) affect this. |
|
I agree, but this is a well-known current limitation. There have been a good number of Phobos PRs that move Voldemorts into private module-level templates precisely because of this problem. However, what I don't understand is, why it's still failing even after I moved the Voldemort out, or worse yet, why it works as a nested struct but not as a separate template. If this can be fixed in the compiler, it would be wonderful. In the meantime, though, |
|
I’d post this code to the bug report and move it to a DMD bug. I’ll close this as it seems the common way to fix this type of issue is a breaking change. |
|
Fine. Though IIRC, the whole thing is one massive hack anyway (allowing alias parameters to bind to lambdas that access the caller's local context), so the chances of this actually getting fixed in dmd are pretty slim. |
The reason is that the inner range of
ChunkByImplmust be a global template, otherwise we run into the infamous problem of nested templates being unable to access the local context of an alias template parameter.The true fix is to fix the compiler to be able to handle this properly, but given the age of this issue, I'm not holding my breath on it. And in the meantime,
chunkByneeds to be usable in current real-world code.