Interpreted closures mixed with "binary" functions don't capture values correctly

[theclapp]

The following test case TestIssue1634 in interp/interp_eval_test.go triggers an unexpected result

func TestIssue1634(t *testing.T) {
	TLogf := t.Logf

	type CPE struct {
		E string
	}

	type CP struct {
		Es []*CPE
	}

	type Child struct {
		I int
		E string
		F func() string
	}

	FL := func(children []*Child) []string {
		s := []string{}
		for _, child := range children {
			s = append(s, child.F())
		}
		return s
	}

	R := func(i int, eE string, f func() string) *Child {
		TLogf("inside R: %d, %s", i, eE)
		return &Child{
			I: i,
			E: eE,
			F: f,
		}
	}

	var CpL func(cp *CP) []string
	// Having this on a line by itself makes it easy to copy into the interpreted
	// code below.
	CpL = func(cp *CP) []string {
		var ch []*Child
		for i, e := range cp.Es {
			i, e := i, e
			ch = append(ch,
				R(i, e.E, func() string {
					TLogf("inside R's f: %d, %v", i, e.E)
					return e.E
				}),
			)
		}
		return FL(ch)
	}

	var Cp *CP
	initCp := func() {
		Cp = &CP{
			Es: []*CPE{
				{E: "foo"},
				{E: "bar"},
				{E: "baz"},
			},
		}
	}
	initCp()

	// Verify that this works in standard Go
	t.Logf("Standard Go:")
	s := CpL(Cp)
	if expected, got := []string{"foo", "bar", "baz"}, s; !reflect.DeepEqual(expected, got) {
		t.Fatalf("Standard Go: CpL: Expected %v, got %v", expected, got)
	}

	i := interp.New(interp.Options{})
	err := i.Use(stdlib.Symbols)
	if err != nil {
		t.Fatalf("i.Use: Expected nil, got %v", err)
	}
	i.Use(interp.Exports{
		"pkg/pkg": {
			// vars
			"Cp":    reflect.ValueOf(&Cp).Elem(),
			"CpL":   reflect.ValueOf(&CpL).Elem(),
			"TLogf": reflect.ValueOf(&TLogf).Elem(),

			// funcs
			"FL": reflect.ValueOf(FL),
			"R":  reflect.ValueOf(R),

			// types
			"CP":    reflect.ValueOf((*CP)(nil)),
			"Child": reflect.ValueOf((*Child)(nil)),
		},
	})
	i.ImportUsed()

	initCp()
	_, err = i.Eval(`
package main

import . "pkg"

func main() {
	CpL = func(cp *CP) []string {
		var ch []*Child
		for i, e := range cp.Es {
			i, e := i, e
			ch = append(ch,
				R(i, e.E, func() string {
					TLogf("inside R's f: %d, %v", i, e.E)
					return e.E
				}),
			)
		}
		return FL(ch)
	}
}
	`)
	if err != nil {
		t.Fatalf("i.Eval: Expected nil, got %v", err)
	}
	t.Logf("Interpreted Go:")
	s = CpL(Cp)
	if expected, got := []string{"foo", "bar", "baz"}, s; !reflect.DeepEqual(expected, got) {
		t.Fatalf("Interpreted Go CpL: Expected %v, got %v", expected, got)
	}
}

Expected result

All tests pass; interpreted output matches compiled output

Got

% go test ./interp -run TestIssue1634
--- FAIL: TestIssue1634 (0.01s)
    interp_eval_test.go:2104: Standard Go:
    interp_eval_test.go:2066: inside R: 0, foo
    interp_eval_test.go:2066: inside R: 1, bar
    interp_eval_test.go:2066: inside R: 2, baz
    interp_eval_test.go:2083: inside R's f: 0, foo
    interp_eval_test.go:2083: inside R's f: 1, bar
    interp_eval_test.go:2083: inside R's f: 2, baz
    interp_eval_test.go:2158: Interpreted Go:
    interp_eval_test.go:2066: inside R: 0, foo
    interp_eval_test.go:2066: inside R: 1, bar
    interp_eval_test.go:2066: inside R: 2, baz
    value.go:596: inside R's f: 2, baz
    value.go:596: inside R's f: 2, baz
    value.go:596: inside R's f: 2, baz
    interp_eval_test.go:2161: Interpreted Go CpL: Expected [foo bar baz], got [baz baz baz]
