diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 6ccc1f13b14..052dbf754f6 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -22,6 +22,7 @@ This package currently provides custom Go analyzers in the following subpackages - `execcommandwithoutcontext` — reports `exec.Command(...)` calls inside functions that already receive `context.Context` and should use `exec.CommandContext(...)`. - `fmterrorfnoverbs` — reports `fmt.Errorf` calls whose format string contains no verbs, recommending `errors.New` instead. - `fprintlnsprintf` — reports `fmt.Fprintln(..., fmt.Sprintf(...))` patterns and recommends direct formatting calls. +- `globwalkignorederror` — reports `filepath.Glob` and `os.ReadDir` calls where the error return is discarded with `_`. - `goroutinemissingrecover` — reports goroutines started via a function literal whose body does not install a top-level `defer func() { recover() }()` guard. - `hardcodedfilepath` — reports hard-coded file path string literals that match known path constants or should be extracted into named constants; also annotates paths that appear in log/print calls. - `httpnoctx` — reports HTTP client and package-level HTTP calls that do not accept a `context.Context`. @@ -92,6 +93,7 @@ This package currently provides custom Go analyzers in the following subpackages | `fileclosenotdeferred` | Custom `go/analysis` analyzer that flags file `Close()` calls that are not deferred immediately | | `fmterrorfnoverbs` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls with no format verbs, recommending `errors.New` | | `fprintlnsprintf` | Custom `go/analysis` analyzer that flags `fmt.Fprintln(..., fmt.Sprintf(...))` patterns | +| `globwalkignorederror` | Custom `go/analysis` analyzer that flags `filepath.Glob` and `os.ReadDir` calls where the error return is discarded with `_` | | `goroutinemissingrecover` | Custom `go/analysis` analyzer that flags goroutines started via a function literal that do not install a top-level defer/recover guard | | `hardcodedfilepath` | Custom `go/analysis` analyzer that flags hard-coded file path string literals that match known path constants or should be extracted as named constants; annotates paths in log/print calls | | `httpnoctx` | Custom `go/analysis` analyzer that flags HTTP calls that do not accept a `context.Context` | @@ -230,6 +232,7 @@ _ = trimleftright.Analyzer - `github.com/github/gh-aw/pkg/linters/fileclosenotdeferred` — file-close-not-deferred analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs` — fmt-errorf-no-verbs analyzer subpackage - `github.com/github/gh-aw/pkg/linters/fprintlnsprintf` — fprintln-sprintf analyzer subpackage +- `github.com/github/gh-aw/pkg/linters/globwalkignorederror` — glob-walk-ignored-error analyzer subpackage - `github.com/github/gh-aw/pkg/linters/hardcodedfilepath` — hard-coded-file-path analyzer subpackage - `github.com/github/gh-aw/pkg/linters/httpnoctx` — HTTP-no-context analyzer subpackage - `github.com/github/gh-aw/pkg/linters/httprespbodyclose` — HTTP-response-body-close analyzer subpackage diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 58284add348..34d83c11327 100644 --- a/pkg/linters/doc.go +++ b/pkg/linters/doc.go @@ -1,6 +1,6 @@ // Package linters is a namespace for gh-aw's custom Go analysis linters. // -// All 63 active analyzers: +// All 64 active analyzers: // // - appendbytestring — flags append(b, []byte(s)...) calls where s is a string that can be simplified to append(b, s...) // - appendoneelement — flags append(s, []T{x}...) calls where a single-element slice literal is spread and can be simplified to append(s, x) @@ -18,6 +18,7 @@ // - fileclosenotdeferred — flags file Close() calls that are not deferred // - fmterrorfnoverbs — flags fmt.Errorf calls with no format verbs, recommending errors.New // - fprintlnsprintf — flags fmt.Fprintln(..., fmt.Sprintf(...)) patterns +// - globwalkignorederror — flags filepath.Glob and os.ReadDir calls where the error return is discarded with _ // - goroutinemissingrecover — flags goroutines started via a function literal whose body does not install a top-level defer/recover guard // - hardcodedfilepath — flags hard-coded file path string literals that match known path constants or should be extracted as named constants // - httpnoctx — flags HTTP calls that do not accept a context.Context diff --git a/pkg/linters/globwalkignorederror/globwalkignorederror.go b/pkg/linters/globwalkignorederror/globwalkignorederror.go new file mode 100644 index 00000000000..64af819e426 --- /dev/null +++ b/pkg/linters/globwalkignorederror/globwalkignorederror.go @@ -0,0 +1,86 @@ +// Package globwalkignorederror implements a Go analysis linter that flags +// filepath.Glob and os.ReadDir calls where the error return is discarded +// with _. +package globwalkignorederror + +import ( + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + + "github.com/github/gh-aw/pkg/linters/internal/analyzerutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// Analyzer is the glob-walk-ignored-error analysis pass. +var Analyzer = analyzerutil.New("globwalkignorederror", "reports filepath.Glob and os.ReadDir calls where the error return is discarded with _", run) + +// checkedFuncs maps package import path to the set of function names within +// that package whose discarded error return should be flagged. +var checkedFuncs = map[string]map[string]bool{ + "path/filepath": {"Glob": true}, + "os": {"ReadDir": true}, +} + +func run(pass *analysis.Pass) (any, error) { + nolintIndex, err := nolint.Index(pass) + if err != nil { + return nil, err + } + generatedFiles, err := filecheck.Index(pass) + if err != nil { + return nil, err + } + + nodeFilter := []ast.Node{(*ast.AssignStmt)(nil)} + return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { + analyzeGlobWalkAssign(pass, n, generatedFiles, nolintIndex) + }) +} + +// analyzeGlobWalkAssign checks whether an assignment discards the error +// return from filepath.Glob or os.ReadDir and reports a diagnostic if so. +func analyzeGlobWalkAssign(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, nolintIndex nolint.DirectiveIndex) { + assign, ok := n.(*ast.AssignStmt) + if !ok { + return + } + if len(assign.Lhs) != 2 || len(assign.Rhs) != 1 { + return + } + blank, ok := assign.Lhs[1].(*ast.Ident) + if !ok || blank.Name != "_" { + return + } + call, ok := assign.Rhs[0].(*ast.CallExpr) + if !ok { + return + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return + } + obj := pass.TypesInfo.Uses[ident] + pkgName, ok := obj.(*types.PkgName) + if !ok { + return + } + funcs, ok := checkedFuncs[pkgName.Imported().Path()] + if !ok || !funcs[sel.Sel.Name] { + return + } + position := pass.Fset.PositionFor(call.Pos(), false) + if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { + return + } + if nolint.HasDirectiveForLinter(position, nolintIndex, "globwalkignorederror") { + return + } + pass.ReportRangef(call, "error return from %s.%s is discarded; malformed patterns or unreadable directories silently produce an empty result", pkgName.Imported().Name(), sel.Sel.Name) +} diff --git a/pkg/linters/globwalkignorederror/globwalkignorederror_test.go b/pkg/linters/globwalkignorederror/globwalkignorederror_test.go new file mode 100644 index 00000000000..443421f1047 --- /dev/null +++ b/pkg/linters/globwalkignorederror/globwalkignorederror_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package globwalkignorederror_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/globwalkignorederror" +) + +func TestAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, globwalkignorederror.Analyzer, "globwalkignorederror") +} diff --git a/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/generated.go b/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/generated.go new file mode 100644 index 00000000000..92c39c2d45c --- /dev/null +++ b/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/generated.go @@ -0,0 +1,10 @@ +// Code generated by tests. DO NOT EDIT. + +package globwalkignorederror + +import "path/filepath" + +func generatedBad() { + files, _ := filepath.Glob("*.go") + _ = files +} diff --git a/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/globwalkignorederror.go b/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/globwalkignorederror.go new file mode 100644 index 00000000000..15f80170408 --- /dev/null +++ b/pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/globwalkignorederror.go @@ -0,0 +1,35 @@ +package globwalkignorederror + +import ( + "os" + "path/filepath" +) + +func bad() { + files, _ := filepath.Glob("*.go") // want `error return from filepath\.Glob is discarded` + _ = files + entries, _ := os.ReadDir(".") // want `error return from os\.ReadDir is discarded` + _ = entries +} + +func good() { + files, err := filepath.Glob("*.go") + if err != nil { + return + } + _ = files + + entries, err2 := os.ReadDir(".") + if err2 != nil { + return + } + _ = entries +} + +func suppressed() { + //nolint:globwalkignorederror + files, _ := filepath.Glob("*.go") + _ = files + entries, _ := os.ReadDir(".") //nolint:globwalkignorederror + _ = entries +} diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index c6f7cdc2a32..2514ca2e45d 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -19,6 +19,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" + "github.com/github/gh-aw/pkg/linters/globwalkignorederror" "github.com/github/gh-aw/pkg/linters/goroutinemissingrecover" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" @@ -94,6 +95,7 @@ var allAnalyzers = []*analysis.Analyzer{ excessivefuncparams.Analyzer, fileclosenotdeferred.Analyzer, fmterrorfnoverbs.Analyzer, + globwalkignorederror.Analyzer, goroutinemissingrecover.Analyzer, hardcodedfilepath.Analyzer, httpnoctx.Analyzer, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 84babacbe22..bdc9a30f548 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -27,6 +27,7 @@ import ( "github.com/github/gh-aw/pkg/linters/fileclosenotdeferred" "github.com/github/gh-aw/pkg/linters/fmterrorfnoverbs" "github.com/github/gh-aw/pkg/linters/fprintlnsprintf" + "github.com/github/gh-aw/pkg/linters/globwalkignorederror" "github.com/github/gh-aw/pkg/linters/goroutinemissingrecover" "github.com/github/gh-aw/pkg/linters/hardcodedfilepath" "github.com/github/gh-aw/pkg/linters/httpnoctx" @@ -89,7 +90,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 62 analyzers +// "Public API > Subpackages" table. The README documents 64 analyzers // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -97,7 +98,7 @@ type docAnalyzer struct { // // appendbytestring, appendoneelement, bytesbufferstring, bytescomparestring, contextcancelnotdeferred, ctxbackground, deferinloop, errorfwrapv, excessivefuncparams, errormessage, // errortypeassertion, errstringmatch, execcommandwithoutcontext, fileclosenotdeferred, fmterrorfnoverbs, fprintlnsprintf, -// goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, +// globwalkignorederror, goroutinemissingrecover, hardcodedfilepath, httpnoctx, httprespbodyclose, httpstatuscode, ioutildeprecated, jsonmarshalignoredeerror, largefunc, lenstringsplit, lenstringzero, // logfatallibrary, manualmutexunlock, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, panic-in-library-code, rawloginlib, // regexpcompileinfunction, regexpdynamicpattern, seenmapbool, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, @@ -120,6 +121,7 @@ func documentedAnalyzers() []docAnalyzer { {"fileclosenotdeferred", fileclosenotdeferred.Analyzer}, {"fmterrorfnoverbs", fmterrorfnoverbs.Analyzer}, {"fprintlnsprintf", fprintlnsprintf.Analyzer}, + {"globwalkignorederror", globwalkignorederror.Analyzer}, {"goroutinemissingrecover", goroutinemissingrecover.Analyzer}, {"hardcodedfilepath", hardcodedfilepath.Analyzer}, {"httpnoctx", httpnoctx.Analyzer},