From 9916a2fb79eada13f9bea34f6604c5c0312b5b23 Mon Sep 17 00:00:00 2001 From: Denis Isaev Date: Mon, 11 Feb 2019 09:02:53 +0300 Subject: [PATCH] Relates #367: update unparam $ git cherry -v cc9d2fb52971 fbb59629db34 + 7362051ae01a0e35956c077c3be5505c70edd200 testdata: add more regression tests + a88ca0234e2c3732a53cd49514fb3877a5d9f1f5 properly record which methods implement interfaces + b762b0b27fa23ebbdfc31df1af4097cfd89a17f6 work properly with method wrapper functions + f59bb08c5c7429b88e0c6e2399b12e71e8d950db testdata: consistently use tabs + 46a5101c55d03117b263b4c5161e5d01353311c1 replace more callgraph code with plain SSA + 1679b9996abdc6431c2147a133e5223ebb86ea60 rewrite foo(bar()) code to work without callgraph + 71b5df77c291f8c5ead5f9ff0a69e0eebf3ae5b2 rewrite "called in return" code without callgraph + fc5b1c74f563d4f2f9ee7d91eda648fc7baebf67 check: replace last use of callgraph + 229ad68a599e2622cf9d3bb87a385b158efe736f fix a minor false negative in callExtract + fbb59629db34a0f69275bc336cc8c3a4dd9fbe5d fix up another false negative in testdata --- go.mod | 2 +- go.sum | 4 +- .../x/tools/go/callgraph/callgraph.go | 129 ------ .../x/tools/go/callgraph/cha/cha.go | 139 ------ .../golang.org/x/tools/go/callgraph/util.go | 181 -------- vendor/modules.txt | 4 +- vendor/mvdan.cc/unparam/check/check.go | 413 +++++++++++------- 7 files changed, 263 insertions(+), 609 deletions(-) delete mode 100644 vendor/golang.org/x/tools/go/callgraph/callgraph.go delete mode 100644 vendor/golang.org/x/tools/go/callgraph/cha/cha.go delete mode 100644 vendor/golang.org/x/tools/go/callgraph/util.go diff --git a/go.mod b/go.mod index 059eb3be..a54429ce 100644 --- a/go.mod +++ b/go.mod @@ -61,7 +61,7 @@ require ( gopkg.in/yaml.v2 v2.2.1 mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b // indirect - mvdan.cc/unparam v0.0.0-20190124213536-cc9d2fb52971 + mvdan.cc/unparam v0.0.0-20190124213536-fbb59629db34 sourcegraph.com/sourcegraph/go-diff v0.0.0-20171119081133-3f415a150aec sourcegraph.com/sqs/pbtypes v0.0.0-20160107090929-4d1b9dc7ffc3 // indirect ) diff --git a/go.sum b/go.sum index ab76f95c..3b2ef110 100644 --- a/go.sum +++ b/go.sum @@ -173,8 +173,8 @@ mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed h1:WX1yoOaKQfddO/mLzdV4wp mvdan.cc/interfacer v0.0.0-20180901003855-c20040233aed/go.mod h1:Xkxe497xwlCKkIaQYRfC7CSLworTXY9RMqwhhCm+8Nc= mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b h1:DxJ5nJdkhDlLok9K6qO+5290kphDJbHOQO1DFFFTeBo= mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b/go.mod h1:2odslEg/xrtNQqCYg2/jCoyKnw3vv5biOc3JnIcYfL4= -mvdan.cc/unparam v0.0.0-20190124213536-cc9d2fb52971 h1:INXoeng6RAblgAds8fdt2cAu1Bs/bgr5zq1fdYcuxMI= -mvdan.cc/unparam v0.0.0-20190124213536-cc9d2fb52971/go.mod h1:H6SUd1XjIs+qQCyskXg5OFSrilMRUkD8ePJpHKDPaeY= +mvdan.cc/unparam v0.0.0-20190124213536-fbb59629db34 h1:B1LAOfRqg2QUyCdzfjf46quTSYUTAK5OCwbh6pljHbM= +mvdan.cc/unparam v0.0.0-20190124213536-fbb59629db34/go.mod h1:H6SUd1XjIs+qQCyskXg5OFSrilMRUkD8ePJpHKDPaeY= sourcegraph.com/sourcegraph/go-diff v0.0.0-20171119081133-3f415a150aec h1:wAAdENPXC7bE1oxY4VqSDdhaA+XQ8TgQHsZMMnrXjEk= sourcegraph.com/sourcegraph/go-diff v0.0.0-20171119081133-3f415a150aec/go.mod h1:R09mWeb9JcPbO+A3cYDc11xjz0wp6r9+KnqdqROAoRU= sourcegraph.com/sqs/pbtypes v0.0.0-20160107090929-4d1b9dc7ffc3 h1:hXy8YsgVLDz5mlngKhNHQhAsAGrSp3dlXZN4b0/4UUI= diff --git a/vendor/golang.org/x/tools/go/callgraph/callgraph.go b/vendor/golang.org/x/tools/go/callgraph/callgraph.go deleted file mode 100644 index 707a3193..00000000 --- a/vendor/golang.org/x/tools/go/callgraph/callgraph.go +++ /dev/null @@ -1,129 +0,0 @@ -// 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 callgraph defines the call graph and various algorithms -and utilities to operate on it. - -A call graph is a labelled directed graph whose nodes represent -functions and whose edge labels represent syntactic function call -sites. The presence of a labelled edge (caller, site, callee) -indicates that caller may call callee at the specified call site. - -A call graph is a multigraph: it may contain multiple edges (caller, -*, callee) connecting the same pair of nodes, so long as the edges -differ by label; this occurs when one function calls another function -from multiple call sites. Also, it may contain multiple edges -(caller, site, *) that differ only by callee; this indicates a -polymorphic call. - -A SOUND call graph is one that overapproximates the dynamic calling -behaviors of the program in all possible executions. One call graph -is more PRECISE than another if it is a smaller overapproximation of -the dynamic behavior. - -All call graphs have a synthetic root node which is responsible for -calling main() and init(). - -Calls to built-in functions (e.g. panic, println) are not represented -in the call graph; they are treated like built-in operators of the -language. - -*/ -package callgraph // import "golang.org/x/tools/go/callgraph" - -// TODO(adonovan): add a function to eliminate wrappers from the -// callgraph, preserving topology. -// More generally, we could eliminate "uninteresting" nodes such as -// nodes from packages we don't care about. - -import ( - "fmt" - "go/token" - - "golang.org/x/tools/go/ssa" -) - -// A Graph represents a call graph. -// -// A graph may contain nodes that are not reachable from the root. -// If the call graph is sound, such nodes indicate unreachable -// functions. -// -type Graph struct { - Root *Node // the distinguished root node - Nodes map[*ssa.Function]*Node // all nodes by function -} - -// New returns a new Graph with the specified root node. -func New(root *ssa.Function) *Graph { - g := &Graph{Nodes: make(map[*ssa.Function]*Node)} - g.Root = g.CreateNode(root) - return g -} - -// CreateNode returns the Node for fn, creating it if not present. -func (g *Graph) CreateNode(fn *ssa.Function) *Node { - n, ok := g.Nodes[fn] - if !ok { - n = &Node{Func: fn, ID: len(g.Nodes)} - g.Nodes[fn] = n - } - return n -} - -// A Node represents a node in a call graph. -type Node struct { - Func *ssa.Function // the function this node represents - ID int // 0-based sequence number - In []*Edge // unordered set of incoming call edges (n.In[*].Callee == n) - Out []*Edge // unordered set of outgoing call edges (n.Out[*].Caller == n) -} - -func (n *Node) String() string { - return fmt.Sprintf("n%d:%s", n.ID, n.Func) -} - -// A Edge represents an edge in the call graph. -// -// Site is nil for edges originating in synthetic or intrinsic -// functions, e.g. reflect.Call or the root of the call graph. -type Edge struct { - Caller *Node - Site ssa.CallInstruction - Callee *Node -} - -func (e Edge) String() string { - return fmt.Sprintf("%s --> %s", e.Caller, e.Callee) -} - -func (e Edge) Description() string { - var prefix string - switch e.Site.(type) { - case nil: - return "synthetic call" - case *ssa.Go: - prefix = "concurrent " - case *ssa.Defer: - prefix = "deferred " - } - return prefix + e.Site.Common().Description() -} - -func (e Edge) Pos() token.Pos { - if e.Site == nil { - return token.NoPos - } - return e.Site.Pos() -} - -// AddEdge adds the edge (caller, site, callee) to the call graph. -// Elimination of duplicate edges is the caller's responsibility. -func AddEdge(caller *Node, site ssa.CallInstruction, callee *Node) { - e := &Edge{caller, site, callee} - callee.In = append(callee.In, e) - caller.Out = append(caller.Out, e) -} diff --git a/vendor/golang.org/x/tools/go/callgraph/cha/cha.go b/vendor/golang.org/x/tools/go/callgraph/cha/cha.go deleted file mode 100644 index 215ff173..00000000 --- a/vendor/golang.org/x/tools/go/callgraph/cha/cha.go +++ /dev/null @@ -1,139 +0,0 @@ -// Copyright 2014 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 cha computes the call graph of a Go program using the Class -// Hierarchy Analysis (CHA) algorithm. -// -// CHA was first described in "Optimization of Object-Oriented Programs -// Using Static Class Hierarchy Analysis", Jeffrey Dean, David Grove, -// and Craig Chambers, ECOOP'95. -// -// CHA is related to RTA (see go/callgraph/rta); the difference is that -// CHA conservatively computes the entire "implements" relation between -// interfaces and concrete types ahead of time, whereas RTA uses dynamic -// programming to construct it on the fly as it encounters new functions -// reachable from main. CHA may thus include spurious call edges for -// types that haven't been instantiated yet, or types that are never -// instantiated. -// -// Since CHA conservatively assumes that all functions are address-taken -// and all concrete types are put into interfaces, it is sound to run on -// partial programs, such as libraries without a main or test function. -// -package cha // import "golang.org/x/tools/go/callgraph/cha" - -import ( - "go/types" - - "golang.org/x/tools/go/callgraph" - "golang.org/x/tools/go/ssa" - "golang.org/x/tools/go/ssa/ssautil" - "golang.org/x/tools/go/types/typeutil" -) - -// CallGraph computes the call graph of the specified program using the -// Class Hierarchy Analysis algorithm. -// -func CallGraph(prog *ssa.Program) *callgraph.Graph { - cg := callgraph.New(nil) // TODO(adonovan) eliminate concept of rooted callgraph - - allFuncs := ssautil.AllFunctions(prog) - - // funcsBySig contains all functions, keyed by signature. It is - // the effective set of address-taken functions used to resolve - // a dynamic call of a particular signature. - var funcsBySig typeutil.Map // value is []*ssa.Function - - // methodsByName contains all methods, - // grouped by name for efficient lookup. - // (methodsById would be better but not every SSA method has a go/types ID.) - methodsByName := make(map[string][]*ssa.Function) - - // An imethod represents an interface method I.m. - // (There's no go/types object for it; - // a *types.Func may be shared by many interfaces due to interface embedding.) - type imethod struct { - I *types.Interface - id string - } - // methodsMemo records, for every abstract method call I.m on - // interface type I, the set of concrete methods C.m of all - // types C that satisfy interface I. - // - // Abstract methods may be shared by several interfaces, - // hence we must pass I explicitly, not guess from m. - // - // methodsMemo is just a cache, so it needn't be a typeutil.Map. - methodsMemo := make(map[imethod][]*ssa.Function) - lookupMethods := func(I *types.Interface, m *types.Func) []*ssa.Function { - id := m.Id() - methods, ok := methodsMemo[imethod{I, id}] - if !ok { - for _, f := range methodsByName[m.Name()] { - C := f.Signature.Recv().Type() // named or *named - if types.Implements(C, I) { - methods = append(methods, f) - } - } - methodsMemo[imethod{I, id}] = methods - } - return methods - } - - for f := range allFuncs { - if f.Signature.Recv() == nil { - // Package initializers can never be address-taken. - if f.Name() == "init" && f.Synthetic == "package initializer" { - continue - } - funcs, _ := funcsBySig.At(f.Signature).([]*ssa.Function) - funcs = append(funcs, f) - funcsBySig.Set(f.Signature, funcs) - } else { - methodsByName[f.Name()] = append(methodsByName[f.Name()], f) - } - } - - addEdge := func(fnode *callgraph.Node, site ssa.CallInstruction, g *ssa.Function) { - gnode := cg.CreateNode(g) - callgraph.AddEdge(fnode, site, gnode) - } - - addEdges := func(fnode *callgraph.Node, site ssa.CallInstruction, callees []*ssa.Function) { - // Because every call to a highly polymorphic and - // frequently used abstract method such as - // (io.Writer).Write is assumed to call every concrete - // Write method in the program, the call graph can - // contain a lot of duplication. - // - // TODO(adonovan): opt: consider factoring the callgraph - // API so that the Callers component of each edge is a - // slice of nodes, not a singleton. - for _, g := range callees { - addEdge(fnode, site, g) - } - } - - for f := range allFuncs { - fnode := cg.CreateNode(f) - for _, b := range f.Blocks { - for _, instr := range b.Instrs { - if site, ok := instr.(ssa.CallInstruction); ok { - call := site.Common() - if call.IsInvoke() { - tiface := call.Value.Type().Underlying().(*types.Interface) - addEdges(fnode, site, lookupMethods(tiface, call.Method)) - } else if g := call.StaticCallee(); g != nil { - addEdge(fnode, site, g) - } else if _, ok := call.Value.(*ssa.Builtin); !ok { - callees, _ := funcsBySig.At(call.Signature()).([]*ssa.Function) - addEdges(fnode, site, callees) - } - } - } - } - } - - return cg -} diff --git a/vendor/golang.org/x/tools/go/callgraph/util.go b/vendor/golang.org/x/tools/go/callgraph/util.go deleted file mode 100644 index a8f89031..00000000 --- a/vendor/golang.org/x/tools/go/callgraph/util.go +++ /dev/null @@ -1,181 +0,0 @@ -// 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 callgraph - -import "golang.org/x/tools/go/ssa" - -// This file provides various utilities over call graphs, such as -// visitation and path search. - -// CalleesOf returns a new set containing all direct callees of the -// caller node. -// -func CalleesOf(caller *Node) map[*Node]bool { - callees := make(map[*Node]bool) - for _, e := range caller.Out { - callees[e.Callee] = true - } - return callees -} - -// GraphVisitEdges visits all the edges in graph g in depth-first order. -// The edge function is called for each edge in postorder. If it -// returns non-nil, visitation stops and GraphVisitEdges returns that -// value. -// -func GraphVisitEdges(g *Graph, edge func(*Edge) error) error { - seen := make(map[*Node]bool) - var visit func(n *Node) error - visit = func(n *Node) error { - if !seen[n] { - seen[n] = true - for _, e := range n.Out { - if err := visit(e.Callee); err != nil { - return err - } - if err := edge(e); err != nil { - return err - } - } - } - return nil - } - for _, n := range g.Nodes { - if err := visit(n); err != nil { - return err - } - } - return nil -} - -// PathSearch finds an arbitrary path starting at node start and -// ending at some node for which isEnd() returns true. On success, -// PathSearch returns the path as an ordered list of edges; on -// failure, it returns nil. -// -func PathSearch(start *Node, isEnd func(*Node) bool) []*Edge { - stack := make([]*Edge, 0, 32) - seen := make(map[*Node]bool) - var search func(n *Node) []*Edge - search = func(n *Node) []*Edge { - if !seen[n] { - seen[n] = true - if isEnd(n) { - return stack - } - for _, e := range n.Out { - stack = append(stack, e) // push - if found := search(e.Callee); found != nil { - return found - } - stack = stack[:len(stack)-1] // pop - } - } - return nil - } - return search(start) -} - -// DeleteSyntheticNodes removes from call graph g all nodes for -// synthetic functions (except g.Root and package initializers), -// preserving the topology. In effect, calls to synthetic wrappers -// are "inlined". -// -func (g *Graph) DeleteSyntheticNodes() { - // Measurements on the standard library and go.tools show that - // resulting graph has ~15% fewer nodes and 4-8% fewer edges - // than the input. - // - // Inlining a wrapper of in-degree m, out-degree n adds m*n - // and removes m+n edges. Since most wrappers are monomorphic - // (n=1) this results in a slight reduction. Polymorphic - // wrappers (n>1), e.g. from embedding an interface value - // inside a struct to satisfy some interface, cause an - // increase in the graph, but they seem to be uncommon. - - // Hash all existing edges to avoid creating duplicates. - edges := make(map[Edge]bool) - for _, cgn := range g.Nodes { - for _, e := range cgn.Out { - edges[*e] = true - } - } - for fn, cgn := range g.Nodes { - if cgn == g.Root || fn.Synthetic == "" || isInit(cgn.Func) { - continue // keep - } - for _, eIn := range cgn.In { - for _, eOut := range cgn.Out { - newEdge := Edge{eIn.Caller, eIn.Site, eOut.Callee} - if edges[newEdge] { - continue // don't add duplicate - } - AddEdge(eIn.Caller, eIn.Site, eOut.Callee) - edges[newEdge] = true - } - } - g.DeleteNode(cgn) - } -} - -func isInit(fn *ssa.Function) bool { - return fn.Pkg != nil && fn.Pkg.Func("init") == fn -} - -// DeleteNode removes node n and its edges from the graph g. -// (NB: not efficient for batch deletion.) -func (g *Graph) DeleteNode(n *Node) { - n.deleteIns() - n.deleteOuts() - delete(g.Nodes, n.Func) -} - -// deleteIns deletes all incoming edges to n. -func (n *Node) deleteIns() { - for _, e := range n.In { - removeOutEdge(e) - } - n.In = nil -} - -// deleteOuts deletes all outgoing edges from n. -func (n *Node) deleteOuts() { - for _, e := range n.Out { - removeInEdge(e) - } - n.Out = nil -} - -// removeOutEdge removes edge.Caller's outgoing edge 'edge'. -func removeOutEdge(edge *Edge) { - caller := edge.Caller - n := len(caller.Out) - for i, e := range caller.Out { - if e == edge { - // Replace it with the final element and shrink the slice. - caller.Out[i] = caller.Out[n-1] - caller.Out[n-1] = nil // aid GC - caller.Out = caller.Out[:n-1] - return - } - } - panic("edge not found: " + edge.String()) -} - -// removeInEdge removes edge.Callee's incoming edge 'edge'. -func removeInEdge(edge *Edge) { - caller := edge.Callee - n := len(caller.In) - for i, e := range caller.In { - if e == edge { - // Replace it with the final element and shrink the slice. - caller.In[i] = caller.In[n-1] - caller.In[n-1] = nil // aid GC - caller.In = caller.In[:n-1] - return - } - } - panic("edge not found: " + edge.String()) -} diff --git a/vendor/modules.txt b/vendor/modules.txt index 98dcc895..86b98e5e 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -207,8 +207,6 @@ golang.org/x/tools/go/internal/packagesdriver golang.org/x/tools/internal/gopathwalk golang.org/x/tools/internal/semver golang.org/x/tools/internal/module -golang.org/x/tools/go/callgraph -golang.org/x/tools/go/callgraph/cha golang.org/x/tools/go/internal/gcimporter golang.org/x/tools/internal/fastwalk # gopkg.in/yaml.v2 v2.2.1 @@ -217,7 +215,7 @@ gopkg.in/yaml.v2 mvdan.cc/interfacer/check # mvdan.cc/lint v0.0.0-20170908181259-adc824a0674b mvdan.cc/lint -# mvdan.cc/unparam v0.0.0-20190124213536-cc9d2fb52971 +# mvdan.cc/unparam v0.0.0-20190124213536-fbb59629db34 mvdan.cc/unparam/check # sourcegraph.com/sourcegraph/go-diff v0.0.0-20171119081133-3f415a150aec sourcegraph.com/sourcegraph/go-diff/diff diff --git a/vendor/mvdan.cc/unparam/check/check.go b/vendor/mvdan.cc/unparam/check/check.go index c52cccfd..7e753063 100644 --- a/vendor/mvdan.cc/unparam/check/check.go +++ b/vendor/mvdan.cc/unparam/check/check.go @@ -21,8 +21,6 @@ import ( "sort" "strings" - "golang.org/x/tools/go/callgraph" - "golang.org/x/tools/go/callgraph/cha" "golang.org/x/tools/go/packages" "golang.org/x/tools/go/ssa" "golang.org/x/tools/go/ssa/ssautil" @@ -50,9 +48,8 @@ func UnusedParams(tests bool, exported, debug bool, args ...string) ([]string, e // UnusedParams instead, unless you want to use a *loader.Program and // *ssa.Program directly. type Checker struct { - pkgs []*packages.Package - prog *ssa.Program - graph *callgraph.Graph + pkgs []*packages.Package + prog *ssa.Program wd string @@ -71,11 +68,20 @@ type Checker struct { // to the declaration, as that could be either a FuncDecl or FuncLit. funcBodyByPos map[token.Pos]*ast.BlockStmt - typesImplementing map[*types.Named]bool + // typesImplementing records the method names that each named type needs + // to typecheck properly, as they're required to implement interfaces. + typesImplementing map[*types.Named][]string - // Funcs used as a struct field or a func call argument. These are very - // often signatures which cannot be changed. - funcUsedAs map[*ssa.Function]string + // localCallSites is a very simple form of a callgraph, only recording + // direct function calls within a single package. + localCallSites map[*ssa.Function][]ssa.CallInstruction + + // These three maps record whether an entire func's signature cannot be + // changed, or only its list of parameters or results. + + signRequiredBy map[*ssa.Function]string + paramsRequiredBy map[*ssa.Function]string + resultsRequiredBy map[*ssa.Function]string } var errorType = types.Universe.Lookup("error").Type() @@ -182,7 +188,7 @@ func (c *Checker) Check() ([]Issue, error) { c.cachedDeclCounts = make(map[string]map[string]int) c.callByPos = make(map[token.Pos]*ast.CallExpr) c.funcBodyByPos = make(map[token.Pos]*ast.BlockStmt) - c.typesImplementing = make(map[*types.Named]bool) + c.typesImplementing = make(map[*types.Named][]string) wantPkg := make(map[*types.Package]*packages.Package) genFiles := make(map[string]bool) @@ -200,16 +206,13 @@ func (c *Checker) Check() ([]Issue, error) { len(node.Names) != 1 || node.Names[0].Name != "_" { break } - _, ok := pkg.TypesInfo.TypeOf(node.Type).Underlying().(*types.Interface) + iface, ok := pkg.TypesInfo.TypeOf(node.Type).Underlying().(*types.Interface) if !ok { break } + // var _ someIface = named valTyp := pkg.TypesInfo.Types[node.Values[0]].Type - named := findNamed(valTyp) - if named == nil { - break - } - c.typesImplementing[named] = true + c.addImplementing(findNamed(valTyp), iface) case *ast.CallExpr: c.callByPos[node.Lparen] = node // ssa.Function.Pos returns the declaring @@ -224,40 +227,121 @@ func (c *Checker) Check() ([]Issue, error) { }) } } - c.graph = cha.CallGraph(c.prog) - c.graph.DeleteSyntheticNodes() - allFuncs := ssautil.AllFunctions(c.prog) - c.funcUsedAs = make(map[*ssa.Function]string) - for fn := range allFuncs { - for _, b := range fn.Blocks { + // map from *ssa.FreeVar to *ssa.Function, to find function literals + // behind closure vars in the simpler scenarios. + freeVars := map[*ssa.FreeVar]*ssa.Function{} + for curFunc := range allFuncs { + for _, b := range curFunc.Blocks { for _, instr := range b.Instrs { + instr, ok := instr.(*ssa.MakeClosure) + if !ok { + continue + } + fn := instr.Fn.(*ssa.Function) + for i, fv := range fn.FreeVars { + binding := instr.Bindings[i] + alloc, ok := binding.(*ssa.Alloc) + if !ok { + continue + } + for _, ref := range *alloc.Referrers() { + store, ok := ref.(*ssa.Store) + if !ok { + continue + } + if fn, ok := store.Val.(*ssa.Function); ok { + freeVars[fv] = fn + break + } + } + } + } + } + } + + c.signRequiredBy = make(map[*ssa.Function]string) + c.paramsRequiredBy = make(map[*ssa.Function]string) + c.resultsRequiredBy = make(map[*ssa.Function]string) + c.localCallSites = make(map[*ssa.Function][]ssa.CallInstruction) + for curFunc := range allFuncs { + if strings.HasPrefix(curFunc.Synthetic, "wrapper for func") { + // Synthetic func wrappers are uninteresting, and can + // lead to false negatives. + continue + } + for _, b := range curFunc.Blocks { + for _, instr := range b.Instrs { + if instr, ok := instr.(ssa.CallInstruction); ok { + if fn := findFunction(freeVars, instr.Common().Value); fn != nil { + c.localCallSites[fn] = append(c.localCallSites[fn], instr) + } + fn := receivesExtractedArgs(freeVars, instr) + if fn != nil { + // fn(someFunc()) fixes params + c.paramsRequiredBy[fn] = "forwarded call" + } + } switch instr := instr.(type) { case *ssa.Call: for _, arg := range instr.Call.Args { - if fn := findFunction(arg); fn != nil { + if fn := findFunction(freeVars, arg); fn != nil { // someFunc(fn) - c.funcUsedAs[fn] = "param" + c.signRequiredBy[fn] = "call" + } + } + case *ssa.Phi: + for _, val := range instr.Edges { + if fn := findFunction(freeVars, val); fn != nil { + // nonConstVar = fn + c.signRequiredBy[fn] = "phi" + } + } + case *ssa.Return: + for _, val := range instr.Results { + if fn := findFunction(freeVars, val); fn != nil { + // return fn + c.signRequiredBy[fn] = "result" + } + } + if call := callExtract(instr, instr.Results); call != nil { + if fn := findFunction(freeVars, call.Call.Value); fn != nil { + // return fn() + c.resultsRequiredBy[fn] = "return" } } case *ssa.Store: - if _, ok := instr.Addr.(*ssa.FieldAddr); !ok { - break - } - if fn := findFunction(instr.Val); fn != nil { + as := "" + switch instr.Addr.(type) { + case *ssa.FieldAddr: // x.someField = fn - c.funcUsedAs[fn] = "field" + as = "field" + case *ssa.IndexAddr: + // x[someIndex] = fn + as = "element" + case *ssa.Global: + // someGlobal = fn + as = "global" + default: + continue + } + if fn := findFunction(freeVars, instr.Val); fn != nil { + c.signRequiredBy[fn] = as } case *ssa.MakeInterface: - if fn := findFunction(instr.X); fn != nil { + // someIface(named) + iface := instr.Type().Underlying().(*types.Interface) + c.addImplementing(findNamed(instr.X.Type()), iface) + + if fn := findFunction(freeVars, instr.X); fn != nil { // emptyIface = fn - c.funcUsedAs[fn] = "interface" + c.signRequiredBy[fn] = "interface" } case *ssa.ChangeType: - if fn := findFunction(instr.X); fn != nil { + if fn := findFunction(freeVars, instr.X); fn != nil { // someType(fn) - c.funcUsedAs[fn] = "type conversion" + c.signRequiredBy[fn] = "type conversion" } } } @@ -303,6 +387,29 @@ func (c *Checker) Check() ([]Issue, error) { return c.issues, nil } +func stringsContains(list []string, elem string) bool { + for _, e := range list { + if e == elem { + return true + } + } + return false +} + +func (c *Checker) addImplementing(named *types.Named, iface *types.Interface) { + if named == nil || iface == nil { + return + } + list := c.typesImplementing[named] + for i := 0; i < iface.NumMethods(); i++ { + name := iface.Method(i).Name() + if !stringsContains(list, name) { + list = append(list, name) + } + } + c.typesImplementing[named] = list +} + func findNamed(typ types.Type) *types.Named { switch typ := typ.(type) { case *types.Pointer: @@ -314,13 +421,36 @@ func findNamed(typ types.Type) *types.Named { } // findFunction returns the function that is behind a value, if any. -func findFunction(value ssa.Value) *ssa.Function { +func findFunction(freeVars map[*ssa.FreeVar]*ssa.Function, value ssa.Value) *ssa.Function { switch value := value.(type) { case *ssa.Function: + name := value.Name() + if strings.HasSuffix(name, "$thunk") || strings.HasSuffix(name, "$bound") { + // Method wrapper funcs contain a single block, which + // calls the function being wrapped, and returns. We + // want the function being wrapped. + for _, instr := range value.Blocks[0].Instrs { + call, ok := instr.(*ssa.Call) + if !ok { + continue + } + if callee := call.Call.StaticCallee(); callee != nil { + return callee + } + } + return nil // no static callee? + } return value case *ssa.MakeClosure: // closure of a func - return value.Fn.(*ssa.Function) + return findFunction(freeVars, value.Fn) + case *ssa.UnOp: + if value.Op != token.MUL { + break + } + if fv, ok := value.X.(*ssa.FreeVar); ok { + return freeVars[fv] + } } return nil } @@ -349,22 +479,14 @@ func (c *Checker) checkFunc(fn *ssa.Function, pkg *packages.Package) { c.debug(" skip - dummy implementation\n") return } - var inboundCalls []*callgraph.Edge - if node := c.graph.Nodes[fn]; node != nil { - inboundCalls = node.In - } - if requiredViaCall(fn, inboundCalls) { - c.debug(" skip - type is required via call\n") - return - } - if usedAs := c.funcUsedAs[fn]; usedAs != "" { - c.debug(" skip - func is used as a %s\n", usedAs) + if by := c.signRequiredBy[fn]; by != "" { + c.debug(" skip - func signature required by %s\n", by) return } if recv := fn.Signature.Recv(); recv != nil { named := findNamed(recv.Type()) - if c.typesImplementing[named] { - c.debug(" skip - receiver must implement an interface\n") + if stringsContains(c.typesImplementing[named], fn.Name()) { + c.debug(" skip - method required to implement an interface\n") return } } @@ -372,12 +494,18 @@ func (c *Checker) checkFunc(fn *ssa.Function, pkg *packages.Package) { c.debug(" skip - multiple implementations via build tags\n") return } + paramsBy := c.paramsRequiredBy[fn] + resultsBy := c.resultsRequiredBy[fn] + callSites := c.localCallSites[fn] results := fn.Signature.Results() sameConsts := make([]*ssa.Const, results.Len()) numRets := 0 allRetsExtracting := true for _, block := range fn.Blocks { + if resultsBy != "" { + continue // we can't change the returns + } last := block.Instrs[len(block.Instrs)-1] ret, ok := last.(*ssa.Return) if !ok { @@ -406,9 +534,6 @@ func (c *Checker) checkFunc(fn *ssa.Function, pkg *packages.Package) { // false positives) continue } - if calledInReturn(inboundCalls) { - continue - } res := results.At(i) name := paramDesc(i, res) c.addIssue(fn, res.Pos(), "result %s is always %s", name, constValueString(cnst)) @@ -416,6 +541,9 @@ func (c *Checker) checkFunc(fn *ssa.Function, pkg *packages.Package) { resLoop: for i := 0; i < results.Len(); i++ { + if resultsBy != "" { + continue // we can't change the returns + } if allRetsExtracting { continue } @@ -426,8 +554,8 @@ resLoop: continue } count := 0 - for _, edge := range inboundCalls { - val := edge.Site.Value() + for _, site := range callSites { + val := site.Value() if val == nil { // e.g. go statement count++ continue @@ -454,6 +582,9 @@ resLoop: } for i, par := range fn.Params { + if paramsBy != "" { + continue // we can't change the params + } if i == 0 && fn.Signature.Recv() != nil { // receiver continue } @@ -468,7 +599,7 @@ resLoop: continue } reason := "is unused" - constStr := c.alwaysReceivedConst(inboundCalls, par, i) + constStr := c.alwaysReceivedConst(callSites, par, i) if constStr != "" { reason = fmt.Sprintf("always receives %s", constStr) } else if c.anyRealUse(par, i, pkg) { @@ -479,43 +610,6 @@ resLoop: } } -// calledInReturn reports whether any of a function's inbound calls happened -// directly as a return statement. That is, if function "foo" was used via -// "return foo()". This means that the result parameters of the function cannot -// be changed without breaking other code. -func calledInReturn(in []*callgraph.Edge) bool { - for _, edge := range in { - val := edge.Site.Value() - if val == nil { // e.g. go statement - continue - } - refs := *val.Referrers() - if len(refs) == 0 { // no use of return values - continue - } - allReturnExtracts := true - for _, instr := range refs { - switch x := instr.(type) { - case *ssa.Return: - return true - case *ssa.Extract: - refs := *x.Referrers() - if len(refs) != 1 { - allReturnExtracts = false - break - } - if _, ok := refs[0].(*ssa.Return); !ok { - allReturnExtracts = false - } - } - } - if allReturnExtracts { - return true - } - } - return false -} - // nodeStr stringifies a syntax tree node. It is only meant for simple nodes, // such as short value expressions. func nodeStr(node ast.Node) string { @@ -534,8 +628,8 @@ func nodeStr(node ast.Node) string { // This function is used to recommend that the parameter be replaced by a direct // use of the constant. To avoid false positives, the function will return false // if the number of inbound calls is too low. -func (c *Checker) alwaysReceivedConst(in []*callgraph.Edge, par *ssa.Parameter, pos int) string { - if len(in) < 4 { +func (c *Checker) alwaysReceivedConst(callSites []ssa.CallInstruction, par *ssa.Parameter, pos int) string { + if len(callSites) < 4 { // We can't possibly receive the same constant value enough // times, hence a potential false positive. return "" @@ -552,8 +646,8 @@ func (c *Checker) alwaysReceivedConst(in []*callgraph.Edge, par *ssa.Parameter, origPos-- } seenOrig := "" - for _, edge := range in { - call := edge.Site.Common() + for _, site := range callSites { + call := site.Common() if pos >= len(call.Args) { // TODO: investigate? Weird crash in // internal/x/net/http2/hpack/hpack_test.go, where we @@ -612,8 +706,15 @@ func (c *Checker) anyRealUse(par *ssa.Parameter, pos int, pkg *packages.Package) if any { return false } - if id, ok := node.(*ast.Ident); ok { - obj := pkg.TypesInfo.Uses[id] + asgn, ok := node.(*ast.AssignStmt) + if !ok || asgn.Tok != token.ASSIGN || len(asgn.Lhs) != 1 || len(asgn.Rhs) != 1 { + return true + } + if left, ok := asgn.Lhs[0].(*ast.Ident); !ok || left.Name != "_" { + return true + } + if right, ok := asgn.Rhs[0].(*ast.Ident); ok { + obj := pkg.TypesInfo.Uses[right] if obj != nil && obj.Pos() == par.Pos() { any = true } @@ -798,29 +899,72 @@ func (c *Checker) multipleImpls(pkg *packages.Package, fn *ssa.Function) bool { return count[name] > 1 } -// receivesExtractedArgs reports whether a function call got all of its -// arguments via another function call. That is, if a call to function "foo" was -// of the form "foo(bar())". -func receivesExtractedArgs(sign *types.Signature, call *ssa.Call) bool { - if call == nil { - return false +// receivesExtractedArgs returns the statically called function, if its multiple +// arguments were all received via another function call. That is, if a call to +// function "foo" was of the form "foo(bar())". This often means that the +// parameters in "foo" are difficult to remove, even if unused. +func receivesExtractedArgs(freeVars map[*ssa.FreeVar]*ssa.Function, call ssa.CallInstruction) *ssa.Function { + comm := call.Common() + callee := findFunction(freeVars, comm.Value) + if callee == nil { + return nil } - if sign.Params().Len() < 2 { - return false // extracting into one param is ok + if callee.Signature.Params().Len() < 2 { + // there aren't multiple parameters + return nil } - args := call.Operands(nil) - for i, arg := range args { - if i == 0 { - continue // *ssa.Function, func itself - } - if i == 1 && sign.Recv() != nil { - continue // method receiver - } - if _, ok := (*arg).(*ssa.Extract); !ok { - return false + args := comm.Args + if callee.Signature.Recv() != nil { + // skip the receiver argument + args = args[1:] + } + if c := callExtract(call, args); c != nil { + return callee + } + return nil +} + +// callExtract returns the call instruction fn(...) if it is used directly as +// arguments to the parent instruction, such as fn2(fn(...)) or return fn(...). +func callExtract(parent ssa.Instruction, values []ssa.Value) *ssa.Call { + if len(values) == 1 { + if call, ok := values[0].(*ssa.Call); ok { + return call } } - return true + var prev *ssa.Call + for i, val := range values { + ext, ok := val.(*ssa.Extract) + if !ok { + return nil + } + if ext.Index != i { + return nil // not extracted in the same order + } + call, ok := ext.Tuple.(*ssa.Call) + if !ok { + return nil // not a call + } + if prev == nil { + prev = call + } else if prev != call { + return nil // not the same calls + } + } + if prev == nil { + return nil + } + if prev.Call.Signature().Results().Len() != len(values) { + return nil // not extracting all the results + } + if prev.Pos() < parent.Pos() { + // Of the form: + // + // a, b := fn() + // fn2(a, b) + return nil + } + return prev } // paramDesc returns a string describing a parameter variable. If the parameter @@ -833,42 +977,3 @@ func paramDesc(i int, v *types.Var) string { } return fmt.Sprintf("%d (%s)", i, v.Type().String()) } - -// requiredViaCall reports whether a function has any inbound call that strongly -// depends on the function's signature. For example, if the function is accessed -// via a field, or if it gets its arguments from another function call. In these -// cases, changing the function signature would mean a larger refactor. -func requiredViaCall(fn *ssa.Function, calls []*callgraph.Edge) bool { - for _, edge := range calls { - call := edge.Site.Value() - if receivesExtractedArgs(fn.Signature, call) { - // called via fn(x()) - return true - } - switch edge.Site.Common().Value.(type) { - case *ssa.Parameter: - // Func used as parameter; not safe. - return true - case *ssa.Call: - // Called through an interface; not safe. - return true - case *ssa.TypeAssert: - // Called through a type assertion; not safe. - return true - case *ssa.Const: - // Conservative "may implement" edge; not safe. - return true - case *ssa.UnOp: - // TODO: why is this not safe? - return true - case *ssa.Phi: - // We may run this function or another; not safe. - return true - case *ssa.Function: - // Called directly; the call site can easily be adapted. - default: - // Other instructions are safe or non-interesting. - } - } - return false -}