Skip to content

Commit e089733

Browse files
gmlewisgmlewis
committed
Implement and use new custom extraneousnew linter
Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
1 parent ff69733 commit e089733

9 files changed

Lines changed: 500 additions & 2 deletions

File tree

‎.custom-gcl.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
version: v2.10.1 # should be in sync with script/setup-custom-gcl.sh
22
plugins:
3+
- module: "github.com/google/go-github/v84/tools/extraneousnew"
4+
path: ./tools/extraneous-new
35
- module: "github.com/google/go-github/v84/tools/fmtpercentv"
46
path: ./tools/fmtpercentv
57
- module: "github.com/google/go-github/v84/tools/sliceofpointers"

‎.golangci.yml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ linters:
1111
- dupl
1212
- errcheck
1313
- errorlint
14+
- extraneousnew
1415
- fmtpercentv
1516
- forbidigo
1617
- gocritic
@@ -180,6 +181,14 @@ linters:
180181
os-create-temp: true
181182
os-temp-dir: true
182183
custom:
184+
extraneousnew:
185+
type: module
186+
description: Reports problematic usage of 'new' or '&SomeStruct{}' when a more idiomatic 'var' pointer would be more appropriate.
187+
original-url: github.com/google/go-github/v84/tools/extraneousnew
188+
settings:
189+
ignored-methods:
190+
- RateLimitService.Get
191+
- RepositoriesService.ReplaceAllTopics
183192
fmtpercentv:
184193
type: module
185194
description: Reports usage of %d or %s in format strings.

‎github/pulls_reviews.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,8 +279,8 @@ func (s *PullRequestsService) UpdateReview(ctx context.Context, owner, repo stri
279279
return nil, nil, err
280280
}
281281

282-
review := &PullRequestReview{}
283-
resp, err := s.client.Do(ctx, req, review)
282+
var review *PullRequestReview
283+
resp, err := s.client.Do(ctx, req, &review)
284284
if err != nil {
285285
return nil, resp, err
286286
}

‎tools/extraneous-new/extraneous_new.go‎

