gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes

adonovan · gopherbot · commit 3c20e3f6cb81 · 2024-11-14T19:44:45.000Z
This analyzer detects failure to check the result of a call to yield (which can cause a range loop to run beyond the sequence, leading to a panic). It is not always a mistake to ignore the result of a call to yield; it depends on whether control can reach another call to yield without checking that the first call returned true. Consequently, this analyzer uses SSA for control flow analysis. We plan to add this analyzer to gopls before we promote it to vet. + test, relnote Fixes golang/go#65795 Change-Id: I75fa272e2f546be0c2acb10a1978c82bc19db5bd Reviewed-on: https://go-review.googlesource.com/c/tools/+/609617 Auto-Submit: Alan Donovan <adonovan@google.com> Reviewed-by: Robert Findley <rfindley@google.com> Commit-Queue: Alan Donovan <adonovan@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md
@@ -996,4 +996,39 @@ Default: off. Enable by setting `"analyses": {"useany": true}`.
 
 Package documentation: [useany](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/useany)
 
+<a id='yield'></a>
+## `yield`: report calls to yield where the result is ignored
+
+
+After a yield function returns false, the caller should not call
+the yield function again; generally the iterator should return
+promptly.
+
+This example fails to check the result of the call to yield,
+causing this analyzer to report a diagnostic:
+
+	yield(1) // yield may be called again (on L2) after returning false
+	yield(2)
+
+The corrected code is either this:
+
+	if yield(1) { yield(2) }
+
+or simply:
+
+	_ = yield(1) && yield(2)
+
+It is not always a mistake to ignore the result of yield.
+For example, this is a valid single-element iterator:
+
+	yield(1) // ok to ignore result
+	return
+
+It is only a mistake when the yield call that returned false may be
+followed by another call.
+
+Default: on.
+
+Package documentation: [yield](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/yield)
+
 <!-- END Analyzers: DO NOT MANUALLY EDIT THIS SECTION -->
diff --git a/gopls/doc/release/v0.17.0.md b/gopls/doc/release/v0.17.0.md
@@ -78,3 +78,9 @@ Gopls now offers a new code action, “Declare missing method of T.f”,
 where T is the concrete type and f is the undefined method.
 The stub method's signature is inferred
 from the context of the call.
