From 9c4dcfc45d05a3f0bb975e4ec1c9a9640865da09 Mon Sep 17 00:00:00 2001 From: Randall O'Reilly Date: Sat, 20 Jul 2024 08:26:03 -0700 Subject: [PATCH] automatic loop variables in for loops ("loop var"), consistent with go 1.22 behavior 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.. --- _test/closure10.go | 6 +-- _test/closure11.go | 6 +-- _test/closure12.go | 6 +-- _test/closure15.go | 18 ++++++++ _test/closure16.go | 18 ++++++++ _test/closure17.go | 22 ++++++++++ _test/closure18.go | 25 +++++++++++ _test/closure19.go | 18 ++++++++ _test/closure20.go | 18 ++++++++ _test/closure9.go | 6 +-- _test/for17.go | 13 ++++++ _test/for18.go | 12 ++++++ _test/for19.go | 12 ++++++ interp/ast.go | 17 +++++++- interp/cfg.go | 72 +++++++++++++++++++++++++++++--- interp/interp_consistent_test.go | 5 +-- interp/interp_file_test.go | 5 ++- interp/run.go | 65 ++++++++++++++++++++++++++++ 18 files changed, 320 insertions(+), 24 deletions(-) create mode 100644 _test/closure15.go create mode 100644 _test/closure16.go create mode 100644 _test/closure17.go create mode 100644 _test/closure18.go create mode 100644 _test/closure19.go create mode 100644 _test/closure20.go create mode 100644 _test/for17.go create mode 100644 _test/for18.go create mode 100644 _test/for19.go diff --git a/_test/closure10.go b/_test/closure10.go index 667404574..c502b3cb8 100644 --- a/_test/closure10.go +++ b/_test/closure10.go @@ -13,6 +13,6 @@ func main() { } // Output: -// 3 0 0 -// 3 1 1 -// 3 2 2 +// 0 0 0 +// 1 1 1 +// 2 2 2 diff --git a/_test/closure11.go b/_test/closure11.go index d619504c6..bdc86555a 100644 --- a/_test/closure11.go +++ b/_test/closure11.go @@ -17,6 +17,6 @@ func main() { } // Output: -// 3 0 -// 3 1 -// 3 2 +// 0 0 +// 1 1 +// 2 2 diff --git a/_test/closure12.go b/_test/closure12.go index a38d884ac..9f1a3f842 100644 --- a/_test/closure12.go +++ b/_test/closure12.go @@ -20,6 +20,6 @@ func main() { } // Output: -// 3 0 i=0 -// 3 1 i=1 -// 3 2 i=2 +// 0 0 i=0 +// 1 1 i=1 +// 2 2 i=2 diff --git a/_test/closure15.go b/_test/closure15.go new file mode 100644 index 000000000..033d4f395 --- /dev/null +++ b/_test/closure15.go @@ -0,0 +1,18 @@ +package main + +func main() { + foos := []func(){} + + for i := range 3 { + a := i + foos = append(foos, func() { println(i, a) }) + } + foos[0]() + foos[1]() + foos[2]() +} + +// Output: +// 0 0 +// 1 1 +// 2 2 diff --git a/_test/closure16.go b/_test/closure16.go new file mode 100644 index 000000000..a535f9a42 --- /dev/null +++ b/_test/closure16.go @@ -0,0 +1,18 @@ +package main + +func main() { + foos := []func(){} + + for i := range 3 { + a, b := i, i + foos = append(foos, func() { println(i, a, b) }) + } + foos[0]() + foos[1]() + foos[2]() +} + +// Output: +// 0 0 0 +// 1 1 1 +// 2 2 2 diff --git a/_test/closure17.go b/_test/closure17.go new file mode 100644 index 000000000..d5b80d379 --- /dev/null +++ b/_test/closure17.go @@ -0,0 +1,22 @@ +package main + +type T struct { + F func() +} + +func main() { + foos := []T{} + + for i := range 3 { + a := i + foos = append(foos, T{func() { println(i, a) }}) + } + foos[0].F() + foos[1].F() + foos[2].F() +} + +// Output: +// 0 0 +// 1 1 +// 2 2 diff --git a/_test/closure18.go b/_test/closure18.go new file mode 100644 index 000000000..f5dc43d72 --- /dev/null +++ b/_test/closure18.go @@ -0,0 +1,25 @@ +package main + +import "fmt" + +type T struct { + F func() +} + +func main() { + foos := []T{} + + for i := range 3 { + a := i + n := fmt.Sprintf("i=%d", i) + foos = append(foos, T{func() { println(i, a, n) }}) + } + foos[0].F() + foos[1].F() + foos[2].F() +} + +// Output: +// 0 0 i=0 +// 1 1 i=1 +// 2 2 i=2 diff --git a/_test/closure19.go b/_test/closure19.go new file mode 100644 index 000000000..7ad87fb54 --- /dev/null +++ b/_test/closure19.go @@ -0,0 +1,18 @@ +package main + +func main() { + foos := []func(){} + + for i := 0; i < 3; i++ { + i := i + foos = append(foos, func() { println(i) }) + } + foos[0]() + foos[1]() + foos[2]() +} + +// Output: +// 0 +// 1 +// 2 diff --git a/_test/closure20.go b/_test/closure20.go new file mode 100644 index 000000000..7afb67084 --- /dev/null +++ b/_test/closure20.go @@ -0,0 +1,18 @@ +package main + +func main() { + foos := []func(){} + + for i := range 3 { + i := i + foos = append(foos, func() { println(i) }) + } + foos[0]() + foos[1]() + foos[2]() +} + +// Output: +// 0 +// 1 +// 2 diff --git a/_test/closure9.go b/_test/closure9.go index 24e937b10..d1dab2218 100644 --- a/_test/closure9.go +++ b/_test/closure9.go @@ -13,6 +13,6 @@ func main() { } // Output: -// 3 0 -// 3 1 -// 3 2 +// 0 0 +// 1 1 +// 2 2 diff --git a/_test/for17.go b/_test/for17.go new file mode 100644 index 000000000..59d769589 --- /dev/null +++ b/_test/for17.go @@ -0,0 +1,13 @@ +package main + +func main() { + mx := 3 + for i := range mx { + println(i) + } +} + +// Output: +// 0 +// 1 +// 2 diff --git a/_test/for18.go b/_test/for18.go new file mode 100644 index 000000000..d70f55697 --- /dev/null +++ b/_test/for18.go @@ -0,0 +1,12 @@ +package main + +func main() { + for i := range 3 { + println(i) + } +} + +// Output: +// 0 +// 1 +// 2 diff --git a/_test/for19.go b/_test/for19.go new file mode 100644 index 000000000..3d98c9cb0 --- /dev/null +++ b/_test/for19.go @@ -0,0 +1,12 @@ +package main + +func main() { + for range 3 { + println("i") + } +} + +// Output: +// i +// i +// i diff --git a/interp/ast.go b/interp/ast.go index 1e71ac439..a94354b5b 100644 --- a/interp/ast.go +++ b/interp/ast.go @@ -597,7 +597,22 @@ func (interp *Interpreter) ast(f ast.Node) (string, *node, error) { st.push(addChild(&root, anc, pos, kind, act), nod) case *ast.BlockStmt: - st.push(addChild(&root, anc, pos, blockStmt, aNop), nod) + b := addChild(&root, anc, pos, blockStmt, aNop) + st.push(b, nod) + var kind nkind + if anc.node != nil { + kind = anc.node.kind + } + switch kind { + case rangeStmt: + k := addChild(&root, astNode{b, nod}, pos, identExpr, aNop) + k.ident = "_" + v := addChild(&root, astNode{b, nod}, pos, identExpr, aNop) + v.ident = "_" + case forStmt7: + k := addChild(&root, astNode{b, nod}, pos, identExpr, aNop) + k.ident = "_" + } case *ast.BranchStmt: var kind nkind diff --git a/interp/cfg.go b/interp/cfg.go index 9175c06ed..933e2a93f 100644 --- a/interp/cfg.go +++ b/interp/cfg.go @@ -123,6 +123,7 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string } case blockStmt: + var rangek, rangev *node if n.anc != nil && n.anc.kind == rangeStmt { // For range block: ensure that array or map type is propagated to iterators // prior to process block. We cannot perform this at RangeStmt pre-order because @@ -202,18 +203,24 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string sc.add(sc.getType("int")) // Add a dummy type to store array shallow copy for range ktyp = sc.getType("int") vtyp = o.typ.val + case intT: + n.anc.gen = rangeInt + sc.add(sc.getType("int")) + ktyp = sc.getType("int") } kindex := sc.add(ktyp) sc.sym[k.ident] = &symbol{index: kindex, kind: varSym, typ: ktyp} k.typ = ktyp k.findex = kindex + rangek = k if v != nil { vindex := sc.add(vtyp) sc.sym[v.ident] = &symbol{index: vindex, kind: varSym, typ: vtyp} v.typ = vtyp v.findex = vindex + rangev = v } } } @@ -221,6 +228,41 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string n.findex = -1 n.val = nil sc = sc.pushBloc() + + if n.anc != nil && n.anc.kind == rangeStmt { + lk := n.child[0] + if rangek != nil { + lk.ident = rangek.ident + lk.typ = rangek.typ + kindex := sc.add(lk.typ) + sc.sym[lk.ident] = &symbol{index: kindex, kind: varSym, typ: lk.typ} + lk.findex = kindex + lk.gen = loopVarKey + } + lv := n.child[1] + if rangev != nil { + lv.ident = rangev.ident + lv.typ = rangev.typ + vindex := sc.add(lv.typ) + sc.sym[lv.ident] = &symbol{index: vindex, kind: varSym, typ: lv.typ} + lv.findex = vindex + lv.gen = loopVarVal + } + } + if n.anc != nil && n.anc.kind == forStmt7 { + lv := n.child[0] + init := n.anc.child[0] + if init.kind == defineStmt && len(init.child) >= 2 && init.child[0].kind == identExpr { + fi := init.child[0] + lv.ident = fi.ident + lv.typ = fi.typ + vindex := sc.add(lv.typ) + sc.sym[lv.ident] = &symbol{index: vindex, kind: varSym, typ: lv.typ} + lv.findex = vindex + lv.gen = loopVarFor + } + } + // Pre-define symbols for labels defined in this block, so we are sure that // they are already defined when met. // TODO(marc): labels must be stored outside of symbols to avoid collisions. @@ -692,6 +734,22 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string return } if sc.global || sc.isRedeclared(dest) { + if n.anc != nil && n.anc.anc != nil && (n.anc.anc.kind == forStmt7 || n.anc.anc.kind == rangeStmt) { + // check for redefine of for loop variables, which are now auto-defined in go1.22 + init := n.anc.anc.child[0] + var fi *node // for ident + if n.anc.anc.kind == forStmt7 { + if init.kind == defineStmt && len(init.child) >= 2 && init.child[0].kind == identExpr { + fi = init.child[0] + } + } else { // range + fi = init + } + if fi != nil && dest.ident == fi.ident { + n.gen = nop + break + } + } // Do not overload existing symbols (defined in GTA) in global scope. sym, _, _ = sc.lookup(dest.ident) } @@ -1523,6 +1581,7 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string err = cond.cfgErrorf("non-bool used as for condition") } n.start = init.start + body.start = body.child[0] // loopvar if cond.rval.IsValid() { // Condition is known at compile time, bypass test. if cond.rval.Bool() { @@ -1755,12 +1814,13 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string } else { k, o, body = n.child[0], n.child[1], n.child[2] } - n.start = o.start // Get array or map object - o.tnext = k.start // then go to iterator init - k.tnext = n // then go to range function - n.tnext = body.start // then go to range body - body.tnext = n // then body go to range function (loop) - k.gen = empty // init filled later by generator + n.start = o.start // Get array or map object + o.tnext = k.start // then go to iterator init + k.tnext = n // then go to range function + body.start = body.child[0] // loopvar + n.tnext = body.start // then go to range body + body.tnext = n // then body go to range function (loop) + k.gen = empty // init filled later by generator } case returnStmt: diff --git a/interp/interp_consistent_test.go b/interp/interp_consistent_test.go index b8a8c55ca..5178d2185 100644 --- a/interp/interp_consistent_test.go +++ b/interp/interp_consistent_test.go @@ -16,10 +16,7 @@ import ( "github.com/traefik/yaegi/stdlib/unsafe" ) -// The following tests depend on an incompatible language change in go1.22, where `for` variables are now -// defined in body (thus reallocated at each loop). We skip them until both supported versions behave the same. -// We will remove this in Go1.23. -var testsToSkipGo122 = map[string]bool{"closure9.go": true, "closure10.go": true, "closure11.go": true, "closure12.go": true} +var testsToSkipGo122 = map[string]bool{} var go122 = strings.HasPrefix(runtime.Version(), "go1.22") diff --git a/interp/interp_file_test.go b/interp/interp_file_test.go index 2f429cb49..459f1ea23 100644 --- a/interp/interp_file_test.go +++ b/interp/interp_file_test.go @@ -19,7 +19,10 @@ import ( // The following tests sometimes (not always) crash with go1.21 but not with go1.20 or go1.22. // The reason of failure is not obvious, maybe due to the runtime itself, and will be investigated separately. -var testsToSkipGo121 = map[string]bool{"cli6.go": true, "cli7.go": true, "issue-1276.go": true, "issue-1330.go": true, "struct11.go": true} +// Also, the closure tests depend on an incompatible language change in go1.22, where `for` variables are now +// defined in body (thus reallocated at each loop). This is now the behavior in yaegi, so 1.21 produces +// different results. +var testsToSkipGo121 = map[string]bool{"cli6.go": true, "cli7.go": true, "issue-1276.go": true, "issue-1330.go": true, "struct11.go": true, "closure9.go": true, "closure10.go": true, "closure11.go": true, "closure12.go": true, "closure15.go": true, "closure16.go": true, "closure17.go": true, "closure18.go": true, "closure20.go": true, "for17.go": true, "for18.go": true, "for19.go": true} var go121 = strings.HasPrefix(runtime.Version(), "go1.21") diff --git a/interp/run.go b/interp/run.go index b8caa9086..e2c90fe6f 100644 --- a/interp/run.go +++ b/interp/run.go @@ -2893,6 +2893,71 @@ func _range(n *node) { } } +func rangeInt(n *node) { + ixn := n.child[0] + index0 := ixn.findex // array index location in frame + index2 := index0 - 1 // max + fnext := getExec(n.fnext) + tnext := getExec(n.tnext) + + var value func(*frame) reflect.Value + mxn := n.child[1] + value = genValue(mxn) + n.exec = func(f *frame) bltn { + rv := f.data[index0] + rv.SetInt(rv.Int() + 1) + if int(rv.Int()) >= int(f.data[index2].Int()) { + return fnext + } + return tnext + } + + // Init sequence + next := n.exec + index := index0 + ixn.exec = func(f *frame) bltn { + f.data[index2] = value(f) // set max + f.data[index].SetInt(-1) // assing index value + return next + } +} + +func loopVarKey(n *node) { + ixn := n.anc.anc.child[0] + next := getExec(n.tnext) + n.exec = func(f *frame) bltn { + rv := f.data[ixn.findex] + nv := reflect.New(rv.Type()).Elem() + nv.Set(rv) + f.data[n.findex] = nv + return next + } +} + +func loopVarVal(n *node) { + vln := n.anc.anc.child[1] + next := getExec(n.tnext) + n.exec = func(f *frame) bltn { + rv := f.data[vln.findex] + nv := reflect.New(rv.Type()).Elem() + nv.Set(rv) + f.data[n.findex] = nv + return next + } +} + +func loopVarFor(n *node) { + ixn := n.anc.anc.child[0].child[0] + next := getExec(n.tnext) + n.exec = func(f *frame) bltn { + fv := f.data[ixn.findex] + nv := reflect.New(fv.Type()).Elem() + nv.Set(fv) + f.data[n.findex] = nv + return next + } +} + func rangeChan(n *node) { i := n.child[0].findex // element index location in frame value := genValue(n.child[1]) // chan