Lines changed: 308 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,308 @@
1+
// Copyright 2026 The go-github AUTHORS. All rights reserved.
2+
//
3+
// Use of this source code is governed by a BSD-style
4+
// license that can be found in the LICENSE file.
5+
6+
// Package extraneousnew is a custom linter to be used by
7+
// golangci-lint to find instances where the Go code could
8+
// replace extraneous and problematic usage of `new` or `&SomeStruct{}`
9+
// with an initialized pointer in very specific instances.
10+
// It promotes the idiomatic Go concept that the zero-value is useful.
11+
package extraneousnew
12+
13+
import (
14+
"go/ast"
15+
"go/token"
16+
17+
"github.com/golangci/plugin-module-register/register"
18+
"golang.org/x/tools/go/analysis"
19+
)
20+
21+
func init() {
22+
register.Plugin("extraneousnew", New)
23+
}
24+
25+
// ExtraneousNewPlugin is a custom linter plugin for golangci-lint.
26+
type ExtraneousNewPlugin struct {
27+
ignoredMethods map[string]bool
28+
}
29+
30+
// Settings is the configuration for the extraneousnew linter.
31+
type Settings struct {
32+
// IgnoredMethods causes the linter to skip linting individual `Receiver.Methods`.
33+
IgnoredMethods []string `json:"ignored-methods" yaml:"ignored-methods"`
34+
}
35+
36+
// New returns an analysis.Analyzer to use with golangci-lint.
37+
func New(cfg any) (register.LinterPlugin, error) {
38+
settings := Settings{}
39+
if cfg != nil {
40+
if data, ok := cfg.(map[string]any); ok {
41+
if ignored, ok := data["ignored-methods"].([]any); ok {
42+
for _, m := range ignored {
43+
if s, ok := m.(string); ok {
44+
settings.IgnoredMethods = append(settings.IgnoredMethods, s)
45+
}
46+
}
47+
}
48+
}
49+
}
50+
51+
ignoredMethods := map[string]bool{}
52+
for _, m := range settings.IgnoredMethods {
53+
ignoredMethods[m] = true
54+
}
55+
56+
return &ExtraneousNewPlugin{
57+
ignoredMethods: ignoredMethods,
58+
}, nil
59+
}
60+
61+
// BuildAnalyzers builds the analyzers for the ExtraneousNewPlugin.
62+
func (f *ExtraneousNewPlugin) BuildAnalyzers() ([]*analysis.Analyzer, error) {
63+
return []*analysis.Analyzer{
64+
{
65+
Name: "extraneousnew",
66+
Doc: `Reports problematic usage of 'new' or '&SomeStruct{}' when a
67+
more idiomatic 'var' pointer would be more appropriate.
68+
It encourages use of the idiomatic Go concept that the zero-value is useful.`,
69+
Run: func(pass *analysis.Pass) (any, error) {
70+
return run(pass, f.ignoredMethods)
71+
},
72+
},
73+
}, nil
74+
}
75+
76+
// GetLoadMode returns the load mode for the ExtraneousNewPlugin.
77+
func (f *ExtraneousNewPlugin) GetLoadMode() string {
78+
return register.LoadModeSyntax
79+
}
80+
81+
func run(pass *analysis.Pass, ignoredMethods map[string]bool) (any, error) {
82+
for _, file := range pass.Files {
83+
ast.Inspect(file, func(n ast.Node) bool {
84+
fn, ok := n.(*ast.FuncDecl)
85+
if !ok {
86+
return true
87+
}
88+
89+
if !fn.Name.IsExported() {
90+
return false
91+
}
92+
93+
// Check if method should be ignored.
94+
if fn.Recv != nil && len(fn.Recv.List) > 0 {
95+
var recvName string
96+
recvType := fn.Recv.List[0].Type
97+
if star, ok := recvType.(*ast.StarExpr); ok {
98+
recvType = star.X
99+
}
100+
if ident, ok := recvType.(*ast.Ident); ok {
101+
recvName = ident.Name
102+
}
103+
104+
if recvName != "" {
105+
fullName := recvName + "." + fn.Name.Name
106+
if ignoredMethods[fullName] {
107+
return false
108+
}
109+
}
110+
}
111+
112+
if fn.Body != nil {
113+
inspectAllBlocks(pass, fn.Body)
114+
}
115+
return true
116+
})
117+
}
118+
return nil, nil
119+
}
120+
121+
func inspectAllBlocks(pass *analysis.Pass, root ast.Node) {
122+
ast.Inspect(root, func(n ast.Node) bool {
123+
block, ok := n.(*ast.BlockStmt)
124+
if !ok {
125+
return true
126+
}
127+
inspectBlock(pass, block)
128+
return true
129+
})
130+
}
131+
132+
func inspectBlock(pass *analysis.Pass, block *ast.BlockStmt) {
133+
// Track pointers that are currently nil.
134+
nilPointers := make(map[string]*ast.Ident)
135+
136+
for i, stmt := range block.List {
137+
// 1. Check for `var v *T` or `var v *struct{...}`
138+
if decl, ok := stmt.(*ast.DeclStmt); ok {
139+
if gen, ok := decl.Decl.(*ast.GenDecl); ok && gen.Tok == token.VAR {
140+
for _, spec := range gen.Specs {
141+
if vSpec, ok := spec.(*ast.ValueSpec); ok {
142+
if _, ok := vSpec.Type.(*ast.StarExpr); ok && len(vSpec.Values) == 0 {
143+
for _, name := range vSpec.Names {
144+
nilPointers[name.Name] = name
145+
}
146+
}
147+
}
148+
}
149+
}
150+
}
151+
152+
// 2. Check for `v = new(T)` or `v := new(T)`
153+
var assignLHS *ast.Ident
154+
var isNewT bool
155+
var typeName string
156+
157+
if assign, ok := stmt.(*ast.AssignStmt); ok && len(assign.Lhs) == 1 && len(assign.Rhs) == 1 {
158+
if lhs, ok := assign.Lhs[0].(*ast.Ident); ok {
159+
assignLHS = lhs
160+
// Any assignment to v means it's no longer a "nil pointer" for our simple tracking.
161+
delete(nilPointers, lhs.Name)
162+
163+
// Check for v := new(T) or v := &T{}
164+
if call, ok := assign.Rhs[0].(*ast.CallExpr); ok {
165+
if fun, ok := call.Fun.(*ast.Ident); ok && fun.Name == "new" && len(call.Args) == 1 {
166+
isNewT = true
167+
if typeIdent, ok := call.Args[0].(*ast.Ident); ok {
168+
typeName = typeIdent.Name
169+
}
170+
}
171+
}
172+
if unary, ok := assign.Rhs[0].(*ast.UnaryExpr); ok && unary.Op == token.AND {
173+
if composite, ok := unary.X.(*ast.CompositeLit); ok && len(composite.Elts) == 0 {
174+
isNewT = true
175+
if typeIdent, ok := composite.Type.(*ast.Ident); ok {
176+
typeName = typeIdent.Name
177+
}
178+
}
179+
}
180+
}
181+
}
182+
183+
if isNewT && assignLHS != nil {
184+
lookAhead(pass, block, i, assignLHS, typeName)
185+
continue
186+
}
187+
188+
// If it's a regular assignment (possibly with multiple variables), it might initialize a nil pointer.
189+
if assign, ok := stmt.(*ast.AssignStmt); ok {
190+
for _, lhs := range assign.Lhs {
191+
if ident, ok := lhs.(*ast.Ident); ok {
192+
delete(nilPointers, ident.Name)
193+
}
194+
}
195+
}
196+
197+
// 3. Check if a nil pointer is passed to Do/Decode.
198+
ast.Inspect(stmt, func(n ast.Node) bool {
199+
call, ok := n.(*ast.CallExpr)
200+
if !ok {
201+
return true
202+
}
203+
204+
fnName := getFunctionName(call.Fun)
205+
if fnName == "" {
206+
return true
207+
}
208+
209+
var targetArg ast.Expr
210+
if fnName == "Do" && len(call.Args) == 3 {
211+
targetArg = call.Args[2]
212+
} else if fnName == "Decode" && len(call.Args) == 1 {
213+
targetArg = call.Args[0]
214+
}
215+
216+
if targetArg != nil {
217+
if ident, ok := targetArg.(*ast.Ident); ok {
218+
if _, isNil := nilPointers[ident.Name]; isNil {
219+
pass.Reportf(ident.Pos(), "pass '&%v' instead", ident.Name)
220+
}
221+
}
222+
}
223+
return true
224+
})
225+
}
226+
}
227+
228+
func getFunctionName(expr ast.Expr) string {
229+
switch f := expr.(type) {
230+
case *ast.SelectorExpr:
231+
return f.Sel.Name
232+
case *ast.Ident:
233+
return f.Name
234+
}
235+
return ""
236+
}
237+
238+
func lookAhead(pass *analysis.Pass, block *ast.BlockStmt, startIndex int, lhsIdent *ast.Ident, typeName string) {
239+
var foundProperUse bool
240+
var foundOtherUse bool
241+
242+
for j := startIndex + 1; j < len(block.List); j++ {
243+
nextStmt := block.List[j]
244+
ast.Inspect(nextStmt, func(un ast.Node) bool {
245+
if foundProperUse || foundOtherUse {
246+
return false
247+
}
248+
249+
// Check if lhsIdent is used here.
250+
ident, ok := un.(*ast.Ident)
251+
if !ok || ident.Name != lhsIdent.Name {
252+
return true
253+
}
254+
255+
// Found a use of lhsIdent. Is it the target argument in a call to Do or Decode?
256+
isSafe := false
257+
ast.Inspect(nextStmt, func(n ast.Node) bool {
258+
call, ok := n.(*ast.CallExpr)
259+
if !ok {
260+
return true
261+
}
262+
263+
fnName := getFunctionName(call.Fun)
264+
var targetArg ast.Expr
265+
if fnName == "Do" && len(call.Args) == 3 {
266+
targetArg = call.Args[2]
267+
} else if fnName == "Decode" && len(call.Args) == 1 {
268+
targetArg = call.Args[0]
269+
}
270+
271+
if targetArg != nil {
272+
if isIdentOrAddressOfIdent(targetArg, lhsIdent.Name) {
273+
isSafe = true
274+
return false
275+
}
276+
}
277+
return true
278+
})
279+
280+
if isSafe {
281+
if typeName != "" {
282+
pass.Reportf(ident.Pos(), "use 'var %v *%v' and pass '&%v' instead", lhsIdent.Name, typeName, lhsIdent.Name)
283+
} else {
284+
pass.Reportf(ident.Pos(), "use 'var %v *T' and pass '&%v' instead", lhsIdent.Name, lhsIdent.Name)
285+
}
286+
foundProperUse = true
287+
} else {
288+
foundOtherUse = true
289+
}
290+
return false
291+
})
292+
if foundProperUse || foundOtherUse {
293+
break
294+
}
295+
}
296+
}
297+
298+
func isIdentOrAddressOfIdent(expr ast.Expr, name string) bool {
299+
if ident, ok := expr.(*ast.Ident); ok {
300+
return ident.Name == name
301+
}
302+
if unary, ok := expr.(*ast.UnaryExpr); ok && unary.Op == token.AND {
303+
if ident, ok := unary.X.(*ast.Ident); ok {
304+
return ident.Name == name
305+
}
306+
}
307+
return false
308+
}

