From ecfdb354a736dcbefbcfda70bc694afad1d3ff4c Mon Sep 17 00:00:00 2001 From: Victor Farazdagi Date: Fri, 2 Oct 2020 23:56:18 +0300 Subject: [PATCH] Add new static analyzer: ineffassign (#7413) * test defined * first working impl * register analyzer * cleanup * removes unused code * secure rand in slasher/detection/testing * Updates test * fixes ineffassign type checks * one more Co-authored-by: Preston Van Loon --- BUILD.bazel | 1 + nogo_config.json | 10 + tools/analyzers/ineffassign/BUILD.bazel | 34 + tools/analyzers/ineffassign/analyzer.go | 56 ++ tools/analyzers/ineffassign/analyzer_test.go | 11 + tools/analyzers/ineffassign/ineffassign.go | 601 ++++++++++++++++++ .../ineffassign/testdata/BUILD.bazel | 8 + .../ineffassign/testdata/ctx_assignment.go | 18 + 8 files changed, 739 insertions(+) create mode 100644 tools/analyzers/ineffassign/BUILD.bazel create mode 100644 tools/analyzers/ineffassign/analyzer.go create mode 100644 tools/analyzers/ineffassign/analyzer_test.go create mode 100644 tools/analyzers/ineffassign/ineffassign.go create mode 100644 tools/analyzers/ineffassign/testdata/BUILD.bazel create mode 100644 tools/analyzers/ineffassign/testdata/ctx_assignment.go diff --git a/BUILD.bazel b/BUILD.bazel index 977976189..eb9cc89ee 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -111,6 +111,7 @@ nogo( "//tools/analyzers/shadowpredecl:go_tool_library", "//tools/analyzers/nop:go_tool_library", "//tools/analyzers/slicedirect:go_tool_library", + "//tools/analyzers/ineffassign:go_tool_library", ] + select({ # nogo checks that fail with coverage enabled. ":coverage_enabled": [], diff --git a/nogo_config.json b/nogo_config.json index d4d2ffa3b..d9bbe6523 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -122,5 +122,15 @@ "rules_go_work-.*": "Third party code", "tools/analyzers/slicedirect/testdata/slice.go": "Analyzer testdata has to break rules" } + }, + "ineffassign": { + "only_files": { + "beacon-chain/.*": "", + "shared/.*": "", + "slasher/.*": "", + "validator/.*": "" + }, + "exclude_files": { + } } } diff --git a/tools/analyzers/ineffassign/BUILD.bazel b/tools/analyzers/ineffassign/BUILD.bazel new file mode 100644 index 000000000..4e86791af --- /dev/null +++ b/tools/analyzers/ineffassign/BUILD.bazel @@ -0,0 +1,34 @@ +load("@prysm//tools/go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_tool_library") + +go_library( + name = "go_default_library", + srcs = [ + "analyzer.go", + "ineffassign.go", + ], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/ineffassign", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_default_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library", + "@org_golang_x_tools//go/ast/inspector:go_default_library", + ], +) + +go_tool_library( + name = "go_tool_library", + srcs = [ + "analyzer.go", + "ineffassign.go", + ], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/ineffassign", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_tool_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library", + "@org_golang_x_tools//go/ast/inspector:go_tool_library", + ], +) + +# gazelle:exclude analyzer_test.go diff --git a/tools/analyzers/ineffassign/analyzer.go b/tools/analyzers/ineffassign/analyzer.go new file mode 100644 index 000000000..ffe62046a --- /dev/null +++ b/tools/analyzers/ineffassign/analyzer.go @@ -0,0 +1,56 @@ +// Package ineffassign implements a static analyzer to ensure that there are no ineffectual +// assignments in source code. +package ineffassign + +import ( + "errors" + "go/ast" + "sort" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +// Doc explaining the tool. +const Doc = "Tool to make sure there are no ineffectual assignments in source code" + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "ineffassign", + Doc: Doc, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + insp, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, errors.New("analyzer is not type *inspector.Inspector") + } + + nodeFilter := []ast.Node{ + (*ast.File)(nil), + } + insp.Preorder(nodeFilter, func(node ast.Node) { + f, ok := node.(*ast.File) + if !ok { + return + } + bld := &builder{vars: map[*ast.Object]*variable{}} + bld.walk(f) + chk := &checker{vars: bld.vars, seen: map[*block]bool{}} + for _, b := range bld.roots { + chk.check(b) + } + sort.Sort(chk.ineff) + // Report ineffectual assignments if any. + for _, id := range chk.ineff { + if id.Name != "ctx" { // We allow ineffectual assignment to ctx (to override ctx). + pass.Reportf(id.Pos(), "ineffectual assignment to %q", id.Name) + } + } + }) + + return nil, nil +} diff --git a/tools/analyzers/ineffassign/analyzer_test.go b/tools/analyzers/ineffassign/analyzer_test.go new file mode 100644 index 000000000..2676f3e85 --- /dev/null +++ b/tools/analyzers/ineffassign/analyzer_test.go @@ -0,0 +1,11 @@ +package ineffassign + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), Analyzer) +} diff --git a/tools/analyzers/ineffassign/ineffassign.go b/tools/analyzers/ineffassign/ineffassign.go new file mode 100644 index 000000000..e9ea293c3 --- /dev/null +++ b/tools/analyzers/ineffassign/ineffassign.go @@ -0,0 +1,601 @@ +package ineffassign + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" +) + +// The core of this code originates here: https://github.com/gordonklaus/ineffassign and was +// adapted for our workflow (the original source code is under the MIT license). +// If you need a standalone CLI ineffassign runner, please, use the one above. +func walkPath(root string) bool { + lintFailed := false + err := filepath.Walk(root, func(path string, fi os.FileInfo, err error) error { + if err != nil { + fmt.Printf("Error during filesystem walk: %v\n", err) + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + fset, _, ineff := checkPath(path) + for _, id := range ineff { + fmt.Printf("%s: ineffectual assignment to %s\n", fset.Position(id.Pos()), id.Name) + lintFailed = true + } + return nil + }) + if err != nil { + return false + } + return lintFailed +} + +func checkPath(path string) (*token.FileSet, []*ast.CommentGroup, []*ast.Ident) { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, path, nil, parser.ParseComments) + if err != nil { + return nil, nil, nil + } + + bld := &builder{vars: map[*ast.Object]*variable{}} + bld.walk(f) + + chk := &checker{vars: bld.vars, seen: map[*block]bool{}} + for _, b := range bld.roots { + chk.check(b) + } + sort.Sort(chk.ineff) + + return fset, f.Comments, chk.ineff +} + +type builder struct { + roots []*block + block *block + vars map[*ast.Object]*variable + results []*ast.FieldList + breaks branchStack + continues branchStack + gotos branchStack + labelStmt *ast.LabeledStmt +} + +type block struct { + children []*block + ops map[*ast.Object][]operation +} + +func (b *block) addChild(c *block) { + b.children = append(b.children, c) +} + +type operation struct { + id *ast.Ident + assign bool +} + +type variable struct { + fundept int + escapes bool +} + +func (bld *builder) walk(n ast.Node) { + if n != nil { + ast.Walk(bld, n) + } +} + +func (bld *builder) Visit(n ast.Node) ast.Visitor { + switch n := n.(type) { + case *ast.FuncDecl: + if n.Body != nil { + bld.fun(n.Type, n.Body) + } + case *ast.FuncLit: + bld.fun(n.Type, n.Body) + case *ast.IfStmt: + bld.walk(n.Init) + bld.walk(n.Cond) + b0 := bld.block + bld.newBlock(b0) + bld.walk(n.Body) + b1 := bld.block + if n.Else != nil { + bld.newBlock(b0) + bld.walk(n.Else) + b0 = bld.block + } + bld.newBlock(b0, b1) + case *ast.ForStmt: + lbl := bld.stmtLabel(n) + brek := bld.breaks.push(lbl) + continu := bld.continues.push(lbl) + bld.walk(n.Init) + start := bld.newBlock(bld.block) + bld.walk(n.Cond) + cond := bld.block + bld.newBlock(cond) + bld.walk(n.Body) + continu.setDestination(bld.newBlock(bld.block)) + bld.walk(n.Post) + bld.block.addChild(start) + brek.setDestination(bld.newBlock(cond)) + bld.breaks.pop() + bld.continues.pop() + case *ast.RangeStmt: + lbl := bld.stmtLabel(n) + brek := bld.breaks.push(lbl) + continu := bld.continues.push(lbl) + bld.walk(n.X) + pre := bld.newBlock(bld.block) + start := bld.newBlock(pre) + if n.Key != nil { + lhs := []ast.Expr{n.Key} + if n.Value != nil { + lhs = append(lhs, n.Value) + } + bld.walk(&ast.AssignStmt{Lhs: lhs, Tok: n.Tok, TokPos: n.TokPos, Rhs: []ast.Expr{&ast.Ident{NamePos: n.X.End()}}}) + } + bld.walk(n.Body) + bld.block.addChild(start) + continu.setDestination(pre) + brek.setDestination(bld.newBlock(pre, bld.block)) + bld.breaks.pop() + bld.continues.pop() + case *ast.SwitchStmt: + bld.walk(n.Init) + bld.walk(n.Tag) + bld.swtch(n, n.Body.List) + case *ast.TypeSwitchStmt: + bld.walk(n.Init) + bld.walk(n.Assign) + bld.swtch(n, n.Body.List) + case *ast.SelectStmt: + brek := bld.breaks.push(bld.stmtLabel(n)) + for _, c := range n.Body.List { + c := c.(*ast.CommClause).Comm + if s, ok := c.(*ast.AssignStmt); ok { + bld.walk(s.Rhs[0]) + } else { + bld.walk(c) + } + } + b0 := bld.block + exits := make([]*block, len(n.Body.List)) + dfault := false + for i, c := range n.Body.List { + c, ok := c.(*ast.CommClause) + if !ok { + continue + } + bld.newBlock(b0) + bld.walk(c) + exits[i] = bld.block + dfault = dfault || c.Comm == nil + } + if !dfault { + exits = append(exits, b0) + } + brek.setDestination(bld.newBlock(exits...)) + bld.breaks.pop() + case *ast.LabeledStmt: + bld.gotos.get(n.Label).setDestination(bld.newBlock(bld.block)) + bld.labelStmt = n + bld.walk(n.Stmt) + case *ast.BranchStmt: + switch n.Tok { + case token.BREAK: + bld.breaks.get(n.Label).addSource(bld.block) + bld.newBlock() + case token.CONTINUE: + bld.continues.get(n.Label).addSource(bld.block) + bld.newBlock() + case token.GOTO: + bld.gotos.get(n.Label).addSource(bld.block) + bld.newBlock() + } + + case *ast.AssignStmt: + if n.Tok == token.QUO_ASSIGN || n.Tok == token.REM_ASSIGN { + bld.maybePanic() + } + + for _, x := range n.Rhs { + bld.walk(x) + } + for i, x := range n.Lhs { + if id, ok := ident(x); ok { + if n.Tok >= token.ADD_ASSIGN && n.Tok <= token.AND_NOT_ASSIGN { + bld.use(id) + } + // Don't treat explicit initialization to zero as assignment; it is often used as shorthand for a bare declaration. + if n.Tok == token.DEFINE && i < len(n.Rhs) && isZeroInitializer(n.Rhs[i]) { + bld.use(id) + } else { + bld.assign(id) + } + } else { + bld.walk(x) + } + } + case *ast.GenDecl: + if n.Tok == token.VAR { + for _, s := range n.Specs { + s, ok := s.(*ast.ValueSpec) + if !ok { + continue + } + for _, x := range s.Values { + bld.walk(x) + } + for _, id := range s.Names { + if len(s.Values) > 0 { + bld.assign(id) + } else { + bld.use(id) + } + } + } + } + case *ast.IncDecStmt: + if id, ok := ident(n.X); ok { + bld.use(id) + bld.assign(id) + } else { + bld.walk(n.X) + } + case *ast.Ident: + bld.use(n) + case *ast.ReturnStmt: + for _, x := range n.Results { + bld.walk(x) + } + if res := bld.results[len(bld.results)-1]; res != nil { + for _, f := range res.List { + for _, id := range f.Names { + if n.Results != nil { + bld.assign(id) + } + bld.use(id) + } + } + } + bld.newBlock() + case *ast.SendStmt: + bld.maybePanic() + return bld + + case *ast.BinaryExpr: + if n.Op == token.EQL || n.Op == token.QUO || n.Op == token.REM { + bld.maybePanic() + } + return bld + case *ast.CallExpr: + bld.maybePanic() + return bld + case *ast.IndexExpr: + bld.maybePanic() + return bld + case *ast.UnaryExpr: + id, ok := ident(n.X) + if ix, isIx := n.X.(*ast.IndexExpr); isIx { + // We don't care about indexing into slices, but without type information we can do no better. + id, ok = ident(ix.X) + } + if ok && n.Op == token.AND { + if v, ok := bld.vars[id.Obj]; ok { + v.escapes = true + } + } + return bld + case *ast.SelectorExpr: + bld.maybePanic() + // A method call (possibly delayed via a method value) might implicitly take + // the address of its receiver, causing it to escape. + // We can't do any better here without knowing the variable's type. + if id, ok := ident(n.X); ok { + if v, ok := bld.vars[id.Obj]; ok { + v.escapes = true + } + } + return bld + case *ast.SliceExpr: + bld.maybePanic() + // We don't care about slicing into slices, but without type information we can do no better. + if id, ok := ident(n.X); ok { + if v, ok := bld.vars[id.Obj]; ok { + v.escapes = true + } + } + return bld + case *ast.StarExpr: + bld.maybePanic() + return bld + case *ast.TypeAssertExpr: + bld.maybePanic() + return bld + + default: + return bld + } + return nil +} + +func isZeroInitializer(x ast.Expr) bool { + // Assume that a call expression of a single argument is a conversion expression. We can't do better without type information. + if c, ok := x.(*ast.CallExpr); ok { + switch c.Fun.(type) { + case *ast.Ident, *ast.SelectorExpr: + default: + return false + } + if len(c.Args) != 1 { + return false + } + x = c.Args[0] + } + + switch x := x.(type) { + case *ast.BasicLit: + switch x.Value { + case "0", "0.0", "0.", ".0", `""`: + return true + } + case *ast.Ident: + return x.Name == "false" && x.Obj == nil + } + + return false +} + +func (bld *builder) fun(typ *ast.FuncType, body *ast.BlockStmt) { + for _, v := range bld.vars { + v.fundept++ + } + bld.results = append(bld.results, typ.Results) + + b := bld.block + bld.newBlock() + bld.roots = append(bld.roots, bld.block) + bld.walk(typ) + bld.walk(body) + bld.block = b + + bld.results = bld.results[:len(bld.results)-1] + for _, v := range bld.vars { + v.fundept-- + } +} + +func (bld *builder) swtch(stmt ast.Stmt, cases []ast.Stmt) { + brek := bld.breaks.push(bld.stmtLabel(stmt)) + b0 := bld.block + list := b0 + exits := make([]*block, 0, len(cases)+1) + var dfault, fallthru *block + for _, c := range cases { + c, ok := c.(*ast.CaseClause) + if !ok { + continue + } + if c.List != nil { + list = bld.newBlock(list) + for _, x := range c.List { + bld.walk(x) + } + } + + parents := []*block{} + if c.List != nil { + parents = append(parents, list) + } + if fallthru != nil { + parents = append(parents, fallthru) + fallthru = nil + } + bld.newBlock(parents...) + if c.List == nil { + dfault = bld.block + } + for _, s := range c.Body { + bld.walk(s) + if s, ok := s.(*ast.BranchStmt); ok && s.Tok == token.FALLTHROUGH { + fallthru = bld.block + } + } + + if fallthru == nil { + exits = append(exits, bld.block) + } + } + if dfault != nil { + list.addChild(dfault) + } else { + exits = append(exits, b0) + } + brek.setDestination(bld.newBlock(exits...)) + bld.breaks.pop() +} + +// An operation that might panic marks named function results as used. +func (bld *builder) maybePanic() { + if len(bld.results) == 0 { + return + } + res := bld.results[len(bld.results)-1] + if res == nil { + return + } + for _, f := range res.List { + for _, id := range f.Names { + bld.use(id) + } + } +} + +func (bld *builder) newBlock(parents ...*block) *block { + bld.block = &block{ops: map[*ast.Object][]operation{}} + for _, b := range parents { + b.addChild(bld.block) + } + return bld.block +} + +func (bld *builder) stmtLabel(s ast.Stmt) *ast.Object { + if ls := bld.labelStmt; ls != nil && ls.Stmt == s { + return ls.Label.Obj + } + return nil +} + +func (bld *builder) assign(id *ast.Ident) { + bld.newOp(id, true) +} + +func (bld *builder) use(id *ast.Ident) { + bld.newOp(id, false) +} + +func (bld *builder) newOp(id *ast.Ident, assign bool) { + if id.Name == "_" || id.Obj == nil { + return + } + + v, ok := bld.vars[id.Obj] + if !ok { + v = &variable{} + bld.vars[id.Obj] = v + } + v.escapes = v.escapes || v.fundept > 0 || bld.block == nil + + if b := bld.block; b != nil { + b.ops[id.Obj] = append(b.ops[id.Obj], operation{id, assign}) + } +} + +type branchStack []*branch + +type branch struct { + label *ast.Object + srcs []*block + dst *block +} + +func (s *branchStack) push(lbl *ast.Object) *branch { + br := &branch{label: lbl} + *s = append(*s, br) + return br +} + +func (s *branchStack) get(lbl *ast.Ident) *branch { + for i := len(*s) - 1; i >= 0; i-- { + if br := (*s)[i]; lbl == nil || br.label == lbl.Obj { + return br + } + } + + // Guard against invalid code (break/continue outside of loop). + if lbl == nil { + return &branch{} + } + + return s.push(lbl.Obj) +} + +func (br *branch) addSource(src *block) { + br.srcs = append(br.srcs, src) + if br.dst != nil { + src.addChild(br.dst) + } +} + +func (br *branch) setDestination(dst *block) { + br.dst = dst + for _, src := range br.srcs { + src.addChild(dst) + } +} + +func (s *branchStack) pop() { + *s = (*s)[:len(*s)-1] +} + +func ident(x ast.Expr) (*ast.Ident, bool) { + if p, ok := x.(*ast.ParenExpr); ok { + return ident(p.X) + } + id, ok := x.(*ast.Ident) + return id, ok +} + +type checker struct { + vars map[*ast.Object]*variable + seen map[*block]bool + ineff idents +} + +func (chk *checker) check(b *block) { + if chk.seen[b] { + return + } + chk.seen[b] = true + + for obj, ops := range b.ops { + if chk.vars[obj].escapes { + continue + } + ops: + for i, op := range ops { + if !op.assign { + continue + } + if i+1 < len(ops) { + if ops[i+1].assign { + chk.ineff = append(chk.ineff, op.id) + } + continue + } + seen := map[*block]bool{} + for _, b := range b.children { + if used(obj, b, seen) { + continue ops + } + } + chk.ineff = append(chk.ineff, op.id) + } + } + + for _, b := range b.children { + chk.check(b) + } +} + +func used(obj *ast.Object, b *block, seen map[*block]bool) bool { + if seen[b] { + return false + } + seen[b] = true + + if ops := b.ops[obj]; len(ops) > 0 { + return !ops[0].assign + } + for _, b := range b.children { + if used(obj, b, seen) { + return true + } + } + return false +} + +type idents []*ast.Ident + +func (ids idents) Len() int { return len(ids) } +func (ids idents) Less(i, j int) bool { return ids[i].Pos() < ids[j].Pos() } +func (ids idents) Swap(i, j int) { ids[i], ids[j] = ids[j], ids[i] } diff --git a/tools/analyzers/ineffassign/testdata/BUILD.bazel b/tools/analyzers/ineffassign/testdata/BUILD.bazel new file mode 100644 index 000000000..9b6ff1012 --- /dev/null +++ b/tools/analyzers/ineffassign/testdata/BUILD.bazel @@ -0,0 +1,8 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["ctx_assignment.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/ineffassign/testdata", + visibility = ["//visibility:public"], +) diff --git a/tools/analyzers/ineffassign/testdata/ctx_assignment.go b/tools/analyzers/ineffassign/testdata/ctx_assignment.go new file mode 100644 index 000000000..ae0d5c1e5 --- /dev/null +++ b/tools/analyzers/ineffassign/testdata/ctx_assignment.go @@ -0,0 +1,18 @@ +package testdata + +import ( + "context" + "fmt" +) + +// This returns the head state. +// It does a full copy on head state for immutability. +func headState(ctx context.Context) { + ctx, span := StartSpan(ctx, "blockChain.headState") + fmt.Print(span) + return +} + +func StartSpan(ctx context.Context, name string) (context.Context, int) { + return ctx, 42 +}