From 86ea9d72099bbc5a335b3455d7193c2152e65b49 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Thu, 29 Aug 2024 18:23:57 -0400 Subject: [PATCH] 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 Reviewed-by: Robert Findley Commit-Queue: Alan Donovan LUCI-TryBot-Result: Go LUCI --- gopls/internal/analysis/yield/testdata/src/a/a.go | 3 +++ gopls/internal/analysis/yield/yield.go | 12 ++++++++++++ gopls/internal/analysis/yield/yield_test.go | 1 + gopls/internal/doc/api.json | 3 +++ gopls/internal/golang/assembly.go | 2 +- 5 files changed, 20 insertions(+), 1 deletion(-) diff --git a/gopls/internal/analysis/yield/testdata/src/a/a.go b/gopls/internal/analysis/yield/testdata/src/a/a.go index 9eb88b5ae69..5960cddb220 100644 --- a/gopls/internal/analysis/yield/testdata/src/a/a.go +++ b/gopls/internal/analysis/yield/testdata/src/a/a.go @@ -83,6 +83,7 @@ func tricky(in io.ReadCloser) func(yield func(string, error) bool) { } } } +<<<<<<< HEAD // Regression test for issue #70598. func shortCircuitAND(yield func(int) bool) { @@ -118,3 +119,5 @@ func tricky3(yield func(int) bool) { yield(3) } } +======= +>>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) diff --git a/gopls/internal/analysis/yield/yield.go b/gopls/internal/analysis/yield/yield.go index 9eaea3f0407..9e580d6872a 100644 --- a/gopls/internal/analysis/yield/yield.go +++ b/gopls/internal/analysis/yield/yield.go @@ -20,7 +20,10 @@ import ( _ "embed" "fmt" "go/ast" +<<<<<<< HEAD "go/constant" +======= +>>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) "go/token" "go/types" @@ -120,6 +123,7 @@ 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] +<<<<<<< HEAD // Strip off any NOT operator. cond, t, f = unnegate(cond, t, f) @@ -148,6 +152,11 @@ func run(pass *analysis.Pass) (interface{}, error) { } } +======= + if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT { + cond, t, f = unop.X, f, t + } +>>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) if cond, ok := cond.(*ssa.Call); ok && ssaYieldCalls[cond] != nil { // Skip the successor reached by "if yield() { ... }". } else { @@ -169,6 +178,7 @@ func run(pass *analysis.Pass) (interface{}, error) { return nil, nil } +<<<<<<< HEAD func unnegate(cond ssa.Value, t, f *ssa.BasicBlock) (_ ssa.Value, _, _ *ssa.BasicBlock) { if unop, ok := cond.(*ssa.UnOp); ok && unop.Op == token.NOT { @@ -176,3 +186,5 @@ func unnegate(cond ssa.Value, t, f *ssa.BasicBlock) (_ ssa.Value, _, _ *ssa.Basi } return cond, t, f } +======= +>>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) diff --git a/gopls/internal/analysis/yield/yield_test.go b/gopls/internal/analysis/yield/yield_test.go index af6784374e2..78944997ea5 100644 --- a/gopls/internal/analysis/yield/yield_test.go +++ b/gopls/internal/analysis/yield/yield_test.go @@ -15,3 +15,4 @@ func Test(t *testing.T) { testdata := analysistest.TestData() analysistest.Run(t, testdata, yield.Analyzer, "a") } +e \ No newline at end of file diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index b64965ab863..e47b65c4044 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -1304,12 +1304,15 @@ "Default": false }, { +<<<<<<< HEAD "Name": "waitgroup", "Doc": "check for misuses of sync.WaitGroup\n\nThis analyzer detects mistaken calls to the (*sync.WaitGroup).Add\nmethod from inside a new goroutine, causing Add to race with Wait:\n\n\t// WRONG\n\tvar wg sync.WaitGroup\n\tgo func() {\n\t wg.Add(1) // \"WaitGroup.Add called from inside new goroutine\"\n\t defer wg.Done()\n\t ...\n\t}()\n\twg.Wait() // (may return prematurely before new goroutine starts)\n\nThe correct code calls Add before starting the goroutine:\n\n\t// RIGHT\n\tvar wg sync.WaitGroup\n\twg.Add(1)\n\tgo func() {\n\t\tdefer wg.Done()\n\t\t...\n\t}()\n\twg.Wait()", "URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/waitgroup", "Default": true }, { +======= +>>>>>>> 9b6e4f21d (gopls/internal/analysis/yield: analyzer for iterator (yield) mistakes) "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", diff --git a/gopls/internal/golang/assembly.go b/gopls/internal/golang/assembly.go index 7f0ace4daf6..e22bd503d7f 100644 --- a/gopls/internal/golang/assembly.go +++ b/gopls/internal/golang/assembly.go @@ -1,4 +1,4 @@ -// Copyright 2024 The Go Authors. All rights reserved. +ew// 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.