Repository: go-critic/go-critic
Pull request #1423 (2024-07-12 10:41:23)

checkers: add rangeAppendAll
This is a bug I discovered during work.

append all its data while range over it is generally not the intended behavior and can lead to duplicate data.

```go
for _, n := range ns {
    ...
    rs = append(rs, n)     // OK
    rs = append(rs, ns...) // Bug
    ...
}
```

I implemented the checker firstly in my repo and use the [go-linter-runner](https://github.com/alingse/go-linter-runner) github  action to scan top 10k Go repos.  This found real bugs, documented in https://github.com/alingse/sundrylint/issues/1 




Patch:
diff --git a/.golangci.yml b/.golangci.yml
index 776019e..8293aa5 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -1,21 +1,19 @@
 {
   "run": {
     # timeout for analysis, e.g. 30s, 5m, default is 1m
-    "deadline": "3m",
-    "skip-dirs": ["checkers/rules"],
+    "timeout": "3m",
   },
-  "fast": false,
   "linters": {
+    "fast": false,
     "enable": [
       "errcheck",
-      "gas",
+      "gosec",
       "gocritic",
       "gofmt",
       "goimports",
       "gosimple",
       "govet",
       "ineffassign",
-      "megacheck",
       "misspell",
       "nakedret",
       "revive",
@@ -24,17 +22,14 @@
       "unparam",
       "unused",
     ],
+    "disable": [
+      "depguard",
+      "dupl",
+      "gocyclo",
+      "lll",
+      "prealloc",
+    ],
   },
-  "disable": [
-    "depguard",
-    "dupl",
-    "gocyclo",
-    "interfacer",
-    "lll",
-    "maligned",
-    "prealloc",
-    "typecheck",
-  ],
   "linters-settings": {
     "gocritic": {
       "enabled-checks": [
@@ -46,5 +41,11 @@
         },
       },
     },
+    "gosec": {
+      "excludes": ["G115"]
+    }
   },
+  "issues": {
+    "exclude-dirs": ["checkers/rules"],
+  }
 }
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 7a14f65..ef97211 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -35,7 +35,7 @@ a typo or missing doc comment. You can also filter issues by using
 
 **s/foo/bar/g** — replace foo with bar throughout your entire change
 
