[dev.regabi] cmd/compile: simplify ir.Find, replace ir.Inspect with ir.Visit

It seems clear after using these for a week that Find need not return
anything other than a bool saying whether the target was found.
The main reason for not using the boolean earlier was to avoid confusion
with Inspect: for Find, returning true means "it was found! stop walking"
while for Inspect, returning true means "keep walking the children".

But it turns out that none of the uses of Inspect need the boolean.
This makes sense because types can contain expressions, expressions
can contain statements (inside function literals), and so on, so there
are essentially no times when you can say based on the current AST node
that the children are irrelevant to a particular operation.

So this CL makes two changes:

1) Change Find to return a boolean and to take a callback function
returning a boolean. This simplifies all existing calls to Find.

2) Rename Inspect to Visit and change it to take a callback with no
result at all. This simplifies all existing calls to Inspect.

Removing the boolean result from Inspect's callback avoids having
two callbacks with contradictory boolean results in different APIs.
Renaming Inspect to Visit avoids confusion with ast.Inspect.

Passes buildall w/ toolstash -cmp.

Change-Id: I344ebb5e00b6842012be33e779db483c28e5f350
Reviewed-on: https://go-review.googlesource.com/c/go/+/277919
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

src/cmd/compile/internal/gc/alg.go

@@ -783,13 +783,11 @@ func geneq(t *types.Type) *obj.LSym {
 }
 
 func hasCall(fn *ir.Func) bool {
-	found := ir.Find(fn, func(n ir.Node) interface{} {
+	return ir.Find(fn, func(n ir.Node) bool {
-		if op := n.Op(); op == ir.OCALL || op == ir.OCALLFUNC {
+		// TODO(rsc): No methods?
-			return n
+		op := n.Op()
-		}
+		return op == ir.OCALL || op == ir.OCALLFUNC
-		return nil
 	})
-	return found != nil
 }
 
 // eqfield returns the node

src/cmd/compile/internal/gc/const.go

@@ -781,7 +781,7 @@ func isGoConst(n ir.Node) bool {
 
 // hasCallOrChan reports whether n contains any calls or channel operations.
 func hasCallOrChan(n ir.Node) bool {
-	found := ir.Find(n, func(n ir.Node) interface{} {
+	return ir.Find(n, func(n ir.Node) bool {
 		switch n.Op() {
 		case ir.OAPPEND,
 			ir.OCALL,
@@ -803,11 +803,10 @@ func hasCallOrChan(n ir.Node) bool {
 			ir.OREAL,
 			ir.ORECOVER,
 			ir.ORECV:
-			return n
+			return true
 		}
-		return nil
+		return false
 	})
-	return found != nil
 }
 
 // A constSet represents a set of Go constant expressions.

src/cmd/compile/internal/gc/dcl.go

@@ -855,22 +855,22 @@ func newNowritebarrierrecChecker() *nowritebarrierrecChecker {
 			continue
 		}
 		c.curfn = n.(*ir.Func)
-		ir.Inspect(n, c.findExtraCalls)
+		ir.Visit(n, c.findExtraCalls)
 	}
 	c.curfn = nil
 	return c
 }
 
-func (c *nowritebarrierrecChecker) findExtraCalls(n ir.Node) bool {
+func (c *nowritebarrierrecChecker) findExtraCalls(n ir.Node) {
 	if n.Op() != ir.OCALLFUNC {
-		return true
+		return
 	}
 	fn := n.Left()
 	if fn == nil || fn.Op() != ir.ONAME || fn.Class() != ir.PFUNC || fn.Name().Defn == nil {
-		return true
+		return
 	}
 	if !isRuntimePkg(fn.Sym().Pkg) || fn.Sym().Name != "systemstack" {
-		return true
+		return
 	}
 
 	var callee *ir.Func
@@ -887,7 +887,6 @@ func (c *nowritebarrierrecChecker) findExtraCalls(n ir.Node) bool {
 		base.Fatalf("expected ODCLFUNC node, got %+v", callee)
 	}
 	c.extraCalls[c.curfn] = append(c.extraCalls[c.curfn], nowritebarrierrecCall{callee, n.Pos()})
-	return true
 }
 
 // recordCall records a call from ODCLFUNC node "from", to function

src/cmd/compile/internal/gc/escape.go

@@ -225,7 +225,7 @@ func (e *Escape) walkFunc(fn *ir.Func) {
 	fn.SetEsc(EscFuncStarted)
 
 	// Identify labels that mark the head of an unstructured loop.
-	ir.InspectList(fn.Body(), func(n ir.Node) bool {
+	ir.Visit(fn, func(n ir.Node) {
 		switch n.Op() {
 		case ir.OLABEL:
 			if e.labels == nil {
@@ -240,8 +240,6 @@ func (e *Escape) walkFunc(fn *ir.Func) {
 				e.labels[n.Sym()] = looping
 			}
 		}
-
-		return true
 	})
 
 	e.curfn = fn

src/cmd/compile/internal/gc/initorder.go

@@ -268,18 +268,25 @@ func collectDeps(n ir.Node, transitive bool) ir.NameSet {
 type initDeps struct {
 	transitive bool
 	seen       ir.NameSet
+	cvisit     func(ir.Node)
 }
 
-func (d *initDeps) inspect(n ir.Node)      { ir.Inspect(n, d.visit) }
+func (d *initDeps) cachedVisit() func(ir.Node) {
-func (d *initDeps) inspectList(l ir.Nodes) { ir.InspectList(l, d.visit) }
+	if d.cvisit == nil {
+		d.cvisit = d.visit // cache closure
+	}
+	return d.cvisit
+}
+
+func (d *initDeps) inspect(n ir.Node)      { ir.Visit(n, d.cachedVisit()) }
+func (d *initDeps) inspectList(l ir.Nodes) { ir.VisitList(l, d.cachedVisit()) }
 
 // visit calls foundDep on any package-level functions or variables
 // referenced by n, if any.
-func (d *initDeps) visit(n ir.Node) bool {
+func (d *initDeps) visit(n ir.Node) {
 	switch n.Op() {
 	case ir.OMETHEXPR:
 		d.foundDep(methodExprName(n))
-		return false
 
 	case ir.ONAME:
 		n := n.(*ir.Name)
@@ -294,8 +301,6 @@ func (d *initDeps) visit(n ir.Node) bool {
 	case ir.ODOTMETH, ir.OCALLPART:
 		d.foundDep(methodExprName(n))
 	}
-
-	return true
 }
 
 // foundDep records that we've found a dependency on n by adding it to

src/cmd/compile/internal/gc/inl.go

@@ -255,7 +255,7 @@ func inlFlood(n *ir.Name) {
 	// Recursively identify all referenced functions for
 	// reexport. We want to include even non-called functions,
 	// because after inlining they might be callable.
-	ir.InspectList(ir.AsNodes(fn.Inl.Body), func(n ir.Node) bool {
+	ir.VisitList(ir.AsNodes(fn.Inl.Body), func(n ir.Node) {
 		switch n.Op() {
 		case ir.OMETHEXPR, ir.ODOTMETH:
 			inlFlood(methodExprName(n))
@@ -282,7 +282,6 @@ func inlFlood(n *ir.Name) {
 			//     inlFlood(n.Func.Closure.Func.Nname)
 			base.Fatalf("unexpected closure in inlinable function")
 		}
-		return true
 	})
 }
 
@@ -458,14 +457,10 @@ func (v *hairyVisitor) doNode(n ir.Node) error {
 
 func isBigFunc(fn *ir.Func) bool {
 	budget := inlineBigFunctionNodes
-	over := ir.Find(fn, func(n ir.Node) interface{} {
+	return ir.Find(fn, func(n ir.Node) bool {
 		budget--
-		if budget <= 0 {
+		return budget <= 0
-			return n
-		}
-		return nil
 	})
-	return over != nil
 }
 
 // Inlcalls/nodelist/node walks fn's statements and expressions and substitutes any
@@ -707,8 +702,6 @@ FindRHS:
 	return rhs
 }
 
-var errFound = errors.New("found")
-
 // reassigned takes an ONAME node, walks the function in which it is defined, and returns a boolean
 // indicating whether the name has any assignments other than its declaration.
 // The second return value is the first such assignment encountered in the walk, if any. It is mostly
@@ -723,22 +716,21 @@ func reassigned(name *ir.Name) bool {
 	if name.Curfn == nil {
 		return true
 	}
-	a := ir.Find(name.Curfn, func(n ir.Node) interface{} {
+	return ir.Find(name.Curfn, func(n ir.Node) bool {
 		switch n.Op() {
 		case ir.OAS:
 			if n.Left() == name && n != name.Defn {
-				return n
+				return true
 			}
 		case ir.OAS2, ir.OAS2FUNC, ir.OAS2MAPR, ir.OAS2DOTTYPE:
 			for _, p := range n.List().Slice() {
 				if p == name && n != name.Defn {
-					return n
+					return true
 				}
 			}
 		}
-		return nil
+		return false
 	})
-	return a != nil
 }
 
 func inlParam(t *types.Field, as ir.Node, inlvars map[*ir.Name]ir.Node) ir.Node {
@@ -916,11 +908,10 @@ func mkinlcall(n ir.Node, fn *ir.Func, maxCost int32, inlMap map[*ir.Func]bool,
 	}
 
 	nreturns := 0
-	ir.InspectList(ir.AsNodes(fn.Inl.Body), func(n ir.Node) bool {
+	ir.VisitList(ir.AsNodes(fn.Inl.Body), func(n ir.Node) {
 		if n != nil && n.Op() == ir.ORETURN {
 			nreturns++
 		}
-		return true
 	})
 
 	// We can delay declaring+initializing result parameters if:
@@ -1287,11 +1278,10 @@ func pruneUnusedAutos(ll []*ir.Name, vis *hairyVisitor) []*ir.Name {
 // concrete-type method calls where applicable.
 func devirtualize(fn *ir.Func) {
 	Curfn = fn
-	ir.InspectList(fn.Body(), func(n ir.Node) bool {
+	ir.VisitList(fn.Body(), func(n ir.Node) {
 		if n.Op() == ir.OCALLINTER {
 			devirtualizeCall(n)
 		}
-		return true
 	})
 }
 

src/cmd/compile/internal/gc/scc.go

@@ -75,7 +75,7 @@ func (v *bottomUpVisitor) visit(n *ir.Func) uint32 {
 	min := v.visitgen
 	v.stack = append(v.stack, n)
 
-	ir.InspectList(n.Body(), func(n ir.Node) bool {
+	ir.Visit(n, func(n ir.Node) {
 		switch n.Op() {
 		case ir.ONAME:
 			if n.Class() == ir.PFUNC {
@@ -111,7 +111,6 @@ func (v *bottomUpVisitor) visit(n *ir.Func) uint32 {
 				min = m
 			}
 		}
-		return true
 	})
 
 	if (min == id || min == id+1) && !n.IsHiddenClosure() {

src/cmd/compile/internal/gc/walk.go

@@ -3764,11 +3764,11 @@ func usefield(n ir.Node) {
 
 // hasSideEffects reports whether n contains any operations that could have observable side effects.
 func hasSideEffects(n ir.Node) bool {
-	found := ir.Find(n, func(n ir.Node) interface{} {
+	return ir.Find(n, func(n ir.Node) bool {
 		switch n.Op() {
 		// Assume side effects unless we know otherwise.
 		default:
-			return n
+			return true
 
 		// No side effects here (arguments are checked separately).
 		case ir.ONAME,
@@ -3824,29 +3824,28 @@ func hasSideEffects(n ir.Node) bool {
 			ir.OREAL,
 			ir.OIMAG,
 			ir.OCOMPLEX:
-			return nil
+			return false
 
 		// Only possible side effect is division by zero.
 		case ir.ODIV, ir.OMOD:
 			if n.Right().Op() != ir.OLITERAL || constant.Sign(n.Right().Val()) == 0 {
-				return n
+				return true
 			}
 
 		// Only possible side effect is panic on invalid size,
 		// but many makechan and makemap use size zero, which is definitely OK.
 		case ir.OMAKECHAN, ir.OMAKEMAP:
 			if !ir.IsConst(n.Left(), constant.Int) || constant.Sign(n.Left().Val()) != 0 {
-				return n
+				return true
 			}
 
 		// Only possible side effect is panic on invalid size.
 		// TODO(rsc): Merge with previous case (probably breaks toolstash -cmp).
 		case ir.OMAKESLICE, ir.OMAKESLICECOPY:
-			return n
+			return true
 		}
-		return nil
+		return false
 	})
-	return found != nil
 }
 
 // Rewrite

src/cmd/compile/internal/ir/visit.go

@@ -57,46 +57,40 @@ import (
 //	}
 //	do(root)
 //
-// The Inspect function illustrates a further simplification of the pattern,
+// The Visit function illustrates a further simplification of the pattern,
-// only considering processing before visiting children, and letting
+// only processing before visiting children and never stopping:
-// that processing decide whether children are visited at all:
 //
-//	func Inspect(n ir.Node, inspect func(ir.Node) bool) {
+//	func Visit(n ir.Node, visit func(ir.Node)) {
 //		var do func(ir.Node) error
 //		do = func(x ir.Node) error {
-//			if inspect(x) {
+//			visit(x)
-//				ir.DoChildren(x, do)
+//			return ir.DoChildren(x, do)
-//			}
-//			return nil
 //		}
 //		if n != nil {
-//			do(n)
+//			visit(n)
 //		}
 //	}
 //
 // The Find function illustrates a different simplification of the pattern,
 // visiting each node and then its children, recursively, until finding
-// a node x such that find(x) returns a non-nil result,
+// a node x for which find(x) returns true, at which point the entire
-// at which point the entire traversal stops:
+// traversal stops and returns true.
 //
-//	func Find(n ir.Node, find func(ir.Node) interface{}) interface{} {
+//	func Find(n ir.Node, find func(ir.Node)) bool {
 //		stop := errors.New("stop")
-//		var found interface{}
 //		var do func(ir.Node) error
 //		do = func(x ir.Node) error {
-//			if v := find(x); v != nil {
+//			if find(x) {
-//				found = v
 //				return stop
 //			}
 //			return ir.DoChildren(x, do)
 //		}
-//		do(n)
+//		return do(n) == stop
-//		return found
 //	}
 //
-// Inspect and Find are presented above as examples of how to use
+// Visit and Find are presented above as examples of how to use
 // DoChildren effectively, but of course, usage that fits within the
-// simplifications captured by Inspect or Find will be best served
+// simplifications captured by Visit or Find will be best served
 // by directly calling the ones provided by this package.
 func DoChildren(n Node, do func(Node) error) error {
 	if n == nil {
@@ -122,71 +116,59 @@ func DoList(list Nodes, do func(Node) error) error {
 	return nil
 }
 
-// Inspect visits each node x in the IR tree rooted at n
+// Visit visits each non-nil node x in the IR tree rooted at n
-// in a depth-first preorder traversal, calling inspect on each node visited.
+// in a depth-first preorder traversal, calling visit on each node visited.
-// If inspect(x) returns false, then Inspect skips over x's children.
+func Visit(n Node, visit func(Node)) {
-//
-// Note that the meaning of the boolean result in the callback function
-// passed to Inspect differs from that of Scan.
-// During Scan, if scan(x) returns false, then Scan stops the scan.
-// During Inspect, if inspect(x) returns false, then Inspect skips x's children
-// but continues with the remainder of the tree (x's siblings and so on).
-func Inspect(n Node, inspect func(Node) bool) {
 	var do func(Node) error
 	do = func(x Node) error {
-		if inspect(x) {
+		visit(x)
-			DoChildren(x, do)
+		return DoChildren(x, do)
-		}
-		return nil
 	}
 	if n != nil {
 		do(n)
 	}
 }
 
-// InspectList calls Inspect(x, inspect) for each node x in the list.
+// VisitList calls Visit(x, visit) for each node x in the list.
-func InspectList(list Nodes, inspect func(Node) bool) {
+func VisitList(list Nodes, visit func(Node)) {
 	for _, x := range list.Slice() {
-		Inspect(x, inspect)
+		Visit(x, visit)
 	}
 }
 
 var stop = errors.New("stop")
 
 // Find looks for a non-nil node x in the IR tree rooted at n
-// for which find(x) returns a non-nil value.
+// for which find(x) returns true.
 // Find considers nodes in a depth-first, preorder traversal.
-// When Find finds a node x such that find(x) != nil,
+// When Find finds a node x such that find(x) is true,
-// Find ends the traversal and returns the value of find(x) immediately.
+// Find ends the traversal and returns true immediately.
-// Otherwise Find returns nil.
+// Otherwise Find returns false after completing the entire traversal.
-func Find(n Node, find func(Node) interface{}) interface{} {
+func Find(n Node, find func(Node) bool) bool {
 	if n == nil {
-		return nil
+		return false
 	}
-	var found interface{}
 	var do func(Node) error
 	do = func(x Node) error {
-		if v := find(x); v != nil {
+		if find(x) {
-			found = v
 			return stop
 		}
 		return DoChildren(x, do)
 	}
-	do(n)
+	return do(n) == stop
-	return found
 }
 
-// FindList calls Find(x, ok) for each node x in the list, in order.
+// FindList calls Find(x, find) for each node x in the list, in order.
-// If any call find(x) returns a non-nil result, FindList stops and
+// If any call Find(x, find) returns true, FindList stops and
 // returns that result, skipping the remainder of the list.
-// Otherwise FindList returns nil.
+// Otherwise FindList returns false.
-func FindList(list Nodes, find func(Node) interface{}) interface{} {
+func FindList(list Nodes, find func(Node) bool) bool {
 	for _, x := range list.Slice() {
-		if v := Find(x, find); v != nil {
+		if Find(x, find) {
-			return v
+			return true
 		}
 	}
-	return nil
+	return false
 }
 
 // EditChildren edits the child nodes of n, replacing each child x with edit(x).