Deprecate "Fix for the non-global template issue 5710"

[ibuclaw]

Reverts #9282

Breaking downstream compilers with reckless abandon is not acceptable. While I agree with the feature, the implementation should find a better solution other than having a dual context in the form of vthis2.

[dlang-bot]

Thanks for your pull request, @ibuclaw!

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 run digger -- build "master + dmd#9702"

[WalterBright]

I also don't want such dramatic changes when there are still bugs in the way nested functions are handled.

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

[thewilsonator]

I'd very much rather not revert the whole thing.

Which bits specifically are you not satisfied with?

[thewilsonator]

Unwarranted complexity added to the code generator,

I think this is par for the course with features that need associated codegen. If you think it can be made less complicated please be my guest.

AST rewriting done in the code generator,

This and this? Can be refactored to a function and moved to the frontend.

horrible implementation detail that makes debugging runtime impossible.

Please expand.

[thewilsonator]

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

If contracts were nested functions that bug wouldn't exist. They're halfway between a nested function and a goto in the name of performance. IIRC LDC doesn't have that bug because we do it like a nested function.

[WalterBright]

If contracts were nested functions that bug wouldn't exist.

Regardless of what the correct fix is, it remains unfixed and is causing problems. The simpler stuff needs to be fixed first before going on to massive upheaval.

[thewilsonator]

Regardless of what the correct fix is, it remains unfixed and is causing problems.

Yes.

The simpler stuff needs to be fixed

Yes

first before going on to massive upheaval.

No, these are completely uncorrelated, for one Interfaces will never have multiple contexts.

[12345swordy]

cc @SSoulaimane

[SSoulaimane]

@ibuclaw Unless you have a good alternative solution yourself. Just complaining that you have to do more work in the gdc backend is not solving anything. This is the best solution I could come with so far I have nothing else. What you have proposed in your other comments I spent time with it and it didn't turn out to be a workable solution, I have a snapshot of it if you can sort it out yourself https://github.com/SSoulaimane/dmd/tree/globtemplate_old even the unittests wouldn't compile. I know you're not going to work on it, I gave up on it, so let's just reopen that bugzilla issue instead right.

[SSoulaimane]

I also don't want such dramatic changes when there are still bugs in the way nested functions are handled.

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

This is unrelated. I can see if I can solve this but I'm pretty confident that the solution would be the same with or without this. The way I chose to handle this issue is the most straight forward for the frontend, the way proposed by Ian was not going to be straight forward for the frontend (in certain situations I just gave up) but he is contempt with it because he thinks he wouldn't have to do anything in the backend.

[SSoulaimane]

Breaking downstream compilers

This is not breaking anything, you can skip it for now if you don't have time to implement it in gdc, otherwise if it is really breaking something just point it out and I'll see how it can be fixed.

What matters is not where the extra work is added but what is the cleanest solution for both the frontend,the backend, and most importantly the user. This solution is clean and balanced between the frontend and the backend. The alternative solution just forget about it you might as well just reopen that bugzilla issue for the next decade. There is just no way I could find that doesn't involve the backend at least for newing aggregate types.

[SSoulaimane]

dramatic changes

What I like about this solution is that it doesn't change anything neither to the frontend nor to the backend, it just adds behavior to DMD. Everything DMD used to do before is exactly the same today it's even bug compliant.

[SSoulaimane]

I haven't seen any serious argument yet aside from not now or I don't have time for it or there might be some undiscovered magical solution that would just implement itself.

[SSoulaimane]

This is yet another argument that the compiler developers don't care about their users. Many people in the community don't care about your implementation problems they just want things to work. And I don't mind doing extra work in the implementation to make my users happy. I myself have used some heavy metaprogramming before and this issue very quickly turns your dreams into nightmares but how would you know if you don't listen to the community. This issue was dragging its feet for almost a decade If someone would have spent ten minutes a week on this it would have been fixed. And now that it got fixed let's revert that and drag it for another decade?

[12345swordy]

I against reverting this unless a better fix is provided. People are excited for this bug to fix:
https://forum.dlang.org/thread/egkpcemeofjffnyhsahk@forum.dlang.org

[ibuclaw]

I think this is par for the course with features that need associated codegen. If you think it can be made less complicated please be my guest.

Then merge and I will put it on my list for after gcc 9.1 is released. My expectation for any new feature would be #7079. And that includes having a DIP.

[aliak00]

@WalterBright @ibuclaw can you guys maybe suggest a simpler solution that’d work? Or what exactly makes this bad? or point out where it breaks downstream code? Just saying it’s too complex/dramatic is not very constructive and It sounds like @SSoulaimane is more than willing to cater to the issues if they were actually pointed out.

[ibuclaw]

They have been pointed out, and alternatives have been suggested in the introducing PR. The only response was "do it yourself".

[thewilsonator]

Then merge and I will put it on my list for after gcc 9.1 is released.

When is gcc 9.1 due in relation to the next DMD release? As long as this makes the next DMD release I have no concerns.

