diff --git a/docs/adr/47375-add-stringbytesroundtrip-linter.md b/docs/adr/47375-add-stringbytesroundtrip-linter.md index ef145fb3ff8..b34b4caabb3 100644 --- a/docs/adr/47375-add-stringbytesroundtrip-linter.md +++ b/docs/adr/47375-add-stringbytesroundtrip-linter.md @@ -8,11 +8,11 @@ ### Context -The repository uses a custom Go static analysis framework (`pkg/linters/`) to enforce code-quality rules beyond what standard tooling catches. Go developers occasionally write `string([]byte(s))` when `s` is already a `string`, or `[]byte(string(b))` when `b` is already a `[]byte`. These round-trip conversions cause two unnecessary memory allocations and produce no semantic difference — the output type is identical to the input type. This pattern can be introduced silently during refactoring or copy-paste and is not caught by any existing linter in this repository. +The repository uses a custom Go static analysis framework (`pkg/linters/`) to enforce code-quality rules beyond what standard tooling catches. Go developers occasionally write `string([]byte(s))` when `s` is already a `string`, or `[]byte(string(b))` when `b` is already a `[]byte`. The two patterns are semantically distinct: `string([]byte(s))` is genuinely redundant (the result equals `s`), while `[]byte(string(b))` is the defensive-copy idiom that produces a non-aliasing clone — but via two memory copies instead of one. Neither pattern is caught by any existing linter in this repository. ### Decision -We will add a new custom Go analysis pass, `stringbytesroundtrip`, to the linter registry (`pkg/linters/registry.go`). The analyzer traverses the AST for nested `CallExpr` nodes, checks type information to confirm the outer and inner conversions form a round-trip (`string→[]byte→string` or `[]byte→string→[]byte`), and reports a diagnostic with the redundant expression identified. It integrates with the existing `nolint`, `filecheck`, and `inspect` analysis passes already used by sibling linters. +We will add a new custom Go analysis pass, `stringbytesroundtrip`, to the linter registry (`pkg/linters/registry.go`). The analyzer traverses the AST for nested `CallExpr` nodes, checks type information to confirm the outer and inner conversions form a round-trip (`string→[]byte→string` or `[]byte→string→[]byte`), and reports a diagnostic with the relevant expression identified. The `string([]byte(s))` arm is flagged as a redundant round-trip (safe to remove); the `[]byte(string(b))` arm is flagged as a wasteful two-copy clone (correct fix is `slices.Clone(b)` or `bytes.Clone(b)`, not removal). The analyzer integrates with the existing `nolint`, `filecheck`, and `inspect` analysis passes already used by sibling linters. ### Alternatives Considered @@ -27,12 +27,13 @@ The team could document this anti-pattern in style guides and rely on reviewers ### Consequences #### Positive -- Redundant `string([]byte(s))` and `[]byte(string(b))` patterns are caught automatically at CI time, preventing unnecessary memory allocations from reaching the main branch. +- `string([]byte(s))` patterns are caught as genuinely redundant at CI time, preventing unnecessary allocations from reaching the main branch. +- `[]byte(string(b))` patterns are flagged as wasteful two-copy clones with a clear recommendation to use `slices.Clone` or `bytes.Clone` instead. - The implementation follows the established custom-analyzer pattern in `pkg/linters/`, keeping the linter ecosystem consistent and easy to maintain. #### Negative - Every new analyzer added to the registry increases overall CI compilation and analysis time, albeit marginally for a single focused pass. -- Contributors unfamiliar with the round-trip anti-pattern may encounter confusing lint errors until they learn why such conversions are redundant. +- Contributors unfamiliar with the round-trip patterns may encounter confusing lint errors until they understand the distinction between a redundant round-trip and a defensive-copy clone. #### Neutral - The analyzer skips generated files (via `filecheck`) and respects `//nolint:stringbytesroundtrip` directives, consistent with all other custom linters in this repository. diff --git a/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go b/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go index c4b8fee14fd..a19341e6533 100644 --- a/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go +++ b/pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go @@ -1,8 +1,10 @@ -// Package stringbytesroundtrip implements a Go analysis linter that flags -// redundant round-trip type conversions: string([]byte(s)) when s is already -// a string, and []byte(string(b)) when b is already a []byte. Both -// conversions create an unnecessary intermediate copy and leave the caller with -// the same underlying type as the input. +// Package stringbytesroundtrip implements a Go analysis linter that flags two +// related but semantically distinct patterns: +// - string([]byte(s)) when s is already a string: genuinely redundant — the +// result is value-identical to s and both conversions can be removed. +// - []byte(string(b)) when b is already a []byte: not redundant but wasteful +// — this is the defensive-copy idiom that produces a non-aliasing clone via +// two copies; prefer slices.Clone(b) or bytes.Clone(b) for a single copy. package stringbytesroundtrip import ( @@ -20,7 +22,7 @@ import ( // Analyzer is the string-bytes-roundtrip analysis pass. var Analyzer = &analysis.Analyzer{ Name: "stringbytesroundtrip", - Doc: "reports redundant string/[]byte round-trip conversions such as string([]byte(s)) or []byte(string(b)) that produce a wasteful intermediate copy", + Doc: "reports string([]byte(s)) as a redundant round-trip when s is already a string, and []byte(string(b)) as a wasteful two-copy clone when b is already a []byte (prefer slices.Clone or bytes.Clone)", URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/stringbytesroundtrip", Requires: []*analysis.Analyzer{inspect.Analyzer, nolint.Analyzer, filecheck.Analyzer}, Run: run, @@ -48,7 +50,8 @@ func run(pass *analysis.Pass) (any, error) { } // analyzeRoundTrip checks whether a conversion expression is a redundant -// string/[]byte round-trip and reports a diagnostic if so. +// string/[]byte round-trip (string([]byte(s))) or a wasteful two-copy clone +// ([]byte(string(b))) and reports a diagnostic if so. func analyzeRoundTrip(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.GeneratedIndex, noLintIndex nolint.DirectiveIndex) { outer, ok := n.(*ast.CallExpr) if !ok { @@ -116,11 +119,14 @@ func analyzeRoundTrip(pass *analysis.Pass, n ast.Node, generatedFiles filecheck. } // Check []byte(string(b)) where b is already a []byte. + // This is the defensive-copy idiom: the result is a non-aliasing copy, not + // a no-op. The diagnostic is therefore not "redundant" but "wasteful": + // two memory copies are made when one would suffice. if isByteSliceType(outerUnderlying) && isStringType(innerUnderlying) && isByteSliceType(innerArgUnderlying) { argText := astutil.NodeText(pass.Fset, inner.Args[0]) pass.ReportRangef(outer, - "[]byte(string(%s)) is a redundant round-trip; the inner string conversion copies the bytes unnecessarily", - argText, + "[]byte(string(%s)) makes two copies to clone %s; use slices.Clone(%s) or bytes.Clone(%s) for a single-copy independent slice", + argText, argText, argText, argText, ) } } diff --git a/pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go b/pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go index 2a4f4839cda..8366a98b459 100644 --- a/pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go +++ b/pkg/linters/stringbytesroundtrip/testdata/src/stringbytesroundtrip/stringbytesroundtrip.go @@ -24,7 +24,7 @@ func bad() { b := []byte{104, 101, 108, 108, 111} _ = string([]byte(s)) // want `string\(\[\]byte\(s\)\) is a redundant round-trip` - _ = []byte(string(b)) // want `\[\]byte\(string\(b\)\) is a redundant round-trip` + _ = []byte(string(b)) // want `\[\]byte\(string\(b\)\) makes two copies to clone b` } func badNamedTypes() { @@ -33,7 +33,7 @@ func badNamedTypes() { // Named-type round-trips: underlying types still match, so these are flagged. _ = string([]byte(ms)) // want `string\(\[\]byte\(ms\)\) is a redundant round-trip` - _ = []byte(string(mb)) // want `\[\]byte\(string\(mb\)\) is a redundant round-trip` + _ = []byte(string(mb)) // want `\[\]byte\(string\(mb\)\) makes two copies to clone mb` } // helperString is a regular function, not a type conversion — must not be flagged.