Fix for the non-global template issue 5710

[SSoulaimane]

Good to go.

I dropped the initial strategy instead I decided to use a static array of two pointer void*[2]* and making this one a closure variable thus it will be GC-allocated only when the function escapes.

example:

struct S
{
    auto func(alias a)() { ++a; }
}

void noClosure()
{
    int a;       // stack allocated
    auto s = S();
                // v implicit var `void*[2] __this;` stack allocated
    s.func!a(); // no closure made
}

void withClosure()
{
    int a;               // closure var GC allocated
    auto s = S();
                         // v implicit closure var `void*[2] __this;` GC allocated
    auto dg = &s.func!a; // closure made
}

See unittests for exmaples.

[dlang-bot]

Thanks for your pull request and interest in making D better, @SSoulaimane! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	5710	normal	cannot use delegates as parameters to non-global template

Testing this PR locally

If 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 + dmd#9282"

[12345swordy]

Reference the bug report in regards to fixing bugs please.

[SSoulaimane]

Reference the bug report in regards to fixing bugs please.

done.

[UplinkCoder]

when you want to go ahead with this, search for examples that it should miscompile.

[SSoulaimane]

when you want to go ahead with this, search for examples that it should miscompile.

Yes. Same template arguments with a different this is probably a good candidate.

[marler8997]

This doesn't work:

import core.stdc.stdio : printf;

struct Adder
{
    int base;
    // works
    uint addBaseCallFunc(alias func)(uint value)
    {
        return func(base + value);
    }

    // these 2 don't work
    uint willNeedThreeContextPtrs(alias func)(uint value)
    {
        int localVar = func(value);
        uint localFunc(uint subValue) { return localVar + subValue; }
        return addBaseCallFuncs!(func, localFunc)(value);
    }
    uint addBaseCallFuncs(alias func1, alias func2)(uint value)
    {
        return func2(func1(base + value));
    }
}

void main()
{
    auto adder = Adder(3);

    int offset = 6;
    uint doOffset(uint value) { return offset + value; }

    // works
    printf("%d\n", adder.addBaseCallFunc!doOffset(4));

    // need 3 context pointers (doesn't work)
    printf("%d\n", adder.willNeedThreeContextPtrs!doOffset(4));
}

[marler8997]

Here's another one that seems to compile, but fails at runtime (it doesn't print anything):

import core.stdc.stdio : printf;

struct Adder
{
    int base;
    uint addBaseCallFuncs(alias func1, alias func2)(uint value)
    {
        return func2(func1(base + value));
    }
}

void main()
{
    auto adder = Adder(3);

    int offset = 6;
    uint doOffset1(uint value) { return offset + value; }

    void anotherFunc()
    {
        int anotherVar = 11;
        uint doOffset2(uint value) { return anotherVar + value; }
        printf("%d\n", adder.addBaseCallFuncs!(doOffset1, doOffset2)(4));
    }
}

[SSoulaimane]

@marler8997 good catch!
In the second example you forgot to call anotherFunc().
Try again. All should work now.
Looking forward to your next case.

[SSoulaimane]

Unwarranted complexity added to the code generator,

Nothing unusually complex.

AST rewriting done in the code generator,

Nothing is rewritten. Just making a VarExp from a VarDeclaration because of a DMD backend limitation is not rewriting the AST, and that variable (vthis2) was inserted into the AST anyway.

horrible implementation detail that makes debugging runtime impossible.

I'm not familiar enough with gdb but I responded earlier.

There are any number of ways that this can be handled without having a dual context that the backend needs to be aware of. I do not believe for a minute that this couldn't be done with only one vthis reference.

That was the initial strategy but I dropped it because it was not going to be seamless for the users.

This adds needless duplication of time and effort in the realm of hundreds of lines of code additions per implementing compiler. That is not acceptable by any measure, and is frankly wasting our time.

I'm not familiar with the gdb backend but the frontend is shared.

[ibuclaw]

Nothing unusually complex.

Breaking already working compilers is a complex change.

That was the initial strategy but I dropped it because it was not going to be seamless for the users.

Why?

You could have just added a vthis field to TemplateInstance, and then added a new case for isTemplateInstance() in the existing machinery that fetches the correct this to use. All done with only a few lines of changes to the backend.

I'm not familiar with the gdb backend but the frontend is shared.

That is no excuse to be breaking other people's code.

[SSoulaimane]

Breaking already working compilers is a complex change.

That is no excuse to be breaking other people's code.

It's not supposed to break existing code.

You could have just added a vthis field to TemplateInstance, and then added a new case for isTemplateInstance() in the existing machinery that fetches the correct this to use. All done with only a few lines of changes to the backend.

I dropped that solution, It was easy to implement but it entailed subtle symbol and type quirks. It was not possible to new classes (without involving expressions in the type semantic). And types were on jeopardy, types would mismatch if found under instantiations which were made with different vthis expressions even when they are perfectly identical typewise.

Example:

struct S
{
    auto inner(alias a)()
    {
        class I {}
        return new I;
    }

    unittest
    {
        // type mismatch
        auto a = 0;  // if changed to enum a = 0; the error goes away
        auto obj0 = S();
        auto obj1 = S();
        alias I0 = typeof(obj0.inner!a());
        alias I1 = typeof(obj1.inner!a());
        static assert(is(I0 == I1)); // error
    }
}

[ibuclaw]

And types were on jeopardy, types would mismatch if found under instantiations which were made with different vthis expressions even when they are perfectly identical typewise.

Bugs in type merging have been reported a while back, and need to be addressed, not constantly worked around by adding more to the pile.

https://issues.dlang.org/show_bug.cgi?id=19021

[SSoulaimane]

Bugs in type merging have been reported a while back, and need to be addressed, not constantly worked around by adding more to the pile.

Sure but this was an inevitable design decision problem that I can't see ever being solved because with that strategy the instantiation trees are in reality different from the AST to mangling to the codegen. In short you either add more work and hacks to the frontend or to the backend and I chose the cleaner solution for the user. I don't mind extra work. This issue was dragging it's feet for almost a decade, so just a few more lines of code in the backend to get it right was no big deal at least for me.

[ghost]

This causes a new ICE, see https://issues.dlang.org/show_bug.cgi?id=20063

[SSoulaimane]

This causes a new ICE, see https://issues.dlang.org/show_bug.cgi?id=20063

#10196

[kinke]

Looks like it also enables code leading to another ICE wrt. fields from base classes: https://issues.dlang.org/show_bug.cgi?id=20333

[SSoulaimane]

Thanks. I noticed that issue. It's not an ICE in DMD. Although it appears to be error on valid behaviour.
getEthis() seems aware of base classes

dmd/src/dmd/toir.d

Line 285 in 73be029

if (cd && cdx && cdx.isBaseOf(cd, null))

I'll spare some time to investigate it when I can.

[kinke]

Thx. I wasn't sure how this is supposed to work, so played with it, and it works fine for a non-inherited field: https://run.dlang.io/is/SEsfqG

[SSoulaimane]

You know what's interesting? At the time, I implemented the logic of getEthis() inside getRightThis() similar to what the inliner does in visit(VarExp) then I retracted it, I don't remember exactly the reason.