diff --git a/README.md b/README.md index 21d8056..1317ce9 100644 --- a/README.md +++ b/README.md @@ -599,6 +599,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`use-any`](./RULES_DESCRIPTIONS.md#use-any) | n/a | Proposes to replace `interface{}` with its alias `any` | no | no | | [`use-errors-new`](./RULES_DESCRIPTIONS.md#use-errors-new) | n/a | Spots calls to `fmt.Errorf` that can be replaced by `errors.New` | no | no | | [`use-fmt-print`](./RULES_DESCRIPTIONS.md#use-fmt-print) | n/a | Proposes to replace calls to built-in `print` and `println` with their equivalents from `fmt`. | no | no | +| [`use-waitgroup-go`](./RULES_DESCRIPTIONS.md#use-waitgroup-go) | n/a | Proposes to replace `wg.Add ... go {... wg.Done ...}` idiom with `wg.Go` | no | no | | [`useless-break`](./RULES_DESCRIPTIONS.md#useless-break) | n/a | Warns on useless `break` statements in case clauses | no | no | | [`useless-fallthrough`](./RULES_DESCRIPTIONS.md#useless-fallthrough) | n/a | Warns on useless `fallthrough` statements in case clauses | no | no | | [`var-declaration`](./RULES_DESCRIPTIONS.md#var-declaration) | n/a | Reduces redundancies around variable declaration. | yes | yes | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index d1c2e51..23328a1 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -1421,6 +1421,18 @@ _Description_: This rule proposes to replace calls to built-in `print` and `prin _Configuration_: N/A +## use-waitgroup-go + +_Description_: Since Go 1.25 the `sync` package proposes the [`WaitGroup.Go`](https://pkg.go.dev/sync#WaitGroup.Go) method. +This method is a shorter and safer replacement for the idiom `wg.Add ... go { ... wg.Done ... }`. +The rule proposes to replace these legacy idioms with calls to the new method. + +_Limitations_: The rule doesn't rely on type information but on variable names to identify waitgroups. +This means the rule search for `wg` (the defacto standard name for wait groups); +if the waitgroup variable is named differently than `wg` the rule will skip it. + +_Configuration_: N/A + ## useless-break _Description_: This rule warns on useless `break` statements in case clauses of switch and select statements. Go, diff --git a/config/config.go b/config/config.go index a926726..0ffaaab 100644 --- a/config/config.go +++ b/config/config.go @@ -112,6 +112,7 @@ var allRules = append([]lint.Rule{ &rule.IdenticalSwitchBranchesRule{}, &rule.UselessFallthroughRule{}, &rule.PackageDirectoryMismatchRule{}, + &rule.UseWaitGroupGoRule{}, }, defaultRules...) // allFormatters is a list of all available formatters to output the linting results. diff --git a/lint/package.go b/lint/package.go index 34bd5d2..cb78cb4 100644 --- a/lint/package.go +++ b/lint/package.go @@ -42,6 +42,8 @@ var ( Go122 = goversion.Must(goversion.NewVersion("1.22")) // Go124 is a constant representing the Go version 1.24. Go124 = goversion.Must(goversion.NewVersion("1.24")) + // Go125 is a constant representing the Go version 1.25. + Go125 = goversion.Must(goversion.NewVersion("1.25")) ) // Files return package's files. diff --git a/rule/use_waitgroup_go.go b/rule/use_waitgroup_go.go new file mode 100644 index 0000000..9dd80d7 --- /dev/null +++ b/rule/use_waitgroup_go.go @@ -0,0 +1,158 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/internal/astutils" + "github.com/mgechev/revive/lint" +) + +// UseWaitGroupGoRule spots Go idioms that might be rewritten using WaitGroup.Go. +type UseWaitGroupGoRule struct{} + +// Apply applies the rule to given file. +func (*UseWaitGroupGoRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + if !file.Pkg.IsAtLeastGoVersion(lint.Go125) { + return nil // skip analysis if Go version < 1.25 + } + + var failures []lint.Failure + + onFailure := func(failure lint.Failure) { + failures = append(failures, failure) + } + + w := &lintUseWaitGroupGo{ + onFailure: onFailure, + } + + // Iterate over declarations looking for function declarations + for _, decl := range file.AST.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue // not a function + } + + if fn.Body == nil { + continue // external (no-Go) function + } + + // Analyze the function body + ast.Walk(w, fn.Body) + } + + return failures +} + +// Name returns the rule name. +func (*UseWaitGroupGoRule) Name() string { + return "use-waitgroup-go" +} + +type lintUseWaitGroupGo struct { + onFailure func(lint.Failure) +} + +func (w *lintUseWaitGroupGo) Visit(node ast.Node) ast.Visitor { + // Only interested in blocks of statements + block, ok := node.(*ast.BlockStmt) + if !ok { + return w // not a block of statements + } + + w.analyzeBlock(block) + + return w +} + +// analyzeBlock searches AST subtrees with the following form +// wg.Add(...) +// ... +// +// go func (...) { +// ... +// wg.Done // or defer wg.Done +// ... +// } +// +// Warning: the analysis only looks for exactly wg.Add and wg.Done, that means +// calls to Add and Done on a WaitGroup struct within a variable named differently than wg will be ignored +// This simplification avoids requiring type information while still makes the rule work in most of the cases. +// This rule assumes the WaitGroup variable is named 'wg', which is the common convention. +func (w *lintUseWaitGroupGo) analyzeBlock(b *ast.BlockStmt) { + // we will iterate over all statements in search for wg.Add() + stmts := b.List + for i := 0; i < len(stmts); i++ { + stmt := stmts[i] + if !w.isCallToWgAdd(stmt) { + continue + } + + call := stmt + + // Here we have identified a call to wg.Add + // Let's iterate over the statements that follow the wg.Add + // to see if there is a go statement that runs a goroutine with a wg.Done + // + // wg.Add is the i-th statement of block.List + // we will iterate from the (i+1)-th statement up to the last statement of block.List + for i++; i < len(stmts); i++ { + stmt := stmts[i] + // looking for a go statement + goStmt, ok := stmt.(*ast.GoStmt) + if !ok { + continue // not a go statement + } + + // here we found a the go statement + // now let's check is the go statement is applied to a function literal that contains a wg.Done + if !w.hasCallToWgDone(goStmt) { + continue + } + + w.onFailure(lint.Failure{ + Confidence: 1, + Node: call, + Category: lint.FailureCategoryCodeStyle, + Failure: "replace wg.Add()...go {...wg.Done()...} with wg.Go(...)", + }) + + break + } + } +} + +// hasCallToWgDone returns true if the given go statement +// calls to a function literal containing a call to wg.Done, false otherwise. +func (*lintUseWaitGroupGo) hasCallToWgDone(goStmt *ast.GoStmt) bool { + funcLit, ok := goStmt.Call.Fun.(*ast.FuncLit) + if !ok { + return false // the go statements runs a function defined elsewhere + } + + // here we found a go statement running a function literal + // now we will look for a wg.Done inside the body of the function literal + wgDoneStmt := astutils.SeekNode[ast.Node](funcLit.Body, wgDonePicker) + + return wgDoneStmt != nil +} + +// isCallToWgAdd returns true if the given statement is a call to wg.Add, false otherwise. +func (*lintUseWaitGroupGo) isCallToWgAdd(stmt ast.Stmt) bool { + expr, ok := stmt.(*ast.ExprStmt) + if !ok { + return false // not an expression statements thus not a function call + } + + // Lets check if the expression statement is a call to wg.Add + call, ok := expr.X.(*ast.CallExpr) + + return ok && astutils.IsPkgDotName(call.Fun, "wg", "Add") +} + +// function used when calling astutils.SeekNode that search for calls to wg.Done. +func wgDonePicker(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + result := ok && astutils.IsPkgDotName(call.Fun, "wg", "Done") + return result +} diff --git a/test/use_waitgroup_go_test.go b/test/use_waitgroup_go_test.go new file mode 100644 index 0000000..482c23c --- /dev/null +++ b/test/use_waitgroup_go_test.go @@ -0,0 +1,13 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestUseWaitGroupGo(t *testing.T) { + testRule(t, "use_waitgroup_go", &rule.UseWaitGroupGoRule{}, &lint.RuleConfig{}) + testRule(t, "go1.25/use_waitgroup_go", &rule.UseWaitGroupGoRule{}, &lint.RuleConfig{}) +} diff --git a/testdata/go1.25/go.mod b/testdata/go1.25/go.mod new file mode 100644 index 0000000..abc1bbf --- /dev/null +++ b/testdata/go1.25/go.mod @@ -0,0 +1,3 @@ +module github.com/mgechev/revive/testdata + +go 1.25 diff --git a/testdata/go1.25/use_waitgroup_go.go b/testdata/go1.25/use_waitgroup_go.go new file mode 100644 index 0000000..a827a49 --- /dev/null +++ b/testdata/go1.25/use_waitgroup_go.go @@ -0,0 +1,53 @@ +package fixtures + +import ( + "sync" +) + +func useWaitGroupGo() { + wg := sync.WaitGroup{} + + wg.Add(1) // MATCH /replace wg.Add()...go {...wg.Done()...} with wg.Go(...)/ + go func() { + defer wg.Done() + doSomething() + }() + + wg.Add(1) // MATCH /replace wg.Add()...go {...wg.Done()...} with wg.Go(...)/ + go func() { + doSomething() + wg.Done() + }() + + // from golang.org/x/tools/go/packages/packages.go/parseFiles + for i, file := range filenames { + wg.Add(1) // MATCH /replace wg.Add()...go {...wg.Done()...} with wg.Go(...)/ + go func(i int, filename string) { + parsed[i], errors[i] = ld.parseFile(filename) + wg.Done() + }(i, file) + } + wg.Wait() + + // from kubernetes/pkg/kubelet/cm/devicemanager/manager_test.go/TestGetTopologyHintsWithUpdates + // notice the rule spots a wg.Add(2) (vs wg.Add(1)) therefore using wg.Go is possible but requires + // replacing the wg.Add and the next two go statements with two wg.Go + + wg.Add(2) // MATCH /replace wg.Add()...go {...wg.Done()...} with wg.Go(...)/ + + go func() { + defer wg.Done() + for i := 0; i < test.count; i++ { + // simulate the device plugin to send device updates + mimpl.genericDeviceUpdateCallback(testResourceName, devs) + } + updated.Store(true) + }() + go func() { + defer wg.Done() + for !updated.Load() { + test.testfunc(mimpl) + } + }() + wg.Wait() +} diff --git a/testdata/use_waitgroup_go.go b/testdata/use_waitgroup_go.go new file mode 100644 index 0000000..4270b43 --- /dev/null +++ b/testdata/use_waitgroup_go.go @@ -0,0 +1,53 @@ +package fixtures + +import ( + "sync" +) + +// Rule use-waitgroup-go shall not match because this file is a package with Go version < 1.25 +func useWaitGroupGo() { + wg := sync.WaitGroup{} + + wg.Add(1) + go func() { + defer wg.Done() + doSomething() + }() + + wg.Add(1) + go func() { + doSomething() + wg.Done() + }() + + // from golang.org/x/tools/go/packages/packages.go/parseFiles + for i, file := range filenames { + wg.Add(1) + go func(i int, filename string) { + parsed[i], errors[i] = ld.parseFile(filename) + wg.Done() + }(i, file) + } + wg.Wait() + + // from kubernetes/pkg/kubelet/cm/devicemanager/manager_test.go/TestGetTopologyHintsWithUpdates + // notice the rule spots a wg.Add(2) (vs wg.Add(1)) therefore using wg.Go is possible but requires + // replacing the wg.Add and the next two go statements with two wg.Go + wg.Add(2) + + go func() { + defer wg.Done() + for i := 0; i < test.count; i++ { + // simulate the device plugin to send device updates + mimpl.genericDeviceUpdateCallback(testResourceName, devs) + } + updated.Store(true) + }() + go func() { + defer wg.Done() + for !updated.Load() { + test.testfunc(mimpl) + } + }() + wg.Wait() +}