diff --git a/README.md b/README.md index cc02ea0..3e4b9bd 100644 --- a/README.md +++ b/README.md @@ -546,6 +546,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`file-length-limit`](./RULES_DESCRIPTIONS.md#file-length-limit) | map (optional)| Enforces a maximum number of lines per file | no | no | | [`filename-format`](./RULES_DESCRIPTIONS.md#filename-format) | regular expression (optional) | Enforces the formatting of filenames | no | no | | [`flag-parameter`](./RULES_DESCRIPTIONS.md#flag-parameter) | n/a | Warns on boolean parameters that create a control coupling | no | no | +| [`forbidden-call-in-wg-go`](./RULES_DESCRIPTIONS.md#forbidden-call-in-wg-go) | n/a | Warns on forbidden calls inside calls to wg.Go | no | no | | [`function-length`](./RULES_DESCRIPTIONS.md#function-length) | int, int (defaults to 50 statements, 75 lines) | Warns on functions exceeding the statements or lines max | no | no | | [`function-result-limit`](./RULES_DESCRIPTIONS.md#function-result-limit) | int (defaults to 3)| Specifies the maximum number of results a function can return | no | no | | [`get-return`](./RULES_DESCRIPTIONS.md#get-return) | n/a | Warns on getters that do not yield any result | no | no | diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 99818b3..2be1d61 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -42,6 +42,7 @@ List of all available rules. - [file-length-limit](#file-length-limit) - [filename-format](#filename-format) - [flag-parameter](#flag-parameter) +- [forbidden-call-in-wg-go](#forbidden-call-in-wg-go) - [function-length](#function-length) - [function-result-limit](#function-result-limit) - [get-return](#get-return) @@ -283,7 +284,7 @@ _Configuration_: N/A _Description_: Function or methods that return multiple, no named, values of the same type could induce error. -### Examples (confusing-naming) +### Examples (confusing-results) Before (violation): @@ -747,6 +748,61 @@ This rule warns on boolean parameters that create a control coupling. _Configuration_: N/A +## forbidden-call-in-wg-go + +_Description_: Since Go 1.25, it is possible to create goroutines with the method `waitgroup.Go`. +The `Go` method calls a function in a new goroutine and adds (`Add`) that task to the WaitGroup. +When the function returns, the task is removed (`Done`) from the WaitGroup. + +This rule ensures that functions don't panic as is specified +in the [documentation of `WaitGroup.Go`](https://pkg.go.dev/sync#WaitGroup.Go). + +The rule also warns against a common mistake when refactoring legacy code: +accidentally leaving behind a call to `WaitGroup.Done`, which can cause subtle bugs or panics. + +### Examples (forbidden-call-in-wg-go) + +Legacy code with a call to `wg.Done`: + +```go +wg := sync.WaitGroup{} + +wg.Add(1) +go func() { + doSomething() + wg.Done() +}() + +wg.Wait +``` + +Refactored, incorrect, code: + +```go +wg := sync.WaitGroup{} + +wg.Go(func() { + doSomething() + wg.Done() +}) + +wg.Wait +``` + +Fixed code: + +```go +wg := sync.WaitGroup{} + +wg.Go(func() { + doSomething() +}) + +wg.Wait +``` + +_Configuration_: N/A + ## function-length _Description_: Functions too long (with many statements and/or lines) can be hard to understand. diff --git a/config/config.go b/config/config.go index 22b2d67..d163308 100644 --- a/config/config.go +++ b/config/config.go @@ -114,6 +114,7 @@ var allRules = append([]lint.Rule{ &rule.UseWaitGroupGoRule{}, &rule.UnsecureURLSchemeRule{}, &rule.InefficientMapLookupRule{}, + &rule.ForbiddenCallInWgGoRule{}, }, defaultRules...) // allFormatters is a list of all available formatters to output the linting results. diff --git a/rule/forbidden_call_in_wg_go.go b/rule/forbidden_call_in_wg_go.go new file mode 100644 index 0000000..63088e5 --- /dev/null +++ b/rule/forbidden_call_in_wg_go.go @@ -0,0 +1,113 @@ +package rule + +import ( + "fmt" + "go/ast" + "strings" + + "github.com/mgechev/revive/internal/astutils" + "github.com/mgechev/revive/lint" +) + +// ForbiddenCallInWgGoRule spots calls to panic or wg.Done when using WaitGroup.Go. +type ForbiddenCallInWgGoRule struct{} + +// Apply applies the rule to given file. +func (*ForbiddenCallInWgGoRule) 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 := &lintForbiddenCallInWgGo{ + 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 (*ForbiddenCallInWgGoRule) Name() string { + return "forbidden-call-in-wg-go" +} + +type lintForbiddenCallInWgGo struct { + onFailure func(lint.Failure) +} + +func (w *lintForbiddenCallInWgGo) Visit(node ast.Node) ast.Visitor { + call, ok := node.(*ast.CallExpr) + if !ok { + return w // not a call of statements + } + + if !astutils.IsPkgDotName(call.Fun, "wg", "Go") { + return w // not a call to wg.Go + } + + if len(call.Args) != 1 { + return nil // no argument (impossible) + } + + funcLit, ok := call.Args[0].(*ast.FuncLit) + if !ok { + return nil // the argument is not a function literal + } + + var callee string + + forbiddenCallPicker := func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return false + } + + if astutils.IsPkgDotName(call.Fun, "wg", "Done") || + astutils.IsIdent(call.Fun, "panic") || + astutils.IsPkgDotName(call.Fun, "log", "Panic") || + astutils.IsPkgDotName(call.Fun, "log", "Panicf") || + astutils.IsPkgDotName(call.Fun, "log", "Panicln") { + callee = astutils.GoFmt(n) + callee, _, _ = strings.Cut(callee, "(") + return true + } + + return false + } + + // search a forbidden call in the body of the function literal + forbiddenCall := astutils.SeekNode[*ast.CallExpr](funcLit.Body, forbiddenCallPicker) + if forbiddenCall == nil { + return nil // there is no forbidden call in the call to wg.Go + } + + msg := fmt.Sprintf("do not call %s inside wg.Go", callee) + w.onFailure(lint.Failure{ + Confidence: 1, + Node: forbiddenCall, + Category: lint.FailureCategoryErrors, + Failure: msg, + }) + + return nil +} diff --git a/test/forbidden_call_in_wg_go_test.go b/test/forbidden_call_in_wg_go_test.go new file mode 100644 index 0000000..74dcd34 --- /dev/null +++ b/test/forbidden_call_in_wg_go_test.go @@ -0,0 +1,13 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestForbiddenCallInWgGo(t *testing.T) { + testRule(t, "forbidden_call_in_wg_go", &rule.ForbiddenCallInWgGoRule{}, &lint.RuleConfig{}) + testRule(t, "go1.25/forbidden_call_in_wg_go", &rule.ForbiddenCallInWgGoRule{}, &lint.RuleConfig{}) +} diff --git a/testdata/forbidden_call_in_wg_go.go b/testdata/forbidden_call_in_wg_go.go new file mode 100644 index 0000000..4994586 --- /dev/null +++ b/testdata/forbidden_call_in_wg_go.go @@ -0,0 +1,61 @@ +package fixtures + +import ( + "fmt" + "log" + "sync" +) + +func forbiddenCallInWgGo() { + wg := sync.WaitGroup{} + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + wg.Done() + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + defer wg.Done() + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + panic("don't panic here") + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panic("don't panic here") + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panicf("don't panic here") + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panicln("don't panic here") + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + }) + } + + wg.Wait() +} diff --git a/testdata/go1.25/forbidden_call_in_wg_go.go b/testdata/go1.25/forbidden_call_in_wg_go.go new file mode 100644 index 0000000..c1f3865 --- /dev/null +++ b/testdata/go1.25/forbidden_call_in_wg_go.go @@ -0,0 +1,61 @@ +package fixtures + +import ( + "fmt" + "log" + "sync" +) + +func forbiddenCallInWgGo() { + wg := sync.WaitGroup{} + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + wg.Done() // MATCH /do not call wg.Done inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + defer wg.Done() // MATCH /do not call wg.Done inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + panic("don't panic here") // MATCH /do not call panic inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panic("don't panic here") // MATCH /do not call log.Panic inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panicf("don't panic here") // MATCH /do not call log.Panicf inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + log.Panicln("don't panic here") // MATCH /do not call log.Panicln inside wg.Go/ + }) + } + + for i := 1; i <= 5; i++ { + wg.Go(func() { + fmt.Println(i) + }) + } + + wg.Wait() +}