Skip to content

[refactor] Consolidate duplicated import-editing and file-lookup helpers across pkg/linters #47897

Description

@github-actions

Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw — scope: pkg/jsonutil, pkg/linters (precomputed slice, non-test files only)

Executive Summary

The shared helper layer (pkg/linters/internal/{astutil,nolint,filecheck}) is well-organized and already centralizes most AST/type predicates. However, several helper clusters remain copy-pasted inside individual linter packages rather than living in internal/astutil. The most significant is a block of import-manipulation helpers duplicated nearly verbatim between sprintfbool and sprintfint, plus a file-containing-position lookup re-implemented in 5+ packages. pkg/jsonutil/json.go is a single-purpose file with no findings.

Identified Issues

1. Duplicate import-editing helpers (sprintfbool and sprintfint)

Five functions are near-identical between the two files (same logic, differ only by literal comparison style):

Function sprintfbool sprintfint
countPkgUsesInFile sprintfbool.go:242 sprintfint.go:208
addStrconvRemoveFmtEdits sprintfbool.go:257 sprintfint.go:229
addImportEdit sprintfbool.go:305 sprintfint.go:285
removeImportEdit sprintfbool.go:343 sprintfint.go:330
importSpecLineRange sprintfbool.go:372 sprintfint.go:365

countPkgUsesInFile, addImportEdit, removeImportEdit, and importSpecLineRange are byte-for-byte equivalent. addStrconvRemoveFmtEdits differs only in the order of the two returned TextEdits and whether it uses importSpecPathEquals vs a raw string compare — a latent drift risk.

Recommendation: Promote these to internal/astutil (e.g. astutil.AddImportEdit, astutil.RemoveImportEdit, astutil.ImportSpecLineRange, astutil.CountPkgUsesInFile, and a generalized swap helper SwapImportEdits(add, remove string)). Both sprintf* linters — and the analogous bytescomparestring/writebytestring import editors — would call the shared versions.

2. Scattered file-containing-position lookup

The idiom for _, f := range files { if f.Pos() <= pos && pos <= f.End() { return f } } is re-implemented in:

  • sprintfbool/sprintfbool.go:385 (fileForPos)
  • sprintfint/sprintfint.go:114 (inline in buildItoaFix)
  • writebytestring/writebytestring.go:309 (fileForPos)
  • bytescomparestring/bytescomparestring.go:120,171 (inline, twice)
  • uncheckedtypeassertion/uncheckedtypeassertion.go:76 (inline)

Recommendation: Add astutil.FileForPos(files []*ast.File, pos token.Pos) *ast.File and replace all five sites. internal/astutil is the natural home — it already owns Inspector, Root, and ImportedAs.

3. Repeated per-node skip/nolint guard (40+ files)

Every Preorder callback opens with the same 3-line preamble computing pos, calling filecheck.ShouldSkipFilename(pos.Filename, generatedFiles), then nolint.HasDirectiveForLinter(pos, noLintIndex, "<name>"). ShouldSkipFilename appears in 40 files; HasDirectiveForLinter in 34.

Recommendation (lower priority): Add a single helper, e.g. filecheck.SkipNode(pass, node, generated, noLintIndex, linterName) bool, collapsing all three lines into one guarded call. Reduces per-linter boilerplate and guarantees consistent skip semantics.

4. Identical run() preamble (53 files)

53 linters begin run with the same three fallible lookups — astutil.Inspector(pass), nolint.Index(pass), filecheck.Index(pass) — each with identical error handling.

Recommendation (lower priority): Provide astutil.StandardPass(pass) returning the inspector, nolint index, generated index, and error in one call. Optional; the current form is explicit and readable, so weigh churn vs. benefit.

Well-Organized (no action)

  • internal/astutil, internal/nolint, internal/filecheck — clean single-purpose helper packages; most shared predicates already live here.
  • registry.go / error_message_linter.go — clear registry + thin alias.
  • pkg/jsonutil/json.go — single function, correct home.
  • One-linter-per-package layout with co-located testdata/ — idiomatic for golang.org/x/tools/go/analysis.

Recommendations Summary

Priority Action Est. effort
P1 Move 5 import-editing helpers (sprintfbool/sprintfint dup) into internal/astutil 2-3h
P1 Add astutil.FileForPos, replace 5 call sites 1h
P2 Add filecheck.SkipNode guard helper (40+ sites) 2-3h
P3 Add astutil.StandardPass preamble helper (53 sites) 2-3h

Analysis Metadata

  • Packages analyzed: pkg/jsonutil, pkg/linters (+ internal/*, 55 linter packages)
  • Non-test Go files in scope: ~55 implementation files
  • Duplicate clusters found: 2 high-confidence (import editors, file-lookup)
  • Boilerplate clusters found: 2 (node guard x40, run preamble x53)
  • Detection method: Grep pattern clustering + direct source comparison
  • Analysis date: 2026-07-25

Generated by 🔧 Semantic Function Refactoring · sonnet46 · 163.1 AIC · ⌖ 16.6 AIC · ⊞ 9.5K ·

  • expires on Jul 26, 2026, 7:31 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

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions