fix(stringbytesroundtrip): reword []byte(string(b)) diagnostic — defensive copy, not redundant round-trip

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.

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,
 		)
 	}
 }

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.