fix foreach closures

[brianush1]

Summary

This PR fixes the age-old bug:

int delegate()[] arr;
foreach (i; 0 .. 10) {
  arr ~= { return i; };
}
writeln(arr[3]()); // prints 9????

Modeled heavily after the opApply-based foreach loop implementation (which has never had this closure bug!)

Code is AI-generated but it seems super reasonable to me and works on my codebase

[zero-vector]

This visitor looks too simple.
If it is enough, I would suggest a comment, why this is sufficient.

[adamdruppe]

so my thought here is that it is doing a conservative sweep to find anything that might be capturing a variable. so a function call might (though not necessarily), mixin, etc. maybe it is missing something but i think it is more likely over broad than under.

[zero-vector]

Yes it is quite eager, for example, any function declaration in the loop will trigger it.
Imho, it also should probably use StoppableVisitor if possible, to not traverse after first positive hit.

[zero-vector]

foreach inside a foreach, I think a comment would help here

[adamdruppe]

looks to me that it is tricking dmd into calling the opApply instead.

so basically the patch is like if this might be a closure, rewrite the loop into something that triggers opApply processing

so think, here's the normal failure we expect:

void main() {
        void delegate()[] tricks;
        foreach(i; 0 .. 10) {
                tricks ~= () { import arsd.core; writeln(i); };
        }

        foreach(trick; tricks)
                trick();
}

prints out a bunch of 9's. But now, do the same thing but with opApply instead:

void main() {
        void delegate()[] tricks;

        int magic(int delegate(int) dg) {
                foreach(i; 0 .. 10) {
                        if(auto ret = dg(i))
                                return ret;
                }
                return 0;
        }

        foreach(i; &magic) {
                tricks ~= () { import arsd.core; writeln(i); };
        }

        foreach(trick; tricks)
                trick();
}

and it magically works.

So you can see the pattern here: make the magic function taking the args which are the same type as the foreach variable. Take the foreach item itself and put it inside - you can see my foreach(i; 0 .. 10) was just moved.

Then rewrite the original to call that opApply delegate.

So thinking about it this way, can we read the patch to confirm this is actually what it is doing? tbh i haven't even tried to compile it yet but i think this is gonna be it

[adamdruppe]

thatks kinda weird

[adamdruppe]

so what it is calling a "callback" is basically my int magic function

tho why then is Parameters empty?

[adamdruppe]

p sure this is teh opApply magic, internally it is like a switch for break/continue to remember where it was.

[adamdruppe]

but now if im right about it reusing existing opApply concepts, why does this function have to exist? it should be somewhere else already......

[adamdruppe]

Taking the ideas discussed here but rewriting most the impl.......
dg.patch

here it is so far, needs a lot more testing.

[adamdruppe]

worth noting the test case from this PR does fail with my patch rn.

also i expect mixes of attributes and crap are liable to break.

[adamdruppe]

also want to restrict the needs closure visitor to when it actually captures the loop variable. other locals i don't think are as important for the bug purposes cuz the opapply rewrite should magic it.

[adamdruppe]

so i put my impl up to compare: #126

but my impl breaks when compiling work projects due to trusted lambdas and nothrow crap

and this impl works on it........ so sigh maybe i should take this particular approach after all.

[adamdruppe]

omg it doesn't set this in the above new branch....... it would have introduced a bug in ldc!! finally found something the gpt just plain did wrong lolololol

[adamdruppe]

I copied some of this and reworked some of it, i think im happy with this other pr now: https://github.com/opendlang/opend/pull/127/changes