diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index b5c53c07c2..0f8415aae9 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -60,8 +60,8 @@ type Testing interface { Errorf(format string, args ...interface{}) } -// Run applies an analysis to each named package. -// It loads each package from the specified GOPATH-style project +// Run applies an analysis to the named package. +// It loads the package from the specified GOPATH-style project // directory using golang.org/x/tools/go/packages, runs the analysis on // it, and checks that each the analysis emits the expected diagnostics // and facts specified by the contents of '// want ...' comments in the @@ -81,7 +81,8 @@ type Testing interface { // // func panicf(format string, args interface{}) { // want panicf:"printfWrapper" // -// Package facts are specified by the name "package". +// Package facts are specified by the name "package" and appear on +// line 1 of the first source file of the package. // // A single 'want' comment may contain a mixture of diagnostic and fact // expectations, including multiple facts about the same object: @@ -93,25 +94,25 @@ type Testing interface { // // You may wish to call this function from within a (*testing.T).Run // subtest to ensure that errors have adequate contextual description. -func Run(t Testing, dir string, a *analysis.Analyzer, pkgnames ...string) { - if pkgnames == nil { - t.Errorf("Run: no packages") +// +// Run returns the pass and the result of the Analyzer's Run function, +// or (nil, nil) if loading or analysis failed. +func Run(t Testing, dir string, a *analysis.Analyzer, pkgname string) (*analysis.Pass, interface{}) { + pkg, err := loadPackage(dir, pkgname) + if err != nil { + t.Errorf("loading %s: %v", pkgname, err) + return nil, nil } - for _, pkgname := range pkgnames { - pkg, err := loadPackage(dir, pkgname) - if err != nil { - t.Errorf("loading %s: %v", pkgname, err) - continue - } - pass, diagnostics, facts, err := checker.Analyze(pkg, a) - if err != nil { - t.Errorf("analyzing %s: %v", pkgname, err) - continue - } - - check(t, dir, pass, diagnostics, facts) + pass, diagnostics, facts, result, err := checker.Analyze(pkg, a) + if err != nil { + t.Errorf("analyzing %s: %v", pkgname, err) + return nil, nil } + + check(t, dir, pass, diagnostics, facts) + + return pass, result } // loadPackage loads the specified package (from source, with diff --git a/go/analysis/internal/checker/checker.go b/go/analysis/internal/checker/checker.go index 6743b9c497..656478df93 100644 --- a/go/analysis/internal/checker/checker.go +++ b/go/analysis/internal/checker/checker.go @@ -157,7 +157,7 @@ func load(patterns []string, allSyntax bool) ([]*packages.Package, error) { // have a nil key. // // It is exposed for use in testing. -func Analyze(pkg *packages.Package, a *analysis.Analyzer) (*analysis.Pass, []analysis.Diagnostic, map[types.Object][]analysis.Fact, error) { +func Analyze(pkg *packages.Package, a *analysis.Analyzer) (*analysis.Pass, []analysis.Diagnostic, map[types.Object][]analysis.Fact, interface{}, error) { act := analyze([]*packages.Package{pkg}, []*analysis.Analyzer{a})[0] facts := make(map[types.Object][]analysis.Fact) @@ -172,7 +172,7 @@ func Analyze(pkg *packages.Package, a *analysis.Analyzer) (*analysis.Pass, []ana } } - return act.pass, act.diagnostics, facts, act.err + return act.pass, act.diagnostics, facts, act.result, act.err } func analyze(pkgs []*packages.Package, analyzers []*analysis.Analyzer) []*action { diff --git a/go/analysis/passes/ctrlflow/ctrlflow.go b/go/analysis/passes/ctrlflow/ctrlflow.go new file mode 100644 index 0000000000..75655c5bad --- /dev/null +++ b/go/analysis/passes/ctrlflow/ctrlflow.go @@ -0,0 +1,225 @@ +// Copyright 2018 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 ctrlflow is an analysis that provides a syntactic +// control-flow graph (CFG) for the body of a function. +// It records whether a function cannot return. +// By itself, it does not report any diagnostics. +package ctrlflow + +import ( + "go/ast" + "go/types" + "log" + "reflect" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/cfg" + "golang.org/x/tools/go/types/typeutil" +) + +var Analyzer = &analysis.Analyzer{ + Name: "ctrlflow", + Doc: "build a control-flow graph", + Run: run, + ResultType: reflect.TypeOf(new(CFGs)), + FactTypes: []analysis.Fact{new(noReturn)}, + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +// noReturn is a fact indicating that a function does not return. +type noReturn struct{} + +func (*noReturn) AFact() {} + +func (*noReturn) String() string { return "noReturn" } + +// A CFGs holds the control-flow graphs +// for all the functions of the current package. +type CFGs struct { + defs map[*ast.Ident]types.Object // from Pass.TypesInfo.Defs + funcDecls map[*types.Func]*declInfo + funcLits map[*ast.FuncLit]*litInfo + pass *analysis.Pass // transient; nil after construction +} + +// CFGs has two maps: funcDecls for named functions and funcLits for +// unnamed ones. Unlike funcLits, the funcDecls map is not keyed by its +// syntax node, *ast.FuncDecl, because callMayReturn needs to do a +// look-up by *types.Func, and you can get from an *ast.FuncDecl to a +// *types.Func but not the other way. + +type declInfo struct { + decl *ast.FuncDecl + cfg *cfg.CFG // iff decl.Body != nil + started bool // to break cycles + noReturn bool +} + +type litInfo struct { + cfg *cfg.CFG + noReturn bool +} + +// FuncDecl returns the control-flow graph for a named function. +// It returns nil if decl.Body==nil. +func (c *CFGs) FuncDecl(decl *ast.FuncDecl) *cfg.CFG { + if decl.Body == nil { + return nil + } + fn := c.defs[decl.Name].(*types.Func) + return c.funcDecls[fn].cfg +} + +// FuncLit returns the control-flow graph for a literal function. +func (c *CFGs) FuncLit(lit *ast.FuncLit) *cfg.CFG { + return c.funcLits[lit].cfg +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + // Because CFG construction consumes and produces noReturn + // facts, CFGs for exported FuncDecls must be built before 'run' + // returns; we cannot construct them lazily. + // (We could build CFGs for FuncLits lazily, + // but the benefit is marginal.) + + // Pass 1. Map types.Funcs to ast.FuncDecls in this package. + funcDecls := make(map[*types.Func]*declInfo) // functions and methods + funcLits := make(map[*ast.FuncLit]*litInfo) + + var decls []*types.Func // keys(funcDecls), in order + var lits []*ast.FuncLit // keys(funcLits), in order + + nodeFilter := []ast.Node{ + (*ast.FuncDecl)(nil), + (*ast.FuncLit)(nil), + } + inspect.Preorder(nodeFilter, func(n ast.Node) { + switch n := n.(type) { + case *ast.FuncDecl: + fn := pass.TypesInfo.Defs[n.Name].(*types.Func) + funcDecls[fn] = &declInfo{decl: n} + decls = append(decls, fn) + + case *ast.FuncLit: + funcLits[n] = new(litInfo) + lits = append(lits, n) + } + }) + + c := &CFGs{ + defs: pass.TypesInfo.Defs, + funcDecls: funcDecls, + funcLits: funcLits, + pass: pass, + } + + // Pass 2. Build CFGs. + + // Build CFGs for named functions. + // Cycles in the static call graph are broken + // arbitrarily but deterministically. + // We create noReturn facts as discovered. + for _, fn := range decls { + c.buildDecl(fn, funcDecls[fn]) + } + + // Build CFGs for literal functions. + // These aren't relevant to facts (since they aren't named) + // but are required for the CFGs.FuncLit API. + for _, lit := range lits { + li := funcLits[lit] + if li.cfg == nil { + li.cfg = cfg.New(lit.Body, c.callMayReturn) + if !hasReachableReturn(li.cfg) { + li.noReturn = true + } + } + } + + // All CFGs are now built. + c.pass = nil + + return c, nil +} + +// di.cfg may be nil on return. +func (c *CFGs) buildDecl(fn *types.Func, di *declInfo) { + // buildDecl may call itself recursively for the same function, + // because cfg.New is passed the callMayReturn method, which + // builds the CFG of the callee, leading to recursion. + // The buildDecl call tree thus resembles the static call graph. + // We mark each node when we start working on it to break cycles. + + if !di.started { // break cycle + di.started = true + + if isIntrinsicNoReturn(fn) { + di.noReturn = true + } + if di.decl.Body != nil { + di.cfg = cfg.New(di.decl.Body, c.callMayReturn) + if !hasReachableReturn(di.cfg) { + di.noReturn = true + } + } + if di.noReturn { + c.pass.ExportObjectFact(fn, new(noReturn)) + } + + // debugging + if false { + log.Printf("CFG for %s:\n%s (noreturn=%t)\n", fn, di.cfg.Format(c.pass.Fset), di.noReturn) + } + } +} + +// callMayReturn reports whether the called function may return. +// It is passed to the CFG builder. +func (c *CFGs) callMayReturn(call *ast.CallExpr) (r bool) { + if id, ok := call.Fun.(*ast.Ident); ok && c.pass.TypesInfo.Uses[id] == panicBuiltin { + return false // panic never returns + } + + // Is this a static call? + fn := typeutil.StaticCallee(c.pass.TypesInfo, call) + if fn == nil { + return true // callee not statically known; be conservative + } + + // Function or method declared in this package? + if di, ok := c.funcDecls[fn]; ok { + c.buildDecl(fn, di) + return !di.noReturn + } + + // Not declared in this package. + // Is there a fact from another package? + return !c.pass.ImportObjectFact(fn, new(noReturn)) +} + +var panicBuiltin = types.Universe.Lookup("panic").(*types.Builtin) + +func hasReachableReturn(g *cfg.CFG) bool { + for _, b := range g.Blocks { + if b.Live && b.Return() != nil { + return true + } + } + return false +} + +// isIntrinsicNoReturn reports whether a function intrinsically never +// returns because it stops execution of the calling thread. +// It is the base case in the recursion. +func isIntrinsicNoReturn(fn *types.Func) bool { + // Add functions here as the need arises, but don't allocate memory. + path, name := fn.Pkg().Path(), fn.Name() + return path == "syscall" && (name == "Exit" || name == "ExitProcess" || name == "ExitThread") || + path == "runtime" && name == "Goexit" +} diff --git a/go/analysis/passes/ctrlflow/ctrlflow_test.go b/go/analysis/passes/ctrlflow/ctrlflow_test.go new file mode 100644 index 0000000000..61e404552a --- /dev/null +++ b/go/analysis/passes/ctrlflow/ctrlflow_test.go @@ -0,0 +1,31 @@ +package ctrlflow_test + +import ( + "go/ast" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/ctrlflow" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + + // load testdata/src/a/a.go + pass, result := analysistest.Run(t, testdata, ctrlflow.Analyzer, "a") + + // Perform a minimal smoke test on + // the result (CFG) computed by ctrlflow. + if result != nil { + cfgs := result.(*ctrlflow.CFGs) + + for _, decl := range pass.Files[0].Decls { + if decl, ok := decl.(*ast.FuncDecl); ok && decl.Body != nil { + if cfgs.FuncDecl(decl) == nil { + t.Errorf("%s: no CFG for func %s", + pass.Fset.Position(decl.Pos()), decl.Name.Name) + } + } + } + } +} diff --git a/go/analysis/passes/ctrlflow/testdata/src/a/a.go b/go/analysis/passes/ctrlflow/testdata/src/a/a.go new file mode 100644 index 0000000000..462aeaa820 --- /dev/null +++ b/go/analysis/passes/ctrlflow/testdata/src/a/a.go @@ -0,0 +1,99 @@ +package a + +// This file tests facts produced by ctrlflow. + +import ( + "log" + "os" + "runtime" + "syscall" + "testing" +) + +var cond bool + +func a() { // want a:"noReturn" + if cond { + b() + } else { + for { + } + } +} + +func b() { // want b:"noReturn" + select {} +} + +func f(x int) { // no fact here + switch x { + case 0: + os.Exit(0) + case 1: + panic(0) + } + // default case returns +} + +type T int + +func (T) method1() { // want method1:"noReturn" + a() +} + +func (T) method2() { // (may return) + if cond { + a() + } +} + +// Checking for the noreturn fact associated with F ensures that +// ctrlflow proved each of the listed functions was "noReturn". +// +func standardFunctions(x int) { // want standardFunctions:"noReturn" + t := new(testing.T) + switch x { + case 0: + t.FailNow() + case 1: + t.Fatal() + case 2: + t.Fatalf("") + case 3: + t.Skip() + case 4: + t.SkipNow() + case 5: + t.Skipf("") + case 6: + log.Fatal() + case 7: + log.Fatalf("") + case 8: + log.Fatalln() + case 9: + os.Exit(0) + case 10: + syscall.Exit(0) + case 11: + runtime.Goexit() + case 12: + log.Panic() + case 13: + log.Panicln() + case 14: + log.Panicf("") + default: + panic(0) + } +} + +// False positives are possible. +// This function is marked noReturn but in fact returns. +// +func spurious() { // want spurious:"noReturn" + defer func() { recover() }() + panic(nil) +} + +func noBody() diff --git a/go/analysis/passes/findcall/findcall_test.go b/go/analysis/passes/findcall/findcall_test.go index c521885c29..f1b9adc13c 100644 --- a/go/analysis/passes/findcall/findcall_test.go +++ b/go/analysis/passes/findcall/findcall_test.go @@ -51,7 +51,5 @@ func main() { // multiple variants of a single scenario. func TestFromFileSystem(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, findcall.Analyzer, - "a", // loads testdata/src/a/a.go. - ) + analysistest.Run(t, testdata, findcall.Analyzer, "a") // loads testdata/src/a/a.go. } diff --git a/go/analysis/passes/inspect/inspect.go b/go/analysis/passes/inspect/inspect.go new file mode 100644 index 0000000000..d9c0c2c0c5 --- /dev/null +++ b/go/analysis/passes/inspect/inspect.go @@ -0,0 +1,45 @@ +// Copyright 2018 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 inspect is an analysis that provides an AST inspector +// (golang.org/x/tools/go/ast/inspect.Inspect) for the syntax trees of a +// package. It is only a building block for other analyzers. +// +// Example of use in another analysis: +// +// import "golang.org/x/tools/go/analysis/passes/inspect" +// +// var Analyzer = &analysis.Analyzer{ +// ... +// Requires: reflect.TypeOf(new(inspect.Analyzer)), +// } +// +// func run(pass *analysis.Pass) (interface{}, error) { +// inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) +// inspect.Preorder(nil, func(n ast.Node) { +// ... +// }) +// return nil +// } +// +package inspect + +import ( + "reflect" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "inspect", + Doc: "optimize AST traversal for later passes", + Run: run, + RunDespiteErrors: true, + ResultType: reflect.TypeOf(new(inspector.Inspector)), +} + +func run(pass *analysis.Pass) (interface{}, error) { + return inspector.New(pass.Files), nil +} diff --git a/go/analysis/passes/pkgfact/pkgfact_test.go b/go/analysis/passes/pkgfact/pkgfact_test.go index 11e6f893a3..1e408833dd 100644 --- a/go/analysis/passes/pkgfact/pkgfact_test.go +++ b/go/analysis/passes/pkgfact/pkgfact_test.go @@ -9,7 +9,5 @@ import ( func Test(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, pkgfact.Analyzer, - "c", // loads testdata/src/c/c.go. - ) + analysistest.Run(t, testdata, pkgfact.Analyzer, "c") // load testdata/src/c/c.go }