-
Notifications
You must be signed in to change notification settings - Fork 475
[linter-miner] linter: add sprintfint — flag fmt.Sprintf("%d", x) where x is int #42538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
18c8a69
52f92c3
0c271b5
0253901
0ad7908
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # ADR-42538: Add sprintfint Linter — Flag fmt.Sprintf("%d", int) in Favor of strconv.Itoa | ||
|
|
||
| **Date**: 2026-06-30 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan, linter-miner automation | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| Two production code sites in the repository (`pkg/stringutil/stringutil.go:59` and `pkg/console/console_wasm.go:41`) use `fmt.Sprintf("%d", x)` where `x` is a plain `int`. This idiom is correct but suboptimal: it incurs format-string parsing at runtime, requires importing the `fmt` package unnecessarily, and obscures intent compared to `strconv.Itoa(x)`. The project already encodes code-quality preferences as custom Go static-analysis linters in `pkg/linters/` (e.g., `sprintferrdot`, `sprintferrorsnew`), making a new linter the natural enforcement mechanism. Automated enforcement prevents this pattern from re-entering the codebase after the two existing instances are fixed. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will add a new custom Go analysis pass, `sprintfint`, that flags every call of the form `fmt.Sprintf("%d", x)` where `x` has the exact type `int` and reports `use strconv.Itoa(x) instead`. The analyzer includes a `SuggestedFix` that rewrites the call in place, skips `_test.go` files, and respects `//nolint:sprintfint` suppressions. It is registered in the project-wide `cmd/linters` multichecker following the same conventions as existing linters. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Rely on code review alone | ||
|
|
||
| Do not add any automated rule; trust that code authors and reviewers catch `fmt.Sprintf("%d", int)` during review. This was the implicit status quo and did not prevent the two existing instances from entering production code. Without automation the pattern will continue to recur as the codebase grows. | ||
|
|
||
| #### Alternative 2: Use an existing external linter (e.g., perfsprint from golangci-lint) | ||
|
|
||
| The `perfsprint` linter (available in `golangci-lint`) covers similar patterns. However, it operates outside the project's custom-linter infrastructure, requires an additional external dependency and configuration layer, and its scope is broader than the narrow `fmt.Sprintf("%d", int)` → `strconv.Itoa` substitution the team wants to enforce. Adopting it would also break the convention of keeping all custom lint rules within `pkg/linters/` where they share internal helpers (`astutil`, `filecheck`, `nolint`). | ||
|
|
||
| #### Alternative 3: Extend the linter to cover all integer types (int8, int32, int64, uint, etc.) | ||
|
|
||
| A broader rule would flag `fmt.Sprintf("%d", n64)` (int64) and suggest `strconv.FormatInt(n64, 10)`, and similarly for other integer widths. This was not chosen because the safe mechanical replacement (`strconv.Itoa`) is only available for the plain `int` type; wider coverage would require type-specific suggestions that complicate the implementation and increase false-positive risk for less common types. The narrow scope can be expanded in a follow-up linter if needed. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Eliminates a class of redundant format-string calls in production code, reducing minor runtime overhead and improving readability. | ||
| - Provides a `SuggestedFix` so editors and `go tool` can auto-apply the rewrite, lowering friction for contributors. | ||
| - Consistent with the project's established pattern of encoding style decisions as independently testable `golang.org/x/tools/go/analysis` passes. | ||
| - Prevents regression: the two existing instances and any future occurrences will be caught in CI. | ||
|
|
||
| #### Negative | ||
| - Scope is restricted to the exact type `int`; the same smell with `int64`, `uint`, `int32`, etc. remains undetected and must be addressed separately. | ||
| - Adds another analyzer to the multichecker that must be maintained as Go's standard library and `x/tools/go/analysis` API evolve. | ||
|
|
||
| #### Neutral | ||
| - Test fixtures follow the project's `analysistest` convention (`testdata/src/<pkg>/<pkg>.go` with `// want` annotations), adding a small amount of checked-in test data. | ||
| - The `//nolint:sprintfint` suppression mechanism follows the same pattern as other linters in the suite, so team norms around nolint comments carry over without new conventions. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| // Package sprintfint implements a Go analysis linter that flags | ||
| // fmt.Sprintf("%d", x) calls where x is a single int value and suggests | ||
| // using strconv.Itoa(x) instead. | ||
| package sprintfint | ||
|
|
||
| import ( | ||
| "go/ast" | ||
| "go/token" | ||
| "go/types" | ||
|
|
||
| "golang.org/x/tools/go/analysis" | ||
| "golang.org/x/tools/go/analysis/passes/inspect" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/internal/astutil" | ||
| "github.com/github/gh-aw/pkg/linters/internal/filecheck" | ||
| "github.com/github/gh-aw/pkg/linters/internal/nolint" | ||
| ) | ||
|
|
||
| // Analyzer is the sprintfint analysis pass. | ||
| var Analyzer = &analysis.Analyzer{ | ||
| Name: "sprintfint", | ||
| Doc: "reports fmt.Sprintf(\"%d\", x) calls where x is a single int value; use strconv.Itoa(x) instead (suggested fixes may require goimports to add/remove imports)", | ||
| URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/sprintfint", | ||
| Requires: []*analysis.Analyzer{inspect.Analyzer}, | ||
| Run: run, | ||
| } | ||
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| insp, err := astutil.Inspector(pass) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| noLintLinesByFile := nolint.BuildLineIndex(pass, "sprintfint") | ||
|
|
||
| nodeFilter := []ast.Node{ | ||
| (*ast.CallExpr)(nil), | ||
| } | ||
|
|
||
| insp.Preorder(nodeFilter, func(n ast.Node) { | ||
| call, ok := n.(*ast.CallExpr) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| pos := pass.Fset.PositionFor(call.Pos(), false) | ||
| if filecheck.IsTestFile(pos.Filename) { | ||
| return | ||
| } | ||
| if nolint.HasDirective(pos, noLintLinesByFile) { | ||
| return | ||
| } | ||
|
|
||
| // Match fmt.Sprintf(format, arg) with exactly two arguments. | ||
| sel, ok := call.Fun.(*ast.SelectorExpr) | ||
| if !ok || sel.Sel.Name != "Sprintf" { | ||
| return | ||
| } | ||
| if !astutil.IsPkgSelector(pass, sel, "fmt") { | ||
| return | ||
| } | ||
| if len(call.Args) != 2 { | ||
| return | ||
| } | ||
|
|
||
| // The format argument must be the string literal "%d". | ||
| formatLit, ok := call.Args[0].(*ast.BasicLit) | ||
| if !ok || formatLit.Kind != token.STRING || formatLit.Value != `"%d"` { | ||
| return | ||
| } | ||
|
|
||
| // The value argument must have the exact type int (not int64, uint, etc.). | ||
| arg := call.Args[1] | ||
| argType := pass.TypesInfo.TypeOf(arg) | ||
| if argType == nil { | ||
| return | ||
| } | ||
| if argType != types.Typ[types.Int] { | ||
| return | ||
| } | ||
|
|
||
| pass.Report(analysis.Diagnostic{ | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| Message: `use strconv.Itoa(x) instead of fmt.Sprintf("%d", x)`, | ||
| SuggestedFixes: buildItoaFix(pass, call, arg), | ||
| }) | ||
| }) | ||
|
|
||
| return nil, nil | ||
| } | ||
|
|
||
| // buildItoaFix returns a SuggestedFix rewriting | ||
| // fmt.Sprintf("%d", x) → strconv.Itoa(x). | ||
| func buildItoaFix(pass *analysis.Pass, call *ast.CallExpr, arg ast.Expr) []analysis.SuggestedFix { | ||
| argText := astutil.NodeText(pass.Fset, arg) | ||
| if argText == "" { | ||
| return nil | ||
| } | ||
| return []analysis.SuggestedFix{{ | ||
| Message: "Replace fmt.Sprintf with strconv.Itoa", | ||
| TextEdits: []analysis.TextEdit{ | ||
| { | ||
| Pos: call.Pos(), | ||
| End: call.End(), | ||
| NewText: []byte("strconv.Itoa(" + argText + ")"), | ||
| }, | ||
| }, | ||
| }} | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| //go:build !integration | ||
|
|
||
| // Package sprintfint_test provides tests for the sprintfint analyzer. | ||
| package sprintfint_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "golang.org/x/tools/go/analysis/analysistest" | ||
|
|
||
| "github.com/github/gh-aw/pkg/linters/sprintfint" | ||
| ) | ||
|
|
||
| func TestSprintfInt(t *testing.T) { | ||
| testdata := analysistest.TestData() | ||
| analysistest.Run(t, testdata, sprintfint.Analyzer, "sprintfint") | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Package sprintfint is the test fixture for the sprintfint analyzer. | ||
| package sprintfint | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strconv" | ||
| ) | ||
|
|
||
| type counter int | ||
|
|
||
| // bad demonstrates the flagged pattern: fmt.Sprintf with a single "%d" verb | ||
| // and an int argument, which should be replaced by strconv.Itoa. | ||
| func bad(n int) string { | ||
| return fmt.Sprintf("%d", n) // want `use strconv\.Itoa\(x\) instead of fmt\.Sprintf\("%d", x\)` | ||
| } | ||
|
|
||
| // badLiteral demonstrates the flagged pattern with an integer literal. | ||
| func badLiteral() string { | ||
| return fmt.Sprintf("%d", 42) // want `use strconv\.Itoa\(x\) instead of fmt\.Sprintf\("%d", x\)` | ||
| } | ||
|
|
||
| // goodStrconvItoa is already using the preferred form — no diagnostic expected. | ||
| func goodStrconvItoa(n int) string { | ||
| return strconv.Itoa(n) | ||
| } | ||
|
|
||
| // goodInt64 uses int64 rather than int — not flagged; use strconv.FormatInt. | ||
| func goodInt64(n int64) string { | ||
| return fmt.Sprintf("%d", n) | ||
| } | ||
|
|
||
| // goodUint uses uint — not flagged. | ||
| func goodUint(n uint) string { | ||
| return fmt.Sprintf("%d", n) | ||
| } | ||
|
|
||
| // goodMultipleVerbs uses more than one verb — not flagged. | ||
| func goodMultipleVerbs(a, b int) string { | ||
| return fmt.Sprintf("%d %d", a, b) | ||
| } | ||
|
|
||
| // goodOtherVerb uses a different verb — not flagged. | ||
| func goodOtherVerb(n int) string { | ||
| return fmt.Sprintf("%v", n) | ||
| } | ||
|
|
||
| // goodNamedInt uses a named int type — not flagged because strconv.Itoa | ||
| // requires the exact predeclared int type. | ||
| func goodNamedInt(n counter) string { | ||
| return fmt.Sprintf("%d", n) | ||
| } | ||
|
|
||
| // suppressed suppresses the linter directive — no diagnostic expected. | ||
| func suppressed(n int) string { | ||
| return fmt.Sprintf("%d", n) //nolint:sprintfint | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test case: named type based on After fixing the // goodNamedInt uses a named type based on int — not flagged, because
// strconv.Itoa does not accept named types.
func goodNamedInt(n Counter) string {
type Counter int // or defined at package level
return fmt.Sprintf("%d", n)
}Without this, a future refactor that reintroduces @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test case for named int types: the fixture has no case like 💡 Suggested additiontype myInt int
// goodNamedInt is a named type; the linter must NOT flag this because the
// suggested fix strconv.Itoa(x) would not compile for a non-int type.
func goodNamedInt(n myInt) string {
return fmt.Sprintf("%d", n) // no diagnostic expected
} |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SuggestedFixomits thestrconvimport, producing uncompilable code when applied in files that don't already import it: theTextEditonly rewrites the call site. Any file where"strconv"is not yet imported will fail to compile after the fix is applied.💡 Suggested fix
Check whether
strconvis already imported and add aTextEditfor the import declaration when it is not. Other analyzers in this repo (e.g.fprintlnsprintf) handle this via a helper that inserts the import block edit.Alternatively, document the limitation clearly in
Analyzer.Docso that tool integrators knowgoimports/goplsmust be run alongside the fix. As-is, a user runninggo vet -fixwill silently get broken code.