mirror of
				https://github.com/mgechev/revive.git
				synced 2025-10-30 23:37:49 +02:00 
			
		
		
		
	feature: new rule forbidden-call-in-wg-go (#1514)
				
					
				
			This commit is contained in:
		| @@ -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   | | ||||
|   | ||||
| @@ -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. | ||||
|   | ||||
| @@ -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. | ||||
|   | ||||
							
								
								
									
										113
									
								
								rule/forbidden_call_in_wg_go.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										113
									
								
								rule/forbidden_call_in_wg_go.go
									
									
									
									
									
										Normal file
									
								
							| @@ -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 | ||||
| } | ||||
							
								
								
									
										13
									
								
								test/forbidden_call_in_wg_go_test.go
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										13
									
								
								test/forbidden_call_in_wg_go_test.go
									
									
									
									
									
										Normal file
									
								
							| @@ -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{}) | ||||
| } | ||||
							
								
								
									
										61
									
								
								testdata/forbidden_call_in_wg_go.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							
							
						
						
									
										61
									
								
								testdata/forbidden_call_in_wg_go.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							| @@ -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() | ||||
| } | ||||
							
								
								
									
										61
									
								
								testdata/go1.25/forbidden_call_in_wg_go.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							
							
						
						
									
										61
									
								
								testdata/go1.25/forbidden_call_in_wg_go.go
									
									
									
									
										vendored
									
									
										Normal file
									
								
							| @@ -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() | ||||
| } | ||||
		Reference in New Issue
	
	Block a user