‎tools/extraneous-new/extraneous_new_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright 2026 The go-github AUTHORS. All rights reserved.
2+
//
3+
// Use of this source code is governed by a BSD-style
4+
// license that can be found in the LICENSE file.
5+
6+
package extraneousnew
7+
8+
import (
9+
"testing"
10+
11+
"golang.org/x/tools/go/analysis/analysistest"
12+
)
13+
14+
func TestRun(t *testing.T) {
15+
t.Parallel()
16+
testdata := analysistest.TestData()
17+
plugin, _ := New(map[string]any{
18+
"ignored-methods": []any{
19+
"Receiver.MethodNameToIgnore",
20+
},
21+
})
22+
analyzers, _ := plugin.BuildAnalyzers()
23+
analysistest.Run(t, testdata, analyzers[0], "has-warnings", "no-warnings")
24+
}

‎tools/extraneous-new/go.mod‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
module github.com/google/go-github/v84/tools/extraneousnew
2+
3+
go 1.25.0
4+
5+
require (
6+
github.com/golangci/plugin-module-register v0.1.1
7+
golang.org/x/tools v0.29.0
8+
)
9+
10+
require (
11+
golang.org/x/mod v0.22.0 // indirect
12+
golang.org/x/sync v0.10.0 // indirect
13+
)

0 commit comments

Comments
 (0)