Skip to content

Linter precision (regression): the syntactic stdlib-package-match migration never landed — 5 holdouts remain, 3 CI-enforced (#40 [Content truncated due to length] #43934

Description

@github-actions

Summary

Five custom linters still identify standard-library calls by the syntactic identifier name (ident.Name == "<pkg>" / != "<pkg>") instead of by package identity via pass.TypesInfo. The canonical helper astutil.IsPkgSelector(pass, sel, pkgPath) (pkg/linters/internal/astutil/astutil.go:161-178) already exists and is proven in production (rawloginlib.go:58, plus regexpcompileinfunction, sortslice, tolowerequalfold, ctxbackground all migrated).

Two earlier issues tracked exactly these holdouts — #40243 (os/fmt/strings) and #40435 (os/context) — but both were closed via tracker auto-expiry with zero migrations applied. Verified at current HEAD (branch main): all five call sites are still syntactic, and three of them are CI-enforced (.github/workflows/cgo.yml:1208 LINTER_FLAGS), where a false positive is a build-blocker.

The remaining holdouts (verified at HEAD)

Linter Location Current code CI-enforced?
errstringmatch pkg/linters/errstringmatch/errstringmatch.go:100 if ident.Name != "strings" { ✅ yes
fprintlnsprintf pkg/linters/fprintlnsprintf/fprintlnsprintf.go:132 return ident.Name == "fmt" && sel.Sel.Name == name ✅ yes
osexitinlibrary pkg/linters/osexitinlibrary/osexitinlibrary.go:59 if ident.Name == "os" && sel.Sel.Name == "Exit" { ✅ yes
fileclosenotdeferred pkg/linters/fileclosenotdeferred/fileclosenotdeferred.go:174 `if !ok
contextcancelnotdeferred pkg/linters/contextcancelnotdeferred/contextcancelnotdeferred.go:140 `if !ok

Why syntactic matching is wrong (two failure modes)

  1. Alias-import false negative. import str "strings" then str.Contains(err.Error(), "x")ident.Name is "str", so the call escapes the linter. Same for import xos "os" / import xfmt "fmt" / import ctx "context". For the three enforced linters this silently lets the targeted anti-pattern ship past CI.
  2. Shadowing false positive (build-blocker for the 3 enforced ones). A local variable/parameter/field named os/fmt/strings/context with a matching method or field name matches by name even though it is not the stdlib package — a spurious diagnostic that fails the build.

Current impact

Latent todaypkg/ currently has no aliased os/fmt/strings/context imports and does not shadow those identifiers, so no live FP/FN exists right now. But this is a correctness gap in enforced tooling that will bite the first time someone adds an alias import or a shadowing local. The fix is cheap, the helper is proven, and the sibling migration is already accepted — the two tracking issues simply expired before the work was done.

Correct approach (already used by the migrated linters)

astutil.IsPkgSelector resolves the selector base to a *types.PkgName and compares the imported path:

func IsPkgSelector(pass *analysis.Pass, sel *ast.SelectorExpr, pkgPath string) bool {
    pkgIdent, ok := sel.X.(*ast.Ident)
    if !ok { return false }
    obj := pass.TypesInfo.ObjectOf(pkgIdent)
    pkgName, ok := obj.(*types.PkgName)
    if !ok || pkgName.Imported() == nil { return false }
    return pkgName.Imported().Path() == pkgPath
}

Reference migration — rawloginlib.go:58: if !astutil.IsPkgSelector(pass, sel, "log") {

Recommendation

Replace each syntactic check with the existing helper (each is a one-line change against the already-extracted *ast.SelectorExpr):

  • errstringmatch.go:100if !astutil.IsPkgSelector(pass, sel, "strings") {
  • fprintlnsprintf.go:132return astutil.IsPkgSelector(pass, sel, "fmt") && sel.Sel.Name == name
  • osexitinlibrary.go:59if astutil.IsPkgSelector(pass, sel, "os") && sel.Sel.Name == "Exit" {
  • fileclosenotdeferred.go:174if !astutil.IsPkgSelector(pass, sel, "os") {
  • contextcancelnotdeferred.go:140 → resolve sel via astutil.IsPkgSelector(pass, sel, "context") (this file constructs sel/pkgIdent inline in isContextWithCancelCall; pass the *ast.SelectorExpr to the helper).

Validation checklist

  • Per linter: add a testdata case with an aliased import (e.g. str "strings") — confirm it is now flagged (FN closed) for the 3 enforced linters, and detected for the 2 not-enforced.
  • Per enforced linter: add a testdata case with a shadowing local named os/fmt/strings — confirm it is NOT flagged (FP / build-blocker closed).
  • Existing testdata expectations for the normal strings.Contains / fmt.Fprintln / os.Exit / os.Open+.Close / context.WithCancel cases unchanged.
  • go test ./pkg/linters/... green; make golint-custom over pkg/ produces no new diagnostics.

Effort

Small — one-line change per linter plus two testdata cases each; the helper already exists. Single PR. This finishes the migration that #40243 and #40435 both started and that already landed for rawloginlib/regexpcompileinfunction/sortslice/tolowerequalfold/ctxbackground.

Related

#40243 (os/fmt/strings holdouts — expired unfixed), #40435 (os/context holdouts — expired unfixed), #39981 (rawloginlib, landed), #39733 (regexpcompileinfunction), #38789 (ctxbackground). Same syntactic-package-identity class.

References:

Generated by 🤖 Sergo - Serena Go Expert · 386 AIC · ⌖ 14.1 AIC · ⊞ 5.9K ·

  • expires on Jul 13, 2026, 9:14 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

cookieIssue Monster Loves Cookies!sergo

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions