Add sliceofpointers custom linter

Signed-off-by: Glenn Lewis <[email protected]>

.custom-gcl.yml

@@ -0,0 +1,4 @@
+version: v1.57.0
+plugins:
+  - module: "github.com/google/go-github/v68/tools/sliceofpointers"
+    path: ./tools/sliceofpointers/sliceofpointers.so

.golangci.yml

@@ -18,12 +18,17 @@ linters:
     - perfsprint
     - paralleltest
     - revive
+    - sliceofpointers
     - stylecheck
     - tparallel
     - unconvert
     - unparam
     - whitespace
 linters-settings:
+  custom:
+    sliceofpointers:
+      type: "module"
+      description: Reports usage of []*string and slices of structs without pointers.
   gocritic:
     disable-all: true
     enabled-checks:
@@ -38,7 +43,7 @@ linters-settings:
   misspell:
     locale: US
     ignore-words:
-      - "analyses"  # returned by the GitHub API
+      - "analyses" # returned by the GitHub API
       - "cancelled" # returned by the GitHub API
     # extra words from https://go.dev//wiki/Spelling
     extra-words:
@@ -102,28 +107,28 @@ issues:
       path: _test\.go
 
     # We need to pass nil Context in order to test DoBare erroring on nil ctx.
-    - linters: [ staticcheck ]
-      text: 'SA1012: do not pass a nil Context'
+    - linters: [staticcheck]
+      text: "SA1012: do not pass a nil Context"
       path: _test\.go
 
     # We need to use sha1 for validating signatures
-    - linters: [ gosec ]
-      text: 'G505: Blocklisted import crypto/sha1: weak cryptographic primitive'
+    - linters: [gosec]
+      text: "G505: Blocklisted import crypto/sha1: weak cryptographic primitive"
 
     # This is adapted from golangci-lint's default exclusions. It disables linting for error checks on
     # os.RemoveAll, fmt.Fprint*, fmt.Scanf, and any function ending in "Close".
-    - linters: [ errcheck ]
+    - linters: [errcheck]
       text: Error return value of .(.*Close|fmt\.Fprint.*|fmt\.Scanf|os\.Remove(All)?). is not checked
 
     # We don't care about file inclusion via variable in examples or internal tools.
-    - linters: [ gosec ]
-      text: 'G304: Potential file inclusion via variable'
+    - linters: [gosec]
+      text: "G304: Potential file inclusion via variable"
       path: '^(example|tools)\/'
 
     # We don't run parallel integration tests
-    - linters: [ paralleltest, tparallel ]
-      path: '^test/integration'
+    - linters: [paralleltest, tparallel]
+      path: "^test/integration"
 
     # Because fmt.Sprint(reset.Unix())) is more readable than strconv.FormatInt(reset.Unix(), 10).
-    - linters: [ perfsprint]
-      text: 'fmt.Sprint.* can be replaced with faster strconv.FormatInt'
+    - linters: [perfsprint]
+      text: "fmt.Sprint.* can be replaced with faster strconv.FormatInt"

tools/sliceofpointers/main.go

@@ -0,0 +1,54 @@
+// sliceofpointers is a custom linter to be used by
+// golangci-lint to find instances of `[]*string` and
+// slices of structs without pointers and report them.
+// See: https://github.com/google/go-github/issues/180
+package main
+
+import (
+	"go/ast"
+	"go/token"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+var analyzer = &analysis.Analyzer{
+	Name: "sliceofpointers",
+	Doc:  "reports usage of []*string and slices of structs without pointers",
+	Run:  run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	for _, file := range pass.Files {
+		ast.Inspect(file, func(n ast.Node) bool {
+			if n == nil {
+				return false
+			}
+
+			switch t := n.(type) {
+			case *ast.ArrayType:
+				checkArrayType(t, t.Pos(), pass)
+			}
+
+			return true
+		})
+	}
+	return nil, nil
+}
+
+func checkArrayType(arrType *ast.ArrayType, tokenPos token.Pos, pass *analysis.Pass) {
+	if starExpr, ok := arrType.Elt.(*ast.StarExpr); ok {
+		if ident, ok := starExpr.X.(*ast.Ident); ok && ident.Name == "string" {
+			const msg = "use []string instead of []*string"
+			pass.Reportf(tokenPos, msg)
+		}
+	} else if ident, ok := arrType.Elt.(*ast.Ident); ok && ident.Obj != nil {
+		if _, ok := ident.Obj.Decl.(*ast.TypeSpec).Type.(*ast.StructType); ok {
+			pass.Reportf(tokenPos, "use []*%v instead of []%[1]v", ident.Name)
+		}
+	}
+}
+
+func main() {
+	singlechecker.Main(analyzer)
+}

tools/no-pointer-to-string-slice/main_test.go → tools/sliceofpointers/main_test.go

@@ -8,5 +8,5 @@ import (
 
 func TestRun(t *testing.T) {
 	testdata := analysistest.TestData()
-	analysistest.Run(t, testdata, Analyzer, "a", "b", "c")
+	analysistest.Run(t, testdata, analyzer, "has-warnings", "no-warnings")
 }

tools/sliceofpointers/testdata/src/has-warnings/main.go

@@ -0,0 +1,16 @@
+package main
+
+type Example struct {
+	Strings []*string `json:"strings,omitempty"` // want `use \[\]string instead of \[\]\*string`
+	Things  []Thing   `json:"things,omitempty"`  // want `use \[\]\*Thing instead of \[\]Thing`
+}
+
+type Thing struct {
+}
+
+func main() {
+	slice1 := []*string{} // want `use \[\]string instead of \[\]\*string`
+	_ = slice1
+	slice2 := []Thing{} // want `use \[\]\*Thing instead of \[\]Thing`
+	_ = slice2
+}

tools/sliceofpointers/testdata/src/no-warnings/main.go

@@ -0,0 +1,16 @@
+package main
+
+type Example struct {
+	Strings []string `json:"strings,omitempty"` // Should not be flagged
+	Things  []*Thing `json:"things,omitempty"`  // Should not be flagged
+}
+
+type Thing struct {
+}
+
+func main() {
+	slice1 := []string{} // Should not be flagged
+	_ = slice1
+	slice2 := []*Thing{} // Should not be flagged
+	_ = slice2
+}