refactor(linters): consolidate duplicated import-editing and file-lookup helpers into internal/astutil#47912
Conversation
…ernal/astutil Add FileForPos, CountPkgUsesInFile, ImportSpecLineRange, AddImportEdit, RemoveImportEdit, and SwapImportEdits to pkg/linters/internal/astutil. Replace duplicated local implementations in: - sprintfbool: remove 7 local functions (countPkgUsesInFile, addStrconvRemoveFmtEdits, addImportEdit, removeImportEdit, importSpecLineRange, fileForPos, importSpecPathEquals) - sprintfint: remove 5 local functions + inline fileForPos loop - writebytestring: remove local fileForPos function - bytescomparestring: replace 2 inline fileForPos loops - uncheckedtypeassertion: replace 1 inline fileForPos loop Add unit tests for FileForPos, CountPkgUsesInFile, and ImportSpecLineRange. Closes #47897 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Consolidates repeated AST file lookup and import-editing logic into shared internal/astutil helpers.
Changes:
- Adds shared file-position and import-editing utilities.
- Migrates five linters to the shared helpers.
- Adds focused unit tests for core utility behavior.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/internal/astutil/astutil.go |
Adds shared AST and import-editing helpers. |
pkg/linters/internal/astutil/astutil_test.go |
Tests new lookup and import-analysis helpers. |
pkg/linters/sprintfbool/sprintfbool.go |
Replaces duplicated helper implementations. |
pkg/linters/sprintfint/sprintfint.go |
Replaces duplicated and inline logic. |
pkg/linters/writebytestring/writebytestring.go |
Uses shared file lookup. |
pkg/linters/bytescomparestring/bytescomparestring.go |
Uses shared file lookup. |
pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go |
Uses shared file lookup for parent maps. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: consolidate import-editing helpers into internal/astutil
The refactoring is clean and well-motivated — 360+ lines of near-verbatim duplication removed, good unit tests added, and the intentional non-migration of buildBytesImportTextEdit is clearly documented.
One non-blocking suggestion (inline comment posted):
AddImportEdit takes *analysis.Pass but uses only pass.Fset, while its sibling RemoveImportEdit correctly takes *token.FileSet directly. Aligning the signature would remove a framework coupling and make the function easier to unit-test in isolation.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.6 AIC · ⌖ 4.63 AIC · ⊞ 5K
| // choosing the least-invasive insertion point: append to an existing grouped | ||
| // block, convert a single non-grouped import to a grouped block, or insert a | ||
| // standalone declaration after the package name. | ||
| func AddImportEdit(pass *analysis.Pass, file *ast.File, pkg string) (analysis.TextEdit, bool) { |
There was a problem hiding this comment.
API inconsistency: AddImportEdit takes *analysis.Pass but only uses pass.Fset
Its sibling RemoveImportEdit correctly takes *token.FileSet directly, making it easy to unit-test and reuse outside analysis passes. AddImportEdit takes the full *analysis.Pass solely to access pass.Fset on the NodeText call inside the single-import expansion branch — an unnecessary coupling.
Suggested fix:
func AddImportEdit(fset *token.FileSet, file *ast.File, pkg string) (analysis.TextEdit, bool) {
...
specText := NodeText(fset, genDecl.Specs[0])
...
}All call sites already have pass.Fset available and can pass it directly.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Verdict
|
…rs into internal/astutil Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (316 new lines in 📄 Draft ADR committed:
📋 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 Matter
ADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
AddImportEdit,RemoveImportEdit, andSwapImportEditseach have 3–4 structural branches with no unit tests, unlike the simpler helpers added in the same PR. - Potential edit-ordering regression:
SwapImportEdits' grouped case emits edits in descending position order, reversed from the originaladdStrconvRemoveFmtEdits. Worth verifying this is safe for theanalysisframework. - Minor:
importSpecPathEqualsis now embedded in the shared package as unexported; exporting it would complete the API surface.
Positive Highlights
- ✅ Excellent reduction in duplication — ~360 lines removed across sprintfbool, sprintfint, writebytestring, bytescomparestring, uncheckedtypeassertion.
- ✅ Good switch from raw string comparison to
astutil.ImportedAs— handles backtick-quoted paths correctly. - ✅ Unit tests added for
FileForPos,CountPkgUsesInFile,ImportSpecLineRange— good pattern to follow for the missing helpers. - ✅ PR description clearly documents the one intentional non-migration (
buildBytesImportTextEdit).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50.5 AIC · ⌖ 4.91 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/linters/internal/astutil/astutil.go:549
[/tdd] AddImportEdit, RemoveImportEdit, and SwapImportEdits — the three most complex new helpers with multiple branching paths — have no unit tests, while simpler peers (FileForPos, CountPkgUsesInFile, ImportSpecLineRange) do. If a branch is wrong it will silently produce a malformed TextEdit or a no-op fix.
<details>
<summary>💡 Suggested coverage</summary>
For each helper, cover the distinct structural cases documented in the godoc:
AddImportEdit: grouped block (append)…
pkg/linters/internal/astutil/astutil.go:240
[/codebase-design] The edit ordering in SwapImportEdits' grouped case is reversed relative to the original sprintfbool.addStrconvRemoveFmtEdits. The original returned [lineStart remove, Rparen insert] (ascending positions); this helper returns [Rparen insert, lineStart remove] (descending). If the analysis framework or downstream consumers require non-overlapping edits in ascending position order, the new order can produce unexpected results.
<details>
<summary>💡 Suggested fix</…
pkg/linters/internal/astutil/astutil.go:474
[/codebase-design] importSpecPathEquals is a private helper embedded inside the shared astutil package. Since callers outside this package (e.g., future linters) will need to check import-spec equality too, keeping it unexported creates pressure to re-duplicate it. Exporting it as ImportSpecPathEquals would complete the surface area of the import-manipulation API.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two blocking issues, one medium gap
The refactor is directionally correct and reduces significant duplication, but there is one likely behavioral regression and one missing safety measure in the new shared helpers.
Blocking findings
1. reverses TextEdit position order (high)
The new grouped-block branch emits — descending position order. Both original implementations always emitted delete first (lower Pos), then insert (higher Pos). If the go/analysis framework sorts edits before applying, this is harmless; if it doesn't, the grouped-block swap path will corrupt the generated fix. The fix is a one-line swap of the two slice elements.
2. Unsanitized pkg string in AddImportEdit / SwapImportEdits (medium)
Package path is concatenated directly into import source text. As a public shared helper this needs strconv.Quote(pkg) to stay safe for all future callers.
3. Missing unit tests for AddImportEdit, RemoveImportEdit, SwapImportEdits (medium)
Three of the six new helpers have no direct unit tests. The golden-file coverage is too coarse to catch TextEdit position bugs.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 63.8 AIC · ⌖ 5.01 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/internal/astutil/astutil.go:229
SwapImportEdits emits TextEdits in descending position order, inverting the order used by both original implementations.
The new helper emits insert-at-Rparen first, then delete-spec-line. Since lineStart < Rparen, this is descending position order. Both sprintfbool and sprintfint consistently emitted delete (lower pos) first, then insert (higher pos). If the framework assumes ascending order or applies edits without sorting, the output will be corrupted.
<details>
<summary>💡 S…
pkg/linters/internal/astutil/astutil.go:137
AddImportEdit performs unsanitized string concatenation with the pkg argument to produce import source text.
As a newly public API, any caller that passes a malformed or attacker-controlled package path (containing quotes, backslashes, or newlines) will produce a syntactically broken import statement that silently corrupts the suggested fix.
<details>
<summary>💡 Suggested fix</summary>
Use strconv.Quote so the output is always a well-formed string literal:
NewText: []byte("…
</details>
<details><summary>pkg/linters/internal/astutil/astutil_test.go:346</summary>
**`AddImportEdit`, `RemoveImportEdit`, and `SwapImportEdits` have no direct unit tests — only the pre-existing golden-file tests cover them indirectly.**
Three of the six new public helpers are completely untested in `astutil_test.go`. Given that these are the most structurally complex helpers (they mutate import blocks), a regression in their edge cases (zero imports, single ungrouped import, grouped single-spec, multi-spec) won't be caught without explicit unit tests.
<details>
<summary>💡 …
</details>
|
Hey What stands out:
This PR is ready for maintainer review and appears aligned with the project's contribution guidelines and refactoring priorities. The consolidation eliminates ~360 lines of duplicated code across 5+ packages while improving maintainability. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
@copilot please run the
|
…kg paths, add unit tests for AddImportEdit/RemoveImportEdit/SwapImportEdits Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
… in applyTextEdits helper Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three blocking items:
|
🤖 PR Triage
Rationale: Consolidates 5 copy-pasted import-manipulation helpers and a file-position lookup into Next step: Address review feedback and confirm CI passes.
|
|
@copilot run pr-finisher skill |
PR-finisher run complete. All previously blocking items were addressed in commits
Ready for human CI re-trigger and merge. |
|
🎉 This pull request is included in a new release. Release: |
Five import-manipulation helpers were copy-pasted nearly verbatim between
sprintfboolandsprintfint, and a file-containing-position lookup was re-implemented inline in 5+ packages. This promotes both clusters intointernal/astutilas shared, tested utilities.New helpers in
internal/astutilFileForPos(files, pos)for _, f := range pass.Files { if f.Pos() <= pos ...}(5 sites)CountPkgUsesInFile(pass, file, pkgPath)countPkgUsesInFileinsprintfboolandsprintfintImportSpecLineRange(fset, spec)importSpecLineRangein both packagesAddImportEdit(pass, file, pkg)addImportEditin both packagesRemoveImportEdit(fset, file, pkg)removeImportEditin both packagesSwapImportEdits(fset, file, addPkg, removePkg)addStrconvRemoveFmtEditsin both packagesCall-site updates
sprintfbool— removes 7 local functions (~190 lines); import-presence checks switch fromimportSpecPathEqualstoastutil.ImportedAs(already handles backtick-quoted paths correctly)sprintfint— removes 5 local functions + 1 inline loop (~170 lines)writebytestring,bytescomparestring,uncheckedtypeassertion— inlinefileForPosloops replaced withastutil.FileForPosNote:
bytescomparestring'sbuildBytesImportTextEditis intentionally not migrated toAddImportEdit— it inserts the new import first in single-import expansion, which differs from the shared helper's trailing-append behavior and is covered by existing golden tests.Tests
Added unit tests for
FileForPos,CountPkgUsesInFile, andImportSpecLineRangeinastutil_test.go. All existing linter golden-file tests continue to pass.