[ibuclaw]

https://gcc.gnu.org/ml/gcc/2019-04/msg00263.html

Temporary reversion until after gcc9.1 is released is fine.

[SSoulaimane]

They have been pointed out, and alternatives have been suggested in the introducing PR. The only response was "do it yourself".

The definition of recklessness is what you are doing here. Absent when the PR was being reviewed and now that it was merged you come to revert it and your justification is "there might be an alternative".
Whether there was an alternative or not this is too late the work is now merged and announced to the community. Next time bring you criticism during the review process.

Regardless I will overlook the unprofessionalism that is going on and I will highlight some of the reasons why I dropped my initial strategy.

The initial strategy was to make a different template instantiation for each this symbol. for example given the following definition S { int inc(alias a)() { ++a } } and variables S o, m; then o.inc!a is a different template instantiation than m.inc!a the two instantiations are completely different, the name mangling is different, the AST is different, and the generated machine code if also different.

This strategy was a can of worms. Here comes a small list of problems many of which are unsolvable design decision side effects.

• Type mismatch

class C
{
    class N(alias a)
    {
        void inc(){ ++ a; }
    }
}

unittest
{
    int a;
    C c = new C;

    static assert( !is(C.N!a == c.N!a) ); // not the same

    C.N!a n = c.new C.N!a() // nop, not the same
                            // `C.N!a` can't be new'd with `c`

    assert( typeid(C.N!a) != typeid(c.N!a) ); // completely different

    // Note: if `int a;` was changed `to enum a;` then all of these issues disappear.
    // Note: these design quirks don't exist in the current merged solution.
}

Q: do the users want to have to deal with this?
Q: should we revert a working clean solution? and ship a half backed solution because it doesn't "recklessly break downstream compilers"?

• Implementation problems
  Given the previous setup, I couldn't manage to get an alias of a nested type. for example alias T = new C().N!a; this wouldn't work because the type semantic only operates on types not expressions such as new C this would require a unknown amount of change in the frontend to make AST modifications in the type semantics and communicate these to the expression semantic.

• Performance concern
  with the current merged solution aggregates and functions have both the declaration and instantiation contexts available at cost of one pointer indirection each. For example:

unittest
{
    int i;
    void set(alias a) { i = a; }

    void nest0()
    {
        void nest1()
        {
            // void nest...()
            void nestn()
            {
                int a = 1;
                set!a();
            }
        }
    }
}

With the current merged solution i = a is compiled to this[0].i = this[1].a.
With my initial strategy (reproposed by Ian as alternative) i = a would be compiled to this.this.this....i = this.a the performance impact gets bigger as the distance between the declaration and the instantiation contexts grows. A win for the merged solution and a loss for the alternative.

You are undermining the effort that has been put into this and just rejecting it all together with the excuse that an alternative could work.

[SSoulaimane]

Breaking downstream compilers with reckless abandon

This PR hasn't broken anything that DMD used to do before. The same mechanisms that were used to fetch and assign the first context pointer were reused to do the same with the second context.

The backend addition were just a handful of code so I don't see much credibility in your complaint. And thus I don't see a need to do a whole rewrite and a re-shift of the fundamental strategy because of this.

[ibuclaw]

Next time bring you criticism during the review process.

I brought up my laments during the review, but they were ignored.

[SSoulaimane]

I brought up my laments during the review, but they were ignored.

Look at the last replies in #9282 and see whose were they.

[kinke]

The long-standing user-side issue being fixed is a big win, nobody is questioning that, so thanks a lot @SSoulaimane.

I haven't looked at in detail, but I noticed the C++-exposed class changes didn't make it into the C++ headers, so that's one reason it wasn't really mergeable yet.

Wrt. back/frontend, I'm (obviously) on Iain's side and prefer as much rewrites/lowerings etc. in the frontend. The changes in dmd/{e2ir,toir}.d aren't just a 'handful' of lines, but rather something like 150 lines, which would need to be understood and duplicated by both GDC and LDC.

[SSoulaimane]

but I noticed the C++-exposed class changes didn't make it into the C++ headers

I might have missed some.

prefer as much rewrites/lowerings etc. in the frontend.

This has been considered but didn't turn out to be workable.

The changes in dmd/{e2ir,toir}.d aren't just a 'handful' of lines, but rather something like 150 lines, which would need to be understood and duplicated by both GDC and LDC.

CC me for assistance.

[ibuclaw]

Unpicking the deletion of ~1700 lines of code will take a long time now as the code has diverged quite a bit since the original revert PR. I've checked again and there is still no compiler support or spec (or proper tests) for this feature, so now this PR has been turned into a deprecation.

[12345swordy]

Do we have no means of contacting the original author on this?

[12345swordy]

Ok let us assert that the author is gone permanently. Is there no other solution to this same problem?
@ibuclaw @thewilsonator

[ibuclaw]

Ok let us assert that the author is gone permanently. Is there no other solution to this same problem?
@ibuclaw @thewilsonator