FAIL
FAIL    github.com/traefik/yaegi/interp 0.365s
FAIL

Yaegi Version

381e045

Additional Notes

No response

[theclapp]

Pulling the closure that captures the loop variables out of the arglist of the "binary" function R works, if that helps:

Change this:

for i, e := range cp.Es {
	i, e := i, e
	ch = append(ch,
		// breaks
		R(i, e.E, func() string {
			TLogf("inside R's f: %d, %v", i, e.E)
			return e.E
		}),
	)
}

To this:

for i, e := range cp.Es {
	i, e := i, e
	// works
	f := func() string {
		TLogf("inside R's f: %d, %v", i, e.E)
		return e.E
	}
	ch = append(ch, R(i, e.E, f))
}

Also, I think that the fact that these are loop variables is a red herring, I think this is probably a problem with any closure being passed as a literal to a "binary" function.

Unfortunately, the code I'm playing with is awash with such things. :)

[rcoreilly]

This issue is very general -- we encountered it as well, and were also able to work around it by assigning a variable to the funLit function literal.

This suggests that the general solution is to add an exec to a funcLit that makes a new reflect.Value copy of the func lit in the frame. Ideally it would detect the outer context of being passed as an arg to another function (doable), in a for loop (probably pretty hard in general).

I just experimented with this and it doesn't seem to work:

in run.go:
func funcLitCopy(n *node) {
	next := getExec(n.tnext)
	n.exec = func(f *frame) bltn {
		ov := f.data[n.findex]
		nv := reflect.New(ov.Type()).Elem()
		nv.Set(ov)
		f.data[n.findex] = nv
		return next
	}
}
in cfg.go, post-processing:
		case funcLit:
			n.types, n.scope = sc.types, sc
			sc = sc.pop()
			err = genRun(n)
			n.gen = funcLitCopy

unfortunately, in this test:

func TestForRangeClosure(t *testing.T) {
	i := New(Options{})
	_, err := i.Eval(`
func main() {
	fs := []func()
	for i := range 3 {
		println(i, &i)
		fs = append(fs, func() { println(i, &i) })
	}
	for _, f := range fs {
		f()
	}
}
`)
	if err != nil {
		t.Error(err)
	}
}

it triggers a "reflect.Value.Call: call of nil function" -- presumably copying the reflect value doesn't do what one might have hoped it would?

it would probably make more sense to figure out what the process of defining a new var for the funcLit is actually accomplishing, and then just replicate that in the exec. apparently making a reflect clone is not it..

[rcoreilly]

finally found and fixed the issue! in run.go, see comments at the end:

func genFunctionWrapper(n *node) func(*frame) reflect.Value {
	var def *node
	var ok bool

	if def, ok = n.val.(*node); !ok {
		return genValueAsFunctionWrapper(n)
	}

	start := def.child[3].start
	numRet := len(def.typ.ret)
	var rcvr func(*frame) reflect.Value

	if n.recv != nil {
		rcvr = genValueRecv(n)
	}
	funcType := n.typ.TypeOf()
	value := genValue(n)

	return func(f *frame) reflect.Value {
		v := value(f)
		if v.Kind() == reflect.Func {
			// per #1634, if v is already a func, then don't re-wrap!  critically, the original wrapping
			// clones the frame, whereas the one here (below) does _not_ clone the frame, so it doesn't
			// generate the proper closure capture effects!
			// this path is the same as genValueAsFunctionWrapper which is the path taken above if
			// the value has an associated node, which happens when you do f := func() ..
			return v
		}
...

the tests reveal that this does break a defer test, so it needs to be further qualified somehow, but at least the crux of the issue here is clear.

[theclapp]

Fixed by #1646. Thanks, @rcoreilly!