go/analysis/passes/waitgroup: report WaitGroup.Add in goroutine

This CL defines a new analyzer, "waitgroup", that reports a
common mistake with sync.WaitGroup: calling wg.Add(1) inside
the new goroutine, instead of before starting it.

This is a port of Dominik Honnef's SA2000 algorithm,
which uses tree-based pattern matching, to elementary
go/{ast,types} + inspector operations.

Fixes golang/go#18022
Updates golang/go#63796

Change-Id: I9d6d3b602ce963912422ee0459bb1f9522fc51f9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/632915
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

go/analysis/passes/waitgroup/doc.go

@@ -0,0 +1,34 @@
+// 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 waitgroup defines an Analyzer that detects simple misuses
+// of sync.WaitGroup.
+//
+// # Analyzer waitgroup
+//
+// waitgroup: check for misuses of sync.WaitGroup
+//
+// This analyzer detects mistaken calls to the (*sync.WaitGroup).Add
+// method from inside a new goroutine, causing Add to race with Wait:
+//
+//	// WRONG
+//	var wg sync.WaitGroup
+//	go func() {
+//	        wg.Add(1) // "WaitGroup.Add called from inside new goroutine"
+//	        defer wg.Done()
+//	        ...
+//	}()
+//	wg.Wait() // (may return prematurely before new goroutine starts)
+//
+// The correct code calls Add before starting the goroutine:
+//
+//	// RIGHT
+//	var wg sync.WaitGroup
+//	wg.Add(1)
+//	go func() {
+//		defer wg.Done()
+//		...
+//	}()
+//	wg.Wait()
+package waitgroup

go/analysis/passes/waitgroup/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 waitgroup command applies the golang.org/x/tools/go/analysis/passes/waitgroup
+// analysis to the specified packages of Go source code.
+package main
+
+import (
+	"golang.org/x/tools/go/analysis/passes/waitgroup"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+func main() { singlechecker.Main(waitgroup.Analyzer) }

go/analysis/passes/waitgroup/testdata/src/a/a.go

@@ -0,0 +1,14 @@
+package a
+
+import "sync"
+
+func f() {
+	var wg sync.WaitGroup
+	wg.Add(1) // ok
+	go func() {
+		wg.Add(1) // want "WaitGroup.Add called from inside new goroutine"
+		// ...
+		wg.Add(1) // ok
+	}()
+	wg.Add(1) // ok
+}

go/analysis/passes/waitgroup/waitgroup.go

@@ -0,0 +1,105 @@
+// Copyright 2023 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 waitgroup defines an Analyzer that detects simple misuses
+// of sync.WaitGroup.
+package waitgroup
+
+import (
+	_ "embed"
+	"go/ast"
+	"go/types"
+	"reflect"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/analysis/passes/internal/analysisutil"
+	"golang.org/x/tools/go/ast/inspector"
+	"golang.org/x/tools/go/types/typeutil"
+	"golang.org/x/tools/internal/typesinternal"
+)
+
+//go:embed doc.go
+var doc string
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "waitgroup",
+	Doc:      analysisutil.MustExtractDoc(doc, "waitgroup"),
+	URL:      "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/waitgroup",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	if !analysisutil.Imports(pass.Pkg, "sync") {
+		return nil, nil // doesn't directly import sync
+	}
+
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) (proceed bool) {
+		if push {
+			call := n.(*ast.CallExpr)
+			if fn, ok := typeutil.Callee(pass.TypesInfo, call).(*types.Func); ok &&
+				isMethodNamed(fn, "sync", "WaitGroup", "Add") &&
+				hasSuffix(stack, wantSuffix) &&
+				backindex(stack, 1) == backindex(stack, 2).(*ast.BlockStmt).List[0] { // ExprStmt must be Block's first stmt
+
+				pass.Reportf(call.Lparen, "WaitGroup.Add called from inside new goroutine")
+			}
+		}
+		return true
+	})
+
+	return nil, nil
+}
+
+//	go func() {
+//	   wg.Add(1)
+//	   ...
+//	}()
+var wantSuffix = []ast.Node{
+	(*ast.GoStmt)(nil),
+	(*ast.CallExpr)(nil),
+	(*ast.FuncLit)(nil),
+	(*ast.BlockStmt)(nil),
+	(*ast.ExprStmt)(nil),
+	(*ast.CallExpr)(nil),
+}
+
+// hasSuffix reports whether stack has the matching suffix,
+// considering only node types.
+func hasSuffix(stack, suffix []ast.Node) bool {
+	// TODO(adonovan): the inspector could implement this for us.
+	if len(stack) < len(suffix) {
+		return false
+	}
+	for i := range len(suffix) {
+		if reflect.TypeOf(backindex(stack, i)) != reflect.TypeOf(backindex(suffix, i)) {
+			return false
+		}
+	}
+	return true
+}
+
+// isMethodNamed reports whether f is a method with the specified
+// package, receiver type, and method names.
+func isMethodNamed(fn *types.Func, pkg, recv, name string) bool {
+	if fn.Pkg() != nil && fn.Pkg().Path() == pkg && fn.Name() == name {
+		if r := fn.Type().(*types.Signature).Recv(); r != nil {
+			if _, gotRecv := typesinternal.ReceiverNamed(r); gotRecv != nil {
+				return gotRecv.Obj().Name() == recv
+			}
+		}
+	}
+	return false
+}
+
+// backindex is like [slices.Index] but from the back of the slice.
+func backindex[T any](slice []T, i int) T {
+	return slice[len(slice)-1-i]
+}

go/analysis/passes/waitgroup/waitgroup_test.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.
+
+package waitgroup_test
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/waitgroup"
+)
+
+func Test(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), waitgroup.Analyzer, "a")
+}

gopls/internal/util/typesutil/typesutil.go

@@ -257,8 +257,3 @@ func EnclosingSignature(path []ast.Node, info *types.Info) *types.Signature {
 	}
 	return nil
 }
-
-func is[T any](x any) bool {
-	_, ok := x.(T)
-	return ok
-}