diff --git a/docs/adr/47912-consolidate-import-editing-helpers-into-astutil.md b/docs/adr/47912-consolidate-import-editing-helpers-into-astutil.md new file mode 100644 index 00000000000..988b71eef3c --- /dev/null +++ b/docs/adr/47912-consolidate-import-editing-helpers-into-astutil.md @@ -0,0 +1,45 @@ +# ADR-47912: Consolidate Import-Editing and File-Lookup Helpers into internal/astutil + +**Date**: 2026-07-25 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +Five import-manipulation helpers (`fileForPos`, `countPkgUsesInFile`, `addImportEdit`, `removeImportEdit`, `importSpecLineRange`) were copy-pasted nearly verbatim between `sprintfbool` and `sprintfint`, and the file-containing-position lookup pattern was re-implemented inline in at least five packages (`sprintfbool`, `sprintfint`, `writebytestring`, `bytescomparestring`, `uncheckedtypeassertion`). A latent drift was already visible: `addStrconvRemoveFmtEdits` in the two packages produced TextEdit slices in different order, and `sprintfint` used raw string comparison while `sprintfbool` used a helper — a behavioral gap that would silently grow over time. The shared helper layer `pkg/linters/internal/astutil` already owned `Inspector`, `ImportedAs`, and related AST utilities and was the natural consolidation point. + +### Decision + +We will promote the duplicated cluster — `FileForPos`, `CountPkgUsesInFile`, `ImportSpecLineRange`, `AddImportEdit`, `RemoveImportEdit`, and a generalized `SwapImportEdits` — into `internal/astutil` as exported, unit-tested functions, and update all call sites in `sprintfbool`, `sprintfint`, `writebytestring`, `bytescomparestring`, and `uncheckedtypeassertion` to use the shared versions. The generalized swap helper replaces the two bespoke `addStrconvRemoveFmtEdits` functions with a parameter-driven implementation, eliminating the ordering drift. + +### Alternatives Considered + +#### Alternative 1: Keep helpers local to each linter package (status quo) + +Each linter independently owns its import-editing logic. No new shared surface area is introduced. Rejected because the near-verbatim duplication across five packages had already produced subtle behavioral drift (`addStrconvRemoveFmtEdits` edit ordering) and would continue to diverge silently as linters evolve independently. + +#### Alternative 2: Extract into a dedicated internal/importutil package + +Create a new package `pkg/linters/internal/importutil` rather than growing `internal/astutil`. Rejected because the helpers operate on the same `*ast.File`, `token.FileSet`, and `analysis.Pass` types that `astutil` already handles, and adding a second internal package would fragment the shared layer without a clear boundary benefit at this scale. + +### Consequences + +#### Positive +- Removes approximately 360 lines of duplicated code across five packages, shrinking per-linter maintenance surface. +- Eliminates the latent `addStrconvRemoveFmtEdits` ordering drift by replacing both copies with a single generic `SwapImportEdits` implementation. +- Adds unit tests for `FileForPos`, `CountPkgUsesInFile`, and `ImportSpecLineRange` in `astutil_test.go`, covering the shared logic for all consumers. +- Import-presence checks in `sprintfbool` switch to `astutil.ImportedAs`, which handles backtick-quoted paths correctly — a correctness improvement over the local `importSpecPathEquals`. + +#### Negative +- `internal/astutil` grows in scope: it now owns import-editing concerns in addition to AST traversal and type predicates, making the package boundary less crisp. +- `bytescomparestring`'s `buildBytesImportTextEdit` was intentionally **not** migrated to `AddImportEdit` because it inserts the new import first (rather than appending), which differs from the shared helper's trailing-append behavior. This leaves a documented behavioural inconsistency between `bytescomparestring` and the other linters. + +#### Neutral +- All existing linter golden-file tests continue to pass unchanged; the refactor is behaviour-preserving at the external test boundary. +- Future linters that need import-editing utilities now have a discoverable, tested API rather than needing to copy-paste from an existing linter. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/bytescomparestring/bytescomparestring.go b/pkg/linters/bytescomparestring/bytescomparestring.go index a08b3b99eb6..8ded36acd00 100644 --- a/pkg/linters/bytescomparestring/bytescomparestring.go +++ b/pkg/linters/bytescomparestring/bytescomparestring.go @@ -115,13 +115,7 @@ func analyzeBinaryExpr(pass *analysis.Pass, n ast.Node, generatedFiles filecheck // determined: dot-import, blank-import, or the qualifier name is shadowed by a // local variable or parameter at pos. func bytesQualifier(pass *analysis.Pass, pos token.Pos) (qualifier string, skipFix bool) { - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= pos && pos <= f.End() { - file = f - break - } - } + file := astutil.FileForPos(pass.Files, pos) qualifier = bytesPkg if file != nil { @@ -166,13 +160,7 @@ func buildFix(pass *analysis.Pass, bin *ast.BinaryExpr, replacement string, seen // (tracked via seenImportFiles to prevent duplicate overlapping edits). // Returns (TextEdit{}, false) when no edit is needed. func addBytesImportEdit(pass *analysis.Pass, pos token.Pos, seenImportFiles map[token.Pos]bool) (analysis.TextEdit, bool) { - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= pos && pos <= f.End() { - file = f - break - } - } + file := astutil.FileForPos(pass.Files, pos) if file == nil { return analysis.TextEdit{}, false } diff --git a/pkg/linters/internal/astutil/astutil.go b/pkg/linters/internal/astutil/astutil.go index 44abedde63e..c8b59f9128c 100644 --- a/pkg/linters/internal/astutil/astutil.go +++ b/pkg/linters/internal/astutil/astutil.go @@ -439,6 +439,209 @@ func CallQualifierText(fset *token.FileSet, call *ast.CallExpr) string { return NodeText(fset, sel.X) } +// FileForPos returns the *ast.File from files that contains pos, or nil if +// not found. It is used by linters that need the enclosing file of a node +// when only the pass.Files slice is available. +func FileForPos(files []*ast.File, pos token.Pos) *ast.File { + for _, f := range files { + if f.Pos() <= pos && pos <= f.End() { + return f + } + } + return nil +} + +// CountPkgUsesInFile returns the number of times the package at pkgPath is +// referenced as a selector base within file (e.g. each "fmt.X" call counts +// as one use of the "fmt" package). +func CountPkgUsesInFile(pass *analysis.Pass, file *ast.File, pkgPath string) int { + fileStart, fileEnd := file.Pos(), file.End() + count := 0 + for ident, obj := range pass.TypesInfo.Uses { + pkgName, ok := obj.(*types.PkgName) + if !ok || pkgName.Imported() == nil || pkgName.Imported().Path() != pkgPath { + continue + } + if p := ident.Pos(); p >= fileStart && p <= fileEnd { + count++ + } + } + return count +} + +// importSpecPathEquals reports whether spec imports the package at pkgPath. +// It handles both double-quoted and backtick-quoted import path literals. +func importSpecPathEquals(spec *ast.ImportSpec, pkgPath string) bool { + if spec == nil || spec.Path == nil { + return false + } + unquoted, err := strconv.Unquote(spec.Path.Value) + if err != nil { + return spec.Path.Value == `"`+pkgPath+`"` || spec.Path.Value == "`"+pkgPath+"`" + } + return unquoted == pkgPath +} + +// ImportSpecLineRange returns the [start, end) byte range that covers the +// entire source line of spec — including any leading whitespace and the +// trailing newline. It uses the token.File's line table so it works correctly +// regardless of indentation style. +func ImportSpecLineRange(fset *token.FileSet, spec *ast.ImportSpec) (token.Pos, token.Pos) { + tokFile := fset.File(spec.Pos()) + if tokFile == nil { + // Unreachable in practice; fall back to simple single-char arithmetic. + return spec.Pos() - 1, spec.End() + 1 + } + line := tokFile.Line(spec.Pos()) + lineStart := tokFile.LineStart(line) + if line < tokFile.LineCount() { + return lineStart, tokFile.LineStart(line + 1) + } + // Last line has no following newline — extend past the spec token end. + return lineStart, spec.End() + 1 +} + +// AddImportEdit returns a TextEdit that inserts an import for pkg into file, +// choosing the least-invasive insertion point: append to an existing grouped +// block, convert a single non-grouped import to a grouped block, or insert a +// standalone declaration after the package name. +func AddImportEdit(pass *analysis.Pass, file *ast.File, pkg string) (analysis.TextEdit, bool) { + quotedPkg := strconv.Quote(pkg) + + // Append to an existing grouped import block. + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { + continue + } + return analysis.TextEdit{ + Pos: genDecl.Rparen, + End: genDecl.Rparen, + NewText: []byte("\t" + quotedPkg + "\n"), + }, true + } + + // Convert a single non-grouped import into a grouped block. + if len(file.Imports) == 1 { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() { + continue + } + specText := NodeText(pass.Fset, genDecl.Specs[0]) + if specText == "" { + continue + } + return analysis.TextEdit{ + Pos: genDecl.Pos(), + End: genDecl.End(), + NewText: []byte("import (\n\t" + specText + "\n\t" + quotedPkg + "\n)"), + }, true + } + } + + // No existing import block; insert a standalone import after the package name. + return analysis.TextEdit{ + Pos: file.Name.End(), + End: file.Name.End(), + NewText: []byte("\n\nimport " + quotedPkg), + }, true +} + +// RemoveImportEdit returns a TextEdit that removes the import of pkg from +// file's import section. For an ungrouped or sole-spec grouped declaration the +// entire decl is removed; for a multi-spec grouped block only the spec line is +// deleted using line-boundary positions from fset to handle any indentation. +func RemoveImportEdit(fset *token.FileSet, file *ast.File, pkg string) (analysis.TextEdit, bool) { + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT { + continue + } + for _, spec := range genDecl.Specs { + imp, ok := spec.(*ast.ImportSpec) + if !ok || !importSpecPathEquals(imp, pkg) { + continue + } + // Ungrouped or single-spec grouped: remove the entire declaration. + if !genDecl.Lparen.IsValid() || len(genDecl.Specs) == 1 { + return analysis.TextEdit{ + Pos: genDecl.Pos(), + End: genDecl.End(), + NewText: nil, + }, true + } + // Multi-spec grouped: remove just this spec's line. + lineStart, lineEnd := ImportSpecLineRange(fset, imp) + return analysis.TextEdit{ + Pos: lineStart, + End: lineEnd, + NewText: nil, + }, true + } + } + return analysis.TextEdit{}, false +} + +// SwapImportEdits returns the TextEdits that simultaneously add addPkg and +// remove removePkg from file's import section. Three structural cases are +// handled: +// - single ungrouped import removePkg → replaced with import addPkg +// - grouped block with only removePkg → replaced with import addPkg +// - grouped block with removePkg + others → insert addPkg, delete removePkg line +func SwapImportEdits(fset *token.FileSet, file *ast.File, addPkg, removePkg string) []analysis.TextEdit { + var removeSpec *ast.ImportSpec + var removeDecl *ast.GenDecl + + for _, decl := range file.Decls { + genDecl, ok := decl.(*ast.GenDecl) + if !ok || genDecl.Tok != token.IMPORT { + continue + } + for _, spec := range genDecl.Specs { + imp, ok := spec.(*ast.ImportSpec) + if ok && importSpecPathEquals(imp, removePkg) { + removeSpec = imp + removeDecl = genDecl + break + } + } + if removeDecl != nil { + break + } + } + if removeDecl == nil { + return nil + } + + // Single ungrouped import or grouped block with only removePkg: + // replace the entire declaration with import addPkg. + if !removeDecl.Lparen.IsValid() || len(removeDecl.Specs) == 1 { + return []analysis.TextEdit{{ + Pos: removeDecl.Pos(), + End: removeDecl.End(), + NewText: []byte("import " + strconv.Quote(addPkg)), + }} + } + + // Grouped block with removePkg alongside other packages: delete the + // removePkg spec line (lower position) then insert addPkg before the + // closing paren (higher position), so edits are ordered by position. + lineStart, lineEnd := ImportSpecLineRange(fset, removeSpec) + return []analysis.TextEdit{ + { + Pos: lineStart, + End: lineEnd, + NewText: nil, + }, + { + Pos: removeDecl.Rparen, + End: removeDecl.Rparen, + NewText: []byte("\t" + strconv.Quote(addPkg) + "\n"), + }, + } +} + // 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"). diff --git a/pkg/linters/internal/astutil/astutil_test.go b/pkg/linters/internal/astutil/astutil_test.go index 3a7cad06262..00f3f1190cc 100644 --- a/pkg/linters/internal/astutil/astutil_test.go +++ b/pkg/linters/internal/astutil/astutil_test.go @@ -9,6 +9,7 @@ import ( "go/parser" "go/token" "go/types" + "sort" "testing" "golang.org/x/tools/go/analysis" @@ -36,6 +37,59 @@ func typecheckSnippet(t *testing.T, src string) (*analysis.Pass, *ast.File) { return &analysis.Pass{TypesInfo: info}, file } +// parseSnippet parses src and returns the fset, ast.File, and an analysis.Pass +// suitable for passing to AddImportEdit. It does not type-check. +func parseSnippet(t *testing.T, src string) (*token.FileSet, *ast.File, *analysis.Pass) { + t.Helper() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "snippet.go", src, 0) + if err != nil { + t.Fatalf("ParseFile() error = %v", err) + } + pass := &analysis.Pass{Fset: fset} + return fset, file, pass +} + +// applyTextEdits applies edits to src and returns the resulting string. +// Edits must be non-overlapping. They are applied in reverse position order so +// that earlier offsets remain valid after each replacement. +func applyTextEdits(t *testing.T, fset *token.FileSet, src []byte, edits []analysis.TextEdit) string { + t.Helper() + if len(edits) == 0 { + return string(src) + } + // Sort edits by Pos descending so we apply from end to start. + sorted := make([]analysis.TextEdit, len(edits)) + copy(sorted, edits) + sort.Slice(sorted, func(i, j int) bool { + return sorted[i].Pos > sorted[j].Pos + }) + + result := make([]byte, len(src)) + copy(result, src) + + for _, e := range sorted { + tf := fset.File(e.Pos) + if tf == nil { + t.Fatalf("applyTextEdits: no token.File for pos %d", e.Pos) + } + start := tf.Offset(e.Pos) + // Handle the case where End == Pos (pure insertion). + var end int + if e.End == e.Pos { + end = start + } else { + end = tf.Offset(e.End) + } + var replacement []byte + if e.NewText != nil { + replacement = e.NewText + } + result = append(result[:start], append(replacement, result[end:]...)...) + } + return string(result) +} + func TestRhsExprForIndex(t *testing.T) { t.Parallel() @@ -539,3 +593,398 @@ func f(s string, b []byte) { t.Fatal("IsStringType(b) = true, want false") } } + +func TestFileForPos(t *testing.T) { + t.Parallel() + + fset := token.NewFileSet() + src := `package p + +func f() {} +` + file, err := parser.ParseFile(fset, "snippet.go", src, 0) + if err != nil { + t.Fatalf("ParseFile() error = %v", err) + } + + // pos inside the file should return the file + got := FileForPos([]*ast.File{file}, file.Pos()) + if got != file { + t.Fatalf("FileForPos(file.Pos()) = %v, want the file", got) + } + + // pos at file end should return the file + got = FileForPos([]*ast.File{file}, file.End()) + if got != file { + t.Fatalf("FileForPos(file.End()) = %v, want the file", got) + } + + // pos before the file should return nil + got = FileForPos([]*ast.File{file}, file.Pos()-1) + if got != nil { + t.Fatalf("FileForPos(before) = %v, want nil", got) + } + + // empty file list should return nil + got = FileForPos(nil, file.Pos()) + if got != nil { + t.Fatalf("FileForPos(nil files) = %v, want nil", got) + } +} + +func TestCountPkgUsesInFile(t *testing.T) { + t.Parallel() + + const src = `package p + +import "fmt" + +func f() { + fmt.Println("hello") + fmt.Println("world") +} +` + pass, file := typecheckSnippet(t, src) + pass.Files = []*ast.File{file} + + count := CountPkgUsesInFile(pass, file, "fmt") + if count != 2 { + t.Fatalf("CountPkgUsesInFile(fmt) = %d, want 2", count) + } + + count = CountPkgUsesInFile(pass, file, "strconv") + if count != 0 { + t.Fatalf("CountPkgUsesInFile(strconv) = %d, want 0", count) + } +} + +func TestImportSpecLineRange(t *testing.T) { + t.Parallel() + + fset := token.NewFileSet() + src := `package p + +import ( + "fmt" + "os" +) +` + file, err := parser.ParseFile(fset, "snippet.go", src, 0) + if err != nil { + t.Fatalf("ParseFile() error = %v", err) + } + + if len(file.Imports) != 2 { + t.Fatalf("expected 2 imports, got %d", len(file.Imports)) + } + spec := file.Imports[0] // "fmt" + start, end := ImportSpecLineRange(fset, spec) + if start >= end { + t.Fatalf("ImportSpecLineRange start=%d >= end=%d, want start < end", start, end) + } + // The line range should include the spec itself. + if spec.Pos() < start || spec.End() > end { + t.Fatalf("ImportSpecLineRange does not contain spec: range [%d, %d), spec [%d, %d)", start, end, spec.Pos(), spec.End()) + } +} + +func TestAddImportEdit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + pkg string + want string + wantOK bool + }{ + { + name: "append to grouped import block", + src: `package p + +import ( + "fmt" +) + +func f() { fmt.Println() } +`, + pkg: "os", + wantOK: true, + want: `package p + +import ( + "fmt" + "os" +) + +func f() { fmt.Println() } +`, + }, + { + name: "convert single ungrouped import to grouped block", + src: `package p + +import "fmt" + +func f() { fmt.Println() } +`, + pkg: "os", + wantOK: true, + want: `package p + +import ( + "fmt" + "os" +) + +func f() { fmt.Println() } +`, + }, + { + name: "no existing imports — insert standalone after package name", + src: `package p + +func f() {} +`, + pkg: "os", + wantOK: true, + want: `package p + +import "os" + +func f() {} +`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fset, file, pass := parseSnippet(t, tt.src) + edit, ok := AddImportEdit(pass, file, tt.pkg) + if ok != tt.wantOK { + t.Fatalf("AddImportEdit() ok = %v, want %v", ok, tt.wantOK) + } + if !tt.wantOK { + return + } + got := applyTextEdits(t, fset, []byte(tt.src), []analysis.TextEdit{edit}) + if got != tt.want { + t.Fatalf("AddImportEdit() result:\ngot:\n%s\nwant:\n%s", got, tt.want) + } + }) + } +} + +func TestRemoveImportEdit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + pkg string + want string + wantOK bool + }{ + { + name: "remove spec from grouped multi-import block", + src: `package p + +import ( + "fmt" + "os" +) + +func f() {} +`, + pkg: "fmt", + wantOK: true, + want: `package p + +import ( + "os" +) + +func f() {} +`, + }, + { + name: "remove sole spec from grouped block removes entire decl", + src: `package p + +import ( + "fmt" +) + +func f() {} +`, + pkg: "fmt", + wantOK: true, + want: `package p + + + +func f() {} +`, + }, + { + name: "remove ungrouped single import removes entire decl", + src: `package p + +import "fmt" + +func f() {} +`, + pkg: "fmt", + wantOK: true, + want: `package p + + + +func f() {} +`, + }, + { + name: "import not present returns false", + src: `package p + +import "fmt" + +func f() {} +`, + pkg: "os", + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fset, file, _ := parseSnippet(t, tt.src) + edit, ok := RemoveImportEdit(fset, file, tt.pkg) + if ok != tt.wantOK { + t.Fatalf("RemoveImportEdit() ok = %v, want %v", ok, tt.wantOK) + } + if !tt.wantOK { + return + } + got := applyTextEdits(t, fset, []byte(tt.src), []analysis.TextEdit{edit}) + if got != tt.want { + t.Fatalf("RemoveImportEdit() result:\ngot:\n%s\nwant:\n%s", got, tt.want) + } + }) + } +} + +func TestSwapImportEdits(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + addPkg string + removePkg string + want string + wantNil bool + }{ + { + name: "ungrouped single import replaced", + src: `package p + +import "fmt" + +func f() {} +`, + addPkg: "os", + removePkg: "fmt", + want: `package p + +import "os" + +func f() {} +`, + }, + { + name: "grouped block with only removePkg replaced", + src: `package p + +import ( + "fmt" +) + +func f() {} +`, + addPkg: "os", + removePkg: "fmt", + want: `package p + +import "os" + +func f() {} +`, + }, + { + name: "grouped block with multiple imports — delete then insert, edits sorted by position", + src: `package p + +import ( + "fmt" + "os" +) + +func f() {} +`, + addPkg: "strconv", + removePkg: "fmt", + want: `package p + +import ( + "os" + "strconv" +) + +func f() {} +`, + }, + { + name: "removePkg not found returns nil", + src: `package p + +import "fmt" + +func f() {} +`, + addPkg: "os", + removePkg: "log", + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + fset, file, _ := parseSnippet(t, tt.src) + edits := SwapImportEdits(fset, file, tt.addPkg, tt.removePkg) + if tt.wantNil { + if edits != nil { + t.Fatalf("SwapImportEdits() = %v, want nil", edits) + } + return + } + if edits == nil { + t.Fatal("SwapImportEdits() = nil, want edits") + } + // Verify edits are sorted by position (required by analysis framework). + for i := 1; i < len(edits); i++ { + if edits[i].Pos < edits[i-1].Pos { + t.Fatalf("SwapImportEdits() edits not sorted by position: edits[%d].Pos=%d < edits[%d].Pos=%d", + i, edits[i].Pos, i-1, edits[i-1].Pos) + } + } + got := applyTextEdits(t, fset, []byte(tt.src), edits) + if got != tt.want { + t.Fatalf("SwapImportEdits() result:\ngot:\n%s\nwant:\n%s", got, tt.want) + } + }) + } +} diff --git a/pkg/linters/sprintfbool/sprintfbool.go b/pkg/linters/sprintfbool/sprintfbool.go index 8cba92eb422..1f899029cf1 100644 --- a/pkg/linters/sprintfbool/sprintfbool.go +++ b/pkg/linters/sprintfbool/sprintfbool.go @@ -7,7 +7,6 @@ import ( "go/ast" "go/token" "go/types" - stdstrconv "strconv" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" @@ -112,7 +111,7 @@ func collectSprintfBoolCandidates(pass *analysis.Pass, insp *inspector.Inspector if argType == nil || argType != types.Typ[types.Bool] { return } - file := fileForPos(pass.Files, call.Pos()) + file := astutil.FileForPos(pass.Files, call.Pos()) if file != nil { targetCallsByFile[file.Pos()]++ filesByPos[file.Pos()] = file @@ -131,15 +130,9 @@ func computeOrphanFmtStatus(pass *analysis.Pass, filesByPos map[token.Pos]*ast.F if file == nil { continue } - fmtImported := false - for _, imp := range file.Imports { - if importSpecPathEquals(imp, fmtPkg) { - fmtImported = true - break - } - } + _, fmtImported := astutil.ImportedAs(file, pass.TypesInfo, fmtPkg) orphanFmtByFile[filePos] = fmtImported && - countPkgUsesInFile(pass, file, fmtPkg) == targetCalls && + astutil.CountPkgUsesInFile(pass, file, fmtPkg) == targetCalls && fixableCallsByFile[filePos] == targetCalls } return orphanFmtByFile @@ -204,16 +197,8 @@ func buildImportEdits( return nil } - strconvImported := false - fmtImported := false - for _, imp := range file.Imports { - switch { - case importSpecPathEquals(imp, strconvPkg): - strconvImported = true - case importSpecPathEquals(imp, fmtPkg): - fmtImported = true - } - } + _, strconvImported := astutil.ImportedAs(file, pass.TypesInfo, strconvPkg) + _, fmtImported := astutil.ImportedAs(file, pass.TypesInfo, fmtPkg) orphanFmt := fmtImported && orphanFmtByFile[file.Pos()] needStrconv := !strconvImported @@ -226,171 +211,19 @@ func buildImportEdits( switch { case needStrconv && needRemoveFmt: - return addStrconvRemoveFmtEdits(pass.Fset, file) + return astutil.SwapImportEdits(pass.Fset, file, strconvPkg, fmtPkg) case needStrconv: - if edit, ok := addImportEdit(pass, file, strconvPkg); ok { + if edit, ok := astutil.AddImportEdit(pass, file, strconvPkg); ok { return []analysis.TextEdit{edit} } case needRemoveFmt: - if edit, ok := removeImportEdit(pass.Fset, file, fmtPkg); ok { + if edit, ok := astutil.RemoveImportEdit(pass.Fset, file, fmtPkg); ok { return []analysis.TextEdit{edit} } } return nil } -func countPkgUsesInFile(pass *analysis.Pass, file *ast.File, pkgPath string) int { - fileStart, fileEnd := file.Pos(), file.End() - count := 0 - for ident, obj := range pass.TypesInfo.Uses { - pkgName, ok := obj.(*types.PkgName) - if !ok || pkgName.Imported() == nil || pkgName.Imported().Path() != pkgPath { - continue - } - if p := ident.Pos(); p >= fileStart && p <= fileEnd { - count++ - } - } - return count -} - -func addStrconvRemoveFmtEdits(fset *token.FileSet, file *ast.File) []analysis.TextEdit { - var fmtSpec *ast.ImportSpec - var fmtDecl *ast.GenDecl - - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT { - continue - } - for _, spec := range genDecl.Specs { - imp, ok := spec.(*ast.ImportSpec) - if ok && importSpecPathEquals(imp, fmtPkg) { - fmtSpec = imp - fmtDecl = genDecl - break - } - } - if fmtDecl != nil { - break - } - } - if fmtDecl == nil { - return nil - } - - if !fmtDecl.Lparen.IsValid() || len(fmtDecl.Specs) == 1 { - return []analysis.TextEdit{{ - Pos: fmtDecl.Pos(), - End: fmtDecl.End(), - NewText: []byte(`import "` + strconvPkg + `"`), - }} - } - - lineStart, lineEnd := importSpecLineRange(fset, fmtSpec) - return []analysis.TextEdit{ - { - Pos: lineStart, - End: lineEnd, - NewText: nil, - }, - { - Pos: fmtDecl.Rparen, - End: fmtDecl.Rparen, - NewText: []byte("\t\"" + strconvPkg + "\"\n"), - }, - } -} - -func addImportEdit(pass *analysis.Pass, file *ast.File, pkg string) (analysis.TextEdit, bool) { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { - continue - } - return analysis.TextEdit{ - Pos: genDecl.Rparen, - End: genDecl.Rparen, - NewText: []byte("\t\"" + pkg + "\"\n"), - }, true - } - - if len(file.Imports) == 1 { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() { - continue - } - specText := astutil.NodeText(pass.Fset, genDecl.Specs[0]) - if specText == "" { - continue - } - return analysis.TextEdit{ - Pos: genDecl.Pos(), - End: genDecl.End(), - NewText: []byte("import (\n\t" + specText + "\n\t\"" + pkg + "\"\n)"), - }, true - } - } - - return analysis.TextEdit{ - Pos: file.Name.End(), - End: file.Name.End(), - NewText: []byte("\n\nimport \"" + pkg + "\""), - }, true -} - -func removeImportEdit(fset *token.FileSet, file *ast.File, pkg string) (analysis.TextEdit, bool) { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT { - continue - } - for _, spec := range genDecl.Specs { - imp, ok := spec.(*ast.ImportSpec) - if !ok || !importSpecPathEquals(imp, pkg) { - continue - } - if !genDecl.Lparen.IsValid() || len(genDecl.Specs) == 1 { - return analysis.TextEdit{ - Pos: genDecl.Pos(), - End: genDecl.End(), - NewText: nil, - }, true - } - lineStart, lineEnd := importSpecLineRange(fset, imp) - return analysis.TextEdit{ - Pos: lineStart, - End: lineEnd, - NewText: nil, - }, true - } - } - return analysis.TextEdit{}, false -} - -func importSpecLineRange(fset *token.FileSet, spec *ast.ImportSpec) (token.Pos, token.Pos) { - tokFile := fset.File(spec.Pos()) - if tokFile == nil { - return spec.Pos() - 1, spec.End() + 1 - } - line := tokFile.Line(spec.Pos()) - lineStart := tokFile.LineStart(line) - if line < tokFile.LineCount() { - return lineStart, tokFile.LineStart(line + 1) - } - return lineStart, spec.End() + 1 -} - -func fileForPos(files []*ast.File, pos token.Pos) *ast.File { - for _, file := range files { - if file.Pos() <= pos && pos <= file.End() { - return file - } - } - return nil -} - func replacementForCall(pass *analysis.Pass, call *ast.CallExpr, arg ast.Expr, file *ast.File) replacement { argText := astutil.NodeText(pass.Fset, arg) if argText == "" { @@ -420,14 +253,3 @@ func replacementForCall(pass *analysis.Pass, call *ast.CallExpr, arg ast.Expr, f canFix: true, } } - -func importSpecPathEquals(spec *ast.ImportSpec, pkgPath string) bool { - if spec == nil || spec.Path == nil { - return false - } - unquoted, err := stdstrconv.Unquote(spec.Path.Value) - if err != nil { - return spec.Path.Value == `"`+pkgPath+`"` || spec.Path.Value == "`"+pkgPath+"`" - } - return unquoted == pkgPath -} diff --git a/pkg/linters/sprintfint/sprintfint.go b/pkg/linters/sprintfint/sprintfint.go index c587291b072..66020a5b10d 100644 --- a/pkg/linters/sprintfint/sprintfint.go +++ b/pkg/linters/sprintfint/sprintfint.go @@ -111,13 +111,7 @@ func buildItoaFix(pass *analysis.Pass, call *ast.CallExpr, arg ast.Expr, seenImp } // Find the file that contains this call so we can inspect its imports. - var file *ast.File - for _, f := range pass.Files { - if f.Pos() <= call.Pos() && call.Pos() <= f.End() { - file = f - break - } - } + file := astutil.FileForPos(pass.Files, call.Pos()) // Determine the local qualifier for "strconv": use the alias when the // package is already imported under a different name, or the default name @@ -164,20 +158,12 @@ func buildImportEdits(pass *analysis.Pass, file *ast.File, seenImportFiles map[t return nil } - strconvImported := false - fmtImported := false - for _, imp := range file.Imports { - switch imp.Path.Value { - case `"` + strconvPkg + `"`: - strconvImported = true - case `"` + fmtPkg + `"`: - fmtImported = true - } - } + _, strconvImported := astutil.ImportedAs(file, pass.TypesInfo, strconvPkg) + _, fmtImported := astutil.ImportedAs(file, pass.TypesInfo, fmtPkg) // If the flagged call is the only "fmt" reference in the file the "fmt" // import will become unused after the fix and must be removed. - orphanFmt := fmtImported && countPkgUsesInFile(pass, file, fmtPkg) == 1 + orphanFmt := fmtImported && astutil.CountPkgUsesInFile(pass, file, fmtPkg) == 1 needStrconv := !strconvImported needRemoveFmt := orphanFmt @@ -189,190 +175,15 @@ func buildImportEdits(pass *analysis.Pass, file *ast.File, seenImportFiles map[t switch { case needStrconv && needRemoveFmt: - return addStrconvRemoveFmtEdits(pass.Fset, file) + return astutil.SwapImportEdits(pass.Fset, file, strconvPkg, fmtPkg) case needStrconv: - if edit, ok := addImportEdit(pass, file, strconvPkg); ok { + if edit, ok := astutil.AddImportEdit(pass, file, strconvPkg); ok { return []analysis.TextEdit{edit} } case needRemoveFmt: - if edit, ok := removeImportEdit(pass.Fset, file, fmtPkg); ok { + if edit, ok := astutil.RemoveImportEdit(pass.Fset, file, fmtPkg); ok { return []analysis.TextEdit{edit} } } return nil } - -// countPkgUsesInFile returns the number of times the package at pkgPath is -// referenced as a selector base within file (e.g. each "fmt.X" call counts -// as one use of the "fmt" package). -func countPkgUsesInFile(pass *analysis.Pass, file *ast.File, pkgPath string) int { - fileStart, fileEnd := file.Pos(), file.End() - count := 0 - for ident, obj := range pass.TypesInfo.Uses { - pkgName, ok := obj.(*types.PkgName) - if !ok || pkgName.Imported() == nil || pkgName.Imported().Path() != pkgPath { - continue - } - if p := ident.Pos(); p >= fileStart && p <= fileEnd { - count++ - } - } - return count -} - -// addStrconvRemoveFmtEdits returns the TextEdits that simultaneously add -// "strconv" and remove "fmt" from file's import section. Three structural -// cases are handled: -// - single ungrouped import "fmt" → replaced with import "strconv" -// - grouped import block with only "fmt" → replaced with import "strconv" -// - grouped block with "fmt" + others → insert "strconv", delete "fmt" line -func addStrconvRemoveFmtEdits(fset *token.FileSet, file *ast.File) []analysis.TextEdit { - var fmtSpec *ast.ImportSpec - var fmtDecl *ast.GenDecl - - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT { - continue - } - for _, spec := range genDecl.Specs { - imp, ok := spec.(*ast.ImportSpec) - if ok && imp.Path.Value == `"`+fmtPkg+`"` { - fmtSpec = imp - fmtDecl = genDecl - break - } - } - if fmtDecl != nil { - break - } - } - if fmtDecl == nil { - return nil - } - - // Single ungrouped import or grouped block with only "fmt": - // replace the entire declaration with import "strconv". - if !fmtDecl.Lparen.IsValid() || len(fmtDecl.Specs) == 1 { - return []analysis.TextEdit{{ - Pos: fmtDecl.Pos(), - End: fmtDecl.End(), - NewText: []byte(`import "` + strconvPkg + `"`), - }} - } - - // Grouped block with "fmt" alongside other packages: insert "strconv" - // before the closing paren and delete the entire "fmt" spec line. - lineStart, lineEnd := importSpecLineRange(fset, fmtSpec) - return []analysis.TextEdit{ - { - Pos: fmtDecl.Rparen, - End: fmtDecl.Rparen, - NewText: []byte("\t\"" + strconvPkg + "\"\n"), - }, - { - Pos: lineStart, - End: lineEnd, - NewText: nil, - }, - } -} - -// addImportEdit returns a TextEdit that inserts an import for pkg into file, -// choosing the least-invasive insertion point: append to an existing grouped -// block, convert a single non-grouped import to a grouped block, or insert a -// standalone declaration after the package name. -func addImportEdit(pass *analysis.Pass, file *ast.File, pkg string) (analysis.TextEdit, bool) { - // Append to an existing grouped import block. - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT || !genDecl.Lparen.IsValid() { - continue - } - return analysis.TextEdit{ - Pos: genDecl.Rparen, - End: genDecl.Rparen, - NewText: []byte("\t\"" + pkg + "\"\n"), - }, true - } - - // Convert a single non-grouped import into a grouped block. - if len(file.Imports) == 1 { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT || genDecl.Lparen.IsValid() { - continue - } - specText := astutil.NodeText(pass.Fset, genDecl.Specs[0]) - if specText == "" { - continue - } - return analysis.TextEdit{ - Pos: genDecl.Pos(), - End: genDecl.End(), - NewText: []byte("import (\n\t" + specText + "\n\t\"" + pkg + "\"\n)"), - }, true - } - } - - // No existing import block; insert a standalone import after the package name. - return analysis.TextEdit{ - Pos: file.Name.End(), - End: file.Name.End(), - NewText: []byte("\n\nimport \"" + pkg + "\""), - }, true -} - -// removeImportEdit returns a TextEdit that removes the import of pkg from -// file's import section. For an ungrouped or sole-spec grouped declaration the -// entire decl is removed; for a multi-spec grouped block only the spec line is -// deleted using line-boundary positions from fset to handle any indentation. -func removeImportEdit(fset *token.FileSet, file *ast.File, pkg string) (analysis.TextEdit, bool) { - for _, decl := range file.Decls { - genDecl, ok := decl.(*ast.GenDecl) - if !ok || genDecl.Tok != token.IMPORT { - continue - } - for _, spec := range genDecl.Specs { - imp, ok := spec.(*ast.ImportSpec) - if !ok || imp.Path.Value != `"`+pkg+`"` { - continue - } - // Ungrouped or single-spec grouped: remove the entire declaration. - if !genDecl.Lparen.IsValid() || len(genDecl.Specs) == 1 { - return analysis.TextEdit{ - Pos: genDecl.Pos(), - End: genDecl.End(), - NewText: nil, - }, true - } - // Multi-spec grouped: remove just this spec's line. - lineStart, lineEnd := importSpecLineRange(fset, imp) - return analysis.TextEdit{ - Pos: lineStart, - End: lineEnd, - NewText: nil, - }, true - } - } - return analysis.TextEdit{}, false -} - -// importSpecLineRange returns the [start, end) byte range that covers the -// entire source line of spec — including any leading whitespace and the -// trailing newline. It uses the token.File's line table so it works correctly -// regardless of indentation style. -func importSpecLineRange(fset *token.FileSet, spec *ast.ImportSpec) (token.Pos, token.Pos) { - tokFile := fset.File(spec.Pos()) - if tokFile == nil { - // Unreachable in practice; fall back to simple single-char arithmetic. - return spec.Pos() - 1, spec.End() + 1 - } - line := tokFile.Line(spec.Pos()) - lineStart := tokFile.LineStart(line) - if line < tokFile.LineCount() { - return lineStart, tokFile.LineStart(line + 1) - } - // Last line has no following newline — extend past the spec token end. - return lineStart, spec.End() + 1 -} diff --git a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go index 63ef8ee1b94..2302e63b3e1 100644 --- a/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go +++ b/pkg/linters/uncheckedtypeassertion/uncheckedtypeassertion.go @@ -71,12 +71,10 @@ func inspectTypeAssertExpr(pass *analysis.Pass, noLintIndex nolint.DirectiveInde } // Find the parent map for the file containing this node. + f := astutil.FileForPos(pass.Files, typeAssert.Pos()) var parents map[ast.Node]ast.Node - for _, f := range pass.Files { - if f.Pos() <= typeAssert.Pos() && typeAssert.Pos() <= f.End() { - parents = fileParents[f] - break - } + if f != nil { + parents = fileParents[f] } // Skip the safe two-value form: v, ok := x.(T) or v, ok = x.(T) diff --git a/pkg/linters/writebytestring/writebytestring.go b/pkg/linters/writebytestring/writebytestring.go index ec93bab78a6..045c99c719d 100644 --- a/pkg/linters/writebytestring/writebytestring.go +++ b/pkg/linters/writebytestring/writebytestring.go @@ -191,7 +191,7 @@ func implementsWriter(pass *analysis.Pass, expr ast.Expr) bool { // still reported). func buildFix(pass *analysis.Pass, call *ast.CallExpr, writerArg, sText string, filesWithImportEdit map[token.Pos]bool) []analysis.SuggestedFix { // Find the file containing this call. - file := fileForPos(pass.Files, call.Pos()) + file := astutil.FileForPos(pass.Files, call.Pos()) // Determine the local qualifier for "io": use the alias when the package // is already imported under a different name, or the default name when it @@ -233,7 +233,7 @@ func buildFix(pass *analysis.Pass, call *ast.CallExpr, writerArg, sText string, // file containing pos, unless "io" is already imported in that file or an // import edit for this file has already been emitted in this pass. func addIOImportEdit(pass *analysis.Pass, pos token.Pos, filesWithImportEdit map[token.Pos]bool) (analysis.TextEdit, bool) { - file := fileForPos(pass.Files, pos) + file := astutil.FileForPos(pass.Files, pos) if file == nil { return analysis.TextEdit{}, false } @@ -304,13 +304,3 @@ func buildIOImportTextEdit(pass *analysis.Pass, file *ast.File) (analysis.TextEd NewText: []byte("\n\nimport \"" + ioPkg + "\""), }, true } - -// fileForPos returns the *ast.File from files that contains pos, or nil if not found. -func fileForPos(files []*ast.File, pos token.Pos) *ast.File { - for _, f := range files { - if f.Pos() <= pos && pos <= f.End() { - return f - } - } - return nil -}