Skip to content
2 changes: 2 additions & 0 deletions cmd/linters/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -82,6 +83,7 @@ func main() {
hardcodedfilepath.Analyzer,
httpnoctx.Analyzer,
httprespbodyclose.Analyzer,
ioutildeprecated.Analyzer,
httpstatuscode.Analyzer,
largefunc.Analyzer,
manualmutexunlock.Analyzer,
Expand Down
48 changes: 48 additions & 0 deletions docs/adr/44998-add-ioutildeprecated-linter.md
Original file line number Diff line number Diff line change
@@ -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.*
123 changes: 123 additions & 0 deletions pkg/linters/ioutildeprecated/ioutildeprecated.go
Original file line number Diff line number Diff line change
@@ -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
}
16 changes: 16 additions & 0 deletions pkg/linters/ioutildeprecated/ioutildeprecated_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
Original file line number Diff line number Diff line change
@@ -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`
}
Original file line number Diff line number Diff line change
@@ -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`
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The GoodReadAll negative test doesn't actually call io.ReadAll — it opens a file and discards it, so it doesn't demonstrate that the modern replacement is accepted correctly.

💡 Suggested fix
func GoodReadAll() {
    f, _ := os.Open("file.txt")
    defer f.Close()
    _, _ = io.ReadAll(f) // no diagnostic expected — this is the modern API
}

This makes the test a genuine specification: "using io.ReadAll produces no warning."

@copilot please address this.


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)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] NopCloser and Discard are in the replacements map but have no corresponding test fixtures — so their detection is untested.

💡 Suggested additions
func BadNopCloser() {
    r := strings.NewReader("hello")
    _ = ioutil.NopCloser(r) // want `ioutil\.NopCloser is deprecated`
}

func BadDiscard() {
    _, _ = fmt.Fprintf(ioutil.Discard, "hello") // want `ioutil\.Discard is deprecated`
}

Without these cases the analyzer could silently stop reporting them and the test suite would still pass.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testdata file covers 6 of the 8 entries in replacements but is missing test cases for NopCloser and Discard. Without these, the linter could silently fail to fire (or fire incorrectly) for those two entries. Please add cases such as:

func BadNopCloser() {
	r := strings.NewReader("")
	_ = ioutil.NopCloser(r) // want `ioutil\.NopCloser is deprecated`
}

func BadDiscard() {
	_, _ = io.Copy(ioutil.Discard, strings.NewReader("")) // want `ioutil\.Discard is deprecated`
}

@copilot please address this.

Loading