Repository: mgechev/revive (go)

Issue:
defer FP on return statement in a function literal passed to the deferred function
**Describe the bug**
The `defer` check misfires on a `return` statement inside a function literal passed to the deferred function.

**To Reproduce**
Steps to reproduce the behavior:
1. I updated revive `go install github.com/mgechev/revive@latest`
2. I run it with the following flags & configuration file:

```shell
revive -config revive.toml test.go
```

revive.toml:
```toml
[rule.defer]
```

test.go:
```go
package test

func verify(fn func() error) {
	if err := fn(); err != nil {
		panic(err)
	}
}

func f() {
	defer verify(func() error {
		return nil
	})
}
```

Here, `verify` executes the given function and verifies that it has succeeded. We use this pattern in our project, and I don't see anything wrong with it.

Although `return` has no effect in a deferred function, this one is not in the deferrede function; it's in a function that's passed as an argument to the deferred function.

**Expected behavior**
No `defer` issue is reported.

**Logs**
```
test.go:11:3: return in a defer function has no effect
```

**Desktop (please complete the following information):**
 - Ubuntu 23.04
 - `go version go1.20.6 linux/amd64`


Pull request:
fix #863:false positive on return statement in a func lit passed to t…
Closes #863 by skipping analysis of function literals passed as arguments of deferred function calls


Patch:
diff --git a/rule/defer.go b/rule/defer.go
index f8224fd..f3ea179 100644
--- a/rule/defer.go
+++ b/rule/defer.go
@@ -97,18 +97,21 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
 			w.newFailure("return in a defer function has no effect", n, 1.0, "logic", "return")
 		}
 	case *ast.CallExpr:
-		if !w.inADefer && isIdent(n.Fun, "recover") {
+		isCallToRecover := isIdent(n.Fun, "recover")
+		switch {
+		case !w.inADefer && isCallToRecover:
 			// func fn() { recover() }
 			//
 			// confidence is not 1 because recover can be in a function that is deferred elsewhere
 			w.newFailure("recover must be called inside a deferred function", n, 0.8, "logic", "recover")
-		} else if w.inADefer && !w.inAFuncLit && isIdent(n.Fun, "recover") {
+		case w.inADefer && !w.inAFuncLit && isCallToRecover:
 			// defer helper(recover())
 			//
 			// confidence is not truly 1 because this could be in a correctly-deferred func,
 			// but it is very likely to be a misunderstanding of defer's behavior around arguments.
 			w.newFailure("recover must be called inside a deferred function, this is executing recover immediately", n, 1, "logic", "immediate-recover")
 		}
+
 	case *ast.DeferStmt:
 		if isIdent(n.Call.Fun, "recover") {
 			// defer recover()
@@ -119,7 +122,12 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
 		}
 		w.visitSubtree(n.Call.Fun, true, false, false)
 		for _, a := range n.Call.Args {
-			w.visitSubtree(a, true, false, false) // check arguments, they should not contain recover()
+			switch a.(type) {
+			case *ast.FuncLit:
+				continue // too hard to analyze deferred calls with func literals args
+			default:
+				w.visitSubtree(a, true, false, false) // check arguments, they should not contain recover()
+			}
 		}
 
 		if w.inALoop {
@@ -136,6 +144,7 @@ func (w lintDeferRule) Visit(node ast.Node) ast.Visitor {
 					w.newFailure("be careful when deferring calls to methods without pointer receiver", fn, 0.8, "bad practice", "method-call")
 				}
 			}
+
 		}
 		return nil
 	}
diff --git a/rule/use-any.go b/rule/use-any.go
index bdf3c93..9c6d5cc 100644
--- a/rule/use-any.go
+++ b/rule/use-any.go
@@ -1,9 +1,8 @@
 package rule
 
 import (
-	"go/ast"
-
 	"github.com/mgechev/revive/lint"
+	"go/ast"
 )
 
 // UseAnyRule lints given else constructs.