The only thing I've got in my notes from the countless times I've tried to implement this is:

fix getRightThis support in front end to fill in outer class deferences.

What it means is that the front-end destroys the pointer chain in this[0].this[1].this[0].foo for dual-context class (and struct) members, it doesn't do this for single context class members (this.this.this.foo). If this was done, maybe I wouldn't need to rewrite from scratch some ~1000 lines of code in gdc (and probably ldc too), but it's rather hard to tell.

[12345swordy]

Ok let us assert that the author is gone permanently. Is there no other solution to this same problem?
@ibuclaw @thewilsonator

The only thing I've got in my notes from the countless times I've tried to implement this is:

fix getRightThis support in front end to fill in outer class deferences.

What it means is that the front-end destroys the pointer chain in this[0].this[1].this[0].foo for dual-context class (and struct) members, it doesn't do this for single context class members (this.this.this.foo). If this was done, maybe I wouldn't need to rewrite from scratch some ~1000 lines of code in gdc (and probably ldc too), but it's rather hard to tell.

If a PR request was submitted so that the front-end didn't destroy the pointer chain for `this[0].this[1].this[0].foo' then would it be easier for you and the ldc folks to implement this then?

[12345swordy]

@ibuclaw can you tell me where the front-end destroy the pointer chain for dual-context class

[ibuclaw]

@ibuclaw can you tell me where the front-end destroy the pointer chain for dual-context class

It would be constructed in getRightThis.

[12345swordy]

LGTM. I am struggling to figure out how this works btw.

[ibuclaw]

LGTM. I am struggling to figure out how this works btw.

Struggling to figure out how the dual context implementation works - along with its many spun-off PRs to help reduce the final diff (toParentLocal, toParentDecl, etc...)?

Welcome to the club. :-)

[ibuclaw]

FYI I've tried out many alternative implementations to get around the void*[2]* type.

The reason why the second context can't be just an extra field in the main frame/closure pointer is because a dual context could be made up of two classes.

The reason why the second context can't be passed as an ordinary (hidden) parameter is because that would break covariance with delegates.

Really it should be a struct { void*, void* }* type, but if any of the contexts are a struct/class, then the type should be updated accordingly (this would be a win for gdb too).

It annoys me greatly that actually, the method seems to be the only way to do it, but the implementation of is just a tangle of spaghetti touching dozens of places throughout the codebase without any regard for encapsulation. And as a result nobody knows what is really going on.

[12345swordy]

I don't have the ability to add automerge to this
Cc @thewilsonator

[ibuclaw]

Ping @WalterBright and @atilaneves on their opinions.

[atilaneves]

It's hard for me to have an opinion when I missed nearly all of this the first time around.

[12345swordy]

@ibuclaw I believe @WalterBright has given the approval to merge this PR, already. If not convinced by this just email the dude until he respond.
My opinion is to merge this with the message that the current fix has been deprecated until the d language foundation decide to go next when it comes to this particular bug.

[Geod24]

It pains me that we have to go this way, but until someone steps up and fixes this, it's going to be another thorn in our side.
Thanks @ibuclaw for your extensive work towards researching this and documenting all the issues you encountered. I'm sure it'll be useful to someone in the future.

[dnadlinger]

Surely the right thing to do here now is to actually work together on implementing this in all compilers, rather than planning on reverting the feature? By the way, if memory serves, I don't think the changelog entry is accurate: No spec change is needed, as issue 5710 was an undocumented limitation – the exact inner workings of the context mechanism aren't something the spec needs to concern itself with (apart from in order to specify an ABI, which we don't do).

I'm confident I could implement a solution in LDC without too much of an issue (and considered doing so before the DMD change was even proposed), but sadly, I don't have too much time to spend on random OSS projects right now. Yes, solving this will require substantial changes to each compiler's context emission implementation, but that's to be expected, as the whole issue here is a fundamental limitation in the current approach. Iain's nice test case collection should help as well.

[CyberShadow]

This wording is not OK and ought to be changed.

• The world does not revolve around GDC+LDC. As pointed out, many people use exclusively DMD, or are happy to use exclusively DMD in order to take advantage of its exclusive features.
• No language specification is needed for this feature because its absence is an implementation bug. The limitation is emergent from the current design of the DMD frontend.

[CyberShadow]

#12111

[ibuclaw]

No spec change is needed, as issue 5710 was an undocumented limitation

Wrong, issue 5710 is a documented limitation. The D language spec explicitly says that code enabled by the PR for issue 5710 should not be compilable.

[CyberShadow]

Thanks for the correction.

Still, it is documented as a limitation, so the only change needed would be to remove the section.

I think it would still be correct to say that "no language specification that describes the feature" is needed. All there is currently is documentation describing the feature's absence, i.e., it describes the shape of the hole in our current implementation. :)

[dnadlinger]

Also, it was added long after the feature was introduced (and clearly to document a current limitation over what should be possible, as Vladimir pointed out): dlang/dlang.org#479

[jmh530]

This is not listed on the list of deprecated features here