diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 2c3571f0d4c..aecf3a99b6a 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -34,6 +34,7 @@ import ( "github.com/github/gh-aw/pkg/linters/httpnoctx" "github.com/github/gh-aw/pkg/linters/httprespbodyclose" "github.com/github/gh-aw/pkg/linters/httpstatuscode" + "github.com/github/gh-aw/pkg/linters/ioutildeprecated" "github.com/github/gh-aw/pkg/linters/jsonmarshalignoredeerror" "github.com/github/gh-aw/pkg/linters/largefunc" "github.com/github/gh-aw/pkg/linters/lenstringsplit" @@ -82,6 +83,7 @@ func main() { hardcodedfilepath.Analyzer, httpnoctx.Analyzer, httprespbodyclose.Analyzer, + ioutildeprecated.Analyzer, httpstatuscode.Analyzer, largefunc.Analyzer, manualmutexunlock.Analyzer, diff --git a/docs/adr/44998-add-ioutildeprecated-linter.md b/docs/adr/44998-add-ioutildeprecated-linter.md new file mode 100644 index 00000000000..395277f7f13 --- /dev/null +++ b/docs/adr/44998-add-ioutildeprecated-linter.md @@ -0,0 +1,48 @@ +# ADR-44998: Add ioutildeprecated Linter to Flag Deprecated io/ioutil Usage + +**Date**: 2026-07-12 +**Status**: Draft +**Deciders**: pelikhan, linter-miner automation + +--- + +### Context + +The `io/ioutil` package was deprecated in Go 1.16 (February 2021). All its functions are thin wrappers around equivalents in the `io` and `os` packages (e.g., `ioutil.ReadAll` → `io.ReadAll`, `ioutil.ReadFile` → `os.ReadFile`). Using the deprecated package adds unnecessary indirection, signals outdated code, and may confuse readers unfamiliar with the deprecation. The repository uses a custom in-house static analysis framework (`golang.org/x/tools/go/analysis`) to enforce codebase conventions. A scan of `pkg/` and `cmd/` found no existing usages, making this a preventive measure to ensure new code does not introduce deprecated API calls. + +### Decision + +We will add a new custom Go analysis linter, `ioutildeprecated`, that flags any call-site reference to functions in the `io/ioutil` package and reports the modern replacement (`io.ReadAll`, `os.ReadFile`, etc.). The linter integrates with the existing in-house linter runner (`cmd/linters/main.go`) and follows the established pattern of custom analyzers in `pkg/linters/`. It skips test files and respects `nolint` directives for escape hatches. + +### Alternatives Considered + +#### Alternative 1: Rely on staticcheck's SA1019 rule + +`staticcheck` detects usage of deprecated symbols, including all `io/ioutil` functions, via its `SA1019` (deprecated API usage) diagnostic. It is already well-known in the Go ecosystem and requires no custom code. + +This was not chosen because the repository maintains its own linter suite for fine-grained control over error messages, nolint escape-hatch semantics, and integration with the internal `filecheck`/`nolint`/`astutil` utilities. Adding another external tool would fragment the linting surface and require separate CI configuration. The in-house linter can produce a more actionable message (naming the exact replacement) and follows the already-established contribution pattern for this codebase. + +#### Alternative 2: Use golangci-lint with depguard or forbidigo + +`golangci-lint` aggregates many linters and offers `depguard` (package import deny-listing) and `forbidigo` (symbol pattern banning) that could block `io/ioutil` usage with configuration only, requiring no new Go code. + +This was not chosen for the same reason as Alternative 1: the project has chosen to own its linter implementations rather than depend on an external aggregator. Additionally, `depguard` operates at the import level and would block the import even in the test fixture files, requiring additional exclusion configuration; the custom linter already handles this by skipping test files explicitly. + +### Consequences + +#### Positive +- New production code calling deprecated `io/ioutil` APIs is caught at CI time before merging, preventing future technical debt. +- The linter message names the exact modern replacement function, making remediation obvious without requiring the developer to look up the deprecation notice. +- Follows the existing contribution pattern for in-house linters, keeping the codebase consistent. + +#### Negative +- A new linter package must be maintained; if Go adds or removes `io/ioutil` symbols in a future version, the `replacements` map requires manual updates. +- The linter only catches selector expressions (`ioutil.X`) resolved through the type system; unusual import aliases (e.g., `import ioutil2 "io/ioutil"`) are handled correctly by the type-based check, but the dependency on `pass.TypesInfo` means incomplete type information (e.g., in ill-formed packages) silently skips the check. + +#### Neutral +- The linter is purely preventive: no existing usages were found in the repository at the time of addition, so there are no immediate remediation tasks. +- The `ioutil.Discard` and `ioutil.NopCloser` symbols are variables/functions (not just functions), but the selector-expression traversal handles them uniformly with the same detection logic. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/ioutildeprecated/ioutildeprecated.go b/pkg/linters/ioutildeprecated/ioutildeprecated.go new file mode 100644 index 00000000000..99ab41252b1 --- /dev/null +++ b/pkg/linters/ioutildeprecated/ioutildeprecated.go @@ -0,0 +1,123 @@ +// Package ioutildeprecated implements a Go analysis linter that flags calls to +// functions from the deprecated io/ioutil package and suggests their replacements +// in the io and os packages (deprecated since Go 1.16). +package ioutildeprecated + +import ( + "go/ast" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// replacements maps deprecated ioutil function names to their modern equivalents. +var replacements = map[string]string{ + "ReadAll": "io.ReadAll", + "ReadFile": "os.ReadFile", + "WriteFile": "os.WriteFile", + "TempFile": "os.CreateTemp", + "TempDir": "os.MkdirTemp", + "ReadDir": "os.ReadDir", + "NopCloser": "io.NopCloser", + "Discard": "io.Discard", +} + +// Analyzer is the ioutil-deprecated analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "ioutildeprecated", + Doc: "reports uses of deprecated io/ioutil functions that should be replaced with io or os package equivalents", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/ioutildeprecated", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + root, err := astutil.Root(pass) + if err != nil { + return nil, err + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "ioutildeprecated") + + if pass.TypesInfo == nil { + return nil, nil + } + + // Handle regular qualified imports: ioutil.ReadAll(...), ioutil.Discard, etc. + for cur := range root.Preorder((*ast.SelectorExpr)(nil)) { + sel, ok := cur.Node().(*ast.SelectorExpr) + if !ok { + continue + } + + pos := pass.Fset.PositionFor(sel.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + continue + } + if nolint.HasDirective(pos, noLintLinesByFile) { + continue + } + + pkgIdent, ok := sel.X.(*ast.Ident) + if !ok { + continue + } + obj := pass.TypesInfo.ObjectOf(pkgIdent) + if obj == nil { + continue + } + pkgName, ok := obj.(*types.PkgName) + if !ok || pkgName.Imported().Path() != "io/ioutil" { + continue + } + + funcName := sel.Sel.Name + if replacement, found := replacements[funcName]; found { + pass.ReportRangef(sel, "ioutil.%s is deprecated; use %s instead", funcName, replacement) + } + } + + // Handle dot imports: import . "io/ioutil" followed by bare ReadAll(r) or Discard. + // In this case the identifier is an *ast.Ident (not a SelectorExpr), and + // TypesInfo.Uses resolves it to an object whose package path is "io/ioutil". + for cur := range root.Preorder((*ast.Ident)(nil)) { + ident, ok := cur.Node().(*ast.Ident) + if !ok { + continue + } + + // Skip identifiers that are the Sel field of a SelectorExpr; those are + // already handled by the qualified-import loop above. + if _, ok := cur.Parent().Node().(*ast.SelectorExpr); ok { + continue + } + + pos := pass.Fset.PositionFor(ident.Pos(), false) + if filecheck.IsTestFile(pos.Filename) { + continue + } + if nolint.HasDirective(pos, noLintLinesByFile) { + continue + } + + obj := pass.TypesInfo.Uses[ident] + if obj == nil { + continue + } + pkg := obj.Pkg() + if pkg == nil || pkg.Path() != "io/ioutil" { + continue + } + + name := obj.Name() + if replacement, found := replacements[name]; found { + pass.ReportRangef(ident, "ioutil.%s is deprecated; use %s instead", name, replacement) + } + } + + return nil, nil +} diff --git a/pkg/linters/ioutildeprecated/ioutildeprecated_test.go b/pkg/linters/ioutildeprecated/ioutildeprecated_test.go new file mode 100644 index 00000000000..36ff1c85853 --- /dev/null +++ b/pkg/linters/ioutildeprecated/ioutildeprecated_test.go @@ -0,0 +1,16 @@ +//go:build !integration + +package ioutildeprecated_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/ioutildeprecated" +) + +func TestIoutilDeprecated(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, ioutildeprecated.Analyzer, "ioutildeprecated") +} diff --git a/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/dot_import.go b/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/dot_import.go new file mode 100644 index 00000000000..55505d505e4 --- /dev/null +++ b/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/dot_import.go @@ -0,0 +1,21 @@ +package ioutildeprecated + +import ( + "os" + // dot-import makes all exported names of io/ioutil available unqualified + . "io/ioutil" +) + +func BadDotReadAll() { + f, _ := os.Open("file.txt") + defer f.Close() + _, _ = ReadAll(f) // want `ioutil\.ReadAll is deprecated; use io\.ReadAll instead` +} + +func BadDotReadFile() { + _, _ = ReadFile("file.txt") // want `ioutil\.ReadFile is deprecated; use os\.ReadFile instead` +} + +func BadDotWriteFile() { + _ = WriteFile("file.txt", []byte{}, 0644) // want `ioutil\.WriteFile is deprecated; use os\.WriteFile instead` +} diff --git a/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go b/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go new file mode 100644 index 00000000000..5427f951189 --- /dev/null +++ b/pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go @@ -0,0 +1,63 @@ +package ioutildeprecated + +import ( + "io" + "io/fs" + "io/ioutil" + "os" +) + +// fileMode is a named permission constant used for good-case examples, avoiding raw octal literals. +const fileMode fs.FileMode = 0o644 + +func BadReadAll() { + f, _ := os.Open("file.txt") + defer f.Close() + _, _ = ioutil.ReadAll(f) // want `ioutil\.ReadAll is deprecated; use io\.ReadAll instead` +} + +func BadReadFile() { + _, _ = ioutil.ReadFile("file.txt") // want `ioutil\.ReadFile is deprecated; use os\.ReadFile instead` +} + +func BadWriteFile() { + _ = ioutil.WriteFile("file.txt", []byte("hello"), 0644) // want `ioutil\.WriteFile is deprecated; use os\.WriteFile instead` +} + +func BadTempFile() { + _, _ = ioutil.TempFile("", "prefix") // want `ioutil\.TempFile is deprecated; use os\.CreateTemp instead` +} + +func BadTempDir() { + _, _ = ioutil.TempDir("", "prefix") // want `ioutil\.TempDir is deprecated; use os\.MkdirTemp instead` +} + +func BadReadDir() { + _, _ = ioutil.ReadDir(".") // want `ioutil\.ReadDir is deprecated; use os\.ReadDir instead` +} + +func BadNopCloser() { + _ = ioutil.NopCloser(nil) // want `ioutil\.NopCloser is deprecated; use io\.NopCloser instead` +} + +func BadDiscard() { + _, _ = io.Copy(ioutil.Discard, nil) // want `ioutil\.Discard is deprecated; use io\.Discard instead` +} + +func GoodReadAll() { + f, _ := os.Open("file.txt") + defer f.Close() + _, _ = io.ReadAll(f) +} + +func GoodReadFile() { + _, _ = os.ReadFile("file.txt") +} + +func GoodWriteFile() { + _ = os.WriteFile("file.txt", []byte("hello"), fileMode) +} + +func GoodDiscard() { + _, _ = io.Copy(io.Discard, nil) +}