[WIP] feat: capture transient values in loop block for closures
this is a fix to #1135 .
package main
func main() {
var fns []func()
for _, v := range []int{1, 2, 3} {
x := v*100 + v
fns = append(fns, func() { println(x) })
}
for _, fn := range fns {
fn()
}
}
As discussed in #1135, the go behaviour (expected) should is:
101
202
303
while gno behavior:
303
303
303
The reason for this is the funcLit inside loop block is not capturing every transient values of v as the loop went on, It only keeps a reference to it. when this funcLit is in execution, the value of v is 3, so the result will always be 303.
This PR provides a solution to capture the transient state as the loop went on, and use these values when the closure is in execution.
The work flow is:
- Identify the specific pattern. It's always showing up with a loop block and a nested funcLit.
- Record transient values for captured named vars. the transient state of v is recorded as [1, 2, 3];
- Use the recorded values to update context. when
fn(the closure) is in execution, using [1, 2, 3] to update the context block(in whichv== 3) to be 1, 2, 3 , and get the expected results.
Here are some examples to show in detail:
- this is generally same with #1135, the fix makes it consistent go Go.
package main
// this is a fix, intuitive, and same with Go
func main() {
var fns []func() int
for i := 0; i < 5; i++ {
x := i
f := func() int {
return x
}
fns = append(fns, f)
}
for _, fn := range fns {
println(fn())
}
}
// Go Output:
// 0
// 1
// 2
// 3
// 4
// Output:
// 0
// 1
// 2
// 3
// 4
- this is a fix to make it consistent with Go's experimental feature of
loopVar. It's a design decision if we want this.
package main
// this is a fix, intuitive, same with go loopVar experiment feature.
func main() {
var fns []func() int
for i := 0; i < 5; i++ {
f := func() int {
return i
}
fns = append(fns, f)
}
for _, fn := range fns {
println(fn())
}
}
// Go Output(without loopVar flag):
// 5
// 5
// 5
// 5
// 5
// Output:
// 0
// 1
// 2
// 3
// 4
- this one is consistent with Go too, but from an intuitive perspective, It should capture
xthe var declared outside of the for block, I'd like to leave it for discussion since it's a design decision.
package main
// this still keeps consistent with go, that a variable out of loop block is not captured
// this is unintuitive(should capture something)
// TODO: leave it for discuss.
func main() {
var fns []func() int
var x int
for i := 0; i < 5; i++ {
x = i
f := func() int {
return x
}
fns = append(fns, f)
}
for _, fn := range fns {
println(fn())
}
}
// Go Output:
// 4
// 4
// 4
// 4
// 4
// Output:
// 4
// 4
// 4
// 4
// 4
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 44.75%. Comparing base (
6452476) to head (dc61511). Report is 124 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #1585 +/- ##
===========================================
- Coverage 55.87% 44.75% -11.13%
===========================================
Files 430 438 +8
Lines 65618 67250 +1632
===========================================
- Hits 36667 30100 -6567
- Misses 26083 34637 +8554
+ Partials 2868 2513 -355
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@ltzmaxwell amazing! 🤩 Thanks as always for tackling these difficult VM issues.
Some things I really like about this:
- it seems like you have a pretty clear understanding of the issue and the approach to fixing it
- the added tests are super helpful to understand behavior and guard against future regressions
I spent quite a bit of time reviewing your solution, as well as time trying to understand both the problem a bit more. After thinking about this, I arrived at a related but different way to solve the problem using fewer abstractions. Do you think that you could give it a look? Perhaps there is a combined solution that would work well; the code you wrote here certainly made it easier for me to commit my own thoughts to code. https://github.com/deelawn/gno/pull/3/files
Also, I was thinking about how a similar issue might arise that doesn't use explicit loops. Consider the following example:
package main
func main() {
var y, counter int
var f []func()
defer func() {
for _, ff := range f {
ff()
}
}()
LABEL:
if counter == 5 {
return
}
x := y
f = append(f, func() { println(x) })
y++
counter++
goto LABEL
}
// Output:
// 0
// 1
// 2
// 3
// 4
The solution you've presented here does not account for this scenario. How easy do you think it would be to incorporate it?
I'm currently not set on either solution; there may be an even better way than both of us have thought of. I'm hoping some others can post their thoughts here as well, so let me know what you think 😄
Just a status update: @jaekwon discussed with maxwell for an alternative approach, which fixes this issue in Preprocessing. @ltzmaxwell, please do ping us for a second review when done!
Just a status update: @jaekwon discussed with maxwell for an alternative approach, which fixes this issue in Preprocessing. @ltzmaxwell, please do ping us for a second review when done!
Make it draft until done.
house keeping.