Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interp: fix the behaviour of goto, continue and break #1392

Merged
merged 8 commits into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions _test/break0.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package main

func main() {
n := 2
m := 2
foo := true
OuterLoop:
println("Boo")
for i := 0; i < n; i++ {
println("I: ", i)
for j := 0; j < m; j++ {
switch foo {
case true:
println(foo)
break OuterLoop
case false:
println(foo)
}
}
}
}

// Error:
// 15:5: invalid break label OuterLoop
24 changes: 24 additions & 0 deletions _test/break1.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package main

func main() {
n := 2
m := 2
foo := true
OuterLoop:
for i := 0; i < n; i++ {
println("I: ", i)
for j := 0; j < m; j++ {
switch foo {
case true:
println(foo)
break OuterLoop
case false:
println(foo)
}
}
}
}

// Output:
// I: 0
// true
26 changes: 26 additions & 0 deletions _test/cont2.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package main

func main() {
n := 2
m := 2
foo := true
OuterLoop:
for i := 0; i < n; i++ {
println("I: ", i)
for j := 0; j < m; j++ {
switch foo {
case true:
println(foo)
continue OuterLoop
case false:
println(foo)
}
}
}
}

// Output:
// I: 0
// true
// I: 1
// true
24 changes: 24 additions & 0 deletions _test/cont3.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package main

func main() {
n := 2
m := 2
foo := true
OuterLoop:
println("boo")
for i := 0; i < n; i++ {
println("I: ", i)
for j := 0; j < m; j++ {
switch foo {
case true:
println(foo)
continue OuterLoop
case false:
println(foo)
}
}
}
}

// Error:
// 15:5: invalid continue label OuterLoop
21 changes: 21 additions & 0 deletions _test/issue-1354.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package main

func main() {
println(test()) // Go prints true, Yaegi false
}

func test() bool {
if true {
goto label
}
goto label
label:
println("Go continues here")
return true
println("Yaegi goes straight to this return (this line is never printed)")
return false
}

// Output:
// Go continues here
// true
82 changes: 51 additions & 31 deletions interp/cfg.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,22 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string
}
}
}

n.findex = -1
n.val = nil
sc = sc.pushBloc()
// 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.
for _, c := range n.child {
if c.kind != labeledStmt {
continue
}
label := c.child[0].ident
sym := &symbol{kind: labelSym, node: c, index: -1}
sc.sym[label] = sym
c.sym = sym
}

case breakStmt, continueStmt, gotoStmt:
if len(n.child) > 0 {
Expand All @@ -223,18 +236,6 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string
}
}

case labeledStmt:
label := n.child[0].ident
// TODO(marc): labels must be stored outside of symbols to avoid collisions
// Used labels are searched in current and sub scopes, not upper ones.
if sym, ok := sc.lookdown(label); ok {
sym.node = n
n.sym = sym
} else {
n.sym = &symbol{kind: labelSym, node: n, index: -1}
}
sc.sym[label] = n.sym

