Guard suggested fixes against comment loss in five linters

docs/adr/57860-guard-linter-autofixes-against-comment-loss.md

@@ -0,0 +1,50 @@
+# ADR-57860: Guard Linter Autofixes Against Comment Loss
+
+**Date**: 2026-09-02
+**Status**: Draft
+**Deciders**: pelikhan, adr-writer agent
+
+---
+
+### Context
+
+This pull request changes five Go linters that currently emit whole-expression `SuggestedFix` text edits for simplifications such as `append`, `time.Now().Sub`, `strings.Join`, `strings.Count`, and `strings.Index`. The PR description and tests show that these fixes can silently delete inline or trailing comments when the replacement span overlaps commented source, because AST printing does not preserve comments inside the rewritten expression. The diff introduces a shared overlap check in `pkg/linters/internal/astutil` and updates each affected linter plus golden tests. Because this changes the policy for when autofixes are emitted across multiple analyzers, the behavior should be captured explicitly.
+
+### Decision
+
+We will keep reporting the diagnostics from these linters, but suppress their `SuggestedFix` autofixes whenever the replacement span overlaps source comments. We will centralize the overlap detection in `astutil.HasOverlappingComment` and make shared fix construction helpers accept the parsed files needed to perform that check. We chose this approach because preserving user comments is more important than always offering an automatic rewrite, and the diff shows a single reusable guard can address the issue consistently across all five linters.
+
+### Alternatives Considered
+
+#### Alternative 1: Continue Emitting Autofixes Unconditionally
+
+Keep the existing behavior and allow the analyzers to replace the full expression even when comments appear inside the rewritten span.
+
+This was considered because it preserves maximum autofix coverage and requires no additional guard logic. It was not chosen because the PR evidence shows this behavior can silently delete user comments, which is a correctness and trust issue for `-fix` output.
+
+#### Alternative 2: Rebuild Fixes to Preserve Comments
+
+Teach each linter to generate narrower edits or comment-aware rewrites that retain inline and trailing comments while still applying an autofix.
+
+This was considered because it could preserve both automation and comments. It was not chosen in this PR because the implemented evidence shows a simpler, shared suppression strategy across five linters, while a fully comment-preserving rewrite mechanism would be more complex and is not demonstrated by the current changes.
+
+### Consequences
+
+#### Positive
+- `go analysis -fix` no longer risks silently deleting overlapping inline or trailing comments for the affected linters.
+- The overlap policy is applied consistently through a shared helper and shared test coverage.
+- Diagnostics still surface the simplification opportunity even when an autofix is unsafe to apply automatically.
+
+#### Negative
+- Some findings that were previously auto-fixable now require manual edits when comments overlap the replacement span.
+- Analyzer code paths become slightly more complex because fix generation now depends on parsed files and overlap checks.
+- Future linters that emit whole-expression rewrites must remember to apply the same safety rule or use the shared helper correctly.
+
+#### Neutral
+- Testdata and golden files now explicitly encode the distinction between reporting a diagnostic and offering a fix.
+- Shared helper signatures change to accept `pass.Files`, which updates caller contracts without changing the diagnostic messages themselves.
+- The decision applies only to overlapping-comment cases; safe autofixes continue to be emitted for uncommented expressions.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/linters/appendoneelement/appendoneelement.go

@@ -74,19 +74,22 @@ func analyzeAppendOneElement(pass *analysis.Pass, n ast.Node, generatedFiles fil
 		return
 	}
 
-	pass.Report(analysis.Diagnostic{
+	diag := analysis.Diagnostic{
 		Pos:     call.Pos(),
 		End:     call.End(),
 		Message: fmt.Sprintf("append(s, %s...) can be simplified to append(s, %s)", litText, elemText),
-		SuggestedFixes: []analysis.SuggestedFix{{
+	}
+	if !astutil.HasOverlappingComment(pass.Files, call.Pos(), call.End()) {
+		diag.SuggestedFixes = []analysis.SuggestedFix{{
 			Message: fmt.Sprintf("Replace %s... with %s", litText, elemText),
 			TextEdits: []analysis.TextEdit{{
 				Pos:     call.Pos(),
 				End:     call.End(),
 				NewText: fmt.Appendf(nil, "append(%s, %s)", sliceText, elemText),
 			}},
-		}},
-	})
+		}}
+	}
+	pass.Report(diag)
 }
 
 // matchSingleElementSpread validates that call.Args[1] is a single-element slice

pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go

@@ -19,6 +19,13 @@ func badVar() {
 	_ = s
 }
 
+func badWithComments() {
+	s := []int{1}
+	x := 99
+	s = append(s, []int{x /* important */}...) // want `append\(s, \[\]int\{x\}\.\.\.\) can be simplified to append\(s, x\)`
+	_ = s
+}
+
 func good() {
 	s := []int{1, 2}
 	// Multiple elements — keep the spread form.

pkg/linters/appendoneelement/testdata/src/appendoneelement/appendoneelement.go.golden

@@ -19,6 +19,13 @@ func badVar() {
 	_ = s
 }
 
+func badWithComments() {
+	s := []int{1}
+	x := 99
+	s = append(s, []int{x /* important */}...) // want `append\(s, \[\]int\{x\}\.\.\.\) can be simplified to append\(s, x\)`
+	_ = s
+}
+
 func good() {
 	s := []int{1, 2}
 	// Multiple elements — keep the spread form.

pkg/linters/internal/astutil/astutil.go

@@ -657,7 +657,11 @@ func SwapImportEdits(fset *token.FileSet, file *ast.File, addPkg, removePkg stri
 // BuildContainsFix builds the suggested fix rewriting a comparison to
 // strings.Contains. fixMessage is used as the SuggestedFix.Message field so
 // callers can identify the rewritten function (e.g. "Index" vs "Count").
-func BuildContainsFix(expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool, fixMessage string) []analysis.SuggestedFix {
+func BuildContainsFix(files []*ast.File, expr *ast.BinaryExpr, pkgText, sText, subText string, negated bool, fixMessage string) []analysis.SuggestedFix {
+	if HasOverlappingComment(files, expr.Pos(), expr.End()) {
+		return nil
+	}
+
 	var replacement string
 	if negated {
 		replacement = "!" + pkgText + ".Contains(" + sText + ", " + subText + ")"

pkg/linters/internal/astutil/astutil_test.go

@@ -484,7 +484,7 @@ func TestBuildContainsFix(t *testing.T) {
 		Y:  ast.NewIdent("b"),
 	}
 
-	fixes := BuildContainsFix(expr, "strings", "s", "sub", false, "test message")
+	fixes := BuildContainsFix(nil, expr, "strings", "s", "sub", false, "test message")
 	if len(fixes) != 1 {
 		t.Fatalf("got %d fixes, want 1", len(fixes))
 	}
@@ -496,13 +496,42 @@ func TestBuildContainsFix(t *testing.T) {
 	}
 
 	// negated
-	fixes = BuildContainsFix(expr, "strings", "s", "sub", true, "negated message")
+	fixes = BuildContainsFix(nil, expr, "strings", "s", "sub", true, "negated message")
 	if got := string(fixes[0].TextEdits[0].NewText); got != "!strings.Contains(s, sub)" {
 		t.Fatalf("negated NewText = %q, want %q", got, "!strings.Contains(s, sub)")
 	}
 	if fixes[0].Message != "negated message" {
 		t.Fatalf("negated Message = %q, want %q", fixes[0].Message, "negated message")
 	}
+
+	// overlapping comment suppresses fix
+	src := `package p
+func f() bool {
+	return strings.Count("a", "b" /* comment */) > 0
+}`
+	fset := token.NewFileSet()
+	file, err := parser.ParseFile(fset, "p.go", src, parser.ParseComments)
+	if err != nil {
+		t.Fatalf("ParseFile failed: %v", err)
+	}
+	var binExpr *ast.BinaryExpr
+	ast.Inspect(file, func(n ast.Node) bool {
+		if be, ok := n.(*ast.BinaryExpr); ok {
+			binExpr = be
+			return false
+		}
+		return true
+	})
+	if binExpr == nil {
+		t.Fatal("expected BinaryExpr")
+	}
+	if !HasOverlappingComment([]*ast.File{file}, binExpr.Pos(), binExpr.End()) {
+		t.Fatal("expected HasOverlappingComment to be true for test expression")
+	}
+	fixesWithComment := BuildContainsFix([]*ast.File{file}, binExpr, "strings", "s", "sub", false, "test message")
+	if len(fixesWithComment) != 0 {
+		t.Fatalf("got %d fixes with overlapping comment, want 0", len(fixesWithComment))
+	}
 }
 
 func TestByteStringTypeHelpers(t *testing.T) {

pkg/linters/stringscountcontains/stringscountcontains.go

@@ -69,7 +69,7 @@ func analyzeCountContains(pass *analysis.Pass, n ast.Node, generatedFiles filech
 		Pos:            expr.Pos(),
 		End:            expr.End(),
 		Message:        msg,
-		SuggestedFixes: astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Count comparison with strings.Contains"),
+		SuggestedFixes: astutil.BuildContainsFix(pass.Files, expr, pkgText, sText, subText, negated, "Replace strings.Count comparison with strings.Contains"),
 	})
 }
 

pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go

@@ -62,3 +62,7 @@ func badParenCountGTR(s, sub string) bool {
 func badParenYodaCountEQL(s, sub string) bool {
 	return 0 == (strings.Count(s, sub)) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison`
 }
+
+func badCountWithComments(s, sub string) bool {
+	return strings.Count(s, sub /* substr */) > 0 // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison`
+}

pkg/linters/stringscountcontains/testdata/src/stringscountcontains/stringscountcontains.go.golden

@@ -62,3 +62,7 @@ func badParenCountGTR(s, sub string) bool {
 func badParenYodaCountEQL(s, sub string) bool {
 	return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Count comparison`
 }
+
+func badCountWithComments(s, sub string) bool {
+	return strings.Count(s, sub /* substr */) > 0 // want `use strings\.Contains\(s, sub\) instead of strings\.Count comparison`
+}

pkg/linters/stringsindexcontains/stringsindexcontains.go

@@ -64,7 +64,7 @@ func analyzeIndexContains(pass *analysis.Pass, n ast.Node, generatedFiles filech
 	} else {
 		msg = "use strings.Contains(" + sText + ", " + subText + ") instead of strings.Index comparison"
 	}
-	fix := astutil.BuildContainsFix(expr, pkgText, sText, subText, negated, "Replace strings.Index comparison with strings.Contains")
+	fix := astutil.BuildContainsFix(pass.Files, expr, pkgText, sText, subText, negated, "Replace strings.Index comparison with strings.Contains")
 	pass.Report(analysis.Diagnostic{
 		Pos:            expr.Pos(),
 		End:            expr.End(),

pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go

@@ -72,3 +72,7 @@ func badParenContains(s, sub string) bool {
 func badParenYodaNotContains(s, sub string) bool {
 	return -1 == (strings.Index(s, sub)) // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison`
 }
+
+func badIndexWithComments(s, sub string) bool {
+	return strings.Index(s, sub /* substr */) != -1 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison`
+}

pkg/linters/stringsindexcontains/testdata/src/stringsindexcontains/stringsindexcontains.go.golden

@@ -72,3 +72,7 @@ func badParenContains(s, sub string) bool {
 func badParenYodaNotContains(s, sub string) bool {
 	return !strings.Contains(s, sub) // want `use !strings\.Contains\(s, sub\) instead of strings\.Index comparison`
 }
+
+func badIndexWithComments(s, sub string) bool {
+	return strings.Index(s, sub /* substr */) != -1 // want `use strings\.Contains\(s, sub\) instead of strings\.Index comparison`
+}

pkg/linters/stringsjoinone/stringsjoinone.go

@@ -78,19 +78,22 @@ func analyzeJoinOne(pass *analysis.Pass, n ast.Node, generatedFiles filecheck.Ge
 		return
 	}
 
-	pass.Report(analysis.Diagnostic{
+	diag := analysis.Diagnostic{
 		Pos:     call.Pos(),
 		End:     call.End(),
 		Message: fmt.Sprintf("strings.Join called with a single-element slice; use %s directly", replacementText),
-		SuggestedFixes: []analysis.SuggestedFix{{
+	}
+	if !astutil.HasOverlappingComment(pass.Files, call.Pos(), call.End()) {
+		diag.SuggestedFixes = []analysis.SuggestedFix{{
 			Message: "Replace strings.Join call with " + replacementText,
 			TextEdits: []analysis.TextEdit{{
 				Pos:     call.Pos(),
 				End:     call.End(),
 				NewText: []byte(replacementText),
 			}},
-		}},
-	})
+		}}
+	}
+	pass.Report(diag)
 }
 
 // isSafeToDiscardSeparator reports whether sep is a compile-time constant and

pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go

@@ -28,6 +28,11 @@ func joinOneSliced(a, b string) string {
 	return strings.Join([]string{a + b}, "")[1:] // want `strings\.Join called with a single-element slice`
 }
 
+// flagged: single-element []string literal with inline comment; fix suppressed to preserve comment.
+func joinOneWithComments(name string) string {
+	return strings.Join([]string{name /* display */}, ",") // want `strings\.Join called with a single-element slice`
+}
+
 // not flagged: two-element slice literal.
 func joinTwo(a, b string) string {
 	return strings.Join([]string{a, b}, ", ")

pkg/linters/stringsjoinone/testdata/src/stringsjoinone/stringsjoinone.go.golden

@@ -28,6 +28,11 @@ func joinOneSliced(a, b string) string {
 	return (a + b)[1:] // want `strings\.Join called with a single-element slice`
 }
 
+// flagged: single-element []string literal with inline comment; fix suppressed to preserve comment.
+func joinOneWithComments(name string) string {
+	return strings.Join([]string{name /* display */}, ",") // want `strings\.Join called with a single-element slice`
+}
+
 // not flagged: two-element slice literal.
 func joinTwo(a, b string) string {
 	return strings.Join([]string{a, b}, ", ")

pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go

@@ -25,6 +25,10 @@ func badIndex(starts []time.Time, i int) time.Duration {
 	return time.Now().Sub(starts[i]) // want `time\.Now\(\)\.Sub\(starts\[i\]\) can be simplified to time\.Since\(starts\[i\]\)`
 }
 
+func badWithComments(start time.Time) time.Duration {
+	return time.Now().Sub(start /* measured from init */) // want `time\.Now\(\)\.Sub\(start\) can be simplified to time\.Since\(start\)`
+}
+
 func good(t time.Time) {
 	_ = time.Since(t)
 }

pkg/linters/timenowsub/testdata/src/timenowsub/timenowsub.go.golden

@@ -25,6 +25,10 @@ func badIndex(starts []time.Time, i int) time.Duration {
 	return time.Since(starts[i]) // want `time\.Now\(\)\.Sub\(starts\[i\]\) can be simplified to time\.Since\(starts\[i\]\)`
 }
 
+func badWithComments(start time.Time) time.Duration {
+	return time.Now().Sub(start /* measured from init */) // want `time\.Now\(\)\.Sub\(start\) can be simplified to time\.Since\(start\)`
+}
+
 func good(t time.Time) {
 	_ = time.Since(t)
 }

pkg/linters/timenowsub/timenowsub.go

@@ -73,19 +73,22 @@ func analyzeTimeNowSub(pass *analysis.Pass, n ast.Node, generatedFiles filecheck
 	}
 	sinceText := qualifier + ".Since(" + argText + ")"
 
-	pass.Report(analysis.Diagnostic{
+	diag := analysis.Diagnostic{
 		Pos:     outer.Pos(),
 		End:     outer.End(),
 		Message: fmt.Sprintf("%s.Now().Sub(%s) can be simplified to %s", qualifier, argText, sinceText),
-		SuggestedFixes: []analysis.SuggestedFix{{
+	}
+	if !astutil.HasOverlappingComment(pass.Files, outer.Pos(), outer.End()) {
+		diag.SuggestedFixes = []analysis.SuggestedFix{{
 			Message: fmt.Sprintf("Replace %s.Now().Sub(%s) with %s", qualifier, argText, sinceText),
 			TextEdits: []analysis.TextEdit{{
 				Pos:     outer.Pos(),
 				End:     outer.End(),
 				NewText: []byte(sinceText),
 			}},
-		}},
-	})
+		}}
+	}
+	pass.Report(diag)
 }
 
 // timeNowQualifier reports the imported identifier used for time.Now().