From 66e50454d5ac8f2b529ab2622ceb245f59e3450f Mon Sep 17 00:00:00 2001
From: Martin Tournoij <martin@arp242.net>
Date: Sun, 2 Jun 2024 00:15:44 +0100
Subject: [PATCH] S1008: don't flag if both branches are commented

Only skip if both branches are documented; in cases where just one is
commented it can just be:

	// Comment it.
	return [..]

Closes: gh-704
Closes: gh-1488
---
 simple/s1008/s1008.go                         | 16 +++++--
 .../testdata/go1.0/CheckIfReturn/comment.go   | 47 +++++++++++++++++++
 2 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 simple/s1008/testdata/go1.0/CheckIfReturn/comment.go

diff --git a/simple/s1008/s1008.go b/simple/s1008/s1008.go
index 8f850de72..90aacdd68 100644
--- a/simple/s1008/s1008.go
+++ b/simple/s1008/s1008.go
@@ -42,8 +42,14 @@ var (
 	checkIfReturnQRet = pattern.MustParse(`(ReturnStmt [ret@(Builtin (Or "true" "false"))])`)
 )
 
-func run(pass *analysis.Pass) (interface{}, error) {
+func run(pass *analysis.Pass) (any, error) {
+	var cm ast.CommentMap
 	fn := func(node ast.Node) {
+		if f, ok := node.(*ast.File); ok {
+			cm = ast.NewCommentMap(pass.Fset, f, f.Comments)
+			return
+		}
+
 		block := node.(*ast.BlockStmt)
 		l := len(block.List)
 		if l < 2 {
@@ -76,13 +82,17 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 		ret1 := m1.State["ret"].(*ast.Ident)
 		ret2 := m2.State["ret"].(*ast.Ident)
-
 		if ret1.Name == ret2.Name {
 			// we want the function to return true and false, not the
 			// same value both times.
 			return
 		}
 
+		// Don't flag if both are commented.
+		if len(cm.Filter(n1).Comments()) > 0 && len(cm.Filter(n2).Comments()) > 0 {
+			return
+		}
+
 		cond := m1.State["cond"].(ast.Expr)
 		origCond := cond
 		if ret1.Name == "false" {
@@ -94,7 +104,7 @@ func run(pass *analysis.Pass) (interface{}, error) {
 				report.Render(pass, origCond), report.Render(pass, ret1), report.Render(pass, ret2)),
 			report.FilterGenerated())
 	}
-	code.Preorder(pass, fn, (*ast.BlockStmt)(nil))
+	code.Preorder(pass, fn, (*ast.File)(nil), (*ast.BlockStmt)(nil))
 	return nil, nil
 }
 
diff --git a/simple/s1008/testdata/go1.0/CheckIfReturn/comment.go b/simple/s1008/testdata/go1.0/CheckIfReturn/comment.go
new file mode 100644
index 000000000..b795a9c4d
--- /dev/null
+++ b/simple/s1008/testdata/go1.0/CheckIfReturn/comment.go
@@ -0,0 +1,47 @@
+package pkg
+
+func cmt1(x string) bool {
+	// A
+	if len(x) > 0 {
+		return false
+	}
+	// B
+	return true
+}
+
+func cmt2(x string) bool {
+	if len(x) > 0 { // A
+		return false
+	}
+	return true // B
+}
+
+func cmt3(x string) bool {
+	if len(x) > 0 {
+		return false // A
+	}
+	return true // B
+}
+
+func cmt4(x string) bool {
+	if len(x) > 0 {
+		return false // A
+	}
+	return true
+	// B
+}
+
+// Hard to test, as the diag line adds a comment.
+//func cmt5(x string) bool {
+//	if len(x) > 0 { // diag(`should use 'return len(x) == 0'`)
+//		return false
+//	}
+//	return true // A
+//}
+
+func cmt6(x string) bool {
+	if len(x) > 0 { //@diag(`should use 'return len(x) == 0'`)
+		return false // A
+	}
+	return true
+}