From 9d19bffb802e824e443f9cf0d573e604c4e91f5c Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 10 Nov 2023 14:03:25 -0500 Subject: [PATCH] internal/cmd/deadcode: simplify function notation in output This change: - uses the simpler notation pkg.T.f instead of (*pkg.T).f in all output, by using a simplified fork of ssa.Function.String that only works on source functions (and package inits). - uses callgraph.Graph.DeleteSyntheticNodes to elide wrappers that aren't interesting to the user. - simplifies the JSON schema now that it's trivial to package-qualify any function by prepending "pkg.". - uses better spellings for JSON fields. Also, tests of new behavior. Updates golang/go#63501 Change-Id: Ie57399c4e05a882e294437b53317eb0c1b322e9f Reviewed-on: https://go-review.googlesource.com/c/tools/+/541238 Reviewed-by: Robert Findley Auto-Submit: Alan Donovan LUCI-TryBot-Result: Go LUCI Commit-Queue: Alan Donovan --- internal/cmd/deadcode/deadcode.go | 94 +++++++++++++++---- internal/cmd/deadcode/doc.go | 40 ++++---- internal/cmd/deadcode/testdata/basic.txtar | 10 +- internal/cmd/deadcode/testdata/jsonflag.txtar | 8 +- internal/cmd/deadcode/testdata/lineflag.txtar | 6 +- internal/cmd/deadcode/testdata/whylive.txtar | 79 +++++++++++++++- 6 files changed, 189 insertions(+), 48 deletions(-) diff --git a/internal/cmd/deadcode/deadcode.go b/internal/cmd/deadcode/deadcode.go index d2d081bff7c..ecb2a055254 100644 --- a/internal/cmd/deadcode/deadcode.go +++ b/internal/cmd/deadcode/deadcode.go @@ -189,7 +189,10 @@ func main() { if *whyLiveFlag != "" { targets := make(map[*ssa.Function]bool) for fn := range ssautil.AllFunctions(prog) { - if fn.String() == *whyLiveFlag { + if fn.Synthetic != "" || fn.Object() == nil { + continue // not a source-level named function + } + if prettyName(fn, true) == *whyLiveFlag { targets[fn] = true } } @@ -220,6 +223,7 @@ func main() { log.Fatalf("function %s is dead code", *whyLiveFlag) } + res.CallGraph.DeleteSyntheticNodes() // inline synthetic wrappers (except inits) root, path := pathSearch(roots, res, targets) if root == nil { // RTA doesn't add callgraph edges for reflective calls. @@ -236,13 +240,13 @@ func main() { var edges []any for _, edge := range path { edges = append(edges, jsonEdge{ - Initial: cond(len(edges) == 0, edge.Caller.Func.String(), ""), - Kind: cond(isStaticCall(edge), "static", "dynamic"), - Posn: toJSONPosition(prog.Fset.Position(edge.Site.Pos())), - Callee: edge.Callee.Func.String(), + Initial: cond(len(edges) == 0, prettyName(edge.Caller.Func, true), ""), + Kind: cond(isStaticCall(edge), "static", "dynamic"), + Position: toJSONPosition(prog.Fset.Position(edge.Site.Pos())), + Callee: prettyName(edge.Callee.Func, true), }) } - format := `{{if .Initial}}{{printf "%19s%s\n" "" .Initial}}{{end}}{{printf "%8s@L%.4d --> %s" .Kind .Posn.Line .Callee}}` + format := `{{if .Initial}}{{printf "%19s%s\n" "" .Initial}}{{end}}{{printf "%8s@L%.4d --> %s" .Kind .Position.Line .Callee}}` if *formatFlag != "" { format = *formatFlag } @@ -325,9 +329,8 @@ func main() { } functions = append(functions, jsonFunction{ - Name: fn.String(), - RelName: fn.RelString(fn.Pkg.Pkg), - Posn: toJSONPosition(posn), + Name: prettyName(fn, false), + Position: toJSONPosition(posn), Generated: gen, }) } @@ -340,13 +343,56 @@ func main() { } // Default format: functions grouped by package. - format := `{{println .Path}}{{range .Funcs}}{{printf "\t%s\n" .RelName}}{{end}}{{println}}` + format := `{{println .Path}}{{range .Funcs}}{{printf "\t%s\n" .Name}}{{end}}{{println}}` if *formatFlag != "" { format = *formatFlag } printObjects(format, packages) } +// prettyName is a fork of Function.String designed to reduce +// go/ssa's fussy punctuation symbols, e.g. "(*pkg.T).F" -> "pkg.T.F". +// +// It only works for functions that remain after +// callgraph.Graph.DeleteSyntheticNodes: source-level named functions +// and methods, their anonymous functions, and synthetic package +// initializers. +func prettyName(fn *ssa.Function, qualified bool) string { + var buf strings.Builder + + // optional package qualifier + if qualified && fn.Pkg != nil { + fmt.Fprintf(&buf, "%s.", fn.Pkg.Pkg.Path()) + } + + var format func(*ssa.Function) + format = func(fn *ssa.Function) { + // anonymous? + if fn.Parent() != nil { + format(fn.Parent()) + i := index(fn.Parent().AnonFuncs, fn) + fmt.Fprintf(&buf, "$%d", i+1) + return + } + + // method receiver? + if recv := fn.Signature.Recv(); recv != nil { + t := recv.Type() + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + buf.WriteString(t.(*types.Named).Obj().Name()) + buf.WriteByte('.') + } + + // function/method name + buf.WriteString(fn.Name()) + } + format(fn) + + return buf.String() +} + // printObjects formats an array of objects, either as JSON or using a // template, following the manner of 'go list (-json|-f=template)'. func printObjects(format string, objects []any) { @@ -480,6 +526,11 @@ func pathSearch(roots []*ssa.Function, res *rta.Result, targets map[*ssa.Functio } for _, rootFn := range roots { root := res.CallGraph.Nodes[rootFn] + if root == nil { + // Missing call graph node for root. + // TODO(adonovan): seems like a bug in rta. + continue + } if path := bfs(root); path != nil { return root, path } @@ -519,9 +570,8 @@ func cond[T any](cond bool, t, f T) T { // Keep in sync with doc comment! type jsonFunction struct { - Name string // name (with package qualifier) - RelName string // name (sans package qualifier) - Posn jsonPosition // file/line/column of declaration + Name string // name (sans package qualifier) + Position jsonPosition // file/line/column of declaration Generated bool // function is declared in a generated .go file } @@ -534,11 +584,12 @@ type jsonPackage struct { func (p jsonPackage) String() string { return p.Path } +// The Initial and Callee names are package-qualified. type jsonEdge struct { - Initial string `json:",omitempty"` // initial entrypoint (main or init); first edge only - Kind string // = static | dynamic - Posn jsonPosition - Callee string + Initial string `json:",omitempty"` // initial entrypoint (main or init); first edge only + Kind string // = static | dynamic + Position jsonPosition + Callee string } type jsonPosition struct { @@ -567,6 +618,15 @@ func indexFunc[S ~[]E, E any](s S, f func(E) bool) int { return -1 } +func index[S ~[]E, E comparable](s S, v E) int { + for i := range s { + if v == s[i] { + return i + } + } + return -1 +} + func reverse[S ~[]E, E any](s S) { for i, j := 0, len(s)-1; i < j; i, j = i+1, j-1 { s[i], s[j] = s[j], s[i] diff --git a/internal/cmd/deadcode/doc.go b/internal/cmd/deadcode/doc.go index 8d28eb31288..098b2391346 100644 --- a/internal/cmd/deadcode/doc.go +++ b/internal/cmd/deadcode/doc.go @@ -45,8 +45,8 @@ https://go.dev/s/generatedcode. Use the -generated flag to include them. In any case, just because a function is reported as dead does not mean it is unconditionally safe to delete it. For example, a dead function -may be referenced (by another dead function), and a dead method may be -required to satisfy an interface (that is never called). +may be referenced by another dead function, and a dead method may be +required to satisfy an interface that is never called. Some judgement is required. The analysis is valid only for a single GOOS/GOARCH/-tags configuration, @@ -68,11 +68,11 @@ With the -f=template flag, the command executes the specified template on each Package record. So, this template produces a result similar to the default format: - -f='{{println .Path}}{{range .Funcs}}{{printf "\t%s\n" .RelName}}{{end}}{{println}}' + -f='{{println .Path}}{{range .Funcs}}{{printf "\t%s\n" .Name}}{{end}}{{println}}' And this template shows only the list of source positions of dead functions: - -f='{{range .Funcs}}{{println .Posn}}{{end}}' + -f='{{range .Funcs}}{{println .Position}}{{end}}' # Why is a function not dead? @@ -92,13 +92,14 @@ the formatting of the list of Edge objects. The default format shows, for each edge in the path, whether the call is static or dynamic, and its source line number. For example: - $ deadcode -whylive="(*bytes.Buffer).String" -test ./internal/cmd/deadcode/... - golang.org/x/tools/internal/cmd/deadcode.main - static@L0321 --> (*golang.org/x/tools/go/ssa.Function).RelString - static@L0428 --> (*golang.org/x/tools/go/ssa.Function).relMethod - static@L0452 --> golang.org/x/tools/go/ssa.relType - static@L0047 --> go/types.TypeString - static@L0051 --> (*bytes.Buffer).String + $ deadcode -whylive=bytes.Buffer.String -test ./internal/cmd/deadcode/... + golang.org/x/tools/internal/cmd/deadcode.main + static@L0117 --> golang.org/x/tools/go/packages.Load + static@L0262 --> golang.org/x/tools/go/packages.defaultDriver + static@L0305 --> golang.org/x/tools/go/packages.goListDriver + static@L0153 --> golang.org/x/tools/go/packages.goListDriver$1 + static@L0154 --> golang.org/x/tools/go/internal/packagesdriver.GetSizesForArgsGolist + static@L0044 --> bytes.Buffer.String # JSON schema @@ -108,26 +109,21 @@ is static or dynamic, and its source line number. For example: } type Function struct { - Name string // name (with package qualifier) - RelName string // name (sans package qualifier) - Posn Position // file/line/column of function declaration + Name string // name (sans package qualifier) + Position Position // file/line/column of function declaration Generated bool // function is declared in a generated .go file } type Edge struct { - Initial string // initial entrypoint (main or init); first edge only - Kind string // = static | dynamic - Posn Position // file/line/column of call site - Callee string // target of the call + Initial string // initial entrypoint (main or init); first edge only + Kind string // = static | dynamic + Position Position // file/line/column of call site + Callee string // target of the call } type Position struct { File string // name of file Line, Col int // line and byte index, both 1-based } - -THIS TOOL IS EXPERIMENTAL and its interface may change. -At some point it may be published at cmd/deadcode. -In the meantime, please give us feedback at github.com/golang/go/issues. */ package main diff --git a/internal/cmd/deadcode/testdata/basic.txtar b/internal/cmd/deadcode/testdata/basic.txtar index b0b380a0ecf..70cc79807cf 100644 --- a/internal/cmd/deadcode/testdata/basic.txtar +++ b/internal/cmd/deadcode/testdata/basic.txtar @@ -2,8 +2,10 @@ deadcode -filter= example.com - want "(T).Goodbye" -!want "(T).Hello" + want "T.Goodbye" + want "T.Goodbye2" + want "T.Goodbye3" +!want "T.Hello" want "unreferenced" want "Scanf" @@ -28,5 +30,9 @@ func main() { func (T) Hello() { fmt.Println("hello") } func (T) Goodbye() { fmt.Println("goodbye") } +func (*T) Goodbye2() { fmt.Println("goodbye2") } +func (*A) Goodbye3() { fmt.Println("goodbye3") } + +type A = T func unreferenced() {} \ No newline at end of file diff --git a/internal/cmd/deadcode/testdata/jsonflag.txtar b/internal/cmd/deadcode/testdata/jsonflag.txtar index f0f3ab21bd0..608657b6580 100644 --- a/internal/cmd/deadcode/testdata/jsonflag.txtar +++ b/internal/cmd/deadcode/testdata/jsonflag.txtar @@ -3,8 +3,7 @@ deadcode -json example.com/p want `"Path": "example.com/p",` - want `"Name": "example.com/p.Dead",` - want `"RelName": "Dead",` + want `"Name": "DeadFunc",` want `"Generated": false` want `"Line": 5,` want `"Col": 6` @@ -18,4 +17,7 @@ package main func main() {} -func Dead() {} +func DeadFunc() {} + +type T int +func (*T) DeadMethod() {} \ No newline at end of file diff --git a/internal/cmd/deadcode/testdata/lineflag.txtar b/internal/cmd/deadcode/testdata/lineflag.txtar index 51940ad3274..f8e5fac8b38 100644 --- a/internal/cmd/deadcode/testdata/lineflag.txtar +++ b/internal/cmd/deadcode/testdata/lineflag.txtar @@ -1,9 +1,9 @@ # Test of line-oriented output. - deadcode "-f={{range .Funcs}}{{println .Name}}{{end}}" -filter= example.com + deadcode `-f={{range .Funcs}}{{printf "%s.%s\n" $.Path .Name}}{{end}}` -filter= example.com - want "(example.com.T).Goodbye" -!want "(example.com.T).Hello" + want "example.com.T.Goodbye" +!want "example.com.T.Hello" want "example.com.unreferenced" want "fmt.Scanf" diff --git a/internal/cmd/deadcode/testdata/whylive.txtar b/internal/cmd/deadcode/testdata/whylive.txtar index 9e7b0e6e4af..4185876779b 100644 --- a/internal/cmd/deadcode/testdata/whylive.txtar +++ b/internal/cmd/deadcode/testdata/whylive.txtar @@ -26,6 +26,37 @@ !deadcode -whylive=example.com.main example.com want "example.com.main is a root" +# Test of path through (*T).m method wrapper. + + deadcode -whylive=example.com/p.live example.com/p + want " example.com/p.main" + want "static@L0006 --> example.com/p.E.Error" + want "static@L0010 --> example.com/p.live" + +# Test of path through (I).m interface method wrapper (thunk). + + deadcode -whylive=example.com/q.live example.com/q + want " example.com/q.main" + want "static@L0006 --> example.com/q.E.Error" + want "static@L0010 --> example.com/q.live" + +# Test of path through synthetic package initializer, +# a declared package initializer, and its anonymous function. + + deadcode -whylive=example.com/q.live2 example.com/q + want " example.com/q.init" + want "static@L0000 --> example.com/q.init#1" + want "static@L0016 --> example.com/q.init#1$1" + want "static@L0015 --> example.com/q.live2" + +# Test of path through synthetic package initializer, +# and a global var initializer. + + deadcode -whylive=example.com/r.live example.com/r + want " example.com/r.init" + want "static@L0007 --> example.com/r.init$1" + want "static@L0006 --> example.com/r.live" + -- go.mod -- module example.com go 1.18 @@ -53,4 +84,50 @@ func f() func init() { (func ())(nil)() // potential dynamic call to c, e -} \ No newline at end of file +} + +-- p/p.go -- +package main + +func main() { + f := (*E).Error + var e E + f(&e) +} + +type E int +func (E) Error() string { return live() } + +func live() string + +-- q/q.go -- +package main + +func main() { + f := error.Error + var e E + f(e) +} + +type E int +func (E) Error() string { return live() } + +func live() string + +func init() { + f := func() { live2() } + f() +} + +func live2() + +-- r/r.go -- +package main + +func main() {} + +var x = func() int { + return live() +}() + +func live() int