gopls/internal/analysis/yield: peephole-optimize phi(false, x)

The yield analyzer reports spurious diagnostics for constructs
such as:

  func f(yield func() bool) {
	ok := yield()      // yield may be called again
	ok = ok && yield() // yield may be called again
	ok = ok && yield()
  }

This CL reduces phi(false, yield()) conditions as a special case.

The test suite includes a counterexample that still elicits
a false positive. If variants of this sort show up in practice,
we may need a more systematic treatment of dominating conditions,
similar to the nilness analysis, or even a full monotone framework
with lattice joins and fixed point iteration.

Fixes golang/go#70598

Change-Id: Ia602602cf5c233820d8ddbe73a286799bb3ea16c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/632117
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>

gopls/internal/analysis/yield/testdata/src/a/a.go

@@ -83,3 +83,38 @@ func tricky(in io.ReadCloser) func(yield func(string, error) bool) {
 		}
 	}
 }
+
+// Regression test for issue #70598.
+func shortCircuitAND(yield func(int) bool) {
+	ok := yield(1)
+	ok = ok && yield(2)
+	ok = ok && yield(3)
+	ok = ok && yield(4)
+}
+
+// This example has a bug because a false yield(2) may be followed by yield(3).
+func tricky2(yield func(int) bool) {
+	cleanup := func() {}
+	ok := yield(1)          // want "yield may be called again .on L104"
+	stop := !ok || yield(2) // want "yield may be called again .on L104"
+	if stop {
+		cleanup()
+	} else {
+		// dominated by !stop => !(!ok || yield(2)) => yield(1) && !yield(2): bad.
+		yield(3)
+	}
+}
+
+// This example is sound, but the analyzer reports a false positive.
+// TODO(adonovan): prune infeasible paths more carefully.
+func tricky3(yield func(int) bool) {
+	cleanup := func() {}
+	ok := yield(1)           // want "yield may be called again .on L118"
+	stop := !ok || !yield(2) // want "yield may be called again .on L118"
+	if stop {
+		cleanup()
+	} else {
+		// dominated by !stop => !(!ok || !yield(2)) => yield(1) && yield(2): good.
+		yield(3)
+	}
+}

gopls/internal/analysis/yield/yield.go

@@ -20,6 +20,7 @@ import (
 	_ "embed"
 	"fmt"
 	"go/ast"
+	"go/constant"
 	"go/token"
 	"go/types"
 
@@ -119,9 +120,34 @@ func run(pass *analysis.Pass) (interface{}, error) {
 							// In that case visit only the "if !yield()" block.
 							cond := instr.Cond
 							t, f := b.Succs[0], b.Succs[1]
-							if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT {
-								cond, t, f = unop.X, f, t
+
+							// Strip off any NOT operator.
+							cond, t, f = unnegate(cond, t, f)
+
+							// As a peephole optimization for this special case:
+							//   ok := yield()
+							//   ok = ok && yield()
+							//   ok = ok && yield()
+							// which in SSA becomes:
+							//   yield()
+							//   phi(false, yield())
+							//   phi(false, yield())
+							// we reduce a cond of phi(false, x) to just x.
+							if phi, ok := cond.(*ssa.Phi); ok {
+								var nonFalse []ssa.Value
+								for _, v := range phi.Edges {
+									if c, ok := v.(*ssa.Const); ok &&
+										!constant.BoolVal(c.Value) {
+										continue // constant false
+									}
+									nonFalse = append(nonFalse, v)
+								}
+								if len(nonFalse) == 1 {
+									cond = nonFalse[0]
+									cond, t, f = unnegate(cond, t, f)
+								}
 							}
+
 							if cond, ok := cond.(*ssa.Call); ok && ssaYieldCalls[cond] != nil {
 								// Skip the successor reached by "if yield() { ... }".
 							} else {
@@ -143,3 +169,10 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 	return nil, nil
 }
+
+func unnegate(cond ssa.Value, t, f *ssa.BasicBlock) (_ ssa.Value, _, _ *ssa.BasicBlock) {
+	if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT {
+		return unop.X, f, t
+	}
+	return cond, t, f
+}