fix: migrate 5 linters from syntactic stdlib-package matching to astutil.IsPkgSelector#43939
Conversation
…til.IsPkgSelector Replaces ident.Name == "<pkg>" checks with the type-resolved astutil.IsPkgSelector(pass, sel, pkgPath) helper in: - errstringmatch (strings) - fprintlnsprintf (fmt) - osexitinlibrary (os) - fileclosenotdeferred (os) - contextcancelnotdeferred (context) Adds testdata alias and shadowing cases for all 5 linters. Closes #43934 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens several go/analysis linters by replacing syntactic stdlib “package name” checks (e.g., ident.Name == "os") with type-resolved package identity checks via astutil.IsPkgSelector, eliminating false positives from local shadowing and false negatives from aliased imports.
Changes:
- Migrates package-selector guards in 5 linters to
astutil.IsPkgSelector(pass, sel, "<pkgPath>")(type-driven package resolution). - Adds per-linter testdata covering aliased imports and (where relevant) local shadowing to prevent regressions.
- Updates helper function signatures (e.g.,
isFmtFunc,isFileOpenCall,isContextWithCancelCall) to accept*analysis.Passwhere type info is needed.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/osexitinlibrary/osexitinlibrary.go | Switches os.Exit detection to astutil.IsPkgSelector to avoid alias/shadowing issues. |
| pkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/osexitinlibrary_alias.go | Adds alias + shadowing coverage for osexitinlibrary. |
| pkg/linters/fprintlnsprintf/fprintlnsprintf.go | Uses type-resolved fmt package matching for Fprintln/Sprintf detection. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf_alias.go | Adds alias + shadowing coverage for fprintlnsprintf diagnostics. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf_alias.go.golden | Adds expected post-fix output for the new alias test case. |
| pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go | Updates os.Open/Create/OpenFile detection to use type-resolved package identity. |
| pkg/linters/fileclosenotdeferred/testdata/src/fileclosenotdeferred/fileclosenotdeferred_alias.go | Adds aliased os import coverage for file-open detection. |
| pkg/linters/errstringmatch/errstringmatch.go | Updates strings.<func> detection to use astutil.IsPkgSelector for alias/shadowing correctness. |
| pkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch_alias.go | Adds alias + shadowing coverage for errstringmatch. |
| pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go | Updates context.With* detection to use type-resolved package identity. |
| pkg/linters/contextcancelnotdeferred/testdata/src/contextcancelnotdeferred/contextcancelnotdeferred_alias.go | Adds aliased context import coverage for cancel detection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Low
| @@ -120,14 +120,10 @@ func buildFprintfFix(call *ast.CallExpr, sprintfCall *ast.CallExpr) []analysis.S | |||
| } | |||
|
|
|||
| // isFmtFunc returns true if call is a call to fmt.<name>. | |||
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR adds testdata fixture files (*_alias.go) used as linter input programs, and modifies production linter source, but no *_test.go or .test.cjs/.test.js test runner files were changed. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (141 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: migrate linters to astutil.IsPkgSelector
The changes correctly fix false negatives (aliased imports) and false positives (shadowed local variables) across five linters by replacing syntactic ident.Name == "<pkg>" guards with the semantic astutil.IsPkgSelector(pass, sel, "<pkg>") helper.
Implementation quality is high:
astutil.IsPkgSelectorresolves throughpass.TypesInfoto the actual imported package path — the right level of resolution.- All five linters (
errstringmatch,fprintlnsprintf,osexitinlibrary,fileclosenotdeferred,contextcancelnotdeferred) are updated consistently. - Each linter gains an
_alias.gotestdata file covering both the alias case (should fire) and the shadow case (should not fire). - The
fprintlnsprintflinter additionally ships a matching.goldenfile, ensuring the suggested fix is correctly applied when an alias is used.
Minor observation (non-blocking): IsFmtErrorf in astutil.go still uses the older three-step pattern instead of delegating to IsPkgSelector. This is outside this PR's scope but is an obvious follow-up to reduce duplication.
No blocking issues found.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 69.5 AIC · ⌖ 5.91 AIC · ⊞ 4.8K
Review SummaryApplied Verdict: No blocking issues — the root cause is correctly fixed. Four targeted test-coverage gaps flagged as inline comments: 📋 Inline findings
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the core bug fix is clean and well-motivated; leaving as COMMENT with four test-coverage suggestions.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: alias test files cover the primary path for each linter, but a few variants inside the same linter go unexercised (see inline comments)
- Asymmetric shadow coverage: three linters (
osexitinlibrary,errstringmatch,fileclosenotdeferred) have avar <pkg> fake<Type>{}shadowing negative case;contextcancelnotdeferreddoes not
Positive Highlights
- ✅ Root cause is properly addressed — syntactic
ident.Namematching replaced with type-awareastutil.IsPkgSelectoracross all five linters - ✅ Every linter gets at least one alias positive case confirming the fix works end-to-end
- ✅ Net code reduction (28 deletions, 141 additions mostly test-data) — the implementation side is simpler after the change
- ✅
fprintlnsprintfcorrectly ships a.goldenfile for the suggested fix on aliased imports
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 78.5 AIC · ⌖ 8.45 AIC · ⊞ 6.6K
Comment /matt to run again
| defer cancel() | ||
| _ = c | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing ctx.WithDeadline alias test — isContextWithCancelCall handles WithCancel, WithTimeout, and WithDeadline, but only the first two are covered with an aliased import.
💡 Suggested addition
// flagged: aliased context import — WithDeadline path
func BadWithDeadlineAliased(parent ctx.Context) error {
c, cancel := ctx.WithDeadline(parent, time.Now().Add(time.Second)) // want `context cancel function should be deferred immediately after context.WithCancel/WithTimeout/WithDeadline`
_ = c
cancel()
return nil
}All three switch cases in isContextWithCancelCall should be covered by alias tests to guard against per-variant regressions.
@copilot please address this.
| defer cancel() | ||
| _ = c | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] No shadowing test for context — fileclosenotdeferred, errstringmatch, and osexitinlibrary all include a var <pkg> fake<Type>{} shadowing case that confirms no false positive; contextcancelnotdeferred is missing the equivalent.
💡 Suggested addition
// not flagged: local type shadows package name
type fakeContext struct{}
func (fakeContext) WithCancel(parent interface{}) (interface{}, func()) { return parent, func() {} }
func noFalsePosContextShadow() {
var context fakeContext
_, cancel := context.WithCancel(nil)
cancel() // direct call, not deferred — should NOT be flagged
}Consistency with the other four linters makes it easier to verify the pattern is fully symmetric.
@copilot please address this.
| } | ||
| defer file.Close() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] os.Create and os.OpenFile are both matched by isFileOpenCall but are absent from the alias test — only os.Open is exercised here. A regression in the Create or OpenFile branch with an aliased import would go undetected.
💡 Suggested additions
// flagged: os.Create with alias
func WriteFileManualCloseAliased() error {
file, err := xos.Create("out.txt") // want `file Close\(\) should be deferred immediately after successful open`
if err != nil { return err }
file.Close()
return nil
}
// flagged: os.OpenFile with alias
func OpenFileManualCloseAliased() error {
file, err := xos.OpenFile("rw.txt", xos.O_RDWR, 0644) // want `file Close\(\) should be deferred ...`
if err != nil { return err }
file.Close()
return nil
}@copilot please address this.
| func notFlaggedShadowedFmt(name string) { | ||
| var fmt fakeFmt | ||
| fmt.Fprintln(os.Stderr, fmt.Sprintf("hello %s", name)) | ||
| } |
There was a problem hiding this comment.
[/tdd] The shadowing negative case only tests that the Fprintln(..., Sprintf(...)) pattern on a fake type is not flagged, but the original bug also manifested on the Sprintf side of the check (isFmtFunc is called twice). Consider adding a case where only one side is a real fmt call to confirm both guards are exercised independently.
💡 Example
// not flagged: fmt.Fprintln with a real fmt, but inner call is from fakeFmt — should not match
func mixedRealFakeFmt(name string) {
var f fakeFmt
xfmt.Fprintln(os.Stderr, f.Sprintf("hello %s", name)) // inner Sprintf not fmt.Sprintf
}This guards against partial matches where only one selector resolves to the stdlib package.
@copilot please address this.
🤖 PR Triage — §28848998303
New PR — 5 custom linters migrated from syntactic to
|
There was a problem hiding this comment.
REQUEST_CHANGES — two missing negative test cases block merge
The core linter migration from syntactic name matching to IsPkgSelector is correct. IsPkgSelector itself is well-tested in astutil_test.go. All five linter source changes are accurate.
However, two of the five linters shipped without the negative (false-positive-guard) test cases that make this migration verifiable:
fileclosenotdeferred_alias.go— no shadow/local-variable testcontextcancelnotdeferred_alias.go— no shadow/local-variable test
The three CI-enforced linters in this PR (errstringmatch, fprintlnsprintf, osexitinlibrary) each include a function with var <pkgname> fakeXxx that asserts no false positive. The two non-CI linters do not, leaving the entire point of the migration — not flagging shadowed locals — untested for those linters.
There is also a minor confusing annotation to fix in the golden file (see inline comment).
Issues requiring changes
- Missing shadow test —
fileclosenotdeferred(medium): add a negative test case with a local struct that mimics theosAPI but is not the stdlib package. - Missing shadow test —
contextcancelnotdeferred(medium): add a negative test case with a local struct that hasWithCancel/WithTimeout/WithDeadlinemethods. - Spurious
// wantin golden file (low/misleading): remove the// wantcomment fromfprintlnsprintf_alias.go.goldenline 10 — it is not processed in golden files and will mislead future editors.
🔎 Code quality review by PR Code Quality Reviewer · 145.2 AIC · ⌖ 5.38 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
| defer file.Close() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing shadow/negative test case: this alias file only tests the true-positive path (alias import → still flagged) and the correct-defer path, but never tests that a local variable shadowing the package name is not flagged — the central regression guard for this migration.
💡 Details
All three CI-enforced linters that received this fix have matching shadow tests in their alias files:
| linter | shadow function |
|---|---|
errstringmatch |
checkShadowedStrings — var strings fakeStrings |
fprintlnsprintf |
notFlaggedShadowedFmt — var fmt fakeFmt |
osexitinlibrary |
notFlaggedShadowedOS — var os fakeOS |
fileclosenotdeferred_alias.go has no such case. Add something like:
// not flagged: local var shadows the alias name — not the stdlib package
type fakeOS struct{}
func (fakeOS) Open(name string) (*os.File, error) { return nil, nil }
func ReadFileShadowedOS() error {
var xos fakeOS
file, err := xos.Open("test.txt")
// no diagnostic expected here
_ = file
return err
}If IsPkgSelector ever regresses, nothing in this PR's tests would catch a false-positive for fileclosenotdeferred.
| defer cancel() | ||
| _ = c | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing shadow/negative test case: same gap as fileclosenotdeferred_alias.go — there is no test verifying that a local variable that shadows the context (or alias) name is not flagged.
💡 Details
The three CI-enforced linters in this PR all have negative shadow cases; the two non-CI-enforced linters (fileclosenotdeferred, contextcancelnotdeferred) do not. Add a function like:
// not flagged: local var named after the context alias — not the stdlib package
type fakeContext struct{}
func (fakeContext) WithCancel(parent context.Context) (context.Context, func()) {
return parent, func() {}
}
func notFlaggedShadowedContext(parent context.Context) {
var ctx fakeContext
c, cancel := ctx.WithCancel(parent) // no diagnostic expected
defer cancel()
_ = c
}Without this, the false-positive regression path for this linter is completely untested.
|
|
||
| // flagged: aliased fmt import still resolves to fmt package | ||
| func flaggedAliased(name string) { | ||
| xfmt.Fprintf(os.Stderr, "hello %s\n", name) // want "use fmt.Fprintf" |
There was a problem hiding this comment.
Spurious // want directive in a .golden file: the analysistest framework does not process // want directives in golden files — they are treated as literal comment text in the expected post-fix source.
💡 Details
Line 10 reads:
xfmt.Fprintf(os.Stderr, "hello %s\n", name) // want "use fmt.Fprintf"After autofix, xfmt.Fprintln is rewritten to xfmt.Fprintf, so no further diagnostic is expected. The // want annotation here is misleading — a future developer might think it is asserting a diagnostic on the fixed line (it isn't), and may edit the golden file incorrectly when updating the linter message in the future.
Remove the trailing // want ... comment from line 10 of the golden file.
|
🎉 This pull request is included in a new release. Release: |
Five custom linters matched stdlib packages by identifier name (
ident.Name == "os"etc.), causing false negatives on aliased imports and false positives (build-blockers for three CI-enforced linters) when a local variable shadows the package name.Changes
errstringmatch,fprintlnsprintf,osexitinlibrary,fileclosenotdeferred,contextcancelnotdeferred: replace every syntacticident.Name == "<pkg>"guard withastutil.IsPkgSelector(pass, sel, "<pkg>"), which resolves throughpass.TypesInfoto the actual imported package pathxos "os") — confirms the diagnostic still firesvar os fakeOS) — confirms no false positive (enforced linters only)