Skip to content

Commit

Permalink
S1008: don't flag if both branches are commented
Browse files Browse the repository at this point in the history
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
Closes: gh-1556 [via git-merge-pr]
  • Loading branch information
arp242 authored and dominikh committed Jan 23, 2025
1 parent 21df14b commit 5aa6ff2
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 3 deletions.
16 changes: 13 additions & 3 deletions simple/s1008/s1008.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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" {
Expand All @@ -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
}

Expand Down
47 changes: 47 additions & 0 deletions simple/s1008/testdata/go1.0/CheckIfReturn/comment.go
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit 5aa6ff2

Please sign in to comment.