Fix false positives for unnecessaryDefer. go-critic#926

By keeping track of the block type we can differentiate between
functions and other blocks. This way we can enure that only blocks
ending with explicit return or end of functions yelds warnings.

checkers/testdata/unnecessaryDefer/negative_tests.go

@@ -35,3 +35,21 @@ func foo_6() int {
 	defer func() {}()
 	return foo_2()
 }
+
+func foo_7() int {
+	if true {
+		defer func() {}()
+	}
+	return 0
+}
+
+func foo_8() int {
+	if true {
+		foo_1()
+		foo_2()
+		foo_3()
+
+		defer func() {}()
+	}
+	return 0
+}

checkers/unnecessaryDefer_checker.go

@@ -31,26 +31,44 @@ func() {
 
 type unnecessaryDeferChecker struct {
 	astwalk.WalkHandler
-	ctx *lintpack.CheckerContext
+	ctx    *lintpack.CheckerContext
+	isFunc bool
+}
+
+// Visit implements the ast.Visitor. This visitor keeps track of the block
+// statement belongs to a function or any other block. If the block is not a
+// function and ends with a defer statement that should be OK since it's
+// defering the outer function.
+func (c *unnecessaryDeferChecker) Visit(node ast.Node) ast.Visitor {
+	switch n := node.(type) {
+	case *ast.FuncDecl, *ast.FuncLit:
+		c.isFunc = true
+	case *ast.BlockStmt:
+		c.checkDeferBeforeReturn(n)
+	default:
+		c.isFunc = false
+	}
+
+	return c
 }
 
 func (c *unnecessaryDeferChecker) VisitFuncDecl(funcDecl *ast.FuncDecl) {
-	ast.Inspect(funcDecl.Body, func(n ast.Node) bool {
-		funDecl, ok := n.(*ast.BlockStmt)
-		if ok {
-			c.checkDeferBeforeReturn(funDecl)
-		}
-		return true
-	})
+	// We always start as a function (*ast.FuncDecl.Body passed)
+	c.isFunc = true
+
+	ast.Walk(c, funcDecl.Body)
 }
 
 func (c *unnecessaryDeferChecker) checkDeferBeforeReturn(funcDecl *ast.BlockStmt) {
+	// Check if we have an explicit return or if it's just the end of the scope.
+	explicitReturn := false
 	retIndex := len(funcDecl.List)
 	for i, stmt := range funcDecl.List {
 		retStmt, ok := stmt.(*ast.ReturnStmt)
 		if !ok {
 			continue
 		}
+		explicitReturn = true
 		if lintutil.ContainsNode(retStmt, astp.IsCallExpr) {
 			continue
 		}
@@ -63,7 +81,12 @@ func (c *unnecessaryDeferChecker) checkDeferBeforeReturn(funcDecl *ast.BlockStmt
 	}
 
 	if deferStmt, ok := funcDecl.List[retIndex-1].(*ast.DeferStmt); ok {
-		c.warn(deferStmt)
+		// If the block is a function and ending with return or if we have an
+		// explicit return in any other block we should warn about
+		// unnecessary defer.
+		if c.isFunc || explicitReturn {
+			c.warn(deferStmt)
+		}
 	}
 }
 

go.sum

@@ -38,11 +38,6 @@ github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c h1:JoUA0uz
 github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c/go.mod h1:5STLWrekHfjyYwxBRVRXNOSewLJ3PWfDJd1VyTS21fI=
 github.com/quasilyte/go-ruleguard v0.1.2-0.20200318202121-b00d7a75d3d8 h1:DvnesvLtRPQOvaUbfXfh0tpMHg29by0H7F2U+QIkSu8=
 github.com/quasilyte/go-ruleguard v0.1.2-0.20200318202121-b00d7a75d3d8/go.mod h1:CGFX09Ci3pq9QZdj86B+VGIdNj4VyCo2iPOGS9esB/k=
-github.com/quasilyte/regex v0.0.0-20200402210348-d7167c1cb489 h1:MaRo2gFtvdQHnxOMUhKxyWAbaRXn7siZJOD4zKrYeQU=
-github.com/quasilyte/regex/syntax v0.0.0-20200402210348-d7167c1cb489 h1:Wg/knSPrrEsisZKcWBk8cGbiroiNk7v6jeHZAcAfzz4=
-github.com/quasilyte/regex/syntax v0.0.0-20200402210348-d7167c1cb489/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0=
-github.com/quasilyte/regex/syntax v0.0.0-20200405164239-f6e043421647 h1:ST0AU0FGuLuKkS3RlulTWjV4pBelyTH7wBLA9/SYAUE=
-github.com/quasilyte/regex/syntax v0.0.0-20200405164239-f6e043421647/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0=
 github.com/quasilyte/regex/syntax v0.0.0-20200407221936-30656e2c4a95 h1:L8QM9bvf68pVdQ3bCFZMDmnt9yqcMBro1pC7F+IPYMY=
 github.com/quasilyte/regex/syntax v0.0.0-20200407221936-30656e2c4a95/go.mod h1:rlzQ04UMyJXu/aOvhd8qT+hvDrFpiwqp8MRXDY9szc0=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=