From 3daa579643df3a1d3c0d1d3834a37edb975389c4 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Mon, 30 Sep 2013 11:05:30 -0700 Subject: [PATCH] go.tools/go/types: implement label checks R=adonovan CC=golang-dev https://golang.org/cl/14036046 --- go/types/api.go | 3 - go/types/check_test.go | 2 + go/types/labels.go | 265 +++++++++++++++++ go/types/resolver.go | 6 +- go/types/resolver_test.go | 15 + go/types/stdlib_test.go | 3 +- go/types/stmt.go | 6 +- go/types/testdata/gotos.src | 560 +++++++++++++++++++++++++++++++++++ go/types/testdata/labels.src | 144 +++++++++ go/types/testdata/stmt0.src | 18 +- 10 files changed, 1010 insertions(+), 12 deletions(-) create mode 100644 go/types/labels.go create mode 100644 go/types/testdata/gotos.src create mode 100644 go/types/testdata/labels.src diff --git a/go/types/api.go b/go/types/api.go index 563dafc7..0bde789b 100644 --- a/go/types/api.go +++ b/go/types/api.go @@ -121,8 +121,6 @@ type Info struct { // in package clauses, blank identifiers on the lhs of assignments, or // symbolic variables t in t := x.(type) of type switch headers), the // corresponding objects are nil. - // BUG(gri) Label identifiers in break, continue, or goto statements - // are not yet mapped. Objects map[*ast.Ident]Object // Implicits maps nodes to their implicitly declared objects, if any. @@ -184,6 +182,5 @@ func IsAssignableTo(V, T Type) bool { } // BUG(gri): Some built-ins don't check parameters fully, yet (e.g. append). -// BUG(gri): Use of labels is only partially checked. // BUG(gri): Interface vs non-interface comparisons are not correctly implemented. // BUG(gri): Switch statements don't check duplicate cases for all types for which it is required. diff --git a/go/types/check_test.go b/go/types/check_test.go index a199f7e9..47f579cd 100644 --- a/go/types/check_test.go +++ b/go/types/check_test.go @@ -66,6 +66,8 @@ var tests = [][]string{ {"testdata/conversions.src"}, {"testdata/stmt0.src"}, {"testdata/stmt1.src"}, + {"testdata/gotos.src"}, + {"testdata/labels.src"}, } var fset = token.NewFileSet() diff --git a/go/types/labels.go b/go/types/labels.go new file mode 100644 index 00000000..da988c86 --- /dev/null +++ b/go/types/labels.go @@ -0,0 +1,265 @@ +// Copyright 2013 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package types + +import ( + "go/ast" + "go/token" +) + +// labels checks correct label use in body. +func (check *checker) labels(body *ast.BlockStmt) { + // set of all labels in this body + all := NewScope(nil) + + fwdJumps := check.blockBranches(all, nil, nil, body.List) + + // If there are any forward jumps left, no label was found for + // the corresponding goto statements. Either those labels were + // never defined, or they are inside blocks and not reachable + // for the respective gotos. + for _, jmp := range fwdJumps { + var msg string + name := jmp.Label.Name + if alt := all.Lookup(name); alt != nil { + msg = "goto %s jumps into block" + alt.(*Label).used = true // avoid another error + } else { + msg = "label %s not declared" + } + check.errorf(jmp.Label.Pos(), msg, name) + } + + // spec: "It is illegal to define a label that is never used." + for _, obj := range all.elems { + if lbl := obj.(*Label); !lbl.used { + check.errorf(lbl.pos, "label %s declared but not used", lbl.name) + } + } +} + +// A block tracks label declarations in a block and its enclosing blocks. +type block struct { + parent *block // enclosing block + lstmt *ast.LabeledStmt // labeled statement to which this block belongs, or nil + labels map[string]*ast.LabeledStmt // allocated lazily +} + +// insert records a new label declaration for the current block. +// The label must not have been declared before in any block. +func (b *block) insert(s *ast.LabeledStmt) { + name := s.Label.Name + if debug { + assert(b.gotoTarget(name) == nil) + } + labels := b.labels + if labels == nil { + labels = make(map[string]*ast.LabeledStmt) + b.labels = labels + } + labels[name] = s +} + +// gotoTarget returns the labeled statement in the current +// or an enclosing block with the given label name, or nil. +func (b *block) gotoTarget(name string) *ast.LabeledStmt { + for s := b; s != nil; s = s.parent { + if t := s.labels[name]; t != nil { + return t + } + } + return nil +} + +// enclosingTarget returns the innermost enclosing labeled +// statement with the given label name, or nil. +func (b *block) enclosingTarget(name string) *ast.LabeledStmt { + for s := b; s != nil; s = s.parent { + if t := s.lstmt; t != nil && t.Label.Name == name { + return t + } + } + return nil +} + +// blockBranches processes a block's statement list and returns the set of outgoing forward jumps. +// all is the scope of all declared labels, parent the set of labels declared in the immediately +// enclosing block, and lstmt is the labeled statement this block is associated with (or nil). +func (check *checker) blockBranches(all *Scope, parent *block, lstmt *ast.LabeledStmt, list []ast.Stmt) []*ast.BranchStmt { + b := &block{parent: parent, lstmt: lstmt} + + var ( + varDeclPos token.Pos + fwdJumps, badJumps []*ast.BranchStmt + ) + + // All forward jumps jumping over a variable declaration are possibly + // invalid (they may still jump out of the block and be ok). + // recordVarDecl records them for the given position. + recordVarDecl := func(pos token.Pos) { + varDeclPos = pos + badJumps = append(badJumps[:0], fwdJumps...) // copy fwdJumps to badJumps + } + + jumpsOverVarDecl := func(jmp *ast.BranchStmt) bool { + if varDeclPos.IsValid() { + for _, bad := range badJumps { + if jmp == bad { + return true + } + } + } + return false + } + + var stmtBranches func(ast.Stmt) + stmtBranches = func(s ast.Stmt) { + switch s := s.(type) { + case *ast.DeclStmt: + if d, _ := s.Decl.(*ast.GenDecl); d != nil && d.Tok == token.VAR { + recordVarDecl(d.Pos()) + } + + case *ast.LabeledStmt: + // declare label + name := s.Label.Name + lbl := NewLabel(s.Label.Pos(), name) + if alt := all.Insert(lbl); alt != nil { + check.errorf(lbl.pos, "label %s already declared", name) + check.reportAltDecl(alt) + // ok to continue + } else { + b.insert(s) + check.recordObject(s.Label, lbl) + } + // resolve matching forward jumps and remove them from fwdJumps + i := 0 + for _, jmp := range fwdJumps { + if jmp.Label.Name == name { + // match + lbl.used = true + check.recordObject(jmp.Label, lbl) + if jumpsOverVarDecl(jmp) { + check.errorf( + jmp.Label.Pos(), + "goto %s jumps over variable declaration at line %d", + name, + check.fset.Position(varDeclPos).Line, + ) + // ok to continue + } + } else { + // no match - record new forward jump + fwdJumps[i] = jmp + i++ + } + } + fwdJumps = fwdJumps[:i] + lstmt = s + stmtBranches(s.Stmt) + + case *ast.BranchStmt: + if s.Label == nil { + return // checked in 1st pass (check.stmt) + } + + // determine and validate target + name := s.Label.Name + switch s.Tok { + case token.BREAK: + // spec: "If there is a label, it must be that of an enclosing + // "for", "switch", or "select" statement, and that is the one + // whose execution terminates." + t := b.enclosingTarget(name) + if t == nil { + check.errorf(s.Label.Pos(), "break label not declared: %s", name) + return + } + switch t.Stmt.(type) { + case *ast.SwitchStmt, *ast.TypeSwitchStmt, *ast.SelectStmt, *ast.ForStmt, *ast.RangeStmt: + // ok + default: + check.errorf(s.Label.Pos(), "invalid break label %s", name) + // continue and mark label as used to avoid another error + } + + case token.CONTINUE: + // spec: "If there is a label, it must be that of an enclosing + // "for" statement, and that is the one whose execution advances." + t := b.enclosingTarget(name) + if t == nil { + check.errorf(s.Label.Pos(), "continue label not declared: %s", name) + return + } + switch t.Stmt.(type) { + case *ast.ForStmt, *ast.RangeStmt: + // ok + default: + check.errorf(s.Label.Pos(), "invalid continue label %s", name) + // continue and mark label as used to avoid another error + } + + case token.GOTO: + if b.gotoTarget(name) == nil { + // label may be declared later - add to forward jumps + fwdJumps = append(fwdJumps, s) + return + } + + default: + check.invalidAST(s.Pos(), "branch statement: %s %s", s.Tok, name) + return + } + + // record label use + obj := all.Lookup(name) + obj.(*Label).used = true + check.recordObject(s.Label, obj) + + case *ast.AssignStmt: + if s.Tok == token.DEFINE { + recordVarDecl(s.Pos()) + } + + case *ast.BlockStmt: + // Unresolved forward jumps inside the nested block + // become forward jumps in the current block. + fwdJumps = append(fwdJumps, check.blockBranches(all, b, lstmt, s.List)...) + + case *ast.IfStmt: + stmtBranches(s.Body) + if s.Else != nil { + stmtBranches(s.Else) + } + + case *ast.CaseClause: + fwdJumps = append(fwdJumps, check.blockBranches(all, b, nil, s.Body)...) + + case *ast.SwitchStmt: + stmtBranches(s.Body) + + case *ast.TypeSwitchStmt: + stmtBranches(s.Body) + + case *ast.CommClause: + fwdJumps = append(fwdJumps, check.blockBranches(all, b, nil, s.Body)...) + + case *ast.SelectStmt: + stmtBranches(s.Body) + + case *ast.ForStmt: + stmtBranches(s.Body) + + case *ast.RangeStmt: + stmtBranches(s.Body) + } + } + + for _, s := range list { + stmtBranches(s) + } + + return fwdJumps +} diff --git a/go/types/resolver.go b/go/types/resolver.go index 53fe3f08..023bbdc5 100644 --- a/go/types/resolver.go +++ b/go/types/resolver.go @@ -385,6 +385,10 @@ func (check *checker) resolveFiles(files []*ast.File) { // Note: funcList may grow while iterating through it - cannot use range clause. for i := 0; i < len(check.funcList); i++ { + // TODO(gri) Factor out this code into a dedicated function + // with its own context so that it can be run concurrently + // eventually. + f := check.funcList[i] if trace { s := "" @@ -400,7 +404,7 @@ func (check *checker) resolveFiles(files []*ast.File) { check.stmtList(0, f.body.List) if check.hasLabel { - // TODO(gri) check label use + check.labels(f.body) } if f.sig.results.Len() > 0 && !check.isTerminating(f.body, "") { diff --git a/go/types/resolver_test.go b/go/types/resolver_test.go index 9125b9e8..91ead4b7 100644 --- a/go/types/resolver_test.go +++ b/go/types/resolver_test.go @@ -60,6 +60,21 @@ var sources = []string{ func (T) _() {} func (T) _() {} `, + ` + package p + func _() { + L0: + L1: + goto L0 + for { + goto L1 + } + if true { + goto L2 + } + L2: + } + `, } var pkgnames = []string{ diff --git a/go/types/stdlib_test.go b/go/types/stdlib_test.go index bbba3827..58618f71 100644 --- a/go/types/stdlib_test.go +++ b/go/types/stdlib_test.go @@ -117,8 +117,7 @@ func testTestDir(t *testing.T, path string, ignore ...string) { func TestStdtest(t *testing.T) { testTestDir(t, filepath.Join(runtime.GOROOT(), "test"), - "cmplxdivide.go", // also needs file cmplxdivide1.go - ignore - "goto.go", "label.go", "label1.go", // TODO(gri) implement missing label checks + "cmplxdivide.go", // also needs file cmplxdivide1.go - ignore "mapnan.go", "sigchld.go", // don't work on Windows; testTestDir should consult build tags "sizeof.go", "switch.go", // TODO(gri) tone down duplicate checking in expr switches "typeswitch2.go", // TODO(gri) implement duplicate checking in type switches diff --git a/go/types/stmt.go b/go/types/stmt.go index e8c26ef3..4851b01f 100644 --- a/go/types/stmt.go +++ b/go/types/stmt.go @@ -243,7 +243,7 @@ func (check *checker) stmt(ctxt stmtContext, s ast.Stmt) { case *ast.BranchStmt: if s.Label != nil { check.hasLabel = true - return // checks handled in separate pass + return // checked in 2nd pass (check.labels) } switch s.Tok { case token.BREAK: @@ -254,14 +254,12 @@ func (check *checker) stmt(ctxt stmtContext, s ast.Stmt) { if ctxt&inContinuable == 0 { check.errorf(s.Pos(), "continue not in for statement") } - case token.GOTO: - check.invalidAST(s.Pos(), "goto without label") case token.FALLTHROUGH: if ctxt&fallthroughOk == 0 { check.errorf(s.Pos(), "fallthrough statement out of place") } default: - check.invalidAST(s.Pos(), "unknown branch statement (%s)", s.Tok) + check.invalidAST(s.Pos(), "branch statement: %s", s.Tok) } case *ast.BlockStmt: diff --git a/go/types/testdata/gotos.src b/go/types/testdata/gotos.src new file mode 100644 index 00000000..0c7ee440 --- /dev/null +++ b/go/types/testdata/gotos.src @@ -0,0 +1,560 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file is a modified copy of $GOROOT/test/goto.go. + +package gotos + +var ( + i, n int + x []int + c chan int + m map[int]int + s string +) + +// goto after declaration okay +func _() { + x := 1 + goto L +L: + _ = x +} + +// goto before declaration okay +func _() { + goto L +L: + x := 1 + _ = x +} + +// goto across declaration not okay +func _() { + goto L /* ERROR "goto L jumps over variable declaration at line 36" */ + x := 1 + _ = x +L: +} + +// goto across declaration in inner scope okay +func _() { + goto L + { + x := 1 + _ = x + } +L: +} + +// goto across declaration after inner scope not okay +func _() { + goto L /* ERROR "goto L jumps over variable declaration at line 58" */ + { + x := 1 + _ = x + } + x := 1 + _ = x +L: +} + +// goto across declaration in reverse okay +func _() { +L: + x := 1 + _ = x + goto L +} + +func _() { +L: L1: + x := 1 + _ = x + goto L + goto L1 +} + +// error shows first offending variable +func _() { + goto L /* ERROR "goto L jumps over variable declaration at line 84" */ + x := 1 + _ = x + y := 1 + _ = y +L: +} + +// goto not okay even if code path is dead +func _() { + goto L /* ERROR "goto L jumps over variable declaration" */ + x := 1 + _ = x + y := 1 + _ = y + return +L: +} + +// goto into outer block okay +func _() { + { + goto L + } +L: +} + +func _() { + { + goto L + goto L1 + } +L: L1: +} + +// goto backward into outer block okay +func _() { +L: + { + goto L + } +} + +func _() { +L: L1: + { + goto L + goto L1 + } +} + +// goto into inner block not okay +func _() { + goto L /* ERROR "goto L jumps into block" */ + { + L: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + goto L1 /* ERROR "goto L1 jumps into block" */ + { + L: L1: + } +} + +// goto backward into inner block still not okay +func _() { + { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + { + L: L1: + } + goto L /* ERROR "goto L jumps into block" */ + goto L1 /* ERROR "goto L1 jumps into block" */ +} + +// error shows first (outermost) offending block +func _() { + goto L /* ERROR "goto L jumps into block" */ + { + { + { + L: + } + } + } +} + +// error prefers block diagnostic over declaration diagnostic +func _() { + goto L /* ERROR "goto L jumps into block" */ + x := 1 + _ = x + { + L: + } +} + +// many kinds of blocks, all invalid to jump into or among, +// but valid to jump out of + +// if + +func _() { +L: + if true { + goto L + } +} + +func _() { +L: + if true { + goto L + } else { + } +} + +func _() { +L: + if false { + } else { + goto L + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + if true { + L: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + if true { + L: + } else { + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + if true { + } else { + L: + } +} + +func _() { + if false { + L: + } else { + goto L /* ERROR "goto L jumps into block" */ + } +} + +func _() { + if true { + goto L /* ERROR "goto L jumps into block" */ + } else { + L: + } +} + +func _() { + if true { + goto L /* ERROR "goto L jumps into block" */ + } else if false { + L: + } +} + +func _() { + if true { + goto L /* ERROR "goto L jumps into block" */ + } else if false { + L: + } else { + } +} + +func _() { + if true { + goto L /* ERROR "goto L jumps into block" */ + } else if false { + } else { + L: + } +} + +func _() { + if true { + goto L /* ERROR "goto L jumps into block" */ + } else { + L: + } +} + +func _() { + if true { + L: + } else { + goto L /* ERROR "goto L jumps into block" */ + } +} + +// for + +func _() { + for { + goto L + } +L: +} + +func _() { + for { + goto L + L: + } +} + +func _() { + for { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for { + goto L + L1: + } +L: + goto L1 /* ERROR "goto L1 jumps into block" */ +} + +func _() { + for i < n { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for i = 0; i < n; i++ { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for i = range x { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for i = range c { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for i = range m { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +func _() { + for i = range s { + L: + } + goto L /* ERROR "goto L jumps into block" */ +} + +// switch + +func _() { +L: + switch i { + case 0: + goto L + } +} + +func _() { +L: + switch i { + case 0: + + default: + goto L + } +} + +func _() { + switch i { + case 0: + + default: + L: + goto L + } +} + +func _() { + switch i { + case 0: + + default: + goto L + L: + } +} + +func _() { + switch i { + case 0: + goto L + L: + ; + default: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + switch i { + case 0: + L: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + switch i { + case 0: + L: + ; + default: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + switch i { + case 0: + default: + L: + } +} + +func _() { + switch i { + default: + goto L /* ERROR "goto L jumps into block" */ + case 0: + L: + } +} + +func _() { + switch i { + case 0: + L: + ; + default: + goto L /* ERROR "goto L jumps into block" */ + } +} + +// select +// different from switch. the statement has no implicit block around it. + +func _() { +L: + select { + case <-c: + goto L + } +} + +func _() { +L: + select { + case c <- 1: + + default: + goto L + } +} + +func _() { + select { + case <-c: + + default: + L: + goto L + } +} + +func _() { + select { + case c <- 1: + + default: + goto L + L: + } +} + +func _() { + select { + case <-c: + goto L + L: + ; + default: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + select { + case c <- 1: + L: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + select { + case c <- 1: + L: + ; + default: + } +} + +func _() { + goto L /* ERROR "goto L jumps into block" */ + select { + case <-c: + default: + L: + } +} + +func _() { + select { + default: + goto L /* ERROR "goto L jumps into block" */ + case <-c: + L: + } +} + +func _() { + select { + case <-c: + L: + ; + default: + goto L /* ERROR "goto L jumps into block" */ + } +} diff --git a/go/types/testdata/labels.src b/go/types/testdata/labels.src new file mode 100644 index 00000000..f4fcdcec --- /dev/null +++ b/go/types/testdata/labels.src @@ -0,0 +1,144 @@ +// Copyright 2011 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file is a modified concatenation of the files +// $GOROOT/test/label.go and $GOROOT/test/label1.go. + +package labels + +var x int + +func f0() { +L1 /* ERROR "label L1 declared but not used" */ : + for { + } +L2 /* ERROR "label L2 declared but not used" */ : + select { + } +L3 /* ERROR "label L3 declared but not used" */ : + switch { + } +L4 /* ERROR "label L4 declared but not used" */ : + if true { + } +L5 /* ERROR "label L5 declared but not used" */ : + f0() +L6: + f0() +L6 /* ERROR "label L6 already declared" */ : + f0() + if x == 20 { + goto L6 + } + +L7: + for { + break L7 + } + +L8: + for { + if x == 21 { + continue L8 + } + } + +L9: + switch { + case true: + break L9 + defalt /* ERROR "label defalt declared but not used" */ : + } + +L10: + select { + default: + break L10 + } +} + +func f1() { +L1: + for { + if x == 0 { + break L1 + } + if x == 1 { + continue L1 + } + goto L1 + } + +L2: + select { + default: + if x == 0 { + break L2 + } + if x == 1 { + continue L2 /* ERROR "invalid continue label L2" */ + } + goto L2 + } + +L3: + switch { + case x > 10: + if x == 11 { + break L3 + } + if x == 12 { + continue L3 /* ERROR "invalid continue label L3" */ + } + goto L3 + } + +L4: + if true { + if x == 13 { + break L4 /* ERROR "invalid break label L4" */ + } + if x == 14 { + continue L4 /* ERROR "invalid continue label L4" */ + } + if x == 15 { + goto L4 + } + } + +L5: + f1() + if x == 16 { + break L5 /* ERROR "invalid break label L5" */ + } + if x == 17 { + continue L5 /* ERROR "invalid continue label L5" */ + } + if x == 18 { + goto L5 + } + + for { + if x == 19 { + break L1 /* ERROR "break label not declared: L1" */ + } + if x == 20 { + continue L1 /* ERROR "continue label not declared: L1" */ + } + if x == 21 { + goto L1 + } + } +} + +// Additional tests not in the original files. + +func f2() { +L1: + if x == 0 { + for { + continue L1 /* ERROR "invalid continue label L1" */ + } + } +} \ No newline at end of file diff --git a/go/types/testdata/stmt0.src b/go/types/testdata/stmt0.src index 7de3844c..c4770988 100644 --- a/go/types/testdata/stmt0.src +++ b/go/types/testdata/stmt0.src @@ -378,16 +378,24 @@ func switches1() { switch x { case 0: + goto L1 L1: fallthrough case 1: + goto L2 + goto L3 + goto L4 L2: L3: L4: fallthrough default: } switch x { case 0: + goto L5 L5: fallthrough default: + goto L6 + goto L7 + goto L8 L6: L7: L8: fallthrough /* ERROR "fallthrough statement out of place" */ } @@ -605,14 +613,20 @@ func rangeloops() { } func labels0() { + goto L0 + goto L1 L0: L1: - L1: + L1 /* ERROR "already declared" */ : if true { + goto L2 L2: - L0: + L0 /* ERROR "already declared" */ : } _ = func() { + goto L0 + goto L1 + goto L2 L0: L1: L2: