checkers: add deferInLoop checker (#1145)

go-critic · Oct 26, 2021 · c217bcd · c217bcd
1 parent 4fbf1c1
commit c217bcd
Show file tree

Hide file tree

Showing 3 changed files with 379 additions and 0 deletions.
diff --git a/checkers/deferInLoop_checker.go b/checkers/deferInLoop_checker.go
@@ -0,0 +1,87 @@
+package checkers
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/ast/astutil"
+
+	"github.com/go-critic/go-critic/checkers/internal/astwalk"
+	"github.com/go-critic/go-critic/framework/linter"
+)
+
+func init() {
+	var info linter.CheckerInfo
+	info.Name = "deferInLoop"
+	info.Tags = []string{"diagnostic", "experimental"}
+	info.Summary = "Detects loops inside functions that use defer"
+	info.Before = `
+for _, filename := range []string{"foo", "bar"} {
+	 f, err := os.Open(filename)
+	
+	defer f.Close()
+}
+`
+	info.After = `
+func process(filename string) {
+	 f, err := os.Open(filename)
+	
+	defer f.Close()
+}
+/* ... */
+for _, filename := range []string{"foo", "bar"} {
+	process(filename)
+}`
+
+	collection.AddChecker(&info, func(ctx *linter.CheckerContext) (linter.FileWalker, error) {
+		return astwalk.WalkerForFuncDecl(&deferInLoopChecker{ctx: ctx}), nil
+	})
+}
+
+type deferInLoopChecker struct {
+	astwalk.WalkHandler
+	ctx *linter.CheckerContext
+}
+
+func (c *deferInLoopChecker) VisitFuncDecl(fn *ast.FuncDecl) {
+	// TODO: add func args check
+	// example: t.Run(123, func() { for { defer println(); break } })
+	var blockParser func(*ast.BlockStmt, bool)
+	blockParser = func(block *ast.BlockStmt, inFor bool) {
+		for _, cur := range block.List {
+			switch n := cur.(type) {
+			case *ast.DeferStmt:
+				if inFor {
+					c.warn(n)
+				}
+			case *ast.RangeStmt:
+				blockParser(n.Body, true)
+			case *ast.ForStmt:
+				blockParser(n.Body, true)
+			case *ast.GoStmt:
+				if f, ok := n.Call.Fun.(*ast.FuncLit); ok {
+					blockParser(f.Body, false)
+				}
+			case *ast.ExprStmt:
+				if f, ok := n.X.(*ast.CallExpr); ok {
+					if anon, ok := f.Fun.(*ast.FuncLit); ok {
+						blockParser(anon.Body, false)
+					}
+				}
+			case *ast.BlockStmt:
+				blockParser(n, inFor)
+			}
+		}
+	}
+	pre := func(cur *astutil.Cursor) bool {
+		if n, ok := cur.Node().(*ast.BlockStmt); ok {
+			blockParser(n, false)
+		}
+		return false
+	}
+
+	astutil.Apply(fn.Body, pre, nil)
+}
+
+func (c *deferInLoopChecker) warn(cause *ast.DeferStmt) {
+	c.ctx.Warn(cause, "Possible resource leak, 'defer' is called in the 'for' loop")
+}
diff --git a/checkers/testdata/deferInLoop/negative_tests.go b/checkers/testdata/deferInLoop/negative_tests.go
@@ -0,0 +1,112 @@
+package checker_test
+
+func testForStmt() {
+	defer println(111)
+
+	for range []int{1, 2, 3, 4} {
+		println(222)
+		break
+	}
+
+	defer println(222)
+}
+
+func testRangeStmt() {
+	defer println(222)
+
+	for i := 0; i < 10; i++ {
+		println(111)
+	}
+
+	defer println(222)
+}
+
+func testClosure() {
+	func() {
+		for {
+			break
+		}
+		defer println(1)
+
+		for {
+			for {
+				break
+			}
+			break
+		}
+
+		defer println(1)
+	}()
+
+	func() {
+		defer println(123)
+
+		for range []int{1, 2, 3, 4} {
+			println(222)
+		}
+
+		defer println(123)
+
+		for range []int{1, 2, 3, 4} {
+			for range []int{1, 2, 3, 4} {
+				println(222)
+			}
+		}
+
+		defer println(123)
+	}()
+
+	for {
+		func() {
+			defer println(123)
+		}()
+		break
+	}
+
+	for {
+		go func() {
+			defer println()
+		}()
+
+		break
+	}
+}
+
+func testBlock() {
+	{
+		for {
+			func() {
+				defer println()
+			}()
+			break
+		}
+	}
+	{
+		for {
+			go func() {
+				defer println()
+			}()
+			break
+		}
+	}
+	{
+		for range []int{1, 2, 3, 4} {
+			go func() {
+				defer println()
+			}()
+			break
+		}
+	}
+	{
+		for range []int{1, 2, 3, 4} {
+			{
+				func() {
+					{
+						defer println()
+					}
+				}()
+			}
+			break
+		}
+	}
+}
diff --git a/checkers/testdata/deferInLoop/positive_tests.go b/checkers/testdata/deferInLoop/positive_tests.go
@@ -0,0 +1,180 @@
+package checker_test
+
+import "fmt"
+
+func deferWithCall() {
+	for {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer fmt.Println("test")
+		break
+	}
+
+	for range []int{1, 2, 3, 4} {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer fmt.Println("test")
+	}
+}
+
+func deferWithClosure() {
+	for {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer func() {}()
+
+		break
+	}
+
+	for range []int{1, 2, 3, 4} {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer func() {}()
+	}
+}
+
+func innerLoops() {
+	for {
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer func() {}()
+
+			break
+		}
+		break
+	}
+
+	for {
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer fmt.Println(123)
+
+			break
+		}
+		break
+	}
+
+	for range []int{1, 2, 3, 4} {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer func() {}()
+
+		for range []int{1, 2, 3, 4} {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer func() {}()
+		}
+	}
+}
+
+func anonFunc() {
+	func() {
+		for range []int{1, 2, 3, 4} {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer func() {}()
+
+			for range []int{1, 2, 3, 4} {
+				/*! Possible resource leak, 'defer' is called in the 'for' loop */
+				defer func() {}()
+			}
+		}
+	}()
+
+	for {
+		/*! Possible resource leak, 'defer' is called in the 'for' loop */
+		defer println(123)
+		break
+	}
+
+	func() {
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer func() {}()
+
+			for range []int{1, 2, 3, 4} {
+				/*! Possible resource leak, 'defer' is called in the 'for' loop */
+				defer func() {}()
+			}
+
+			break
+		}
+
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer func() {}()
+			break
+		}
+	}()
+
+	go func() {
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer println(123)
+			break
+		}
+	}()
+}
+
+func contextBlock() {
+	{
+		go func() {
+			for {
+				/*! Possible resource leak, 'defer' is called in the 'for' loop */
+				defer println(123)
+				break
+			}
+		}()
+	}
+
+	{
+		func() {
+			for {
+				/*! Possible resource leak, 'defer' is called in the 'for' loop */
+				defer println(123)
+				break
+			}
+		}()
+	}
+
+	{
+		{
+			func() {
+				for {
+					/*! Possible resource leak, 'defer' is called in the 'for' loop */
+					defer println(123)
+					break
+				}
+			}()
+		}
+	}
+
+	{
+		for {
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer println(123)
+			break
+		}
+	}
+
+	for {
+		{
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer println(123)
+			break
+		}
+	}
+
+	for range []int{1, 2, 3} {
+		{
+			/*! Possible resource leak, 'defer' is called in the 'for' loop */
+			defer println(123)
+			break
+		}
+	}
+
+	go func() {
+		{
+			for {
+				{
+					/*! Possible resource leak, 'defer' is called in the 'for' loop */
+					defer println(123)
+					break
+				}
+			}
+		}
+	}()
+}