[linter-miner] linter: add sprintfbool — flag fmt.Sprintf("%t", b) → strconv.FormatBool(b)#46898
Conversation
Reports fmt.Sprintf("%t", b) calls where b is a single bool value and
suggests using strconv.FormatBool(b) instead. This is a natural
complement to the existing sprintfint linter (which flags Sprintf("%d",n)
→ strconv.Itoa).
The linter:
- Matches only the exact predeclared bool type (not named bool aliases)
- Provides a SuggestedFix that rewrites the call and manages imports
- Skips generated files and respects //nolint:sprintfbool directives
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds the sprintfbool Go analyzer, recommending strconv.FormatBool over fmt.Sprintf("%t", ...).
Changes:
- Implements detection, suppression, and suggested fixes.
- Registers the analyzer with the multichecker.
- Adds analyzer fixtures and golden tests.
Show a summary per file
| File | Description |
|---|---|
cmd/linters/main.go |
Registers sprintfbool. |
pkg/linters/sprintfbool/sprintfbool.go |
Implements analysis and fixes. |
pkg/linters/sprintfbool/sprintfbool_test.go |
Runs analyzer tests. |
pkg/linters/sprintfbool/testdata/src/sprintfbool/sprintfbool.go |
Provides test cases. |
pkg/linters/sprintfbool/testdata/src/sprintfbool/sprintfbool.go.golden |
Defines expected fixes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/linters/sprintfbool/sprintfbool.go:155
- A suggested fix is attached to one diagnostic and must be usable independently. In a file with two violations and no existing
strconvimport, this guard causes the second diagnostic's fix to emitstrconv.FormatBool(...)without addingstrconv, so applying that editor code action alone leaves uncompilable code. Please restructure reporting so each offered action is self-contained (for example, expose a file-wide fix once rather than statefully stripping edits from later diagnostics).
if seenImportFiles[file.Pos()] {
return nil
pkg/linters/sprintfbool/sprintfbool.go:169
- This only removes
fmtwhen there is exactly one package use. If a file consists of two flaggedfmt.Sprintf("%t", ...)calls, applying all suggested replacements addsstrconvonce but leavesfmtimported even though both uses disappeared, producing an unused-import compile error. Count whether allfmtuses are target calls, or build one file-level fix that rewrites every target and updates imports consistently.
orphanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == 1
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Medium
| switch imp.Path.Value { | ||
| case `"` + strconvPkg + `"`: | ||
| strconvImported = true | ||
| case `"` + fmtPkg + `"`: | ||
| fmtImported = true |
| if astutil.QualifierShadowed(pass.Pkg, call.Pos(), qualifier, strconvPkg) { | ||
| return nil | ||
| } | ||
|
|
||
| edits := []analysis.TextEdit{{ |
| if file != nil { | ||
| edits = append(edits, buildImportEdits(pass, file, seenImportFiles)...) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on test coverage gaps and a correctness issue in the import-management logic.
📋 Key Themes & Highlights
Key Themes
- Misleading diagnostic message: hardcodes
bregardless of the actual argument expression - Missing import-edge-case fixtures: the three branching paths in
buildImportEditsare not independently covered by test fixtures - Correctness risk in orphan-
fmtdetection: a file with multiple flagged calls wherefmtis used only by those calls may not havefmtremoved correctly
Positive Highlights
- ✅ Clean structural symmetry with
sprintfint— easy to review and maintain - ✅ Correct named-bool exclusion using
types.Typ[types.Bool]identity - ✅
seenImportFilesguard prevents duplicate overlapping edits - ✅
nolintand generated-file skipping wired correctly
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 41.9 AIC · ⌖ 7.5 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), |
There was a problem hiding this comment.
[/tdd] The diagnostic message hardcodes b as the variable name, but the actual argument may be any expression (e.g. true, isFoo). The message reads use strconv.FormatBool(b) instead of fmt.Sprintf("%t", b) even when the argument is a literal true, which is confusing.
💡 Suggestion
Incorporate the actual argText into the message (same opportunity exists in sprintfint):
Message: fmt.Sprintf(`use strconv.FormatBool(%s) instead of fmt.Sprintf("%%t", %s)`, argText, argText),@copilot please address this.
|
|
||
| func TestSprintfBool(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.RunWithSuggestedFixes(t, testdata, sprintfbool.Analyzer, "sprintfbool") |
There was a problem hiding this comment.
[/tdd] The test file contains only a single RunWithSuggestedFixes call covering all cases in the fixture. There are no targeted sub-tests for the import-management edge cases: (a) file where strconv is already imported, (b) file where fmt has multiple callers (so fmt should be kept), and (c) file with a single non-parenthesised import. These paths in buildImportEdits are hard to exercise from a single golden file.
💡 Suggestion
Add additional fixture files / sub-directories for each import scenario, e.g.:
testdata/src/sprintfbool_already_strconv/—strconvalready imported, only call rewrite neededtestdata/src/sprintfbool_keep_fmt/—fmtused elsewhere, only addstrconvtestdata/src/sprintfbool_single_import/— single bareimport "fmt"that must be rewritten to a grouped import
This mirrors how other linters in this repo organise multi-fixture tests.
@copilot please address this.
| strconvImported := false | ||
| fmtImported := false | ||
| for _, imp := range file.Imports { | ||
| switch imp.Path.Value { |
There was a problem hiding this comment.
[/tdd] countPkgUsesInFile counts all uses of fmt to decide if fmt is an orphan, but the Preorder visitor has already matched exactly one fmt.Sprintf call. If the file has two flagged fmt.Sprintf("%t", b) calls and nothing else using fmt, orphanFmt will be false on the first call (count == 2) and true on the second — leading to an incorrect edit order where fmt is only removed for the second occurrence. seenImportFiles stops further edits, so the removal is skipped entirely.
💡 Analysis
The root issue is that countPkgUsesInFile counts at static analysis time (before any rewrites), so it cannot know how many uses will remain after all fixes are applied. Consider computing the orphan check lazily or deferring import cleanup to a separate pass — as golang.org/x/tools/go/analysis/passes/buildssa does.
@copilot please address this.
|
|
||
| // badLiteral demonstrates the flagged pattern with a bool literal. | ||
| func badLiteral() string { | ||
| return fmt.Sprintf("%t", true) // want `use strconv\.FormatBool\(b\) instead of fmt\.Sprintf\("%t", b\)` |
There was a problem hiding this comment.
[/tdd] The golden file keeps the // want comment on the rewritten line, but RunWithSuggestedFixes verifies the fixed output — the want directive in the golden file is a false marker that will never match and could mislead future maintainers.
💡 Suggestion
The analysistest golden-file convention is that the .golden file shows the state after the fix, with no // want annotations. Remove the // want lines from sprintfbool.go.golden (lines 15 and 19) to keep the golden file clean and consistent with the rest of the codebase.
@copilot please address this.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (443 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 MatterADRs 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.
The implementation is clean and follows established linter patterns in this repo.
- Named bool types are correctly excluded via
types.Typ[types.Bool]identity check - Import management (add strconv / remove orphaned fmt) handles all edge cases correctly
seenImportFilesguard prevents overlapping edits when a file has multiple flagged calls- Test fixture and golden file are consistent
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 32 AIC · ⌖ 4.32 AIC · ⊞ 5K
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs in the suggested-fix path must be addressed before merging.
Blocking issues
1. orphanFmt count is wrong for files with multiple flagged calls (HIGH)
Line 195: countPkgUsesInFile returns the total number of fmt uses in the file. When a file has 2+ fmt.Sprintf("%t", b) calls and nothing else that uses fmt, orphanFmt is false for every diagnostic. After all fixes are applied, the fmt import is orphaned and the fixed file does not compile. The test fixture only exercises the single-call case, so this is untested.
2. addStrconvRemoveFmtEdits emits edits in reverse-position order (MEDIUM)
Line 267–279: the strconv insertion (at fmtDecl.Rparen, higher offset) is emitted before the fmt-line deletion (at lineStart, lower offset). analysistest sorts before applying (so tests pass), but gopls and golangci-lint consume edits in slice order, corrupting the import block when both edits are applied.
🔎 Code quality review by PR Code Quality Reviewer · 37 AIC · ⌖ 4.7 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| func countPkgUsesInFile(pass *analysis.Pass, file *ast.File, pkgPath string) int { | ||
| fileStart, fileEnd := file.Pos(), file.End() | ||
| count := 0 |
There was a problem hiding this comment.
Orphan-fmt removal silently fails when a file has multiple flagged calls, leaving an unused fmt import after the fix is applied — this produces a compile error.
💡 Explanation and suggested fix
countPkgUsesInFile counts all uses of fmt in the file at analysis time (original AST). When a file has two fmt.Sprintf("%t", b) calls and no other fmt use, the count is 2, so orphanFmt is false for both diagnostics. Neither fix emits a fmt import removal. After both suggested fixes are accepted, fmt is still imported but has no remaining uses, causing a compile error.
A clean fix: before emitting diagnostics, do a single pre-scan of the file to count matching calls, then:
orthanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == matchingCallsInThisFileAlternatively, emit the import-removal edit only for the first diagnostic (index 0) so it fires once per file regardless of call count, paired with the seenImportFiles guard.
The golden file in testdata only covers a single call — this bug is untested.
| if specText == "" { | ||
| continue | ||
| } | ||
| return analysis.TextEdit{ |
There was a problem hiding this comment.
addStrconvRemoveFmtEdits returns TextEdits in reverse-position order, which can corrupt source when tools apply edits sequentially without sorting.
💡 Explanation and fix
The returned slice is:
[]analysis.TextEdit{
{Pos: fmtDecl.Rparen, ...}, // INSERT strconv — higher offset
{Pos: lineStart, End: lineEnd, NewText: nil}, // DELETE fmt line — lower offset
}The Go analysis framework's analysistest sorts edits before applying them (so tests pass), but gopls and golangci-lint's autofixer apply edits in slice order. Applying the higher-offset insertion first shifts byte offsets, so the subsequent delete targets a now-wrong range, corrupting the import block.
Fix: reverse the slice order so the lower-position deletion comes first:
return []analysis.TextEdit{
{Pos: lineStart, End: lineEnd, NewText: nil}, // delete fmt line first
{Pos: fmtDecl.Rparen, End: fmtDecl.Rparen,
NewText: []byte("\t\"strconv\"\n")}, // then insert strconv
}
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
✅ Test Coverage HighlightsTest Function: Testdata Coverage (7 scenarios):
Build tag compliance: ✅ Correctly includes No mocking violations: ✅ No gomock, testify/mock, or .EXPECT() patterns Verdict
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29769168911 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed unresolved review feedback in 26e0756. I ran |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/29777731070 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Checked the current head ( |
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Reviewed unresolved threads on the current head and found no additional actionable code issues beyond what was fixed in |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new
sprintfboolGo static analysis linter that flagsfmt.Sprintf("%t", b)calls wherebis the exact predeclaredbooltype and suggests rewriting tostrconv.FormatBool(b). The suggested fix automatically managesfmt/strconvimport additions and removals.Changes
New:
pkg/linters/sprintfbool/sprintfbool.go— Analysis pass (~450 lines): matchesfmt.Sprintf("%t", b)with exact predeclared bool type; excludes named bool types, multi-arg calls, non-%tverbs; skips generated files; honours//nolint:sprintfbool; generates import-aware SuggestedFix edits (add strconv, remove fmt if unused, swap sole import, preserve aliases).sprintfbool_test.go— Test harness usinganalysistest.RunWithSuggestedFixes.testdata/src/sprintfbool/— 9 fixture pairs: basic variable/literal, named bool (no diagnostic), multiple calls (single import edit), single fmt use (swap import), mixed use (keep fmt), aliased import, raw backtick imports, shadowed identifier (no fix), comment overlap (no fix).Modified:
cmd/linters/main.goAdds
sprintfboolimport and registerssprintfbool.Analyzeralongside existing passes.New:
docs/adr/46898-add-sprintfbool-linter.mdDraft ADR documenting the decision, alternatives considered (extend sprintfint; use external linter), and consequences.
Files Changed
cmd/linters/main.gopkg/linters/sprintfbool/sprintfbool.gopkg/linters/sprintfbool/sprintfbool_test.gopkg/linters/sprintfbool/testdata/src/sprintfbool/(9 fixture pairs)docs/adr/46898-add-sprintfbool-linter.md