Skip to content

Commit fed2eb5

Browse files
CopilotCopilot
authored
Close recurring Go linter analyzer blind spots (#67544)
1 parent 79fc81c commit fed2eb5

11 files changed

Lines changed: 276 additions & 96 deletions

File tree

‎pkg/linters/deferinloop/deferinloop.go‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
11
// Package deferinloop implements a Go analysis linter that flags defer
2-
// statements placed directly inside for or range loop bodies. A defer inside
3-
// a loop does not execute at the end of each iteration — it runs when the
4-
// enclosing function returns, which can cause resource leaks and unexpected
5-
// cleanup ordering.
2+
// statements inside loops where they run when the enclosing function returns,
3+
// rather than at the end of each iteration.
64
package deferinloop
75

86
import (
97
"go/ast"
8+
"go/types"
109

1110
"golang.org/x/tools/go/analysis"
1211
"golang.org/x/tools/go/ast/inspector"
@@ -21,7 +20,7 @@ import (
2120
var pkgLog = logger.New("linters:deferinloop")
2221

2322
// Analyzer is the defer-in-loop analysis pass.
24-
var Analyzer = analyzerutil.New("deferinloop", "reports defer statements enclosed anywhere within a for or range loop body; a function literal between a defer and an enclosing loop is treated as a new scope boundary, making the defer exempt; test files are not checked", run)
23+
var Analyzer = analyzerutil.New("deferinloop", "reports defer statements inside loops where they run when the enclosing function returns rather than at the end of each iteration; range-over-function iterators and function literals form scope boundaries; test files are not checked", run)
2524

2625
func run(pass *analysis.Pass) (any, error) {
2726
insp, err := astutil.Inspector(pass)
@@ -50,7 +49,7 @@ func run(pass *analysis.Pass) (any, error) {
5049
continue
5150
}
5251

53-
if !isInsideLoop(cur) {
52+
if !isInsideLoop(pass, cur) {
5453
continue
5554
}
5655

@@ -63,17 +62,25 @@ func run(pass *analysis.Pass) (any, error) {
6362
}
6463

6564
// isInsideLoop reports whether cur (a DeferStmt) is enclosed anywhere within a
66-
// for or range loop body, without crossing a function literal boundary.
67-
// Defers inside func literals are exempt because they form a new function scope
68-
// and execute when the literal returns, not the outer function.
69-
func isInsideLoop(cur inspector.Cursor) bool {
65+
// for or range loop body, without crossing a function literal or range-over-func
66+
// boundary.
67+
func isInsideLoop(pass *analysis.Pass, cur inspector.Cursor) bool {
7068
for encl := range cur.Enclosing(
7169
(*ast.ForStmt)(nil),
7270
(*ast.RangeStmt)(nil),
7371
(*ast.FuncLit)(nil),
7472
) {
75-
switch encl.Node().(type) {
76-
case *ast.ForStmt, *ast.RangeStmt:
73+
switch node := encl.Node().(type) {
74+
case *ast.ForStmt:
75+
return true
76+
case *ast.RangeStmt:
77+
if pass.TypesInfo != nil {
78+
if typ := pass.TypesInfo.TypeOf(node.X); typ != nil {
79+
if _, ok := typ.Underlying().(*types.Signature); ok {
80+
return false
81+
}
82+
}
83+
}
7784
return true
7885
case *ast.FuncLit:
7986
return false

‎pkg/linters/deferinloop/testdata/src/deferinloop/deferinloop.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,15 @@ func GoodGoFuncLitInsideLoop(paths []string) {
8686
}
8787
}
8888

89+
func GoodRangeOverFunc() {
90+
iterator := func(yield func() bool) {
91+
yield()
92+
}
93+
for range iterator {
94+
defer println()
95+
}
96+
}
97+
8998
// GoodDeferOutsideLoop is fine — defer is not inside a loop.
9099
func GoodDeferOutsideLoop() {
91100
f, _ := os.Open("file")

‎pkg/linters/globwalkignorederror/globwalkignorederror.go‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
// Package globwalkignorederror implements a Go analysis linter that flags
2-
// filepath.Glob and os.ReadDir calls where the error return is discarded
3-
// with _.
2+
// filepath.Glob and os.ReadDir calls where the error return is discarded.
43
package globwalkignorederror
54

65
import (
@@ -16,7 +15,7 @@ import (
1615
)
1716

1817
// Analyzer is the glob-walk-ignored-error analysis pass.
19-
var Analyzer = analyzerutil.New("globwalkignorederror", "reports filepath.Glob and os.ReadDir calls where the error return is discarded with _", run)
18+
var Analyzer = analyzerutil.New("globwalkignorederror", "reports filepath.Glob and os.ReadDir calls where the error return is discarded", run)
2019

2120
// checkedFuncs maps package import path to the set of function names within
2221
// that package whose discarded error return should be flagged.
@@ -31,20 +30,20 @@ func run(pass *analysis.Pass) (any, error) {
3130
return nil, err
3231
}
3332

34-
nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)}
33+
nodeFilter := []ast.Node{(*ast.AssignStmt)(nil), (*ast.ExprStmt)(nil)}
3534
return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) {
36-
analyzeGlobWalkAssign(pass, n, generatedFiles, nolintIndex)
35+
analyzeGlobWalkStmt(pass, n, generatedFiles, nolintIndex)
3736
})
3837
}
3938

40-
// analyzeGlobWalkAssign checks whether an assignment discards the error
41-
// return from filepath.Glob or os.ReadDir and reports a diagnostic if so.
42-
func analyzeGlobWalkAssign(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, nolintIndex nolint.DirectiveIndex) {
43-
assign, ok := n.(*ast.AssignStmt)
39+
// analyzeGlobWalkStmt checks whether a statement discards the error return from
40+
// filepath.Glob or os.ReadDir and reports a diagnostic if so.
41+
func analyzeGlobWalkStmt(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, nolintIndex nolint.DirectiveIndex) {
42+
stmt, ok := n.(ast.Stmt)
4443
if !ok {
4544
return
4645
}
47-
call, pkgPath, funcName, ok := astutil.MatchDiscardedErrorCall(pass, assign)
46+
call, pkgPath, funcName, ok := astutil.MatchDiscardedErrorCall(pass, stmt)
4847
if !ok {
4948
return
5049
}

‎pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/globwalkignorederror.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ func bad() {
1212
_ = entries
1313
}
1414

15+
func badBareCalls() {
16+
filepath.Glob("*.go") // want `error return from filepath\.Glob is discarded`
17+
os.ReadDir(".") // want `error return from os\.ReadDir is discarded`
18+
}
19+
1520
func good() {
1621
files, err := filepath.Glob("*.go")
1722
if err != nil {

‎pkg/linters/internal/astutil/astutil.go‎

Lines changed: 123 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"go/types"
1212
"slices"
1313
"strconv"
14+
"strings"
1415

1516
"golang.org/x/tools/go/analysis"
1617
"golang.org/x/tools/go/analysis/passes/inspect"
@@ -37,37 +38,53 @@ func RhsExprForIndex(rhs []ast.Expr, idx int) (ast.Expr, bool) {
3738
case len(rhs) == 0:
3839
return nil, false
3940
case len(rhs) == 1 && idx == 0:
40-
return rhs[0], true
41-
case idx < len(rhs):
42-
return rhs[idx], true
41+
for _, expr := range rhs {
42+
return expr, true
43+
}
44+
case idx >= 0 && idx < len(rhs):
45+
for i, expr := range rhs {
46+
if i == idx {
47+
return expr, true
48+
}
49+
}
4350
default:
4451
return nil, false
4552
}
53+
return nil, false
4654
}
4755

48-
// MatchDiscardedErrorCall reports whether assign has the form value, _ = pkg.Func(...)
49-
// and resolves the selected function to its imported package.
50-
func MatchDiscardedErrorCall(pass *analysis.Pass, assign *ast.AssignStmt) (call *ast.CallExpr, pkgPath, funcName string, ok bool) {
51-
if pass == nil || pass.TypesInfo == nil || assign == nil || len(assign.Lhs) != 2 || len(assign.Rhs) != 1 {
56+
// MatchDiscardedErrorCall reports whether stmt discards the error result of a
57+
// package-qualified call and resolves the function to its imported package.
58+
func MatchDiscardedErrorCall(pass *analysis.Pass, stmt ast.Stmt) (call *ast.CallExpr, pkgPath, funcName string, ok bool) {
59+
if pass == nil || pass.TypesInfo == nil || stmt == nil {
5260
return nil, "", "", false
5361
}
54-
var secondLHS ast.Expr
55-
for index, lhs := range assign.Lhs {
56-
if index == 1 {
57-
secondLHS = lhs
62+
63+
switch stmt := stmt.(type) {
64+
case *ast.AssignStmt:
65+
if len(stmt.Lhs) != 2 || len(stmt.Rhs) != 1 {
66+
return nil, "", "", false
67+
}
68+
var blankExpr ast.Expr
69+
for i, lhs := range stmt.Lhs {
70+
if i == 1 {
71+
blankExpr = lhs
72+
break
73+
}
74+
}
75+
blank, isBlank := blankExpr.(*ast.Ident)
76+
if !isBlank || blank.Name != "_" {
77+
return nil, "", "", false
78+
}
79+
for _, rhs := range stmt.Rhs {
80+
call, ok = rhs.(*ast.CallExpr)
5881
break
5982
}
60-
}
61-
blank, ok := secondLHS.(*ast.Ident)
62-
if !ok || blank.Name != "_" {
83+
case *ast.ExprStmt:
84+
call, ok = stmt.X.(*ast.CallExpr)
85+
default:
6386
return nil, "", "", false
6487
}
65-
var rhs ast.Expr
66-
for _, expr := range assign.Rhs {
67-
rhs = expr
68-
break
69-
}
70-
call, ok = rhs.(*ast.CallExpr)
7188
if !ok {
7289
return nil, "", "", false
7390
}
@@ -78,25 +95,87 @@ func MatchDiscardedErrorCall(pass *analysis.Pass, assign *ast.AssignStmt) (call
7895
return call, pkgPath, funcName, true
7996
}
8097

81-
// PackageCall resolves a package-qualified call and returns its import path and
82-
// selected function name.
98+
// PackageCall resolves a package-qualified call, including calls through a
99+
// local function-value alias, and returns its import path and function name.
83100
func PackageCall(pass *analysis.Pass, call *ast.CallExpr) (pkgPath, funcName string, ok bool) {
84101
if pass == nil || pass.TypesInfo == nil || call == nil {
85102
return "", "", false
86103
}
87-
sel, ok := call.Fun.(*ast.SelectorExpr)
88-
if !ok {
89-
return "", "", false
104+
return packageFunction(pass, call.Fun, nil)
105+
}
106+
107+
func packageFunction(pass *analysis.Pass, expr ast.Expr, seen map[types.Object]bool) (pkgPath, funcName string, ok bool) {
108+
switch expr := UnwrapParenExpr(expr).(type) {
109+
case *ast.SelectorExpr:
110+
ident, ok := expr.X.(*ast.Ident)
111+
if !ok {
112+
return "", "", false
113+
}
114+
pkgName, ok := pass.TypesInfo.Uses[ident].(*types.PkgName)
115+
if !ok || pkgName.Imported() == nil {
116+
return "", "", false
117+
}
118+
return pkgName.Imported().Path(), expr.Sel.Name, true
119+
case *ast.Ident:
120+
return packageFunctionAlias(pass, pass.TypesInfo.Uses[expr], seen)
90121
}
91-
ident, ok := sel.X.(*ast.Ident)
92-
if !ok {
122+
return "", "", false
123+
}
124+
125+
func packageFunctionAlias(pass *analysis.Pass, obj types.Object, seen map[types.Object]bool) (string, string, bool) {
126+
if obj == nil {
93127
return "", "", false
94128
}
95-
pkgName, ok := pass.TypesInfo.Uses[ident].(*types.PkgName)
96-
if !ok || pkgName.Imported() == nil {
129+
if seen == nil {
130+
seen = make(map[types.Object]bool)
131+
}
132+
if seen[obj] {
97133
return "", "", false
98134
}
99-
return pkgName.Imported().Path(), sel.Sel.Name, true
135+
seen[obj] = true
136+
for _, file := range pass.Files {
137+
if pkgPath, funcName, ok := packageFunctionAliasInFile(pass, file, obj, seen); ok {
138+
return pkgPath, funcName, true
139+
}
140+
}
141+
return "", "", false
142+
}
143+
144+
func packageFunctionAliasInFile(pass *analysis.Pass, file *ast.File, obj types.Object, seen map[types.Object]bool) (string, string, bool) {
145+
var pkgPath, funcName string
146+
ast.Inspect(file, func(node ast.Node) bool {
147+
if node == nil || pkgPath != "" {
148+
return false
149+
}
150+
value := packageFunctionAliasInitializer(pass, node, obj)
151+
if value == nil {
152+
return true
153+
}
154+
pkgPath, funcName, _ = packageFunction(pass, value, seen)
155+
return false
156+
})
157+
return pkgPath, funcName, pkgPath != ""
158+
}
159+
160+
func packageFunctionAliasInitializer(pass *analysis.Pass, node ast.Node, obj types.Object) ast.Expr {
161+
switch node := node.(type) {
162+
case *ast.ValueSpec:
163+
for i, name := range node.Names {
164+
if pass.TypesInfo.Defs[name] == obj {
165+
value, _ := RhsExprForIndex(node.Values, i)
166+
return value
167+
}
168+
}
169+
case *ast.AssignStmt:
170+
for i, lhs := range node.Lhs {
171+
ident, ok := lhs.(*ast.Ident)
172+
if ok && pass.TypesInfo.Defs[ident] == obj {
173+
value, _ := RhsExprForIndex(node.Rhs, i)
174+
return value
175+
}
176+
}
177+
}
178+
return nil
100179
}
101180

102181
// IsStringLiteral reports whether expr is a string literal.
@@ -357,11 +436,8 @@ func ImportedAs(file *ast.File, info *types.Info, importPath string) (string, bo
357436
}
358437
// Fallback: last segment of the path.
359438
last := importPath
360-
for j := len(importPath) - 1; j >= 0; j-- {
361-
if importPath[j] == '/' {
362-
last = importPath[j+1:]
363-
break
364-
}
439+
if slash := strings.LastIndexByte(importPath, '/'); slash >= 0 {
440+
last = importPath[slash+1:]
365441
}
366442
return last, true
367443
}
@@ -530,7 +606,7 @@ func BuildParentMap(root ast.Node) map[ast.Node]ast.Node {
530606
return false
531607
}
532608
if len(stack) > 0 {
533-
parents[n] = stack[len(stack)-1]
609+
parents[n] = stack[len(stack)-1] //nolint:uncheckedsliceindex // stack is non-empty
534610
}
535611
stack = append(stack, n)
536612
return true
@@ -554,12 +630,12 @@ func TwoValueTypeAssertionOKIdent(typeAssert *ast.TypeAssertExpr, parents map[as
554630
switch p := parent.(type) {
555631
case *ast.AssignStmt:
556632
if len(p.Lhs) == 2 && len(p.Rhs) == 1 {
557-
okIdent, ok := p.Lhs[1].(*ast.Ident)
633+
okIdent, ok := p.Lhs[1].(*ast.Ident) //nolint:uncheckedsliceindex // len(p.Lhs) is checked above
558634
return okIdent, ok
559635
}
560636
case *ast.ValueSpec:
561637
if len(p.Names) == 2 && len(p.Values) == 1 {
562-
return p.Names[1], true
638+
return p.Names[1], true //nolint:uncheckedsliceindex // len(p.Names) is checked above
563639
}
564640
}
565641
return nil, false
@@ -642,7 +718,15 @@ func AddImportEdit(pass *analysis.Pass, file *ast.File, pkg string) (analysis.Te
642718
if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() {
643719
continue
644720
}
645-
specText := NodeText(pass.Fset, genDecl.Specs[0])
721+
var spec ast.Spec
722+
for _, candidate := range genDecl.Specs {
723+
spec = candidate
724+
break
725+
}
726+
if spec == nil {
727+
continue
728+
}
729+
specText := NodeText(pass.Fset, spec)
646730
if specText == "" {
647731
continue
648732
}

0 commit comments

Comments
 (0)