diff --git a/.custom-gcl.yml b/.custom-gcl.yml new file mode 100644 index 00000000000..d3bf8857d00 --- /dev/null +++ b/.custom-gcl.yml @@ -0,0 +1,4 @@ +version: v1.62.0 +plugins: + - module: "github.com/google/go-github/v68/tools/sliceofpointers" + path: ./tools/sliceofpointers diff --git a/.gitignore b/.gitignore index 8e24389445d..b81979c9430 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,6 @@ vendor/ # goenv local version. See https://github.com/syndbg/goenv/blob/master/COMMANDS.md#goenv-local for more info. .go-version + +# golangci-lint -v custom generates the following local file: +custom-gcl diff --git a/.golangci.yml b/.golangci.yml index eae5a3bdd6a..fae768c5a57 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -18,12 +18,18 @@ 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. + original-url: github.com/google/go-github/v68/tools/sliceofpointers gocritic: disable-all: true enabled-checks: @@ -38,7 +44,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 +108,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" diff --git a/github/codespaces_secrets_test.go b/github/codespaces_secrets_test.go index 543e0651def..987cfaab612 100644 --- a/github/codespaces_secrets_test.go +++ b/github/codespaces_secrets_test.go @@ -25,7 +25,7 @@ func TestCodespacesService_ListSecrets(t *testing.T) { methodName string } opts := &ListOptions{Page: 2, PerPage: 2} - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -128,7 +128,7 @@ func TestCodespacesService_GetSecret(t *testing.T) { badCall func(context.Context, *Client) (*Secret, *Response, error) methodName string } - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -222,7 +222,7 @@ func TestCodespacesService_CreateOrUpdateSecret(t *testing.T) { badCall func(context.Context, *Client, *EncryptedSecret) (*Response, error) methodName string } - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -318,7 +318,7 @@ func TestCodespacesService_DeleteSecret(t *testing.T) { badCall func(context.Context, *Client) (*Response, error) methodName string } - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -401,7 +401,7 @@ func TestCodespacesService_GetPublicKey(t *testing.T) { methodName string } - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -496,7 +496,7 @@ func TestCodespacesService_ListSelectedReposForSecret(t *testing.T) { methodName string } opts := &ListOptions{Page: 2, PerPage: 2} - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -581,7 +581,7 @@ func TestCodespacesService_SetSelectedReposForSecret(t *testing.T) { methodName string } ids := SelectedRepoIDs{64780797} - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -653,7 +653,7 @@ func TestCodespacesService_AddSelectedReposForSecret(t *testing.T) { methodName string } repo := &Repository{ID: Ptr(int64(1234))} - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { @@ -721,7 +721,7 @@ func TestCodespacesService_RemoveSelectedReposFromSecret(t *testing.T) { methodName string } repo := &Repository{ID: Ptr(int64(1234))} - tests := []test{ + tests := []*test{ { name: "User", handleFunc: func(mux *http.ServeMux) { diff --git a/github/github.go b/github/github.go index d4ba320aa5d..ab74c5b72da 100644 --- a/github/github.go +++ b/github/github.go @@ -1117,7 +1117,8 @@ GitHub API docs: https://docs.github.com/rest/#client-errors type ErrorResponse struct { Response *http.Response `json:"-"` // HTTP response that caused this error Message string `json:"message"` // error message - Errors []Error `json:"errors"` // more detail on individual errors + //nolint:sliceofpointers + Errors []Error `json:"errors"` // more detail on individual errors // Block is only populated on certain types of errors such as code 451. Block *ErrorBlock `json:"block,omitempty"` // Most errors will also include a documentation_url field pointing diff --git a/github/strings_test.go b/github/strings_test.go index 4d53f21aac6..a164cebcac0 100644 --- a/github/strings_test.go +++ b/github/strings_test.go @@ -49,6 +49,7 @@ func TestStringify(t *testing.T) { {Ptr(123), `123`}, {Ptr(false), `false`}, { + //nolint:sliceofpointers []*string{Ptr("a"), Ptr("b")}, `["a" "b"]`, }, diff --git a/scrape/apps.go b/scrape/apps.go index fe99ea84a03..686cf773800 100644 --- a/scrape/apps.go +++ b/scrape/apps.go @@ -46,13 +46,13 @@ func (c *Client) AppRestrictionsEnabled(org string) (bool, error) { // ListOAuthApps lists the reviewed OAuth Applications for the // specified organization (whether approved or denied). -func (c *Client) ListOAuthApps(org string) ([]OAuthApp, error) { +func (c *Client) ListOAuthApps(org string) ([]*OAuthApp, error) { doc, err := c.get("/organizations/%s/settings/oauth_application_policy", org) if err != nil { return nil, err } - var apps []OAuthApp + var apps []*OAuthApp doc.Find(".oauth-application-allowlist ul > li").Each(func(i int, s *goquery.Selection) { var app OAuthApp app.Name = s.Find(".request-info strong").First().Text() @@ -73,7 +73,7 @@ func (c *Client) ListOAuthApps(org string) ([]OAuthApp, error) { } else if r := s.Find(".request-indicator .denied-request"); r.Length() > 0 { app.State = OAuthAppDenied } - apps = append(apps, app) + apps = append(apps, &app) }) return apps, nil diff --git a/scrape/apps_test.go b/scrape/apps_test.go index ae5b191eed8..7e9688e5efb 100644 --- a/scrape/apps_test.go +++ b/scrape/apps_test.go @@ -61,7 +61,7 @@ func Test_ListOAuthApps(t *testing.T) { if err != nil { t.Fatalf("ListOAuthApps(e) returned err: %v", err) } - want := []OAuthApp{ + want := []*OAuthApp{ { ID: 22222, Name: "Coveralls", diff --git a/scrape/forms.go b/scrape/forms.go index 6e01171e155..7beb46cf6e1 100644 --- a/scrape/forms.go +++ b/scrape/forms.go @@ -35,7 +35,7 @@ type htmlForm struct { // // In the future, we might want to allow a custom selector to be passed in to // further restrict what forms will be returned. -func parseForms(node *html.Node) (forms []htmlForm) { +func parseForms(node *html.Node) (forms []*htmlForm) { if node == nil { return nil } @@ -71,7 +71,7 @@ func parseForms(node *html.Node) (forms []htmlForm) { value := s.Text() form.Values.Add(name, value) }) - forms = append(forms, form) + forms = append(forms, &form) }) return forms diff --git a/scrape/forms_test.go b/scrape/forms_test.go index 28c30c6c670..b10f250127d 100644 --- a/scrape/forms_test.go +++ b/scrape/forms_test.go @@ -16,14 +16,14 @@ func Test_ParseForms(t *testing.T) { tests := []struct { description string html string - forms []htmlForm + forms []*htmlForm }{ {"no forms", ``, nil}, - {"empty form", `
`, []htmlForm{{Values: url.Values{}}}}, + {"empty form", `
`, []*htmlForm{{Values: url.Values{}}}}, { "single form with one value", `
`, - []htmlForm{{Action: "a", Method: "m", Values: url.Values{"n1": {"v1"}}}}, + []*htmlForm{{Action: "a", Method: "m", Values: url.Values{"n1": {"v1"}}}}, }, { "two forms", @@ -31,7 +31,7 @@ func Test_ParseForms(t *testing.T) {
`, - []htmlForm{ + []*htmlForm{ {Action: "a1", Method: "m1", Values: url.Values{"n1": {"v1"}}}, {Action: "a2", Method: "m2", Values: url.Values{"n2": {"v2"}}}, }, @@ -43,7 +43,7 @@ func Test_ParseForms(t *testing.T) { `, - []htmlForm{{Values: url.Values{}}}, + []*htmlForm{{Values: url.Values{}}}, }, { "form with radio buttons", @@ -52,7 +52,7 @@ func Test_ParseForms(t *testing.T) { `, - []htmlForm{{Values: url.Values{"n1": {"v3"}}}}, + []*htmlForm{{Values: url.Values{"n1": {"v3"}}}}, }, { "form with checkboxes", @@ -61,12 +61,12 @@ func Test_ParseForms(t *testing.T) { `, - []htmlForm{{Values: url.Values{"n1": {"v1"}, "n3": {"v3"}}}}, + []*htmlForm{{Values: url.Values{"n1": {"v1"}, "n3": {"v3"}}}}, }, { "single form with textarea", `
`, - []htmlForm{{Values: url.Values{"n1": {"v1"}}}}, + []*htmlForm{{Values: url.Values{"n1": {"v1"}}}}, }, } diff --git a/script/lint.sh b/script/lint.sh index a52e9bfa2b6..8decafeb7e5 100755 --- a/script/lint.sh +++ b/script/lint.sh @@ -19,9 +19,10 @@ fail() { EXIT_CODE=1 } -# install golangci-lint bin/golangci-lint doesn't exist with the correct version -if ! "$BIN"/golangci-lint --version 2> /dev/null | grep -q "$GOLANGCI_LINT_VERSION"; then +# install golangci-lint and custom-gcl in ./bin if they don't exist with the correct version +if ! "$BIN"/custom-gcl --version 2> /dev/null | grep -q "$GOLANGCI_LINT_VERSION"; then GOBIN="$BIN" go install "github.com/golangci/golangci-lint/cmd/golangci-lint@v$GOLANGCI_LINT_VERSION" + "$BIN"/golangci-lint custom && mv ./custom-gcl "$BIN" fi MOD_DIRS="$(git ls-files '*go.mod' | xargs dirname | sort)" @@ -33,9 +34,9 @@ for dir in $MOD_DIRS; do cd "$dir" # github actions output when running in an action if [ -n "$GITHUB_ACTIONS" ]; then - "$BIN"/golangci-lint run --path-prefix "$dir" --out-format colored-line-number + "$BIN"/custom-gcl run --path-prefix "$dir" --out-format colored-line-number else - "$BIN"/golangci-lint run --path-prefix "$dir" + "$BIN"/custom-gcl run --path-prefix "$dir" fi ) || fail "failed linting $dir" done diff --git a/tools/sliceofpointers/go.mod b/tools/sliceofpointers/go.mod new file mode 100644 index 00000000000..759fa734d9e --- /dev/null +++ b/tools/sliceofpointers/go.mod @@ -0,0 +1,13 @@ +module tools/sliceofpointers + +go 1.22.0 + +require ( + github.com/golangci/plugin-module-register v0.1.1 + golang.org/x/tools v0.29.0 +) + +require ( + golang.org/x/mod v0.22.0 // indirect + golang.org/x/sync v0.10.0 // indirect +) diff --git a/tools/sliceofpointers/go.sum b/tools/sliceofpointers/go.sum new file mode 100644 index 00000000000..c2f7392bb23 --- /dev/null +++ b/tools/sliceofpointers/go.sum @@ -0,0 +1,10 @@ +github.com/golangci/plugin-module-register v0.1.1 h1:TCmesur25LnyJkpsVrupv1Cdzo+2f7zX0H6Jkw1Ol6c= +github.com/golangci/plugin-module-register v0.1.1/go.mod h1:TTpqoB6KkwOJMV8u7+NyXMrkwwESJLOkfl9TxR1DGFc= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +golang.org/x/mod v0.22.0 h1:D4nJWe9zXqHOmWqj4VMOJhvzj7bEZg4wEYa759z1pH4= +golang.org/x/mod v0.22.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.10.0 h1:3NQrjDixjgGwUOCaF8w2+VYHv0Ve/vGYSbdkTa98gmQ= +golang.org/x/sync v0.10.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.29.0 h1:Xx0h3TtM9rzQpQuR4dKLrdglAmCEN5Oi+P74JdhdzXE= +golang.org/x/tools v0.29.0/go.mod h1:KMQVMRsVxU6nHCFXrBPhDB8XncLNLM0lIy/F14RP588= diff --git a/tools/sliceofpointers/sliceofpointers.go b/tools/sliceofpointers/sliceofpointers.go new file mode 100644 index 00000000000..d6a59b38f28 --- /dev/null +++ b/tools/sliceofpointers/sliceofpointers.go @@ -0,0 +1,74 @@ +// Copyright 2025 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package 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 sliceofpointers + +import ( + "go/ast" + "go/token" + + "github.com/golangci/plugin-module-register/register" + "golang.org/x/tools/go/analysis" +) + +func init() { + register.Plugin("sliceofpointers", New) +} + +type SliceOfPointersPlugin struct{} + +// New returns an analysis.Analyzer to use with golangci-lint. +func New(settings any) (register.LinterPlugin, error) { + return &SliceOfPointersPlugin{}, nil +} + +func (f *SliceOfPointersPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { + return []*analysis.Analyzer{ + { + Name: "sliceofpointers", + Doc: "Reports usage of []*string and slices of structs without pointers.", + Run: run, + }, + }, nil +} + +func (f *SliceOfPointersPlugin) GetLoadMode() string { + return register.LoadModeSyntax +} + +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) + } + } +} diff --git a/tools/sliceofpointers/sliceofpointers_test.go b/tools/sliceofpointers/sliceofpointers_test.go new file mode 100644 index 00000000000..8fb0787f022 --- /dev/null +++ b/tools/sliceofpointers/sliceofpointers_test.go @@ -0,0 +1,20 @@ +// Copyright 2025 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package sliceofpointers + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestRun(t *testing.T) { + t.Parallel() + testdata := analysistest.TestData() + plugin, _ := New(nil) + analyzers, _ := plugin.BuildAnalyzers() + analysistest.Run(t, testdata, analyzers[0], "has-warnings", "no-warnings") +} diff --git a/tools/sliceofpointers/testdata/src/has-warnings/main.go b/tools/sliceofpointers/testdata/src/has-warnings/main.go new file mode 100644 index 00000000000..84e0d024e4b --- /dev/null +++ b/tools/sliceofpointers/testdata/src/has-warnings/main.go @@ -0,0 +1,21 @@ +// Copyright 2025 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +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 +} diff --git a/tools/sliceofpointers/testdata/src/no-warnings/main.go b/tools/sliceofpointers/testdata/src/no-warnings/main.go new file mode 100644 index 00000000000..529d1d2fdad --- /dev/null +++ b/tools/sliceofpointers/testdata/src/no-warnings/main.go @@ -0,0 +1,21 @@ +// Copyright 2025 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +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 +}