diff --git a/pkg/linters/README.md b/pkg/linters/README.md index b2177d5a05a..f3a8b8e5cbc 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -79,6 +79,7 @@ This package currently provides custom Go analyzers in the following subpackages - `typeassertionnil` — reports single-value type assertions to pointer types and recommends the two-value form. - `typeassertionokdiscarded` — reports two-value type assertions whose `ok` result is discarded. - `uncheckedsliceindex` — reports slice and string indexes without proven bounds checks. +- `unchecked_deferredclose` — reports deferred `Close()` calls that ignore error return values. - `uncheckedtypeassertion` — reports single-value type assertions where unchecked panics are possible. - `uncheckedflushreturn` — reports `Flush()` method calls where the error return is discarded, which silently drops buffered data on failure. - `wgdonenotdeferred` — reports non-deferred `sync.WaitGroup.Done()` calls that can deadlock on panics or early returns. @@ -185,6 +186,7 @@ environment variable and gates findings on the recorded execution hit count for | `typeassertionnil` | Custom `go/analysis` analyzer that flags single-value type assertions to pointer types and recommends the two-value form | | `typeassertionokdiscarded` | Custom `go/analysis` analyzer that flags two-value type assertions whose `ok` result is discarded | | `uncheckedsliceindex` | Custom `go/analysis` analyzer that reports slice and string indexes without proven bounds checks | +| `unchecked_deferredclose` | Custom `go/analysis` analyzer that flags deferred `Close()` calls whose error return is ignored | | `uncheckedtypeassertion` | Custom `go/analysis` analyzer that flags unchecked single-value type assertions | | `uncheckedflushreturn` | Custom `go/analysis` analyzer that flags `Flush()` method calls where the error return is discarded | | `walkfuncerrshadow` | Custom `go/analysis` analyzer that flags `filepath.Walk`/`filepath.WalkDir` callbacks whose `err` parameter shadows an outer `err` variable assigned from the walk call | @@ -350,6 +352,7 @@ _ = typeassertionnil.Analyzer - The package is intentionally organized as a namespace (`pkg/linters/*`) so individual analyzers remain isolated and independently testable. - CI enforces selected production analyzers via the native and WebAssembly `LINTER_FLAGS` lists in `.github/workflows/cgo.yml`; `fprintferrorunchecked` is excluded from blocking checks and reported separately as a non-blocking warning on native Go code. - `typeassertionnil` is registered but excluded from CI enforcement pending a cross-platform enforcement-readiness audit. +- `unchecked_deferredclose` is registered but excluded from CI enforcement until existing deferred `Close()` findings are remediated. - `excessivefuncparams` exposes a `-max-params` analyzer flag and defaults to `8` parameters (`DefaultMaxParams`). - `largefunc` exposes a `-max-lines` analyzer flag, defaults to `60` lines (`DefaultMaxLines`), and skips `_test.go` files. - `osexitinlibrary` helps enforce separation between library logic and process-level termination. diff --git a/pkg/linters/doc.go b/pkg/linters/doc.go index 2265e75f9bc..39781518934 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 78 active analyzers: +// All 79 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) @@ -75,6 +75,7 @@ // - typeassertionnil — flags single-value type assertions to pointer types and recommends the two-value form // - typeassertionokdiscarded — flags two-value type assertions whose ok result is discarded // - uncheckedsliceindex — reports slice and string indexes without proven bounds +// - unchecked_deferredclose — reports defer Close() calls that ignore error return values // - uncheckedtypeassertion — flags unchecked single-value type assertions // - uncheckedflushreturn — flags Flush() method calls where the error return is discarded // - walkfuncerrshadow — flags filepath.Walk/WalkDir callbacks whose err parameter shadows an outer err variable assigned from the walk call diff --git a/pkg/linters/doc_sync_test.go b/pkg/linters/doc_sync_test.go index 12c804a08d9..e3231b9e4e2 100644 --- a/pkg/linters/doc_sync_test.go +++ b/pkg/linters/doc_sync_test.go @@ -17,8 +17,8 @@ import ( ) var ( - docBulletRe = regexp.MustCompile(`^//\s+-\s+([a-z0-9-]+)\s+—`) - readmeTableRe = regexp.MustCompile(`^\|\s+` + "`" + `([a-z0-9-]+)` + "`" + `\s+\|`) + docBulletRe = regexp.MustCompile(`^//\s+-\s+([a-z0-9_-]+)\s+—`) + readmeTableRe = regexp.MustCompile(`^\|\s+` + "`" + `([a-z0-9_-]+)` + "`" + `\s+\|`) linterFlagsRe = regexp.MustCompile(`LINTER_FLAGS="([^"]+)"`) ) @@ -43,6 +43,7 @@ var notYetEnforced = map[string]string{ "stringsconcatloop": "has not yet completed an enforcement-readiness audit", "typeassertionnil": "needs a cross-platform enforcement-readiness audit before CI enables it", "typeassertionokdiscarded": "existing production violations need remediation before enforcement; nolint suppression already works", + "unchecked_deferredclose": "existing deferred Close() findings need remediation before CI enforcement", "uncheckedsliceindex": "existing production violations need remediation before enforcement; nolint suppression already works", } diff --git a/pkg/linters/registry.go b/pkg/linters/registry.go index 17699e244d8..e02891d77b1 100644 --- a/pkg/linters/registry.go +++ b/pkg/linters/registry.go @@ -75,6 +75,7 @@ import ( "github.com/github/gh-aw/pkg/linters/trimleftright" "github.com/github/gh-aw/pkg/linters/typeassertionnil" "github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded" + unchecked_deferredclose "github.com/github/gh-aw/pkg/linters/unchecked-deferred-close" uncheckedsliceindex "github.com/github/gh-aw/pkg/linters/unchecked-slice-index" "github.com/github/gh-aw/pkg/linters/uncheckedflushreturn" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" @@ -166,6 +167,7 @@ var allAnalyzers = []*analysis.Analyzer{ typeassertionnil.Analyzer, typeassertionokdiscarded.Analyzer, uncheckedsliceindex.Analyzer, + unchecked_deferredclose.Analyzer, uncheckedtypeassertion.Analyzer, uncheckedflushreturn.Analyzer, walkfuncerrshadow.Analyzer, diff --git a/pkg/linters/spec_test.go b/pkg/linters/spec_test.go index 75a777f865a..48196b77653 100644 --- a/pkg/linters/spec_test.go +++ b/pkg/linters/spec_test.go @@ -83,6 +83,7 @@ import ( "github.com/github/gh-aw/pkg/linters/trimleftright" "github.com/github/gh-aw/pkg/linters/typeassertionnil" "github.com/github/gh-aw/pkg/linters/typeassertionokdiscarded" + unchecked_deferredclose "github.com/github/gh-aw/pkg/linters/unchecked-deferred-close" uncheckedsliceindex "github.com/github/gh-aw/pkg/linters/unchecked-slice-index" "github.com/github/gh-aw/pkg/linters/uncheckedflushreturn" "github.com/github/gh-aw/pkg/linters/uncheckedtypeassertion" @@ -104,7 +105,7 @@ type docAnalyzer struct { } // documentedAnalyzers returns the analyzer subpackages documented in the README -// "Public API > Subpackages" table. The README documents 78 analyzer +// "Public API > Subpackages" table. The README documents 79 analyzer // subpackages (the non-analyzer `internal` helper subpackage is excluded because // it exposes no Analyzer). // @@ -116,7 +117,7 @@ type docAnalyzer struct { // logfatallibrary, manualmutexunlock, manualpathconcat, mapclearloop, mapdeletecheck, nilctxpassed, osexitinlibrary, osgetenvlibrary, ossetenvlibrary, packagelevelmutableslicemap, panic-in-library-code, rawloginlib, // reflectdeepequalusage, regexpcompileinfunction, regexpdynamicpattern, seenmapbool, slicemakezerolength, sortslice, sprintferrdot, sprintferrorsnew, sprintfbool, sprintfint, ssljson, // strconvparseignorederror, stringbytesroundtrip, stringreplaceminusone, stringsconcatloop, stringscountcontains, stringsindexcontains, stringsindexhasprefix, stringsjoinone, timeafterleak, timesleepnocontext, timenowsub, -// tolowerequalfold, trimleftright, typeassertionnil, typeassertionokdiscarded, uncheckedflushreturn, uncheckedsliceindex, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring +// tolowerequalfold, trimleftright, typeassertionnil, typeassertionokdiscarded, unchecked_deferredclose, uncheckedflushreturn, uncheckedsliceindex, uncheckedtypeassertion, walkfuncerrshadow, wgdonenotdeferred, writebytestring func documentedAnalyzers() []docAnalyzer { return []docAnalyzer{ {"appendbytestring", appendbytestring.Analyzer}, @@ -191,6 +192,7 @@ func documentedAnalyzers() []docAnalyzer { {"trimleftright", trimleftright.Analyzer}, {"typeassertionnil", typeassertionnil.Analyzer}, {"typeassertionokdiscarded", typeassertionokdiscarded.Analyzer}, + {"unchecked_deferredclose", unchecked_deferredclose.Analyzer}, {"uncheckedsliceindex", uncheckedsliceindex.Analyzer}, {"uncheckedtypeassertion", uncheckedtypeassertion.Analyzer}, {"uncheckedflushreturn", uncheckedflushreturn.Analyzer}, diff --git a/pkg/linters/unchecked-deferred-close/testdata/src/basic/bad.go b/pkg/linters/unchecked-deferred-close/testdata/src/basic/bad.go new file mode 100644 index 00000000000..c1cd46bf95d --- /dev/null +++ b/pkg/linters/unchecked-deferred-close/testdata/src/basic/bad.go @@ -0,0 +1,44 @@ +package basic + +import ( + "io" + "os" +) + +type NamedCloser interface { + io.Closer +} + +type EmbeddedCloser struct { + io.Closer +} + +type GenericCloser interface { + Close() error +} + +func bad(filename string) error { + f, err := os.Open(filename) + if err != nil { + return err + } + defer f.Close() // want "defer close\\(\\) call ignores error return value" + // ... use f + return nil +} + +func badIOCloser(c io.Closer) { + defer c.Close() // want "defer close\\(\\) call ignores error return value" +} + +func badNamedInterface(c NamedCloser) { + defer c.Close() // want "defer close\\(\\) call ignores error return value" +} + +func badEmbeddedCloser(c EmbeddedCloser) { + defer c.Close() // want "defer close\\(\\) call ignores error return value" +} + +func badTypeParameter[T GenericCloser](c T) { + defer c.Close() // want "defer close\\(\\) call ignores error return value" +} diff --git a/pkg/linters/unchecked-deferred-close/testdata/src/basic/good.go b/pkg/linters/unchecked-deferred-close/testdata/src/basic/good.go new file mode 100644 index 00000000000..d8b18f75ad0 --- /dev/null +++ b/pkg/linters/unchecked-deferred-close/testdata/src/basic/good.go @@ -0,0 +1,30 @@ +package basic + +import ( + "fmt" + "os" +) + +func good(filename string) error { + f, err := os.Open(filename) + if err != nil { + return err + } + defer func() { + if err := f.Close(); err != nil { + fmt.Fprintf(os.Stderr, "close error: %v\n", err) + } + }() + // ... use f + return nil +} + +// NoReturnClose is a mock type that has Close() with no error return +type NoReturnClose struct{} + +func (n *NoReturnClose) Close() {} + +func goodNoError() { + n := &NoReturnClose{} + defer n.Close() // OK: Close() returns nothing +} diff --git a/pkg/linters/unchecked-deferred-close/unchecked_deferredclose.go b/pkg/linters/unchecked-deferred-close/unchecked_deferredclose.go new file mode 100644 index 00000000000..03a73ce9f40 --- /dev/null +++ b/pkg/linters/unchecked-deferred-close/unchecked_deferredclose.go @@ -0,0 +1,94 @@ +// Package unchecked deferredclose implements a Go analysis linter that flags +// defer close() calls without error checking, which can hide resource cleanup failures. +package unchecked_deferredclose + +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 unchecked-deferred-close analysis pass. +var Analyzer = analyzerutil.New("unchecked_deferredclose", "reports defer close() calls without error checking that may hide resource cleanup failures", run) + +var builtinErrorType = types.Universe.Lookup("error").Type() + +func run(pass *analysis.Pass) (any, error) { + noLintIndex, generatedFiles, err := analyzerutil.Indexes(pass) + if err != nil { + return nil, err + } + + nodeFilter := []ast.Node{(*ast.DeferStmt)(nil)} + return analyzerutil.Preorder(pass, nodeFilter, func(n ast.Node) { + analyzeDeferClose(pass, n, generatedFiles, noLintIndex) + }) +} + +// analyzeDeferClose checks for defer close() calls that don't handle errors. +func analyzeDeferClose(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { + deferStmt, ok := n.(*ast.DeferStmt) + if !ok { + return + } + + // The defer statement's Call is already a *CallExpr + callExpr := deferStmt.Call + if callExpr == nil { + return + } + + // Check if the call is to a Close() method + selector, ok := callExpr.Fun.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != "Close" { + return + } + + // Ensure it has no arguments + if len(callExpr.Args) != 0 { + return + } + + position := pass.Fset.PositionFor(deferStmt.Pos(), false) + if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) { + return + } + + if nolint.HasDirectiveForLinter(position, noLintIndex, "unchecked_deferredclose") { + return + } + + if pass.TypesInfo == nil { + return + } + + selection := pass.TypesInfo.Selections[selector] + if selection == nil || selection.Kind() != types.MethodVal { + return + } + + sig, ok := selection.Obj().Type().(*types.Signature) + if !ok || !closeReturnsError(sig) { + return + } + + // Report: defer close() ignores error return + pass.Reportf( + deferStmt.Pos(), + "defer close() call ignores error return value; consider handling errors explicitly", + ) +} + +func closeReturnsError(sig *types.Signature) bool { + if sig.Results() == nil || sig.Results().Len() == 0 { + return false + } + + lastResult := sig.Results().At(sig.Results().Len() - 1) + return lastResult.Type() == builtinErrorType +} diff --git a/pkg/linters/unchecked-deferred-close/unchecked_deferredclose_test.go b/pkg/linters/unchecked-deferred-close/unchecked_deferredclose_test.go new file mode 100644 index 00000000000..037cd9b8911 --- /dev/null +++ b/pkg/linters/unchecked-deferred-close/unchecked_deferredclose_test.go @@ -0,0 +1,17 @@ +//go:build !integration + +// Package unchecked_deferredclose_test provides tests for the unchecked_deferredclose analyzer. +package unchecked_deferredclose_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + unchecked_deferredclose "github.com/github/gh-aw/pkg/linters/unchecked-deferred-close" +) + +func TestUncheckedDeferredClose(t *testing.T) { + t.Parallel() + analysistest.Run(t, analysistest.TestData(), unchecked_deferredclose.Analyzer, "basic") +}