@@ -14,6 +13,7 @@ func (*UseAnyRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
 
 	walker := lintUseAny{
+		commentPositions: getCommentsPositions(file.AST.Comments),
 		onFailure: func(failure lint.Failure) {
 			failures = append(failures, failure)
 		},
@@ -30,7 +30,8 @@ func (*UseAnyRule) Name() string {
 }
 
 type lintUseAny struct {
-	onFailure func(lint.Failure)
+	commentPositions []int
+	onFailure        func(lint.Failure)
 }
 
 func (w lintUseAny) Visit(n ast.Node) ast.Visitor {
@@ -40,7 +41,13 @@ func (w lintUseAny) Visit(n ast.Node) ast.Visitor {
 	}
 
 	if len(it.Methods.List) != 0 {
-		return w // it is not and empty interface
+		return w // it is not an empty interface
+	}
+
+	for _, pos := range w.commentPositions {
+		if pos > int(it.Pos()) && pos < int(it.End()) {
+			return w // it is a comment inside the interface
+		}
 	}
 
 	w.onFailure(lint.Failure{
@@ -52,3 +59,14 @@ func (w lintUseAny) Visit(n ast.Node) ast.Visitor {
 
 	return w
 }
+
+func getCommentsPositions(commentGroups []*ast.CommentGroup) []int {
+	result := []int{}
+	for _, commentGroup := range commentGroups {
+		for _, comment := range commentGroup.List {
+			result = append(result, int(comment.Pos()))
+		}
+	}
+
+	return result
+}
diff --git a/rule/var-naming.go b/rule/var-naming.go
index c6a6305..286ff9d 100644
--- a/rule/var-naming.go
+++ b/rule/var-naming.go
@@ -141,15 +141,6 @@ func (w *lintNames) check(id *ast.Ident, thing string) {
 		})
 		return
 	}
-	if len(id.Name) > 2 && id.Name[0] == 'k' && id.Name[1] >= 'A' && id.Name[1] <= 'Z' {
-		should := string(id.Name[1]+'a'-'A') + id.Name[2:]
-		w.onFailure(lint.Failure{
-			Failure:    fmt.Sprintf("don't use leading k in Go names; %s %s should be %s", thing, id.Name, should),
-			Confidence: 0.8,
-			Node:       id,
-			Category:   "naming",
-		})
-	}
 
 	should := lint.Name(id.Name, w.whitelist, w.blacklist)
 	if id.Name == should {


Test patch:
diff --git a/testdata/defer.go b/testdata/defer.go
index a32b0cc..a035cfe 100644
--- a/testdata/defer.go
+++ b/testdata/defer.go
@@ -33,3 +33,17 @@ func deferrer() {
 	// does not work, but not currently blocked.
 	defer helper(func() { recover() })
 }
+
+// Issue #863
+
+func verify(fn func() error) {
+	if err := fn(); err != nil {
+		panic(err)
+	}
+}
+
+func f() {
+	defer verify(func() error {
+		return nil
+	})
+}
diff --git a/testdata/golint/var-naming.go b/testdata/golint/var-naming.go
index f4e1fbc..4d26acd 100644
--- a/testdata/golint/var-naming.go
+++ b/testdata/golint/var-naming.go
@@ -57,8 +57,7 @@ func f_it() { // MATCH /don't use underscores in Go names; func f_it should be f
 
 // Common styles in other languages that don't belong in Go.
 const (
-	CPP_CONST   = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/
-	kLeadingKay = 2 // MATCH /don't use leading k in Go names; const kLeadingKay should be leadingKay/
+	CPP_CONST = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/
 
 	HTML  = 3 // okay; no underscore
 	X509B = 4 // ditto
diff --git a/testdata/use-any.go b/testdata/use-any.go
index bc6e647..a624d5e 100644
--- a/testdata/use-any.go
+++ b/testdata/use-any.go
@@ -17,6 +17,10 @@ func any2(a int) interface{} {} // MATCH /since GO 1.18 'interface{}' can be rep
 
 var ni interface{ Close() }
 
+var ni interface {
+	// Close()
+}
+
 type nt interface{ Close() }
 type na = interface{ Close() }