+
+## `yield` analyzer
+
+The new `yield` analyzer detects mistakes using the `yield` function
+in a Go 1.23 iterator, such as failure to check its boolean result and
+break out of a loop.
diff --git a/gopls/internal/analysis/yield/doc.go b/gopls/internal/analysis/yield/doc.go
@@ -0,0 +1,38 @@
+// Copyright 2024 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 yield defines an Analyzer that checks for mistakes related
+// to the yield function used in iterators.
+//
+// # Analyzer yield
+//
+// yield: report calls to yield where the result is ignored
+//
+// After a yield function returns false, the caller should not call
+// the yield function again; generally the iterator should return
+// promptly.
+//
+// This example fails to check the result of the call to yield,
+// causing this analyzer to report a diagnostic:
+//
+//	yield(1) // yield may be called again (on L2) after returning false
+//	yield(2)
+//
+// The corrected code is either this:
+//
+//	if yield(1) { yield(2) }
+//
+// or simply:
+//
+//	_ = yield(1) && yield(2)
+//
+// It is not always a mistake to ignore the result of yield.
+// For example, this is a valid single-element iterator:
+//
+//	yield(1) // ok to ignore result
+//	return
+//
+// It is only a mistake when the yield call that returned false may be
+// followed by another call.
+package yield
diff --git a/gopls/internal/analysis/yield/main.go b/gopls/internal/analysis/yield/main.go
@@ -0,0 +1,16 @@
+// Copyright 2024 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.
+
+//go:build ignore
+
+// The yield command applies the yield analyzer to the specified
+// packages of Go source code.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/singlechecker"
+	"golang.org/x/tools/gopls/internal/analysis/yield"
+)
+
+func main() { singlechecker.Main(yield.Analyzer) }
diff --git a/gopls/internal/analysis/yield/testdata/src/a/a.go b/gopls/internal/analysis/yield/testdata/src/a/a.go
@@ -0,0 +1,85 @@
+// Copyright 2024 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 yield
+
+import (
+	"bufio"
+	"io"
+)
+
+//
+//
+// Modify this block of comment lines as needed when changing imports
+// to avoid perturbing subsequent line numbers (and thus error messages).
+//
+// This is L16.
+
+func goodIter(yield func(int) bool) {
+	_ = yield(1) && yield(2) && yield(3) // ok
+}
+
+func badIterOR(yield func(int) bool) {
+	_ = yield(1) || // want `yield may be called again \(on L25\) after returning false`
+		yield(2) || // want `yield may be called again \(on L26\) after returning false`
+		yield(3)
+}
+
+func badIterSeq(yield func(int) bool) {
+	yield(1) // want `yield may be called again \(on L31\) after returning false`
+	yield(2) // want `yield may be called again \(on L32\) after returning false`
+	yield(3) // ok
+}
+
+func badIterLoop(yield func(int) bool) {
+	for {
+		yield(1) // want `yield may be called again after returning false`
+	}
+}
+
+func goodIterLoop(yield func(int) bool) {
+	for {
+		if !yield(1) {
+			break
+		}
+	}
+}
+
+func badIterIf(yield func(int) bool) {
+	ok := yield(1) // want `yield may be called again \(on L52\) after returning false`
+	if !ok {
+		yield(2)
+	} else {
+		yield(3)
+	}
+}
+
+func singletonIter(yield func(int) bool) {
+	yield(1) // ok
+}
+
+func twoArgumentYield(yield func(int, int) bool) {
+	_ = yield(1, 1) || // want `yield may be called again \(on L64\) after returning false`
+		yield(2, 2)
+}
+
+func zeroArgumentYield(yield func() bool) {
+	_ = yield() || // want `yield may be called again \(on L69\) after returning false`
+		yield()
+}
+
+func tricky(in io.ReadCloser) func(yield func(string, error) bool) {
+	return func(yield func(string, error) bool) {
+		scan := bufio.NewScanner(in)
+		for scan.Scan() {
+			if !yield(scan.Text(), nil) { // want `yield may be called again \(on L82\) after returning false`
+				_ = in.Close()
+				break
+			}
+		}
+		if err := scan.Err(); err != nil {
+			yield("", err)
+		}
+	}
+}
diff --git a/gopls/internal/analysis/yield/yield.go b/gopls/internal/analysis/yield/yield.go
@@ -0,0 +1,145 @@
+// Copyright 2024 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 yield
+
+// TODO(adonovan): also check for this pattern:
+//
+// 	for x := range seq {
+// 		yield(x)
+// 	}
+//
+// which should be entirely rewritten as
+//
+// 	seq(yield)
+//
+// to avoid unnecesary range desugaring and chains of dynamic calls.
+
+import (
+	_ "embed"
+	"fmt"
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/buildssa"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/ssa"
+	"golang.org/x/tools/gopls/internal/util/safetoken"
+	"golang.org/x/tools/internal/analysisinternal"
+)
+
+//go:embed doc.go
+var doc string
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "yield",
+	Doc:      analysisinternal.MustExtractDoc(doc, "yield"),
+	Requires: []*analysis.Analyzer{inspect.Analyzer, buildssa.Analyzer},
+	Run:      run,
+	URL:      "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/yield",
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspector := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	// Find all calls to yield of the right type.
+	yieldCalls := make(map[token.Pos]*ast.CallExpr) // keyed by CallExpr.Lparen.
+	nodeFilter := []ast.Node{(*ast.CallExpr)(nil)}
+	inspector.Preorder(nodeFilter, func(n ast.Node) {
+		call := n.(*ast.CallExpr)
+		if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "yield" {
+			if sig, ok := pass.TypesInfo.TypeOf(id).(*types.Signature); ok &&
+				sig.Params().Len() < 3 &&
+				sig.Results().Len() == 1 &&
+				types.Identical(sig.Results().At(0).Type(), types.Typ[types.Bool]) {
+				yieldCalls[call.Lparen] = call
+			}
+		}
+	})
+
+	// Common case: nothing to do.
+	if len(yieldCalls) == 0 {
+		return nil, nil
+	}
+
+	// Study the control flow using SSA.
+	buildssa := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+	for _, fn := range buildssa.SrcFuncs {
+		// TODO(adonovan): opt: skip functions that don't contain any yield calls.
+
+		// Find the yield calls in SSA.
+		type callInfo struct {
+			syntax   *ast.CallExpr
+			index    int // index of instruction within its block
+			reported bool
+		}
+		ssaYieldCalls := make(map[*ssa.Call]*callInfo)
+		for _, b := range fn.Blocks {
+			for i, instr := range b.Instrs {
+				if call, ok := instr.(*ssa.Call); ok {
+					if syntax, ok := yieldCalls[call.Pos()]; ok {
+						ssaYieldCalls[call] = &callInfo{syntax: syntax, index: i}
+					}
+				}
+			}
+		}
+
+		// Now search for a control path from the instruction after a
+		// yield call to another yield call--possible the same one,
+		// following all block successors except "if yield() { ... }";
+		// in such cases we know that yield returned true.
+		for call, info := range ssaYieldCalls {
+			visited := make([]bool, len(fn.Blocks)) // visited BasicBlock.Indexes
+
+			// visit visits the instructions of a block (or a suffix if start > 0).
+			var visit func(b *ssa.BasicBlock, start int)
+			visit = func(b *ssa.BasicBlock, start int) {
+				if !visited[b.Index] {
+					if start == 0 {
+						visited[b.Index] = true
+					}
+					for _, instr := range b.Instrs[start:] {
+						switch instr := instr.(type) {
+						case *ssa.Call:
+							if !info.reported && ssaYieldCalls[instr] != nil {
+								info.reported = true
+								where := "" // "" => same yield call (a loop)
+								if instr != call {
+									otherLine := safetoken.StartPosition(pass.Fset, instr.Pos()).Line
+									where = fmt.Sprintf("(on L%d) ", otherLine)
+								}
+								pass.Reportf(call.Pos(), "yield may be called again %safter returning false", where)
+							}
+						case *ssa.If:
+							// Visit both successors, unless cond is yield() or its negation.
+							// 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
+							}
+							if cond, ok := cond.(*ssa.Call); ok && ssaYieldCalls[cond] != nil {
+								// Skip the successor reached by "if yield() { ... }".
+							} else {
+								visit(t, 0)
+							}
+							visit(f, 0)
+
+						case *ssa.Jump:
+							visit(b.Succs[0], 0)
+						}
+					}
+				}
+			}
+
+			// Start at the instruction after the yield call.
+			visit(call.Block(), info.index+1)
+		}
+	}
+
+	return nil, nil
+}
diff --git a/gopls/internal/analysis/yield/yield_test.go b/gopls/internal/analysis/yield/yield_test.go
@@ -0,0 +1,17 @@
+// Copyright 2024 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 yield_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/gopls/internal/analysis/yield"
+)
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	analysistest.Run(t, testdata, yield.Analyzer, "a")
+}
diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json
@@ -613,6 +613,11 @@
 							"Name": "\"useany\"",
 							"Doc": "check for constraints that could be simplified to \"any\"",
 							"Default": "false"
