Make writebytestring SuggestedFix self-contained by adding missing io import edits#44665
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
writebytestring SuggestedFix self-contained by adding missing io import edits
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR updates the writebytestring analyzer’s autofix so it can add an io import when rewriting w.Write([]byte(s)) to io.WriteString(w, s), and adds analysistest fixtures covering files that previously would not compile after applying the fix.
Changes:
- Extend
buildFixto optionally include an import insertionTextEditforio. - Add
addIOImportEditto handle grouped imports, single ungrouped imports, and no-import files. - Add new analysistest fixtures/goldens for missing-
ioimport scenarios.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/writebytestring/writebytestring.go | Adds logic to emit an io import edit alongside the call rewrite fix. |
| pkg/linters/writebytestring/testdata/src/writebytestring/noio_import.go | New fixture: single ungrouped import without io and multiple violations. |
| pkg/linters/writebytestring/testdata/src/writebytestring/noio_import.go.golden | Expected output after suggested fixes (rewritten import block including io). |
| pkg/linters/writebytestring/testdata/src/writebytestring/no_import_block.go | New fixture: no imports in file. |
| pkg/linters/writebytestring/testdata/src/writebytestring/no_import_block.go.golden | Expected output after suggested fixes (inserts import "io"). |
| pkg/linters/writebytestring/testdata/src/writebytestring/grouped_noio_import.go | New fixture: grouped imports missing io. |
| pkg/linters/writebytestring/testdata/src/writebytestring/grouped_noio_import.go.golden | Expected output after suggested fixes (adds io spec to grouped import). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Low
| for _, imp := range file.Imports { | ||
| if imp.Path.Value == `"`+ioPkg+`"` { | ||
| return analysis.TextEdit{}, false | ||
| } | ||
| } |
| for _, decl := range file.Decls { | ||
| genDecl, ok := decl.(*ast.GenDecl) | ||
| if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { | ||
| continue | ||
| } | ||
| // Keep the edit minimal by appending at the end of the grouped import; | ||
| // ordering/formatting can be normalized by formatters if desired. | ||
| return markAndReturn(analysis.TextEdit{ | ||
| Pos: genDecl.Rparen, | ||
| End: genDecl.Rparen, | ||
| NewText: []byte(importSpecIndent + `"` + ioPkg + `"` + "\n"), | ||
| }) | ||
| } |
| specText := astutil.NodeText(pass.Fset, singleUngroupedImportDecl.Specs[0]) | ||
| if specText != "" { | ||
| // Rebuild as a grouped import while preserving the existing import spec | ||
| // text and adding "io". | ||
| return markAndReturn(analysis.TextEdit{ | ||
| Pos: singleUngroupedImportDecl.Pos(), | ||
| End: singleUngroupedImportDecl.End(), | ||
| NewText: []byte("import (\n" + importSpecIndent + specText + "\n" + importSpecIndent + `"` + ioPkg + `"` + "\n)"), | ||
| }) |
There was a problem hiding this comment.
Review
One blocking correctness issue.
filesWithImportEdit breaks fix independence (line 57 of writebytestring.go)
The shared deduplication map means only the first diagnostic in a file gets a self-contained SuggestedFix. Any subsequent fix applied in isolation leaves the file without an io import, producing a compile error. Since analysis.SuggestedFix is meant to be independently applicable, every fix should carry its own import edit; deduplication (if needed at all) should happen at apply-time, not emit-time.
Minor: unsorted import in golden file
grouped_noio_import.go.golden inserts "io" after "os", which violates standard Go import ordering (goimports would place "io" before "os"). While not a compile error, it will cause goimports to reformat the file on next save.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 18.8 AIC · ⌖ 4.42 AIC · ⊞ 4.8K
| @@ -52,6 +57,7 @@ func run(pass *analysis.Pass) (any, error) { | |||
| return nil, err | |||
There was a problem hiding this comment.
The filesWithImportEdit map is shared across all calls to buildFix, so only the first violation per file gets a self-contained fix that includes the io import edit. If a user applies any subsequent violation's fix in isolation (e.g., as a standalone code action), the file will be left without the io import and will not compile.
Each SuggestedFix is supposed to be independently applicable. Consider including the import edit in every fix and letting the tool chain (gopls / go fix) handle deduplication, rather than skipping it in later fixes.
@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (156 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 Matter
ADRs 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.
Multiple correctness issues in addIOImportEdit
This PR fixes a real bug (non-compiling fixes when io is absent), but addIOImportEdit has edge-case correctness issues that can produce broken fixes.
- Multiple ungrouped imports produce broken output (see inline comment on line 278)
- file.Name.End() insertion point can corrupt source (see inline comment on line 294)
- map[token.Pos]bool key is fragile; use map[*ast.File]bool instead
🔎 Code quality review by PR Code Quality Reviewer · 48.7 AIC · ⌖ 4.67 AIC · ⊞ 5.4K
Comment /review to run again
| singleUngroupedImportDecl = genDecl | ||
| } | ||
| } | ||
| if importDeclCount == 1 && singleUngroupedImportDecl != nil { |
There was a problem hiding this comment.
Multiple ungrouped imports silently fall through to broken fallback: when a file has 2+ separate ungrouped import declarations (e.g. import "bytes" and import "os" on separate lines), importDeclCount > 1 so this branch is skipped, and the code falls through to insert a bare import "io" after the package name — leaving the existing ungrouped imports untouched and producing three separate import blocks that will not compile cleanly.
💡 Suggested fix
Handle the multi-ungrouped case explicitly by converting all existing single-spec ungrouped imports into one grouped block:
if importDeclCount >= 1 && singleUngroupedImportDecl != nil {
// collect all ungrouped import specs and rewrite into a single grouped block
}Alternatively, document and test that the linter intentionally only handles the single-ungrouped case and add a comment explaining the fallback behaviour for the multi-ungrouped case so future maintainers don't introduce silent breakage.
| } | ||
|
|
||
| return markAndReturn(analysis.TextEdit{ | ||
| Pos: file.Name.End(), |
There was a problem hiding this comment.
file.Name.End() inserts after the identifier, not the end of the package clause line: ast.Ident.End() returns the position immediately after the last byte of the package name, not after the newline. If the package clause has a trailing comment (e.g. package foo // build tag) or extra whitespace, the inserted \n\nimport "io" is injected into the middle of that comment/line rather than after it.
💡 Suggested fix
Use pass.Fset to find the line-end of the package clause, e.g.:
pkgLine := pass.Fset.Position(file.Package).Line
tokenFile := pass.Fset.File(file.Package)
insertPos := tokenFile.LineStart(pkgLine + 1) // start of the next lineOr, more simply, locate file.Decls[0].Pos() (the first declaration after the package clause) and insert before it.
| return analysis.TextEdit{}, false | ||
| } | ||
|
|
||
| if filesWithImportEdit[file.Pos()] { |
There was a problem hiding this comment.
Use *ast.File as the map key instead of token.Pos: file.Pos() is an implicit unique key — it works in practice but is fragile in test harnesses or synthetic ASTs where two ast.File nodes could share a starting position. Using the pointer itself as the key (map[*ast.File]bool) eliminates the ambiguity with zero extra cost.
💡 Suggested fix
// in run()
filesWithImportEdit := make(map[*ast.File]bool)
// in addIOImportEdit()
func addIOImportEdit(pass *analysis.Pass, pos token.Pos, filesWithImportEdit map[*ast.File]bool) (analysis.TextEdit, bool) {
...
if filesWithImportEdit[file] { return analysis.TextEdit{}, false }
markAndReturn := func(edit analysis.TextEdit) (analysis.TextEdit, bool) {
filesWithImportEdit[file] = true
return edit, true
}
...
}There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness issue.
📋 Key Themes & Highlights
Key Themes
- SuggestedFix independence violated: The shared
filesWithImportEditmap across all fixes means only the first violation's fix includes theioimport edit. Applying any other fix in isolation still producesundefined: io. Eachanalysis.SuggestedFixmust be self-contained and independently applicable. - Import sort order: The grouped-import insertion appends
ioat the end of the block rather than maintaining alphabetical order, which diverges from goimports conventions.
Positive Highlights
- ✅ Solid root-cause fix — moving from "tools handle it" to "fix is self-contained" is the right direction
- ✅ Good test matrix: three fixture pairs covering each import shape
- ✅ Clean separation of
addIOImportEditfrombuildFix - ✅ The fallback chain (grouped → ungrouped → no imports) handles real file shapes well
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38.4 AIC · ⌖ 4.62 AIC · ⊞ 6.6K
Comment /matt to run again
| NewText: []byte(replacement), | ||
| }} | ||
| if importEdit, ok := addIOImportEdit(pass, call.Pos(), filesWithImportEdit); ok { | ||
| edits = append(edits, importEdit) |
There was a problem hiding this comment.
[/diagnosing-bugs] The filesWithImportEdit dedup map is shared across all SuggestedFix values, so only the first violation in a file gets the import edit baked in. Each SuggestedFix must be independently applicable — applying fix #2 alone will still produce undefined: io.
💡 Suggested approach
Remove the shared dedup state and include the import edit in every fix unconditionally. Overlapping edits on the same range are safe (they're identical), and the analysistest framework deduplicates them during apply. Alternatively, gate the import edit per-fix via a fresh lookup each time:
// No shared state — check freshly per fix:
func buildFix(pass *analysis.Pass, call *ast.CallExpr, writerArg, sText string) []analysis.SuggestedFix {
replacement := fmt.Sprintf("io.WriteString(%s, %s)", writerArg, sText)
edits := []analysis.TextEdit{{Pos: call.Pos(), End: call.End(), NewText: []byte(replacement)}}
if importEdit, ok := buildIOImportEdit(pass, call.Pos()); ok {
edits = append(edits, importEdit)
}
return []analysis.SuggestedFix{{Message: "Replace with " + replacement, TextEdits: edits}}
}@copilot please address this.
|
|
||
| import ( | ||
| "bytes" | ||
| "os" |
There was a problem hiding this comment.
[/tdd] The golden file inserts "io" at the end of the import group (after "os"), but goimports/gofmt convention sorts imports alphabetically — "io" should appear before "os". If a formatter runs after applying the fix, the golden won't match exactly.
💡 Suggested golden
import (
"bytes"
"io"
"os"
)Consider sorting when building the import-insertion text, or document that post-fix formatting is expected.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
writebytestringrewrotew.Write([]byte(s))toio.WriteString(w, s)but emitted only the call replacement edit, producing non-compiling fixes (undefined: io) when the file did not already importio. This PR makes the SuggestedFix complete in analyzer-driven fix flows (not editor-assisted import management).SuggestedFix now includes import edits when needed
buildFixnow appends anioimportTextEditwhen the target file lacksio.Import insertion handles real file shapes
iointo the existing block.io.import "io"after package declaration.Regression coverage for missing-import scenarios
writebytestringanalysistest fixtures/goldens for:io(with multiple violations in one file),io,