[linter-miner] feat(linters): add ioutildeprecated linter to flag deprecated io/ioutil usage#44998
Conversation
Flags calls to deprecated io/ioutil functions (deprecated since Go 1.16) and reports the io or os package replacements: - ioutil.ReadAll → io.ReadAll - ioutil.ReadFile → os.ReadFile - ioutil.WriteFile → os.WriteFile - ioutil.TempFile → os.CreateTemp - ioutil.TempDir → os.MkdirTemp - ioutil.ReadDir → os.ReadDir - ioutil.NopCloser → io.NopCloser - ioutil.Discard → io.Discard Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great addition from the Linter Miner workflow! 🎉 This
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds a custom Go analyzer that detects deprecated io/ioutil APIs and recommends modern replacements.
Changes:
- Implements and registers
ioutildeprecated. - Adds analysistest coverage and fixtures.
- Two gaps remain: dot imports and incomplete replacement assertions.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers the analyzer. |
pkg/linters/ioutildeprecated/ioutildeprecated.go |
Implements deprecated API detection. |
pkg/linters/ioutildeprecated/ioutildeprecated_test.go |
Runs analyzer tests. |
pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go |
Provides test fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
| } | ||
| noLintLinesByFile := nolint.BuildLineIndex(pass, "ioutildeprecated") | ||
|
|
||
| for cur := range root.Preorder((*ast.SelectorExpr)(nil)) { |
|
|
||
| funcName := sel.Sel.Name | ||
| if replacement, found := replacements[funcName]; found { | ||
| pass.ReportRangef(sel, "ioutil.%s is deprecated; use %s instead", funcName, replacement) |
There was a problem hiding this comment.
Skills-Based Review
Applied /tdd - requesting changes on test coverage gaps.
Key Themes
- Missing test cases:
NopCloserandDiscardare in the replacements map but absent from the test fixture, leaving their detection unverified. - Weak negative test:
GoodReadAlldoes not actually callio.ReadAll, so it does not confirm the modern API produces no warning.
Positive Highlights
- Clean type-based detection using
types.PkgName- robust against aliased imports. - Correctly skips test files and respects
nolintdirectives via shared internal utilities. - Error messages are clear and actionable.
- Follows the established analyzer pattern consistently with other linters.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 25.9 AIC · ⌖ 4.52 AIC · ⊞ 6.6K
Comment /matt to run again
| defer f.Close() | ||
| // Using io.ReadAll is fine | ||
| _ = f | ||
| } |
There was a problem hiding this comment.
[/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.
|
|
||
| func BadReadDir() { | ||
| _, _ = ioutil.ReadDir(".") // want `ioutil\.ReadDir is deprecated` | ||
| } |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (1 test)
Notes
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Review: ioutildeprecated linter
Overall the implementation is clean and follows existing linter patterns correctly. One non-blocking issue found:
Missing test coverage for NopCloser and Discard — the replacements map has 8 entries but the testdata file only exercises 6. See the inline comment for suggested additions.
Everything else looks good: type-resolution via types.PkgName is correct and avoids false positives from local ioutil-named variables; nolint and filecheck integrations follow the established pattern; registration order in main.go is alphabetically correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19.7 AIC · ⌖ 4.4 AIC · ⊞ 4.8K
| defer f.Close() | ||
| // Using io.ReadAll is fine | ||
| _ = f | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Non-blocking observations on ioutildeprecated
The linter logic is correct and the type-based package detection approach is sound. Four issues worth fixing before or after merge:
Findings summary
- Analyzer list ordering (
cmd/linters/main.go:87) —ioutildeprecatedwas inserted beforehttpstatuscode, breaking alphabetical order; it belongs after allhttp*entries. - Missing fixtures for
NopCloserandDiscard— two replacements map entries have no testdata coverage, so regressions won't be caught. pass.TypesInfo == nilinside the hot loop — should be a single guard at the top ofrun(), not a per-node check.GoodReadAllfixture is a no-op — it doesn't call any ioutil or io symbol, so it doesn't verify the linter's silence on modern APIs.
🔎 Code quality review by PR Code Quality Reviewer · 41.2 AIC · ⌖ 4.73 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
cmd/linters/main.go:87
Alphabetical ordering broken: ioutildeprecated is inserted before httpstatuscode, but 'i' sorts after 'h' — this violates the alphabetical convention all other entries follow.
<details>
<summary>💡 Fix</summary>
Move the registration so the list reads:
httprespbodyclose.Analyzer,
httpstatuscode.Analyzer,
ioutildeprecated.Analyzer, // ← after all http* entries
largefunc.Analyzer,The full list uses strict alphabetical ordering; keeping it sorted makes future insertions pre…
pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go:39
Missing test fixtures for NopCloser and Discard: both entries are in the replacements map but have zero coverage in the testdata file — any regression in detecting them goes unnoticed.
<details>
<summary>💡 Suggested additions</summary>
Add to the testdata file:
func BadNopCloser() {
r := strings.NewReader("hello")
_ = ioutil.NopCloser(r) // want `ioutil\.NopCloser is deprecated`
}
func BadDiscard() {
_, _ = io.Copy(ioutil.Discard, strings.NewReader("hi")) // want …
</details>
<details><summary>pkg/linters/ioutildeprecated/ioutildeprecated.go:90</summary>
**`pass.TypesInfo == nil` check belongs at the top of `run()`, not inside the loop**: `TypesInfo` is set by the analysis framework before `Run` is called and will never be nil in a valid pass. Checking it on every `SelectorExpr` node is dead-branch overhead.
<details>
<summary>💡 Fix</summary>
Move the guard to the top of `run()`:
```go
func run(pass *analysis.Pass) (any, error) {
if pass.TypesInfo == nil {
return nil, nil
}
root, err := astutil.Root(pass)
// ...…
pkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.go:37
GoodReadAll doesn't test the actual true-negative case: the function opens a file and assigns to blank but never calls io.ReadAll or any ioutil symbol. It doesn't verify the linter stays silent when modern APIs are used.
<details>
<summary>💡 Suggested improvement</summary>
Replace with something that actually exercises the linter's non-flagging path:
func GoodReadAll() {
f, _ := os.Open("file.txt")
defer f.Close()
_, _ = io.ReadAll(f) // no diagnostic expected
}
``…
</details>Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (138 new lines in Draft ADR committed: This PR cannot merge until an ADR is linked in the PR body. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29187567992
|
…stic assertions - Add second Ident-walk loop that uses TypesInfo.Uses to detect io/ioutil symbols imported via `import . "io/ioutil"` (bare identifiers, no selector) - Add dot_import.go test fixture exercising ReadAll, ReadFile, WriteFile via dot import - Extend all want-patterns to assert the full diagnostic message including the replacement (e.g. `use io.ReadAll instead`) - Add NopCloser and Discard test cases (previously untested) - Replace GoodReadAll stub with real modern-API good cases (io.ReadAll, os.ReadFile, os.WriteFile, io.Discard) - Use named fileMode constant in good-case fixture to satisfy the repo-wide no-raw-octal-permission-literal policy Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…e in fixture Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review threads from the previous session are already addressed in commits CI needs a human re-trigger (close/reopen or |
|
\n@copilot please run the skill, address the unresolved review feedback on dot-import handling and missing/weak replacement tests for , then rerun checks once the branch is ready.
|
|
@copilot please run the
|
All of this is already in the current HEAD. Commits
All five threads are addressed in commits
|
|
\n@copilot please run the skill, address any unresolved review feedback, and rerun checks once the branch is up to date.
|
|
@copilot please run the
|
All review feedback was already addressed in commits
Hand-off: CI must be re-triggered by a maintainer before merge. |
|
@copilot please run the
|
No new feedback to address. All five open threads remain covered by commits
Branch is up to date, nothing uncommitted. |
|
@copilot please run the Unresolved review threads (newest first):
|
All five threads are already addressed in commits
|
|
@copilot please run the
|
…precated-c8e0c4cd0482d7b4 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Branch refreshed — merged latest |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom Go analysis linter,
ioutildeprecated, that flags references to the deprecatedio/ioutilpackage (deprecated since Go 1.16) and reports the modernioorosreplacement for each symbol.Changes
cmd/linters/main.goioutildeprecated.Analyzerin the linter runnerpkg/linters/ioutildeprecated/ioutildeprecated.gopkg/linters/ioutildeprecated/ioutildeprecated_test.goanalysistest-based unit testpkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/ioutildeprecated.gopkg/linters/ioutildeprecated/testdata/src/ioutildeprecated/dot_import.godocs/adr/44998-add-ioutildeprecated-linter.mdBehavior
ioutilsymbols:ReadAll→io.ReadAll,ReadFile→os.ReadFile,WriteFile→os.WriteFile,TempFile→os.CreateTemp,TempDir→os.MkdirTemp,ReadDir→os.ReadDir,NopCloser→io.NopCloser,Discard→io.Discard.ioutil.ReadAll(r)) viaast.SelectorExprtraversal and dot-import usage (ReadAll(r)after. "io/ioutil") viaast.Identtraversal withTypesInfo.Usesresolution.pass.TypesInfo) so arbitrary import aliases (e.g.,import ioutil2 "io/ioutil") are handled correctly.filecheck.IsTestFile) and respectsnolint:ioutildeprecateddirectives.pkg/orcmd/at time of addition.Diagnostic format
Alternatives considered
staticcheckSA1019 — not chosen; project owns its linter suite for actionable messages and nolint semantics.golangci-lintdepguard/forbidigo — not chosen; same reason plus import-level blocking conflicts with test fixtures.ADR
See
docs/adr/44998-add-ioutildeprecated-linter.md(status: Draft).