-See [CodeReview](https://github.com/golang/go/wiki/CodeReview).
+See [CodeReview](https://go.dev/wiki/CodeReview).
 
 ### Code review: main rules
 
diff --git a/Makefile b/Makefile
index 0accec8..a278f48 100644
--- a/Makefile
+++ b/Makefile
@@ -40,9 +40,8 @@ ci-generate:
 	@git diff --exit-code --quiet || (echo "Please run 'go generate ./...' to update precompiled rules."; false)
 
 ci-linter:
-	# TODO(cristaloleg): enable back
-	# @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.54.0
-	# @$(GOPATH_DIR)/bin/golangci-lint run
+	@curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(GOPATH_DIR)/bin v1.60.2
+	@$(GOPATH_DIR)/bin/golangci-lint run
 	# cd tools && go install github.com/quasilyte/go-consistent
 	# @$(GOPATH_DIR)/bin/go-consistent ./...
 	go build -o gocritic ./cmd/gocritic
diff --git a/README.md b/README.md
index ee99ef2..d5e4795 100644
--- a/README.md
+++ b/README.md
@@ -121,7 +121,7 @@ We're using an optimistic merging strategy most of the time.
 In short, this means that if your contribution has some flaws, we can still merge it and then
 fix them by ourselves. Experimental and work-in-progress checkers are isolated, so nothing bad will happen.
 
-Code style is the same as in Go project, see [CodeReviewComments](https://github.com/golang/go/wiki/codereviewcomments).
+Code style is the same as in Go project, see [CodeReviewComments](https://go.dev/wiki/CodeReviewComments).
 
 See [CONTRIBUTING.md](CONTRIBUTING.md) for more details.
 It also describes how to develop a new checker for the linter.
diff --git a/checkers/caseOrder_checker.go b/checkers/caseOrder_checker.go
index 3067568..345274f 100644
--- a/checkers/caseOrder_checker.go
+++ b/checkers/caseOrder_checker.go
@@ -82,7 +82,7 @@ func (c *caseOrderChecker) warnUnknownType(cause, concrete ast.Node) {
 	c.ctx.Warn(cause, "type is not defined %s", concrete)
 }
 
-func (c *caseOrderChecker) checkSwitch(s *ast.SwitchStmt) {
+func (c *caseOrderChecker) checkSwitch(_ *ast.SwitchStmt) {
 	// TODO(quasilyte): can handle expression cases that overlap.
 	// Cases that have narrower value range should go before wider ones.
 }
diff --git a/checkers/internal/astwalk/walk_handler.go b/checkers/internal/astwalk/walk_handler.go
index 1f6e948..96d2dd0 100644
--- a/checkers/internal/astwalk/walk_handler.go
+++ b/checkers/internal/astwalk/walk_handler.go
@@ -17,7 +17,7 @@ type WalkHandler struct {
 
 // EnterFile is a default walkerEvents.EnterFile implementation
 // that reports every file as accepted candidate for checking.
-func (w *WalkHandler) EnterFile(f *ast.File) bool {
+func (w *WalkHandler) EnterFile(_ *ast.File) bool {
 	return true
 }
 
diff --git a/checkers/rangeAppendAll_checker.go b/checkers/rangeAppendAll_checker.go
new file mode 100644
index 0000000..f4851d4
--- /dev/null
+++ b/checkers/rangeAppendAll_checker.go
@@ -0,0 +1,100 @@
+package checkers
+
+import (
+	"go/ast"
+	"go/token"
+
+	"github.com/go-critic/go-critic/checkers/internal/astwalk"
+	"github.com/go-critic/go-critic/linter"
+	"github.com/go-toolsmith/astcast"
+	"golang.org/x/tools/go/ast/astutil"
+)
+
+func init() {
+	var info linter.CheckerInfo
+	info.Name = "rangeAppendAll"
+	info.Tags = []string{linter.DiagnosticTag, linter.ExperimentalTag}
+	info.Summary = "Detects append all its data while range it"
+	info.Before = `for _, n := range ns {
+	...
+		rs = append(rs, ns...) // append all slice data
+	}
+}`
+	info.After = `for _, n := range ns {
+	...
+		rs = append(rs, n)
+	}
+}`
+
+	collection.AddChecker(&info, func(ctx *linter.CheckerContext) (linter.FileWalker, error) {
+		c := &rangeAppendAllChecker{ctx: ctx}
+		return astwalk.WalkerForStmt(c), nil
+	})
+}
+
+type rangeAppendAllChecker struct {
+	astwalk.WalkHandler
+	ctx *linter.CheckerContext
+}
+
+func (c *rangeAppendAllChecker) VisitStmt(stmt ast.Stmt) {
+	rangeStmt, ok := stmt.(*ast.RangeStmt)
+	if !ok || len(rangeStmt.Body.List) == 0 {
+		return
+	}
+	rangeIdent, ok := rangeStmt.X.(*ast.Ident)
+	if !ok {
+		return
+	}
+	rangeObj := c.ctx.TypesInfo.ObjectOf(rangeIdent)
+
+	astutil.Apply(rangeStmt.Body, nil, func(cur *astutil.Cursor) bool {
+		appendFrom := c.getValidAppendFrom(cur.Node())
+		if appendFrom != nil {
+			appendFromObj := c.ctx.TypesInfo.ObjectOf(appendFrom)
+			if appendFromObj == rangeObj {
+				c.warn(appendFrom)
+			}
+		}
+		return true
+	})
+}
+
+func (c *rangeAppendAllChecker) getValidAppendFrom(expr ast.Node) *ast.Ident {
+	call := astcast.ToCallExpr(expr)
+	if len(call.Args) != 2 || call.Ellipsis == token.NoPos {
+		return nil
+	}
+	if qualifiedName(call.Fun) != "append" {
+		return nil
+	}
+	if c.isSliceLiteral(call.Args[0]) {
+		return nil
+	}
+	appendFrom, ok := call.Args[1].(*ast.Ident)
+	if !ok {
+		return nil
+	}
+	return appendFrom
+}
+
+func (c *rangeAppendAllChecker) isSliceLiteral(arg ast.Expr) bool {
+	switch v := arg.(type) {
+	// []T{}, []T{n}
+	case *ast.CompositeLit:
+		return true
+	// []T(nil)
+	case *ast.CallExpr:
+		if astcast.ToArrayType(v.Fun) != astcast.NilArrayType && len(v.Args) == 1 {
+			id := astcast.ToIdent(v.Args[0])
+			return id.Name == "nil" && id.Obj == nil
+		}
+		return false
+	default:
+		return false
+	}
+}
+
+func (c *rangeAppendAllChecker) warn(appendFrom *ast.Ident) {
+	c.ctx.Warn(appendFrom, "append all `%s` data while range it", appendFrom)
+}
diff --git a/checkers/ruleguard_checker.go b/checkers/ruleguard_checker.go
index 29723a6..4858198 100644
--- a/checkers/ruleguard_checker.go
+++ b/checkers/ruleguard_checker.go
@@ -87,7 +87,7 @@ func newErrorHandler(failOnErrorFlag string) (*parseErrorHandler, error) {
 	failOnErrorPredicates := map[string]func(error) bool{
 		"dsl":    func(err error) bool { var e *ruleguard.ImportError; return !errors.As(err, &e) },
 		"import": func(err error) bool { var e *ruleguard.ImportError; return errors.As(err, &e) },
-		"all":    func(err error) bool { return true },
+		"all":    func(_ error) bool { return true },
 	}
 	for _, k := range strings.Split(failOnErrorFlag, ",") {
 		if k == "" {
diff --git a/checkers/rules/precompile.go b/checkers/rules/precompile.go
index c40a24f..b68ca28 100644
--- a/checkers/rules/precompile.go
+++ b/checkers/rules/precompile.go
@@ -1,5 +1,4 @@
 //go:build generate
-// +build generate
 
 package main
 
diff --git a/cmd/gocritic/check.go b/cmd/gocritic/check.go
index b804398..ba22d2b 100644
--- a/cmd/gocritic/check.go
+++ b/cmd/gocritic/check.go
@@ -162,11 +162,10 @@ func (p *program) checkFile(f *ast.File) {
 				if err, ok := r.(error); ok {
 					log.Printf("%s: error: %v\n", c.Info.Name, err)
 					panic(err)
-				} else {
-					// Some other kind of run-time panic.
-					// Undo the recover and resume panic.
-					panic(r)
 				}
+				// Some other kind of run-time panic.
+				// Undo the recover and resume panic.
+				panic(r)
 			}()
 
 			warnings[i] = append(warnings[i], c.Check(f)...)
diff --git a/cmd/makedocs/main.go b/cmd/makedocs/main.go
index 2a8f27c..880f69a 100644
--- a/cmd/makedocs/main.go
+++ b/cmd/makedocs/main.go
@@ -35,7 +35,7 @@ func main() {
 	if err != nil {
 		log.Fatalf("render template: %v", err)
 	}
-	
+
 	// A bit hacky but works, see https://github.com/go-critic/go-critic/pull/1403.
 	buff := bytes.ReplaceAll(buf.Bytes(), []byte("<all>"), []byte("&lt;all&gt;"))
 
diff --git a/go.mod b/go.mod
index f522686..e942895 100644
--- a/go.mod
+++ b/go.mod
@@ -16,13 +16,13 @@ require (
 	github.com/quasilyte/go-ruleguard v0.4.2
 	github.com/quasilyte/go-ruleguard/dsl v0.3.22
 	github.com/quasilyte/regex/syntax v0.0.0-20210819130434-b3f0c404a727
-	golang.org/x/tools v0.22.0
+	golang.org/x/tools v0.23.0
 )
 
 require (
 	github.com/quasilyte/gogrep v0.5.0 // indirect
 	github.com/quasilyte/stdinfo v0.0.0-20220114132959-f7386bf02567 // indirect
 	golang.org/x/exp/typeparams v0.0.0-20240213143201-ec583247a57a // indirect
-	golang.org/x/mod v0.18.0 // indirect
+	golang.org/x/mod v0.19.0 // indirect
 	golang.org/x/sync v0.7.0 // indirect
 )
diff --git a/go.sum b/go.sum
index c4b96fe..3e54759 100644
--- a/go.sum
+++ b/go.sum
@@ -36,9 +36,9 @@ golang.org/x/exp/typeparams v0.0.0-20220428152302-39d4317da171/go.mod h1:AbB0pIl
 golang.org/x/exp/typeparams v0.0.0-20230203172020-98cc5a0785f9/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk=
 golang.org/x/exp/typeparams v0.0.0-20240213143201-ec583247a57a h1:rrd/FiSCWtI24jk057yBSfEfHrzzjXva1VkDNWRXMag=
 golang.org/x/exp/typeparams v0.0.0-20240213143201-ec583247a57a/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk=
-golang.org/x/mod v0.18.0 h1:5+9lSbEzPSdWkH32vYPBwEpX8KwDbM52Ud9xBUvNlb0=
-golang.org/x/mod v0.18.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
+golang.org/x/mod v0.19.0 h1:fEdghXQSo20giMthA7cd28ZC+jts4amQ3YMXiP5oMQ8=
+golang.org/x/mod v0.19.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
 golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
 golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
-golang.org/x/tools v0.22.0 h1:gqSGLZqv+AI9lIQzniJ0nZDRG5GBPsSi+DRNHWNz6yA=
-golang.org/x/tools v0.22.0/go.mod h1:aCwcsjqvq7Yqt6TNyX7QMU2enbQ/Gt0bo6krSeEri+c=
+golang.org/x/tools v0.23.0 h1:SGsXPZ+2l4JsgaCKkx+FQ9YZ5XEtA1GZYuoDjenLjvg=
+golang.org/x/tools v0.23.0/go.mod h1:pnu6ufv6vQkll6szChhK3C3L/ruaIv5eBeztNG8wtsI=
diff --git a/linter/helpers.go b/linter/helpers.go
index 0a3fc02..d5110df 100644
--- a/linter/helpers.go
+++ b/linter/helpers.go
@@ -116,7 +116,7 @@ func validateCheckerName(info *CheckerInfo) error {
 	return nil
 }
 
-func validateCheckerDocumentation(info *CheckerInfo) error {
+func validateCheckerDocumentation(_ *CheckerInfo) error {
 	// TODO(quasilyte): validate documentation.
 	return nil
 }
diff --git a/tools/tools.go b/tools/tools.go
index a68db79..4644b17 100644
--- a/tools/tools.go
+++ b/tools/tools.go
@@ -1,9 +1,8 @@
 //go:build tools
-// +build tools
 
 package critic
 
-// See https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module.
+// See https://go.dev/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module.
 
 import (
 	// Used for CI linting.


Test patch:
diff --git a/checkers/testdata/_integration/ruleguard-glob/rules1.go b/checkers/testdata/_integration/ruleguard-glob/rules1.go
index 51d60e2..448c3f0 100644
--- a/checkers/testdata/_integration/ruleguard-glob/rules1.go
+++ b/checkers/testdata/_integration/ruleguard-glob/rules1.go
@@ -1,4 +1,4 @@
-// +build ignore
+//go:build ignore
 
 package gorules
 
diff --git a/checkers/testdata/_integration/ruleguard-glob/rules2.go b/checkers/testdata/_integration/ruleguard-glob/rules2.go
index ea126c4..57fb1f8 100644
--- a/checkers/testdata/_integration/ruleguard-glob/rules2.go
+++ b/checkers/testdata/_integration/ruleguard-glob/rules2.go
@@ -1,4 +1,4 @@
-// +build ignore
+//go:build ignore
 
 package gorules
 
diff --git a/checkers/testdata/_integration/ruleguard-multi/rules1.go b/checkers/testdata/_integration/ruleguard-multi/rules1.go
index 51d60e2..448c3f0 100644
--- a/checkers/testdata/_integration/ruleguard-multi/rules1.go
+++ b/checkers/testdata/_integration/ruleguard-multi/rules1.go
@@ -1,4 +1,4 @@
-// +build ignore
+//go:build ignore
 
 package gorules
 
diff --git a/checkers/testdata/_integration/ruleguard-multi/rules2.go b/checkers/testdata/_integration/ruleguard-multi/rules2.go
index ea126c4..57fb1f8 100644
--- a/checkers/testdata/_integration/ruleguard-multi/rules2.go
+++ b/checkers/testdata/_integration/ruleguard-multi/rules2.go
@@ -1,4 +1,4 @@
-// +build ignore
+//go:build ignore
 
 package gorules
 
diff --git a/checkers/testdata/_integration/ruleguard/rules.go b/checkers/testdata/_integration/ruleguard/rules.go
index 47022e6..da6c24a 100644
--- a/checkers/testdata/_integration/ruleguard/rules.go
+++ b/checkers/testdata/_integration/ruleguard/rules.go
@@ -1,5 +1,4 @@
 //go:build ignore
-// +build ignore
 
 package gorules
 
diff --git a/checkers/testdata/hugeParam/negative_tests_go118.go b/checkers/testdata/hugeParam/negative_tests_go118.go
index 9114a0f..7f6b090 100644
--- a/checkers/testdata/hugeParam/negative_tests_go118.go
+++ b/checkers/testdata/hugeParam/negative_tests_go118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test
 
diff --git a/checkers/testdata/newDeref/negative_tests118.go b/checkers/testdata/newDeref/negative_tests118.go
index b81ca96..2988fa8 100644
--- a/checkers/testdata/newDeref/negative_tests118.go
+++ b/checkers/testdata/newDeref/negative_tests118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test
 
diff --git a/checkers/testdata/paramTypeCombine/negative_tests_go118.go b/checkers/testdata/paramTypeCombine/negative_tests_go118.go
index 1cfc493..0f3f9fa 100644
--- a/checkers/testdata/paramTypeCombine/negative_tests_go118.go
+++ b/checkers/testdata/paramTypeCombine/negative_tests_go118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test
 
diff --git a/checkers/testdata/paramTypeCombine/positive_tests_go118.go b/checkers/testdata/paramTypeCombine/positive_tests_go118.go
index 35e771a..026db08 100644
--- a/checkers/testdata/paramTypeCombine/positive_tests_go118.go
+++ b/checkers/testdata/paramTypeCombine/positive_tests_go118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test
 
diff --git a/checkers/testdata/rangeAppendAll/negative_tests.go b/checkers/testdata/rangeAppendAll/negative_tests.go
new file mode 100644
index 0000000..1fc14d5
--- /dev/null
+++ b/checkers/testdata/rangeAppendAll/negative_tests.go
@@ -0,0 +1,18 @@
+package checker_test
+
+func collectWithNil(ns []string, k int) bool {
+	rs := make([]string, 0)
+	rs2 := make([]string, 0)
+	rs3 := make([]string, 0)
+	for _, n := range ns {
+		if len(n) > k {
+			rs = append([]string{n}, ns...)
+			rs2 = append([]string{}, ns...)
+			rs3 = append([]string(nil), ns...)
+		}
+	}
+	if len(rs) > 0 || len(rs2) > 0 || len(rs3) > 0 {
+		return true
+	}
+	return false
+}
diff --git a/checkers/testdata/rangeAppendAll/positive_tests.go b/checkers/testdata/rangeAppendAll/positive_tests.go
new file mode 100644
index 0000000..030b34b
--- /dev/null
+++ b/checkers/testdata/rangeAppendAll/positive_tests.go
@@ -0,0 +1,49 @@
+package checker_test
+
+func collectBigger(ns []int, k int) []int {
+	rs := make([]int, 0)
+	for _, n := range ns {
+		if n > k {
+			/*! append all `ns` data while range it */
+			rs = append(rs, ns...)
+		}
+	}
+	return rs
+}
+
+func collectLong(ns []string, k int) []string {
+	rs := make([]string, 0)
+	for _, n := range ns {
+		if len(n) > k {
+			/*! append all `ns` data while range it */
+			rs = append(rs, ns...)
+		}
+	}
+	return rs
+}
+
+func someFuncCall(n int) {
+}
+
+func collectBasic(ns []int, k int) []int {
+	rs := make([]int, 0)
+	for _, n := range ns {
+		someFuncCall(n)
+		/*! append all `ns` data while range it */
+		rs = append(rs, ns...)
+	}
+	return rs
+}
+
+func collectLongCorrect(ns []string, k int) []string {
+	rs := make([]string, 0)
+	n := ns[0]
+	rs = append(rs, n)
+	for _, n := range ns {
+		if len(n) > k {
+			rs = append(rs, n)
+		}
+	}
+	rs = append(rs, ns...)
+	return rs
+}
diff --git a/checkers/testdata/typeDefFirst/negative_tests_go118.go b/checkers/testdata/typeDefFirst/negative_tests_go118.go
index 2e29104..78be006 100644
--- a/checkers/testdata/typeDefFirst/negative_tests_go118.go
+++ b/checkers/testdata/typeDefFirst/negative_tests_go118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test
 
diff --git a/checkers/testdata/typeDefFirst/positive_tests_go118.go b/checkers/testdata/typeDefFirst/positive_tests_go118.go
index 408fec3..5281045 100644
--- a/checkers/testdata/typeDefFirst/positive_tests_go118.go
+++ b/checkers/testdata/typeDefFirst/positive_tests_go118.go
@@ -1,5 +1,4 @@
 //go:build go1.18
-// +build go1.18
 
 package checker_test