fix(sprintfint): make suggested fix self-contained — add strconv import, remove orphaned fmt import - #44911
Conversation
…suggested fix
The buildItoaFix SuggestedFix previously only contained the call-site
TextEdit, so applying it to a file where fmt.Sprintf("%d", n) was the
sole fmt use produced non-compiling code: undefined: strconv and
"fmt" imported and not used.
Changes:
- buildItoaFix now emits import TextEdits alongside the call replacement:
* Adds "strconv" using the same grouped/single/standalone logic as
bytescomparestring and writebytestring.
* Removes "fmt" when countPkgUsesInFile shows the flagged call is the
file's only fmt reference (single-use file case).
- Three import-section shapes are handled: ungrouped single import,
grouped-only, and grouped-with-others.
- importSpecLineRange uses token.File.LineStart for robust line-boundary
deletion (works regardless of indentation style).
- seenImportFiles prevents duplicate overlapping import edits when a
file contains multiple violations.
- Stale goimports caveat removed from the Doc string.
- New singleuse.go test fixture / golden exercises both strconv-add and
fmt-removal via analysistest.RunWithSuggestedFixes.
Closes #44864
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Makes sprintfint suggested fixes manage strconv and orphaned fmt imports.
Changes:
- Adds import-edit generation and usage tracking.
- Adds a single-use fixture and golden output.
- Updates analyzer documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/sprintfint/sprintfint.go |
Adds self-managed import edits. |
pkg/linters/sprintfint/testdata/src/sprintfint/singleuse.go |
Adds single-use test input. |
pkg/linters/sprintfint/testdata/src/sprintfint/singleuse.go.golden |
Defines expected fixed output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
| if seenImportFiles[file.Pos()] { | ||
| return nil |
| // If the flagged call is the only "fmt" reference in the file the "fmt" | ||
| // import will become unused after the fix and must be removed. | ||
| orphanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == 1 |
| for _, imp := range file.Imports { | ||
| switch imp.Path.Value { | ||
| case `"` + strconvPkg + `"`: | ||
| strconvImported = true |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #44911 only modifies production code (pkg/linters/sprintfint/sprintfint.go) and test data fixtures (singleuse.go and singleuse.go.golden). Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Review\n\nThe implementation is well-structured and all three import-section shapes are handled correctly. One non-blocking suggestion below.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall the PR is well-implemented: the three import-section shape cases are correctly handled, seenImportFiles properly prevents duplicate edits, and the new singleuse.go fixture gives good end-to-end coverage. One non-blocking suggestion was left inline about TextEdit ordering in addStrconvRemoveFmtEdits.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 31.6 AIC · ⌖ 4.42 AIC · ⊞ 4.8K
| lineStart, lineEnd := importSpecLineRange(fset, fmtSpec) | ||
| return []analysis.TextEdit{ | ||
| { | ||
| Pos: fmtDecl.Rparen, |
There was a problem hiding this comment.
The two TextEdits for the multi-spec grouped case are returned in reverse position order: the insert at Rparen (the higher byte offset) is the first element, and the delete of the fmt spec line (lower offset) is the second. The analysistest framework sorts edits before applying them so tests pass, but tools that apply fixes without sorting (e.g. gopls apply) may reject or misapply non-monotonically-ordered edits.
Swap the two elements so edits are emitted in ascending position order:
lineStart, lineEnd := importSpecLineRange(fset, fmtSpec)
return []analysis.TextEdit{
{Pos: lineStart, End: lineEnd, NewText: nil},
{Pos: fmtDecl.Rparen, End: fmtDecl.Rparen, NewText: []byte("\t\"" + strconvPkg + "\"\n")},
}@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (260 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.
REQUEST_CHANGES — two correctness bugs that produce non-compiling output
The import self-management logic has two interacting bugs that affect the multi-violation case (a file with two or more fmt.Sprintf("%d", ...) calls and no other fmt uses). The single-violation test fixture (singleuse.go) doesn't exercise these paths, so they pass CI today.
Blocking issues summary
1. orphanFmt threshold is wrong for multi-violation files (line 158)
countPkgUsesInFile counts current fmt references including the calls that are about to be replaced. With two violations and no other fmt use, count == 2, so orphanFmt is always false, the fmt import is never removed, and the post-fix file has an unused import — compile error.
2. Import edits are attached only to the first fix (line 43)
seenImportFiles intentionally omits import edits from every fix after the first in a file. Applying any non-first fix in isolation produces a file missing import "strconv" — compile error. Each SuggestedFix must be independently applicable.
Both issues require a test fixture with multiple violations in one file to be caught.
🔎 Code quality review by PR Code Quality Reviewer · 53.5 AIC · ⌖ 4.83 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| // If the flagged call is the only "fmt" reference in the file the "fmt" | ||
| // import will become unused after the fix and must be removed. | ||
| orphanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == 1 |
There was a problem hiding this comment.
** threshold is wrong for files with multiple violations** — a file with two fmt.Sprintf calls and no other fmt uses has countPkgUsesInFile == 2, so orphanFmt is never true, the fmt import is never removed, and the post-fix code fails to compile.
💡 Details and suggested fix
countPkgUsesInFile returns the current count of fmt selector references, including all the calls that are about to be rewritten. A file like:
import "fmt"
func a(n int) string { return fmt.Sprintf("%d", n) }
func b(n int) string { return fmt.Sprintf("%d", n) }...has count == 2, so orphanFmt is false for both diagnostics and the fmt import is never scheduled for removal. After applying both fixes the file still contains import "fmt" with zero uses — a compile error.
Root cause: the threshold == 1 only works when there is exactly one violation. For the general case, fmt should be removed when countPkgUsesInFile == numberOfViolationsInFile (i.e. all remaining fmt uses will be replaced).
One approach: count violations per file before the inspector walk, then compare:
// pre-pass: count fmt.Sprintf violations per file
violationsPerFile := countViolationsPerFile(pass, ...)
// then in buildImportEdits:
orphanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == violationsPerFile[file]The existing test only exercises the single-violation path (singleuse.go); a fixture with two flagged calls and no other fmt references would have caught this.
| // seenImportFiles tracks files that have already received an import edit in | ||
| // this pass, preventing duplicate overlapping edits when a single file | ||
| // contains multiple flagged calls. | ||
| seenImportFiles := make(map[token.Pos]bool) |
There was a problem hiding this comment.
seenImportFiles makes import edits non-self-contained — applying any fix other than the first one in a multi-violation file produces non-compiling code.
💡 Details and suggested fix
Import edits are attached only to whichever SuggestedFix is emitted first during the analysis walk. Subsequent violations in the same file get a fix with only the call-site rewrite and no import changes.
If a user (or an IDE) applies any fix other than that first one — perfectly reasonable behaviour — the result has:
strconv.Itoa(...)but noimport "strconv"→ compile error- Still has
import "fmt"with no remaining uses → compile error
This violates the expectation that every SuggestedFix is independently applicable.
Suggested fix: remove seenImportFiles and include the full import edits unconditionally in every fix. The golang.org/x/tools/go/analysis apply machinery is expected to handle overlapping / duplicate TextEdits across fixes. If deduplication is not guaranteed, model the import change as a separate per-file diagnostic (emitted once) rather than piggy-backing it on the first violation's fix.
| // seenImportFiles tracks files that have already received an import edit in | ||
| // this pass, preventing duplicate overlapping edits when a single file | ||
| // contains multiple flagged calls. | ||
| seenImportFiles := make(map[token.Pos]bool) |
There was a problem hiding this comment.
seenImportFiles is keyed on token.Pos rather than *ast.File — fragile and misleading.
💡 Details
token.Pos is an integer offset. The code relies on the undocumented behaviour that each file's start offset is unique, but the type carries no semantic meaning that this is a file identity. A reviewer (or future refactor) has no way to know the intent without reading the comment.
The idiomatic type is map[*ast.File]bool:
seenImportFiles := make(map[*ast.File]bool)
// ...
if seenImportFiles[file] { return nil }
seenImportFiles[file] = trueThis is self-documenting, pointer-equality is correct by definition for AST file nodes within a single pass, and avoids any concern about Pos uniqueness.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — 3 comments, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- TextEdit ordering (line 245–256): The multi-spec grouped branch returns the
Rpareninsertion (higherPos) before the line deletion (lowerPos). Not wrong foranalysistest(which sorts internally), but unconventional and worth fixing for consumers that apply edits sequentially. - Missing
seenImportFilestest (line 141): The duplicate-edit guard has no fixture exercising it. Amultiviolation.go+ golden would close this gap. // wantin golden file (singleuse.go.golden:11): Matches the existing convention, but the directive is semantically meaningless there. Low priority.
Positive Highlights
- ✅ Well-structured decomposition:
buildImportEdits→addStrconvRemoveFmtEdits/addImportEdit/removeImportEdit - ✅
importSpecLineRangeusingtoken.File.LineStartavoids fragilePos()-1arithmetic - ✅
countPkgUsesInFileviapass.TypesInfo.Usesis type-aware — no false positives from string literals - ✅
seenImportFilesmap correctly prevents overlapping edits - ✅ Stale may
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 67.5 AIC · ⌖ 4.76 AIC · ⊞ 6.6K
Comment /matt to run again
| End: lineEnd, | ||
| NewText: nil, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The two TextEdits in the grouped multi-spec branch are returned with the Rparen insert (higher Pos) placed before the fmtSpec line deletion (lower Pos). While analysistest sorts edits internally, returning them in descending position order is unconventional and risks subtle issues with tools that apply edits sequentially.
💡 Suggested fix — return edits in ascending Pos order
return []analysis.TextEdit{
{
Pos: lineStart, // lower position — delete "fmt" line first
End: lineEnd,
NewText: nil,
},
{
Pos: fmtDecl.Rparen, // higher position — append "strconv"
End: fmtDecl.Rparen,
NewText: []byte("\t\"" + strconvPkg + "\"\n"),
},
}The go/analysis spec does not mandate sorted edits from analysers, but matching the ascending-position convention is safer for all consumers.
@copilot please address this.
| // "fmt" import. seenImportFiles prevents duplicate overlapping edits in files | ||
| // with multiple violations. | ||
| func buildImportEdits(pass *analysis.Pass, file *ast.File, seenImportFiles map[token.Pos]bool) []analysis.TextEdit { | ||
| if seenImportFiles[file.Pos()] { |
There was a problem hiding this comment.
[/tdd] The seenImportFiles guard prevents import edits for the second violation in a multi-flagged file, but there's no test fixture that exercises this path. Without a test, a regression (e.g. emitting duplicate overlapping edits) would not be caught.
💡 Suggested test fixture
Add multiviolation.go / multiviolation.go.golden to the testdata with two fmt.Sprintf("%d", n) calls in a single-import file. The golden output should import strconv exactly once.
This directly exercises the seenImportFiles[file.Pos()] short-circuit on the second call.
@copilot please address this.
| // singleUseFmt is the only "fmt" reference in this file, so the fix must add | ||
| // "strconv" and remove the now-unused "fmt" import. | ||
| func singleUseFmt(n int) string { | ||
| return strconv.Itoa(n) // want `use strconv\.Itoa\(x\) instead of fmt\.Sprintf\("%d", x\)` |
There was a problem hiding this comment.
[/tdd] The golden file still contains the // want annotation. Golden files represent the expected output after the fix — the // want comment is an analysistest directive for the unfixed source and has no meaning in the golden file; it just adds noise.
💡 Suggested fix
Remove the // want ... comment from singleuse.go.golden line 11:
// Before (line 11 in golden):
return strconv.Itoa(n) // want `use strconv\.Itoa\(x\) instead of fmt\.Sprintf\("%d", x\)`
// After:
return strconv.Itoa(n)Compare with how sprintfint.go.golden handles this — it keeps the // want in the golden file too, so this is a pre-existing pattern, but it is still confusing. If the project convention is to keep them, this comment can be ignored.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
sprintfint'sSuggestedFixonly rewrote the call site. For any file wherefmt.Sprintf("%d", n)was the solefmtreference, applying the fix without a trailinggoimportspass produced non-compiling code (undefined: strconv+"fmt" imported and not used). This is inconsistent withbytescomparestringandwritebytestring, which already self-manage imports.Core fix —
buildItoaFixnow emits import editsbuildImportEdits— inspects the file and decides what's needed:"strconv"when not already imported"fmt"whencountPkgUsesInFile == 1(flagged call is the onlyfmtreference)addStrconvRemoveFmtEdits— handles the three import-section shapes atomically:import "fmt"→ replaced withimport "strconv"in one edit"fmt"→ same single-edit replacement"fmt"+ others → two non-overlapping edits: insert"strconv"before), delete"fmt"spec lineimportSpecLineRange— usestoken.File.LineStartfor spec-line deletion (no fragilePos()-1/End()+1arithmetic)seenImportFiles— prevents duplicate overlapping import edits when a file has multiple flagged callsDocstring — stale "may require goimports" caveat removedConcrete before/after
Test coverage
Added
singleuse.go+singleuse.go.goldento theanalysistest.RunWithSuggestedFixessuite — a fixture with onlyimport "fmt"and one flagged call, which exercises both the strconv-add and fmt-removal paths end-to-end. The existing multi-use fixture (wherefmthas other live references) continues to pass unchanged.