diff --git a/go/analysis/passes/buildssa/buildssa.go b/go/analysis/passes/buildssa/buildssa.go new file mode 100644 index 00000000..02b7b18b --- /dev/null +++ b/go/analysis/passes/buildssa/buildssa.go @@ -0,0 +1,117 @@ +// 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 buildssa defines an Analyzer that constructs the SSA +// representation of an error-free package and returns the set of all +// functions within it. It does not report any diagnostics itself but +// may be used as an input to other analyzers. +// +// THIS INTERFACE IS EXPERIMENTAL AND MAY BE SUBJECT TO INCOMPATIBLE CHANGE. +package buildssa + +import ( + "go/ast" + "go/types" + "reflect" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/ssa" +) + +var Analyzer = &analysis.Analyzer{ + Name: "buildssa", + Doc: "build SSA-form IR for later passes", + Run: run, + ResultType: reflect.TypeOf(new(SSA)), +} + +// SSA provides SSA-form intermediate representation for all the +// non-blank source functions in the current package. +type SSA struct { + Pkg *ssa.Package + SrcFuncs []*ssa.Function +} + +func run(pass *analysis.Pass) (interface{}, error) { + // Plundered from ssautil.BuildPackage. + + // We must create a new Program for each Package because the + // analysis API provides no place to hang a Program shared by + // all Packages. Consequently, SSA Packages and Functions do not + // have a canonical representation across an analysis session of + // multiple packages. This is unlikely to be a problem in + // practice because the analysis API essentially forces all + // packages to be analysed independently, so any given call to + // Analysis.Run on a package will see only SSA objects belonging + // to a single Program. + + // Some Analyzers may need GlobalDebug, in which case we'll have + // to set it globally, but let's wait till we need it. + mode := ssa.BuilderMode(0) + + prog := ssa.NewProgram(pass.Fset, mode) + + // Create SSA packages for all imports. + // Order is not significant. + created := make(map[*types.Package]bool) + var createAll func(pkgs []*types.Package) + createAll = func(pkgs []*types.Package) { + for _, p := range pkgs { + if !created[p] { + created[p] = true + prog.CreatePackage(p, nil, nil, true) + createAll(p.Imports()) + } + } + } + createAll(pass.Pkg.Imports()) + + // Create and build the primary package. + ssapkg := prog.CreatePackage(pass.Pkg, pass.Files, pass.TypesInfo, false) + ssapkg.Build() + + // Compute list of source functions, including literals, + // in source order. + var funcs []*ssa.Function + for _, f := range pass.Files { + for _, decl := range f.Decls { + if fdecl, ok := decl.(*ast.FuncDecl); ok { + + // SSA will not build a Function + // for a FuncDecl named blank. + // That's arguably too strict but + // relaxing it would break uniqueness of + // names of package members. + if fdecl.Name.Name == "_" { + continue + } + + // (init functions have distinct Func + // objects named "init" and distinct + // ssa.Functions named "init#1", ...) + + fn := pass.TypesInfo.Defs[fdecl.Name].(*types.Func) + if fn == nil { + panic(fn) + } + + f := ssapkg.Prog.FuncValue(fn) + if f == nil { + panic(fn) + } + + var addAnons func(f *ssa.Function) + addAnons = func(f *ssa.Function) { + funcs = append(funcs, f) + for _, anon := range f.AnonFuncs { + addAnons(anon) + } + } + addAnons(f) + } + } + } + + return &SSA{Pkg: ssapkg, SrcFuncs: funcs}, nil +} diff --git a/go/analysis/passes/buildssa/buildssa_test.go b/go/analysis/passes/buildssa/buildssa_test.go new file mode 100644 index 00000000..0b381500 --- /dev/null +++ b/go/analysis/passes/buildssa/buildssa_test.go @@ -0,0 +1,29 @@ +// 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 buildssa_test + +import ( + "fmt" + "os" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/buildssa" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + result := analysistest.Run(t, testdata, buildssa.Analyzer, "a")[0].Result + + ssainfo := result.(*buildssa.SSA) + got := fmt.Sprint(ssainfo.SrcFuncs) + want := `[a.Fib (a.T).fib]` + if got != want { + t.Errorf("SSA.SrcFuncs = %s, want %s", got, want) + for _, f := range ssainfo.SrcFuncs { + f.WriteTo(os.Stderr) + } + } +} diff --git a/go/analysis/passes/buildssa/testdata/src/a/a.go b/go/analysis/passes/buildssa/testdata/src/a/a.go new file mode 100644 index 00000000..ddb13dac --- /dev/null +++ b/go/analysis/passes/buildssa/testdata/src/a/a.go @@ -0,0 +1,16 @@ +package a + +func Fib(x int) int { + if x < 2 { + return x + } + return Fib(x-1) + Fib(x-2) +} + +type T int + +func (T) fib(x int) int { return Fib(x) } + +func _() { + print("hi") +} diff --git a/go/analysis/passes/nilness/cmd/nilness/main.go b/go/analysis/passes/nilness/cmd/nilness/main.go new file mode 100644 index 00000000..bac26bd8 --- /dev/null +++ b/go/analysis/passes/nilness/cmd/nilness/main.go @@ -0,0 +1,10 @@ +// The nilness command applies the golang.org/x/tools/go/analysis/passes/nilness +// analysis to the specified packages of Go source code. +package main + +import ( + "golang.org/x/tools/go/analysis/passes/nilness" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { singlechecker.Main(nilness.Analyzer) } diff --git a/go/analysis/passes/nilness/nilness.go b/go/analysis/passes/nilness/nilness.go new file mode 100644 index 00000000..5fe3f267 --- /dev/null +++ b/go/analysis/passes/nilness/nilness.go @@ -0,0 +1,271 @@ +// 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 nilness inspects the control-flow graph of an SSA function +// and reports errors such as nil pointer dereferences and degenerate +// nil pointer comparisons. +package nilness + +import ( + "fmt" + "go/token" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/buildssa" + "golang.org/x/tools/go/ssa" +) + +const Doc = `check for redundant or impossible nil comparisons + +The nilness checker inspects the control-flow graph of each function in +a package and reports nil pointer dereferences and degenerate nil +pointers. A degenerate comparison is of the form x==nil or x!=nil where x +is statically known to be nil or non-nil. These are often a mistake, +especially in control flow related to errors. + +This check reports conditions such as: + + if f == nil { // impossible condition (f is a function) + } + +and: + + p := &v + ... + if p != nil { // tautological condition + } + +and: + + if p == nil { + print(*p) // nil dereference + } +` + +var Analyzer = &analysis.Analyzer{ + Name: "nilness", + Doc: Doc, + Run: run, + Requires: []*analysis.Analyzer{buildssa.Analyzer}, +} + +func run(pass *analysis.Pass) (interface{}, error) { + ssainput := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) + for _, fn := range ssainput.SrcFuncs { + runFunc(pass, fn) + } + return nil, nil +} + +func runFunc(pass *analysis.Pass, fn *ssa.Function) { + reportf := func(category string, pos token.Pos, format string, args ...interface{}) { + pass.Report(analysis.Diagnostic{ + Pos: pos, + Category: category, + Message: fmt.Sprintf(format, args...), + }) + } + + // notNil reports an error if v is provably nil. + notNil := func(stack []fact, instr ssa.Instruction, v ssa.Value, descr string) { + if nilnessOf(stack, v) == isnil { + reportf("nilderef", instr.Pos(), "nil dereference in "+descr) + } + } + + // visit visits reachable blocks of the CFG in dominance order, + // maintaining a stack of dominating nilness facts. + // + // By traversing the dom tree, we can pop facts off the stack as + // soon as we've visited a subtree. Had we traversed the CFG, + // we would need to retain the set of facts for each block. + seen := make([]bool, len(fn.Blocks)) // seen[i] means visit should ignore block i + var visit func(b *ssa.BasicBlock, stack []fact) + visit = func(b *ssa.BasicBlock, stack []fact) { + if seen[b.Index] { + return + } + seen[b.Index] = true + + // Report nil dereferences. + for _, instr := range b.Instrs { + switch instr := instr.(type) { + case ssa.CallInstruction: + notNil(stack, instr, instr.Common().Value, + instr.Common().Description()) + case *ssa.FieldAddr: + notNil(stack, instr, instr.X, "field selection") + case *ssa.IndexAddr: + notNil(stack, instr, instr.X, "index operation") + case *ssa.MapUpdate: + notNil(stack, instr, instr.Map, "map update") + case *ssa.Slice: + // A nilcheck occurs in ptr[:] iff ptr is a pointer to an array. + if _, ok := instr.X.Type().Underlying().(*types.Pointer); ok { + notNil(stack, instr, instr.X, "slice operation") + } + case *ssa.Store: + notNil(stack, instr, instr.Addr, "store") + case *ssa.TypeAssert: + notNil(stack, instr, instr.X, "type assertion") + case *ssa.UnOp: + if instr.Op == token.MUL { // *X + notNil(stack, instr, instr.X, "load") + } + } + } + + // For nil comparison blocks, report an error if the condition + // is degenerate, and push a nilness fact on the stack when + // visiting its true and false successor blocks. + if binop, tsucc, fsucc := eq(b); binop != nil { + xnil := nilnessOf(stack, binop.X) + ynil := nilnessOf(stack, binop.Y) + + if ynil != unknown && xnil != unknown && (xnil == isnil || ynil == isnil) { + // Degenerate condition: + // the nilness of both operands is known, + // and at least one of them is nil. + var adj string + if (xnil == ynil) == (binop.Op == token.EQL) { + adj = "tautological" + } else { + adj = "impossible" + } + reportf("cond", binop.Pos(), "%s condition: %s %s %s", adj, xnil, binop.Op, ynil) + + // If tsucc's or fsucc's sole incoming edge is impossible, + // it is unreachable. Prune traversal of it and + // all the blocks it dominates. + // (We could be more precise with full dataflow + // analysis of control-flow joins.) + var skip *ssa.BasicBlock + if xnil == ynil { + skip = fsucc + } else { + skip = tsucc + } + for _, d := range b.Dominees() { + if d == skip && len(d.Preds) == 1 { + continue + } + visit(d, stack) + } + return + } + + // "if x == nil" or "if nil == y" condition; x, y are unknown. + if xnil == isnil || ynil == isnil { + var f fact + if xnil == isnil { + // x is nil, y is unknown: + // t successor learns y is nil. + f = fact{binop.Y, isnil} + } else { + // x is nil, y is unknown: + // t successor learns x is nil. + f = fact{binop.X, isnil} + } + + for _, d := range b.Dominees() { + // Successor blocks learn a fact + // only at non-critical edges. + // (We could do be more precise with full dataflow + // analysis of control-flow joins.) + s := stack + if len(d.Preds) == 1 { + if d == tsucc { + s = append(s, f) + } else if d == fsucc { + s = append(s, f.negate()) + } + } + visit(d, s) + } + return + } + } + + for _, d := range b.Dominees() { + visit(d, stack) + } + } + + // Visit the entry block. No need to visit fn.Recover. + if fn.Blocks != nil { + visit(fn.Blocks[0], make([]fact, 0, 20)) // 20 is plenty + } +} + +// A fact records that a block is dominated +// by the condition v == nil or v != nil. +type fact struct { + value ssa.Value + nilness nilness +} + +func (f fact) negate() fact { return fact{f.value, -f.nilness} } + +type nilness int + +const ( + isnonnil = -1 + unknown nilness = 0 + isnil = 1 +) + +var nilnessStrings = []string{"non-nil", "unknown", "nil"} + +func (n nilness) String() string { return nilnessStrings[n+1] } + +// nilnessOf reports whether v is definitely nil, definitely not nil, +// or unknown given the dominating stack of facts. +func nilnessOf(stack []fact, v ssa.Value) nilness { + // Is value intrinsically nil or non-nil? + switch v := v.(type) { + case *ssa.Alloc, + *ssa.FieldAddr, + *ssa.FreeVar, + *ssa.Function, + *ssa.Global, + *ssa.IndexAddr, + *ssa.MakeChan, + *ssa.MakeClosure, + *ssa.MakeInterface, + *ssa.MakeMap, + *ssa.MakeSlice: + return isnonnil + case *ssa.Const: + if v.IsNil() { + return isnil + } else { + return isnonnil + } + } + + // Search dominating control-flow facts. + for _, f := range stack { + if f.value == v { + return f.nilness + } + } + return unknown +} + +// If b ends with an equality comparison, eq returns the operation and +// its true (equal) and false (not equal) successors. +func eq(b *ssa.BasicBlock) (op *ssa.BinOp, tsucc, fsucc *ssa.BasicBlock) { + if If, ok := b.Instrs[len(b.Instrs)-1].(*ssa.If); ok { + if binop, ok := If.Cond.(*ssa.BinOp); ok { + switch binop.Op { + case token.EQL: + return binop, b.Succs[0], b.Succs[1] + case token.NEQ: + return binop, b.Succs[1], b.Succs[0] + } + } + } + return nil, nil, nil +} diff --git a/go/analysis/passes/nilness/nilness_test.go b/go/analysis/passes/nilness/nilness_test.go new file mode 100644 index 00000000..b258c1ef --- /dev/null +++ b/go/analysis/passes/nilness/nilness_test.go @@ -0,0 +1,17 @@ +// 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 nilness_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/nilness" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, nilness.Analyzer, "a") +} diff --git a/go/analysis/passes/nilness/testdata/src/a/a.go b/go/analysis/passes/nilness/testdata/src/a/a.go new file mode 100644 index 00000000..94d25f6a --- /dev/null +++ b/go/analysis/passes/nilness/testdata/src/a/a.go @@ -0,0 +1,91 @@ +package a + +type X struct{ f, g int } + +func f(x, y *X) { + if x == nil { + print(x.f) // want "nil dereference in field selection" + } else { + print(x.f) + } + + if x == nil { + if nil != y { + print(1) + panic(0) + } + x.f = 1 // want "nil dereference in field selection" + y.f = 1 // want "nil dereference in field selection" + } + + var f func() + if f == nil { // want "tautological condition: nil == nil" + go f() // want "nil dereference in dynamic function call" + } else { + // This block is unreachable, + // so we don't report an error for the + // nil dereference in the call. + defer f() + } +} + +func f2(ptr *[3]int, i interface{}) { + if ptr != nil { + print(ptr[:]) + *ptr = [3]int{} + print(*ptr) + } else { + print(ptr[:]) // want "nil dereference in slice operation" + *ptr = [3]int{} // want "nil dereference in store" + print(*ptr) // want "nil dereference in load" + + if ptr != nil { // want "impossible condition: nil != nil" + // Dominated by ptr==nil and ptr!=nil, + // this block is unreachable. + // We do not report errors within it. + print(*ptr) + } + } + + if i != nil { + print(i.(interface{ f() })) + } else { + print(i.(interface{ f() })) // want "nil dereference in type assertion" + } +} + +func g() error + +func f3() error { + err := g() + if err != nil { + return err + } + if err != nil && err.Error() == "foo" { // want "impossible condition: nil != nil" + print(0) + } + ch := make(chan int) + if ch == nil { // want "impossible condition: non-nil == nil" + print(0) + } + if ch != nil { // want "tautological condition: non-nil != nil" + print(0) + } + return nil +} + +func h(err error, b bool) { + if err != nil && b { + return + } else if err != nil { + panic(err) + } +} + +func i(*int) error { + for { + if err := g(); err != nil { + return err + } + } +}