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 <[email protected]> Auto-Submit: Alan Donovan <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Commit-Queue: Alan Donovan <[email protected]>
golang · Nov 13, 2023 · 9d19bff · 9d19bff
1 parent b03756e
commit 9d19bff
Show file tree

Hide file tree

Showing 6 changed files with 189 additions and 48 deletions.
diff --git 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
@@ -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
@@ -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() {}
diff --git 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() {}
diff --git 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
@@ -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
-}
+}
+
+-- 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