Fix false positives for unnecessaryDefer. #926

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=