-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes
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>
- Loading branch information
Showing
9 changed files
with
356 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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") | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.