Bug 21912 – delegate assigned to return scope variable needs closure

Status
RESOLVED
Resolution
FIXED
Severity
critical
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2021-05-10T19:01:17Z
Last change time
2022-06-22T09:45:31Z
Keywords
pull
Assigned to
No Owner
Creator
Per Nordlöw

Comments

Comment #0 by per.nordlow — 2021-05-10T19:01:17Z
Compiled with -dip1000 -dip25 -dip1008, the code @safe: import std.stdio; auto foo(DG)(DG dg, int p) { return () => dg(p); } auto bla(int i) { return foo((int p)=>i*p, 66); } void main() { writeln(bla(5)()); } should produce 5*66 = 330. But instead produces seemingly random values. See https://run.dlang.io/?compiler=dmd&source=@safe:%0Aimport%20std.stdio;%0A%0Aauto%20foo(DG)(DG%20dg,%20int%20p)%20%7B%0A%20%20%20%20return%20()%20%3D%3E%20dg(p);%0A%7D%0A%0Aauto%20bla(int%20i)%20%7B%0A%20%20%20%20return%20foo((int%20p)%3D%3Ei*p,%2066);%0A%7D%0A%0Avoid%20main()%20%7B%0A%20%20%20%20writeln(bla(5)());%0A%7D&args=-dip1000%20-dip25%20-dip1008
Comment #1 by mail — 2021-05-10T19:22:08Z
The above is a reproduction of a problem I have seen occur several times now. It always involves a delegate that refers to a parameter. D is supposed to move the parameter to the heap, creating a closure. But as you can see above, in more complex cases it produces garbage.
Comment #2 by moonlightsentinel — 2021-05-10T19:38:49Z
Slightly reduced: @safe: auto foo(DG)(DG dg, int p) { return () => dg(p); } auto bla(int i) { return foo((int p)=>i*p, 66); } void main() { assert(bla(5)() == 330); } ===================================== Only requires -dip1000 which probably hints at invalid scope inference.
Comment #3 by moonlightsentinel — 2021-05-10T19:49:47Z
Even smaller: ================================= DG foo(DG)(DG dg) { return dg; } auto bla(int i) { return foo(() => i); } void main() { assert(bla(5)() == 5); } =================================
Comment #4 by dkorpel — 2021-05-12T13:26:23Z
This has to do with pure bypassing scope checks: ``` int delegate() pure foo(int delegate() pure dg) pure { return dg; } int delegate() bla(int i) pure { return foo(() => i); } void main() { assert(bla(5)() == 5); } ``` *** This issue has been marked as a duplicate of issue 20150 ***
Comment #5 by mail — 2021-05-14T19:42:07Z
Could you explain what that has to do with the compiler deciding not to allocate the closure?
Comment #6 by dkorpel — 2021-05-14T20:17:37Z
(In reply to Sebastiaan Koppe from comment #5) > Could you explain what that has to do with the compiler deciding not to > allocate the closure? The closure is not allocated because parameter `dg` of `foo` is inferred `scope`. `dg` is inferred `scope` because `foo` is inferred (or annotated) `pure`. The faulty logic that `pure implies scope` is described in the linked issue. See also the forum post I made about it: https://forum.dlang.org/post/[email protected]
Comment #7 by mail — 2021-05-15T08:18:54Z
I read the post, and it sure looks plausible, I just don't understand it. If this has to do with "pure bypassing scope checks", then making one of the above examples un-pure would solve it. Although it does work with your reduced case, not with the ones where foo has a DG template parameter. That still crashes: ``` void unpure() @trusted { static int g; g = 5; } DG foo(DG)(DG dg) { unpure(); return dg; } auto bla(int i) { return foo((){ unpure(); return i;}); } void main() { pragma(msg, typeof(bla)); // @safe int delegate() @safe(int i) assert(bla(5)() == 5); // Assertion failure } ``` What does work though, is to assign the delegate to a variable first. This compiles (also with the other DG template examples): ``` int delegate() pure foo(int delegate() pure dg) pure { return dg; } int delegate() bla(int i) pure { auto dg = () => i; return foo(dg); } void main() { assert(bla(5)() == 5); // Pass! } ```
Comment #8 by dkorpel — 2021-05-15T13:58:30Z
(In reply to Sebastiaan Koppe from comment #7) > If this has to do with "pure bypassing scope checks", then making one of the > above examples un-pure would solve it. Although it does work with your > reduced case, not with the ones where foo has a DG template parameter. You're right, it turns out there is another issue at play. Reduced to explicit attributes: ``` int delegate() foo(return int delegate() dg) { return dg; } int delegate() bla(int i) { return foo(() => i); } void main() { assert(bla(5)() == 5); } ``` Since () => i is assigned to a `return` parameter, the expression `foo(() => i)` should be `scope` and cannot be returned from `bla` without allocating a closure with i on the heap. I quickly looked at the existing dip1000 issues but couldn't find something similar enough, so let's re-open it.
Comment #9 by dlang-bot — 2021-07-15T22:00:28Z
@dkorpel created dlang/dmd pull request #12880 "Fix issue 21912 - delegate assigned to return scope variable needs closure" fixing this issue: - Fix issue 21912 - delegate assigned to return scope variable needs closure https://github.com/dlang/dmd/pull/12880
Comment #10 by dlang-bot — 2021-07-22T01:04:04Z
dlang/dmd pull request #12880 "Fix issue 21912 - delegate assigned to return scope variable needs closure" was merged into stable: - 12221d04c4c7259ebd7f7570e2555659142d8516 by dkorpel: Fix issue 21912 - delegate assigned to return scope variable needs closure https://github.com/dlang/dmd/pull/12880
Comment #11 by dlang-bot — 2021-08-03T02:32:12Z
dlang/dmd pull request #12944 "Merge branch 'stable' into merge_master" was merged into master: - f8d4e318a5bc8c71fe20b56ec9fc3f4400afe83d by Dennis: Fix issue 21912 - delegate assigned to return scope variable needs closure (#12880) https://github.com/dlang/dmd/pull/12944
Comment #12 by johanengelen — 2021-12-22T14:38:11Z
I do not believe the linked PR a correct fix for the problem, and I do not agree (in all cases) with what the title of this bug report states. The linked PR fixes the problem of bad codegen, but over-constraints and limits correct use cases. Whether a delegate assigned to return scope parameter needs a closure or not depends on what is done with the return value, that is exactly what the return attribute is for. The return attribute results in the return value being scope in the caller. If it the return value escapes from the caller, only then is a gc-allocated closure needed. ``` // This should compile fine, without gc allocation: auto invoker(Dlg)(scope Dlg dlg) { return dlg; } // <--- `return` is infered on parameter @nogc void f(int x) { invoker({x++;}); } // <--- return value is discarded, hence does not escape the scope. // This should compile fine, without gc allocation: auto invoker(Dlg)(scope Dlg dlg) { return dlg; } // <--- `return` is infered on parameter @nogc void f(int x) { scope inv = invoker({x++;}); inv(); } // This should give a compile error: auto invoker(Dlg)(scope Dlg dlg) { return dlg; } @nogc auto f(int x) { return invoker({x++;}); } // <--- Error: `scope` variable escapes scope OR allocation in @nogc ``` The breakage of the fix is perhaps larger than expected because return is still infered on template parameters, without -dip1000.
Comment #13 by johanengelen — 2022-06-22T09:45:31Z
Filed new issue for the above. https://issues.dlang.org/show_bug.cgi?id=23204