linters: migrate osexitinlibrary, fprintlnsprintf, errstringmatch to type-based package identity#40247
linters: migrate osexitinlibrary, fprintlnsprintf, errstringmatch to type-based package identity#40247Copilot wants to merge 4 commits into
Conversation
…ctor Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves the correctness of three CI-enforced Go analyzers by switching stdlib package detection from syntactic identifier checks (prone to alias/import shadowing bugs) to type-based package identity via astutil.IsPkgSelector(pass, sel, "<import-path>"), aligning them with existing patterns in other linters.
Changes:
- Migrate
osexitinlibrary,fprintlnsprintf, anderrstringmatchto useastutil.IsPkgSelectorfor stdlib package resolution (fixes alias-import false negatives and shadowing false positives). - Update helper signatures where needed (
isFmtFunc,isStringsContains) to accept*analysis.PassforTypesInfo-backed resolution. - Add testdata covering both aliased imports (should be flagged) and local shadowing (should not be flagged), plus suggested-fix golden updates for
fprintlnsprintf.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/osexitinlibrary/osexitinlibrary.go | Switch os.Exit detection to astutil.IsPkgSelector to avoid alias/shadowing mis-detection. |
| pkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/osexitinlibrary.go | Add a shadowing case to ensure local os variables aren’t flagged. |
| pkg/linters/osexitinlibrary/testdata/src/osexitinlibrary/alias_import.go | Add aliased import coverage to ensure xos.Exit is still flagged. |
| pkg/linters/fprintlnsprintf/fprintlnsprintf.go | Make isFmtFunc type-aware via pass and astutil.IsPkgSelector for robust fmt detection. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go | Add a fmt-shadowing case to ensure local fmt variables aren’t flagged. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/fprintlnsprintf.go.golden | Mirror new shadowing scenario in the post-fix golden output. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/alias_import.go | Add aliased fmt import coverage to ensure the pattern is flagged and fixed. |
| pkg/linters/fprintlnsprintf/testdata/src/fprintlnsprintf/alias_import.go.golden | Expected output after suggested fix when fmt is aliased. |
| pkg/linters/errstringmatch/errstringmatch.go | Make strings.Contains detection type-aware via pass and astutil.IsPkgSelector. |
| pkg/linters/errstringmatch/testdata/src/errstringmatch/errstringmatch.go | Add a strings-shadowing case to ensure local strings variables aren’t flagged. |
| pkg/linters/errstringmatch/testdata/src/errstringmatch/alias_import.go | Add aliased import coverage to ensure str.Contains is still flagged. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40247 does not have the 'implementation' label (has_implementation_label=false) and has 88 new lines of code in business logic directories, which is ≤100 (requires_adr_by_default_volume=false, no custom config). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR modifies linter implementation files (osexitinlibrary.go, fprintlnsprintf.go, errstringmatch.go) and adds/updates testdata fixture files (alias_import.go, *.golden) used as analyzer inputs — none of these are *_test.go, *.test.cjs, or *.test.js files. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — approving with one minor inline suggestion.
📋 Key Themes & Highlights
Key Themes
- Correct root-cause fix: Moving from syntactic
ident.Name == "os"comparisons toastutil.IsPkgSelector(type-resolved viapass.TypesInfo) is the right approach and already proven byrawloginlibandsortslice. - Both failure modes tested: Each linter gets an
alias_import.gofixture (closes the FN) and a shadow-variable case in the main testdata file (closes the FP). This is the complete set needed. - Golden files correct: The
fprintlnsprintfgolden files correctly preserve the alias prefix (f.Fprintfnotfmt.Fprintf) becausebuildFprintfFixonly replaces the selector name, not the package qualifier — a subtle but important property that the newalias_import.go.goldendirectly validates.
Positive Highlights
- ✅ Net negative diff in implementation files (–20/+8): the fix simplifies the code, removing the intermediate
identextraction entirely. - ✅
fprintlnsprintfhas two call sites (FprintlnandSprintfdetection) that both needed updating — both are covered by a single alias-import test case. - ✅
wantannotations use correct regex escaping and theerrstringmatchpattern correctly matches the prefix of the full diagnostic message.
Follow-up Opportunity (out of scope)
astutil.IsFmtErrorf in astutil.go already uses TypesInfo correctly (it was never broken), but its structure inlines the package+method check rather than composing IsPkgSelector. A small follow-up could simplify it to sel.Sel.Name == "Errorf" && IsPkgSelector(pass, sel, "fmt") for consistency.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 104.9 AIC · ⌖ 7.77 AIC · ⊞ 6.9K
| // shadowed: a local variable named "fmt" with Fprintln/Sprintf methods should NOT be flagged. | ||
| type fmtWriter struct{} | ||
|
|
||
| func (fmtWriter) Fprintln(_ interface{}, _ ...interface{}) {} |
There was a problem hiding this comment.
[/tdd] The first parameter of Fprintln accepts interface{}, but fmt.Fprintln actually requires io.Writer. The shadow test still works correctly (any *os.File satisfies interface{}), but using io.Writer would more accurately model the real function signature. This also brings the stub in line with the codebase preference for any over interface{}.
There was a problem hiding this comment.
Fixed in the latest commit: updated fmtWriter.Fprintln to use io.Writer as the first parameter and any in place of interface{} throughout (both the source testdata and its golden file). Tests pass.
…estion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all comments and address unresolved review feedback. Please resolve the outstanding review thread on the signature/type change.
|
|
Please refresh the branch and rerun checks after the update.
|
|
@copilot merge main and recompile |
…dex/LastIndex/Compare with IsPkgSelector Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
Three CI-enforced linters identified stdlib package calls by syntactic identifier name (
ident.Name == "os"etc.), creating alias-import false negatives and shadowing false positives. This migrates all three toastutil.IsPkgSelector, which resolves viapass.TypesInfoto a*types.PkgNameand compares the imported path — the same pattern already applied torawloginlibandsortslice.Changes
osexitinlibrary— removes the explicitidentextraction; delegates entirely toastutil.IsPkgSelector(pass, sel, "os")fprintlnsprintf— addspass *analysis.PasstoisFmtFunc; replaces name comparison withastutil.IsPkgSelector(pass, sel, "fmt")errstringmatch— renamesisStringsContainstobrittleErrStringFuncNameand extends coverage toContains,HasPrefix,HasSuffix,EqualFold,Index,LastIndex, andCompare; usesastutil.IsPkgSelector(pass, sel, "strings")for type-resolved detectionBefore (syntactic — misses aliases, fires on shadowed locals):
After (type-resolved):
Testdata
Each linter gains two new cases:
alias_import.go— aliased import (e.g.import xos "os") confirmed flagged (closes FN)os/fmt/stringswith matching method confirmed NOT flagged (closes FP)errstringmatchtestdata additionally covers the six new brittle string functions (HasPrefix,HasSuffix,EqualFold,Index,LastIndex,Compare) with both flagged and non-flagged cases.