+						},
+						{
+							"Name": "\"yield\"",
+							"Doc": "report calls to yield where the result is ignored\n\nAfter a yield function returns false, the caller should not call\nthe yield function again; generally the iterator should return\npromptly.\n\nThis example fails to check the result of the call to yield,\ncausing this analyzer to report a diagnostic:\n\n\tyield(1) // yield may be called again (on L2) after returning false\n\tyield(2)\n\nThe corrected code is either this:\n\n\tif yield(1) { yield(2) }\n\nor simply:\n\n\t_ = yield(1) \u0026\u0026 yield(2)\n\nIt is not always a mistake to ignore the result of yield.\nFor example, this is a valid single-element iterator:\n\n\tyield(1) // ok to ignore result\n\treturn\n\nIt is only a mistake when the yield call that returned false may be\nfollowed by another call.",
+							"Default": "true"
 						}
 					]
 				},
@@ -1303,6 +1308,12 @@
 			"Doc": "check for constraints that could be simplified to \"any\"",
 			"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/useany",
 			"Default": false
+		},
+		{
+			"Name": "yield",
+			"Doc": "report calls to yield where the result is ignored\n\nAfter a yield function returns false, the caller should not call\nthe yield function again; generally the iterator should return\npromptly.\n\nThis example fails to check the result of the call to yield,\ncausing this analyzer to report a diagnostic:\n\n\tyield(1) // yield may be called again (on L2) after returning false\n\tyield(2)\n\nThe corrected code is either this:\n\n\tif yield(1) { yield(2) }\n\nor simply:\n\n\t_ = yield(1) \u0026\u0026 yield(2)\n\nIt is not always a mistake to ignore the result of yield.\nFor example, this is a valid single-element iterator:\n\n\tyield(1) // ok to ignore result\n\treturn\n\nIt is only a mistake when the yield call that returned false may be\nfollowed by another call.",
+			"URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/yield",
+			"Default": true
 		}
 	],
 	"Hints": [
diff --git a/gopls/internal/settings/analysis.go b/gopls/internal/settings/analysis.go