case caseClause:
sc = sc.pushBloc()
if sn := n.anc.anc; sn.kind == typeSwitch && sn.child[1].action == aAssign {
Expand Down Expand Up @@ -853,28 +854,49 @@ func (interp *Interpreter) cfg(root *node, sc *scope, importPath, pkgName string
n.rval = l.rval

case breakStmt:
if len(n.child) > 0 {
gotoLabel(n.sym)
} else {
if len(n.child) == 0 {
n.tnext = sc.loop
break
}
if !n.hasAnc(n.sym.node) {
err = n.cfgErrorf("invalid break label %s", n.child[0].ident)
break
}
for _, c := range n.sym.from {
mpl marked this conversation as resolved.
Show resolved Hide resolved
c.tnext = n.sym.node
}

case continueStmt:
if len(n.child) > 0 {
gotoLabel(n.sym)
} else {
if len(n.child) == 0 {
n.tnext = sc.loopRestart
break
}
if !n.hasAnc(n.sym.node) {
err = n.cfgErrorf("invalid continue label %s", n.child[0].ident)
break
}
for _, c := range n.sym.from {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as above.

c.tnext = n.sym.node.child[1].lastChild().start
}

case gotoStmt:
gotoLabel(n.sym)
if n.sym.node == nil {
mpl marked this conversation as resolved.
Show resolved Hide resolved
break
}
for _, c := range n.sym.from {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same remark as in break and continue.

c.tnext = n.sym.node.start
}

case labeledStmt:
wireChild(n)
if len(n.child) > 1 {
n.start = n.child[1].start
}
gotoLabel(n.sym)
for _, c := range n.sym.from {
if c.kind == gotoStmt {
c.tnext = n.start
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not in a forward case, it seems useless to do that, since the code in the goto case (L887) will overwrite c.tnext.

If we are in the forward case, we are overwriting c.tnext here. Is that what we want to do?

}
}

case callExpr:
wireChild(n)
Expand Down Expand Up @@ -2426,6 +2448,15 @@ func (n *node) fieldType(m int) *node {
// lastChild returns the last child of a node.
func (n *node) lastChild() *node { return n.child[len(n.child)-1] }

func (n *node) hasAnc(nod *node) bool {
for a := n.anc; a != nil; a = a.anc {
if a == nod {
return true
}
}
return false
}

func isKey(n *node) bool {
return n.anc.kind == fileStmt ||
(n.anc.kind == selectorExpr && n.anc.child[0] != n) ||
Expand Down Expand Up @@ -2592,17 +2623,6 @@ func typeSwichAssign(n *node) bool {
return ts.kind == typeSwitch && ts.child[1].action == aAssign
}

func gotoLabel(s *symbol) {
if s.node == nil {
return
}
for _, c := range s.from {
if c.tnext == nil {
c.tnext = s.node.start
}
}
}

func compositeGenerator(n *node, typ *itype, rtyp reflect.Type) (gen bltnGenerator) {
switch typ.cat {
case aliasT, ptrT:
Expand Down
12 changes: 12 additions & 0 deletions interp/interp_consistent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ func TestInterpConsistencyBuild(t *testing.T) {
file.Name() == "assign12.go" || // expect error
file.Name() == "assign15.go" || // expect error
file.Name() == "bad0.go" || // expect error
file.Name() == "break0.go" || // expect error
file.Name() == "cont3.go" || // expect error
file.Name() == "const9.go" || // expect error
file.Name() == "export1.go" || // non-main package
file.Name() == "export0.go" || // non-main package
Expand Down Expand Up @@ -198,6 +200,16 @@ func TestInterpErrorConsistency(t *testing.T) {
expectedInterp: "1:1: expected 'package', found println",
expectedExec: "1:1: expected 'package', found println",
},
{
fileName: "break0.go",
expectedInterp: "15:5: invalid break label OuterLoop",
expectedExec: "15:11: invalid break label OuterLoop",
},
{
fileName: "cont3.go",
expectedInterp: "15:5: invalid continue label OuterLoop",
expectedExec: "15:14: invalid continue label OuterLoop",
},
{
fileName: "const9.go",
expectedInterp: "5:2: constant definition loop",
Expand Down
14 changes: 0 additions & 14 deletions interp/scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,20 +144,6 @@ func (s *scope) lookup(ident string) (*symbol, int, bool) {
return nil, 0, false
}

// lookdown searches for a symbol in the current scope and included ones, recursively.
// It returns the first found symbol and true, or nil and false.
func (s *scope) lookdown(ident string) (*symbol, bool) {
if sym, ok := s.sym[ident]; ok {
return sym, true
}
for _, c := range s.child {
if sym, ok := c.lookdown(ident); ok {
return sym, true
}
}
return nil, false
}

func (s *scope) rangeChanType(n *node) *itype {
if sym, _, found := s.lookup(n.child[1].ident); found {
if t := sym.typ; len(n.child) == 3 && t != nil && (t.cat == chanT || t.cat == chanRecvT) {
Expand Down