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 <preston@prysmaticlabs.com>

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": [],

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": {
+    }
   }
 }

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

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
+}

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)
+}

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] }

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"],
+)

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
+}