traefik / yaegi

Yaegi is Another Elegant Go Interpreter
https://pkg.go.dev/github.com/traefik/yaegi
Apache License 2.0
6.94k stars 343 forks source link

automatic loop variables in for loops ("loop var"), consistent with go 1.22 behavior #1645

Closed rcoreilly closed 1 month ago

rcoreilly commented 1 month ago

This builds on #1644 and adds automatic per-loop variables that are consistent with go 1.22 behavior. See #1643 for discussion.

This is still a draft because the for7 version ends up capturing the per-loop var values when they are +1 relative to what they should be. Maybe somehow the incrementing and conditional code is somehow capturing the within loop variables and incrementing them? not sure how that would work. anyway, need to investigate further before this is ready to go, but pushing it here in case there are other issues or someone might figure out this bug before I do..

rcoreilly commented 1 month ago

note that this one is ready to go now. it has the mysterious error on the "main / checks code and generated code" test which doesn't replicate locally, and the test doesn't report any specific issues to fix, so I'm not sure how to proceed on that.

mvertes commented 1 month ago

In the logs from CI, the tests fail with the following:

=== RUN   TestExportClosureArg
    interp_issue_1634_test.go:57: 
        Got: "0\n0\n0\n",
         want: "0\n1\n2\n"
--- FAIL: TestExportClosureArg (0.00s)

I have not investigated further.

rcoreilly commented 1 month ago

it appears that the closure PR interacts with this one, wrt the definition of shadow local variables:

    for i := 0; i < 3; i++ {
        i := i // eliminate this or use a variable with a different name here and it works
        tmp.Func(&fs, func() { println(i) })
    }

will investiage.

rcoreilly commented 1 month ago

actually it looks like all of the for loop closure tests used a new variable name (e.g., a := i) and so this issue was not caught by the prior tests, and still occurs if you undo the closure PR. Needs an additional tweak to this PR and some tests!

mvertes commented 1 month ago

The test case TestInterpConsistencyBuild/closure20.go fails in CI in oldstable which means go1.21, where range on numbers is not yet supported for the go compiler version of the test.

It's possible to blacklist the test (see interp/interp_consistency_test.go), or hold this PR until release of go1.23, which will force the removal of go1.21. Or get rid of TestInterpConsistencyBuild which is mostly redundant and a bit of a PITA.

traefiker commented 1 month ago

:no_entry_sign: the milestone is missing

rcoreilly commented 1 month ago

Thanks and thank you so much for writing this amazing system! Incidentally I had written something similar to this in overall structure, for C++ (which I called "C Super Script" or CSS, just before the internet started...). It is so much nicer to be working in Go, and the ability to bidirectionally interoperate with compiled code is so powerful.