From 05e096cadda62d00dd3c6915f3cccc5610c82028 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 00:42:15 -0600 Subject: [PATCH 1/6] Refactor: Split the cross-file check into source and target rules The cross-file move condition is one bool per file, but the two directions need different import rules. A source file can lose the only use of an import when a declaration leaves it, while a target file can need an import that the moved declaration uses. Extract the file-level rule and give each direction its own predicate that still returns the current result. The next commits relax the two predicates separately. Change-Id: I44f874490aa5f7b37b69dcfd0765c85c95a02159 --- internal/analysis/methodical/fix.go | 39 +++++++++++++++++++---------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 5bdff9f..649b794 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -32,10 +32,9 @@ func (r *diagnosticReporter) report(d analysis.Diagnostic, obj types.Object) { } type fixFile struct { - file *ast.File - token *token.File - source []byte - crossSafe bool + file *ast.File + token *token.File + source []byte } type fixType struct { @@ -87,18 +86,32 @@ func readFixFile(pass *analysis.Pass, file *ast.File) *fixFile { if err != nil || generated.IsGenerated(file) || hasStandaloneComments(pass.Fset, file) { return nil } - base := strings.TrimSuffix(filepath.Base(tf.Name()), ".go") - return &fixFile{file: file, token: tf, source: src, crossSafe: crossFileSafe(base, file, src)} + return &fixFile{file: file, token: tf, source: src} } -func crossFileSafe(base string, file *ast.File, src []byte) bool { +// Moving a declaration out of a file can orphan an import, and moving one in +// can need an import, so each direction gets its own rule. +func (f *fixFile) moveSourceSafe() bool { + return f.plain() && hasNoImports(f.file) +} + +func (f *fixFile) moveTargetSafe() bool { + return f.plain() && hasNoImports(f.file) +} + +// A cross-file move must not cross build constraints or position directives, +// or the method could leave some builds. The file name can carry a constraint, +// and so can a directive inside the file. +func (f *fixFile) plain() bool { + base := strings.TrimSuffix(filepath.Base(f.token.Name()), ".go") if strings.Contains(base, "_") { return false } - if len(file.Imports) != 0 { - return false - } - return !containsDirective(src) && !bytes.Contains(src, []byte("// +build")) + return !containsDirective(f.source) && !bytes.Contains(f.source, []byte("// +build")) +} + +func hasNoImports(file *ast.File) bool { + return len(file.Imports) == 0 } func containsDirective(src []byte) bool { @@ -165,10 +178,10 @@ func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fi return fixMethod{}, false } if srcFile != target.file { - if !srcFile.crossSafe { + if !srcFile.moveSourceSafe() { return fixMethod{}, false } - if !target.file.crossSafe { + if !target.file.moveTargetSafe() { return fixMethod{}, false } } From 86af42c59f5f29237e722b4807e606cd93725260 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 00:45:52 -0600 Subject: [PATCH 2/6] Allow cross-file method moves when the target file has the imports A target file with any import used to refuse the move, which excluded almost every real type definition file. A moved declaration only needs the imports it actually references, so compare those references with the names the target file already imports. A cgo target stays excluded because its preamble sits in the import block. The source file still has no imports, so this only lets a target file carry imports it already uses. Change-Id: I4a3325a9e633dfdd80b7d27da4354e274ea4de2d --- internal/analysis/methodical/fix.go | 119 +++++++++++++++++- .../analysis/methodical/methodical_test.go | 2 +- .../testdata/src/imports/methods.go | 4 + .../testdata/src/imports/methods.go.golden | 1 + .../methodical/testdata/src/imports/types.go | 7 ++ .../testdata/src/imports/types.go.golden | 10 ++ 6 files changed, 139 insertions(+), 4 deletions(-) create mode 100644 internal/analysis/methodical/testdata/src/imports/methods.go create mode 100644 internal/analysis/methodical/testdata/src/imports/methods.go.golden create mode 100644 internal/analysis/methodical/testdata/src/imports/types.go create mode 100644 internal/analysis/methodical/testdata/src/imports/types.go.golden diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 649b794..5602f55 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -8,6 +8,7 @@ import ( "go/types" "path/filepath" "slices" + "strconv" "strings" "golang.org/x/tools/go/analysis" @@ -95,8 +96,20 @@ func (f *fixFile) moveSourceSafe() bool { return f.plain() && hasNoImports(f.file) } -func (f *fixFile) moveTargetSafe() bool { - return f.plain() && hasNoImports(f.file) +func (f *fixFile) moveTargetSafe(pass *analysis.Pass, decl *ast.FuncDecl) bool { + if !f.plain() { + return false + } + if isCgoFile(f.file) { + return false + } + imports := fileImports(pass, f.file) + for _, ref := range declImports(pass, decl) { + if !hasImport(imports, ref) { + return false + } + } + return true } // A cross-file move must not cross build constraints or position directives, @@ -110,6 +123,106 @@ func (f *fixFile) plain() bool { return !containsDirective(f.source) && !bytes.Contains(f.source, []byte("// +build")) } +// importRef names an import the way the code it serves names it. +type importRef struct { + name string + path string +} + +// declImports lists the imports that a declaration needs to compile in a +// different file. A qualified identifier needs the name that qualifies it, and +// a bare identifier from another package needs a dot import. +func declImports(pass *analysis.Pass, decl *ast.FuncDecl) []importRef { + var refs []importRef + selectors := make(map[*ast.Ident]bool) + seen := make(map[importRef]bool) + ast.Inspect(decl, func(node ast.Node) bool { + if sel, ok := node.(*ast.SelectorExpr); ok { + selectors[sel.Sel] = true + } + id, ok := node.(*ast.Ident) + if !ok { + return true + } + if selectors[id] { + return true + } + ref, ok := identImport(pass, id) + if !ok { + return true + } + if seen[ref] { + return true + } + seen[ref] = true + refs = append(refs, ref) + return true + }) + return refs +} + +func identImport(pass *analysis.Pass, id *ast.Ident) (importRef, bool) { + obj := pass.TypesInfo.Uses[id] + if pkgName, ok := obj.(*types.PkgName); ok { + return importRef{name: pkgName.Name(), path: pkgName.Imported().Path()}, true + } + // A bare, unqualified identifier with an object from another package comes + // from a dot import. + if obj == nil { + return importRef{}, false + } + pkg := obj.Pkg() + if pkg == nil { + return importRef{}, false + } + if pkg == pass.Pkg { + return importRef{}, false + } + return importRef{name: ".", path: pkg.Path()}, true +} + +// fileImports maps each name that the file imports to the path it names. +func fileImports(pass *analysis.Pass, file *ast.File) map[string]string { + imports := make(map[string]string) + for _, spec := range file.Imports { + pkgName := importPkgName(pass, spec) + if pkgName == nil { + continue + } + imports[pkgName.Name()] = pkgName.Imported().Path() + } + return imports +} + +func importPkgName(pass *analysis.Pass, spec *ast.ImportSpec) *types.PkgName { + if spec.Name != nil { + if pkgName, ok := pass.TypesInfo.Defs[spec.Name].(*types.PkgName); ok { + return pkgName + } + return nil + } + if pkgName, ok := pass.TypesInfo.Implicits[spec].(*types.PkgName); ok { + return pkgName + } + return nil +} + +func hasImport(imports map[string]string, ref importRef) bool { + return imports[ref.name] == ref.path +} + +func isCgoFile(file *ast.File) bool { + return slices.ContainsFunc(file.Imports, isCgoImport) +} + +func isCgoImport(spec *ast.ImportSpec) bool { + path, err := strconv.Unquote(spec.Path.Value) + if err != nil { + return false + } + return path == "C" +} + func hasNoImports(file *ast.File) bool { return len(file.Imports) == 0 } @@ -181,7 +294,7 @@ func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fi if !srcFile.moveSourceSafe() { return fixMethod{}, false } - if !target.file.moveTargetSafe() { + if !target.file.moveTargetSafe(pass, m.decl) { return fixMethod{}, false } } diff --git a/internal/analysis/methodical/methodical_test.go b/internal/analysis/methodical/methodical_test.go index 0801107..f7ffd25 100644 --- a/internal/analysis/methodical/methodical_test.go +++ b/internal/analysis/methodical/methodical_test.go @@ -13,7 +13,7 @@ func TestAnalyzer(t *testing.T) { } func TestSuggestedFixes(t *testing.T) { - analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "imports") } func TestAnalyzerSkipsGeneratedFiles(t *testing.T) { diff --git a/internal/analysis/methodical/testdata/src/imports/methods.go b/internal/analysis/methodical/testdata/src/imports/methods.go new file mode 100644 index 0000000..a3576a8 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/imports/methods.go @@ -0,0 +1,4 @@ +package imports + +// Alpha sorts before Trim. +func (Widget) Alpha() {} // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/imports/methods.go.golden b/internal/analysis/methodical/testdata/src/imports/methods.go.golden new file mode 100644 index 0000000..dd3e4a8 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/imports/methods.go.golden @@ -0,0 +1 @@ +package imports diff --git a/internal/analysis/methodical/testdata/src/imports/types.go b/internal/analysis/methodical/testdata/src/imports/types.go new file mode 100644 index 0000000..ea80054 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/imports/types.go @@ -0,0 +1,7 @@ +package imports + +import "strings" + +type Widget struct{} + +func (Widget) Trim(s string) string { return strings.TrimSpace(s) } diff --git a/internal/analysis/methodical/testdata/src/imports/types.go.golden b/internal/analysis/methodical/testdata/src/imports/types.go.golden new file mode 100644 index 0000000..ba8b96b --- /dev/null +++ b/internal/analysis/methodical/testdata/src/imports/types.go.golden @@ -0,0 +1,10 @@ +package imports + +import "strings" + +type Widget struct{} + +// Alpha sorts before Trim. +func (Widget) Alpha() {} // want "method Alpha of type Widget should be in same file" + +func (Widget) Trim(s string) string { return strings.TrimSpace(s) } From 387c18f293a8d9b18e3e1b4460e1720853a378ab Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 00:48:40 -0600 Subject: [PATCH 3/6] Allow cross-file method moves when the source imports stay used A source file with any import used to refuse the move, even when the method leaves every import in use. Compare the imports with the uses that stay behind, and judge a file by all of the declarations that leave it at once, because no single method can orphan an import that another method still uses. The move still needs the target file to provide the imports, and a method that is the only use of an import keeps its diagnostic. Change-Id: Id44cdafcd6cd798a1124c09bcd2c2ae71f39a169 --- internal/analysis/methodical/fix.go | 77 +++++++++++++++++-- .../analysis/methodical/methodical_test.go | 2 +- .../methodical/testdata/src/fix/restricted.go | 1 + .../methodical/testdata/src/keeps/other.go | 10 +++ .../testdata/src/keeps/other.go.golden | 7 ++ .../methodical/testdata/src/keeps/types.go | 7 ++ .../testdata/src/keeps/types.go.golden | 10 +++ 7 files changed, 105 insertions(+), 9 deletions(-) create mode 100644 internal/analysis/methodical/testdata/src/keeps/other.go create mode 100644 internal/analysis/methodical/testdata/src/keeps/other.go.golden create mode 100644 internal/analysis/methodical/testdata/src/keeps/types.go create mode 100644 internal/analysis/methodical/testdata/src/keeps/types.go.golden diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 5602f55..2d410e5 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -92,8 +92,20 @@ func readFixFile(pass *analysis.Pass, file *ast.File) *fixFile { // Moving a declaration out of a file can orphan an import, and moving one in // can need an import, so each direction gets its own rule. -func (f *fixFile) moveSourceSafe() bool { - return f.plain() && hasNoImports(f.file) +func (f *fixFile) moveSourceSafe(pass *analysis.Pass, moved []*ast.FuncDecl) bool { + if !f.plain() { + return false + } + if isCgoFile(f.file) { + return false + } + used := usedImports(pass, f.file, moved) + for _, spec := range f.file.Imports { + if !isImportUsed(pass, used, spec) { + return false + } + } + return true } func (f *fixFile) moveTargetSafe(pass *analysis.Pass, decl *ast.FuncDecl) bool { @@ -132,7 +144,7 @@ type importRef struct { // declImports lists the imports that a declaration needs to compile in a // different file. A qualified identifier needs the name that qualifies it, and // a bare identifier from another package needs a dot import. -func declImports(pass *analysis.Pass, decl *ast.FuncDecl) []importRef { +func declImports(pass *analysis.Pass, decl ast.Node) []importRef { var refs []importRef selectors := make(map[*ast.Ident]bool) seen := make(map[importRef]bool) @@ -223,8 +235,49 @@ func isCgoImport(spec *ast.ImportSpec) bool { return path == "C" } -func hasNoImports(file *ast.File) bool { - return len(file.Imports) == 0 +// usedImports lists the imports that the declarations which stay in the file +// still reference. The moved declarations leave the file, so their uses do not +// keep an import alive. +func usedImports(pass *analysis.Pass, file *ast.File, moved []*ast.FuncDecl) map[importRef]bool { + skipped := make(map[*ast.FuncDecl]bool, len(moved)) + for _, decl := range moved { + skipped[decl] = true + } + used := make(map[importRef]bool) + for _, decl := range file.Decls { + if isMovedDecl(decl, skipped) { + continue + } + for _, ref := range declImports(pass, decl) { + used[ref] = true + } + } + return used +} + +func isMovedDecl(decl ast.Decl, skipped map[*ast.FuncDecl]bool) bool { + funcDecl, ok := decl.(*ast.FuncDecl) + if !ok { + return false + } + return skipped[funcDecl] +} + +func isImportUsed(pass *analysis.Pass, used map[importRef]bool, spec *ast.ImportSpec) bool { + pkgName := importPkgName(pass, spec) + if pkgName == nil { + return false + } + // A blank import only runs package initialization, so a file cannot + // orphan it. + if isBlankName(pkgName.Name()) { + return true + } + return used[importRef{name: pkgName.Name(), path: pkgName.Imported().Path()}] +} + +func isBlankName(name string) bool { + return name == "_" } func containsDirective(src []byte) bool { @@ -269,6 +322,7 @@ func safeLineSuffix(suffix []byte) bool { func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types.Object, files map[string]*fixFile, typesByObject map[types.Object]fixType) ([]fixMethod, map[types.Object]bool) { var candidates []fixMethod unsafe := make(map[types.Object]bool) + moved := make(map[*fixFile][]*ast.FuncDecl) for _, m := range methods { if m.typeObj != receiver { continue @@ -279,6 +333,16 @@ func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types continue } candidates = append(candidates, candidate) + if candidate.file != candidate.target.file { + moved[candidate.file] = append(moved[candidate.file], candidate.method.decl) + } + } + // A file can lose an import when several declarations leave it at once, so + // judge each source file by everything that leaves it. + for file, decls := range moved { + if !file.moveSourceSafe(pass, decls) { + unsafe[receiver] = true + } } return candidates, unsafe } @@ -291,9 +355,6 @@ func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fi return fixMethod{}, false } if srcFile != target.file { - if !srcFile.moveSourceSafe() { - return fixMethod{}, false - } if !target.file.moveTargetSafe(pass, m.decl) { return fixMethod{}, false } diff --git a/internal/analysis/methodical/methodical_test.go b/internal/analysis/methodical/methodical_test.go index f7ffd25..1f16045 100644 --- a/internal/analysis/methodical/methodical_test.go +++ b/internal/analysis/methodical/methodical_test.go @@ -13,7 +13,7 @@ func TestAnalyzer(t *testing.T) { } func TestSuggestedFixes(t *testing.T) { - analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "imports") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "imports", "keeps") } func TestAnalyzerSkipsGeneratedFiles(t *testing.T) { diff --git a/internal/analysis/methodical/testdata/src/fix/restricted.go b/internal/analysis/methodical/testdata/src/fix/restricted.go index 684235c..a9227e6 100644 --- a/internal/analysis/methodical/testdata/src/fix/restricted.go +++ b/internal/analysis/methodical/testdata/src/fix/restricted.go @@ -2,4 +2,5 @@ package fix import "fmt" +// Show stays here because the move would leave the fmt import unused. func (Restricted) Show() { fmt.Print("x") } // want "method Show of type Restricted should be in same file" diff --git a/internal/analysis/methodical/testdata/src/keeps/other.go b/internal/analysis/methodical/testdata/src/keeps/other.go new file mode 100644 index 0000000..cf4e0cb --- /dev/null +++ b/internal/analysis/methodical/testdata/src/keeps/other.go @@ -0,0 +1,10 @@ +package keeps + +import "fmt" + +type Gadget struct{} + +func (Gadget) Show() { fmt.Print("g") } + +// Alpha keeps the fmt import alive through Show alone. +func (Widget) Alpha() {} // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/keeps/other.go.golden b/internal/analysis/methodical/testdata/src/keeps/other.go.golden new file mode 100644 index 0000000..4ddf664 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/keeps/other.go.golden @@ -0,0 +1,7 @@ +package keeps + +import "fmt" + +type Gadget struct{} + +func (Gadget) Show() { fmt.Print("g") } diff --git a/internal/analysis/methodical/testdata/src/keeps/types.go b/internal/analysis/methodical/testdata/src/keeps/types.go new file mode 100644 index 0000000..061a533 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/keeps/types.go @@ -0,0 +1,7 @@ +package keeps + +import "strings" + +type Widget struct{} + +func (Widget) Trim(s string) string { return strings.TrimSpace(s) } diff --git a/internal/analysis/methodical/testdata/src/keeps/types.go.golden b/internal/analysis/methodical/testdata/src/keeps/types.go.golden new file mode 100644 index 0000000..e1c71e2 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/keeps/types.go.golden @@ -0,0 +1,10 @@ +package keeps + +import "strings" + +type Widget struct{} + +// Alpha keeps the fmt import alive through Show alone. +func (Widget) Alpha() {} // want "method Alpha of type Widget should be in same file" + +func (Widget) Trim(s string) string { return strings.TrimSpace(s) } From 0ee2f732e6ab58d81f072d90fdd8c070fb88f5cb Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 00:52:23 -0600 Subject: [PATCH 4/6] Add missing imports when a fix moves a method to another file A moved method can need an import that its new file does not have. Add the import with the name that the method already uses, in the group that holds the imports it sorts among, so gofmt leaves the result alone. A file that already binds the name to another package keeps its diagnostic, and an import block with comments gets no new import because the comment would no longer sit beside the import it describes. Judge the source file before adding anything: a method that is the only use of an import still keeps its diagnostic, because removing imports is not part of this fix. Change-Id: I79b66d5e6fd565a36421e9f21e73f838e45cbf62 --- internal/analysis/methodical/fix.go | 324 +++++++++++++++++- .../analysis/methodical/methodical_test.go | 2 +- .../methodical/testdata/src/added/other.go | 10 + .../testdata/src/added/other.go.golden | 7 + .../methodical/testdata/src/added/types.go | 3 + .../testdata/src/added/types.go.golden | 8 + .../testdata/src/commented/other.go | 9 + .../testdata/src/commented/types.go | 10 + internal/driver/iterate_test.go | 40 ++- 9 files changed, 394 insertions(+), 19 deletions(-) create mode 100644 internal/analysis/methodical/testdata/src/added/other.go create mode 100644 internal/analysis/methodical/testdata/src/added/other.go.golden create mode 100644 internal/analysis/methodical/testdata/src/added/types.go create mode 100644 internal/analysis/methodical/testdata/src/added/types.go.golden create mode 100644 internal/analysis/methodical/testdata/src/commented/other.go create mode 100644 internal/analysis/methodical/testdata/src/commented/types.go diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 2d410e5..ce7e2f2 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -6,6 +6,7 @@ import ( "go/ast" "go/token" "go/types" + "maps" "path/filepath" "slices" "strconv" @@ -55,8 +56,8 @@ type fixMethod struct { // Offer at most one receiver fix per analysis to prevent overlapping edits. func suggestedFix(pass *analysis.Pass, methods []methodInfo, receiver types.Object) *analysis.SuggestedFix { files, typesByObject := collectFixFiles(pass) - candidates, unsafe := collectFixMethods(pass, methods, receiver, files, typesByObject) - return makeSuggestedFix(candidates, unsafe) + candidates, imports, unsafe := collectFixMethods(pass, methods, receiver, files, typesByObject) + return makeSuggestedFix(candidates, imports, unsafe) } func collectFixFiles(pass *analysis.Pass) (map[string]*fixFile, map[types.Object]fixType) { @@ -108,20 +109,13 @@ func (f *fixFile) moveSourceSafe(pass *analysis.Pass, moved []*ast.FuncDecl) boo return true } -func (f *fixFile) moveTargetSafe(pass *analysis.Pass, decl *ast.FuncDecl) bool { +// A target file only gains declarations, so it must be one that the move may +// write to at all. +func (f *fixFile) moveTargetSafe() bool { if !f.plain() { return false } - if isCgoFile(f.file) { - return false - } - imports := fileImports(pass, f.file) - for _, ref := range declImports(pass, decl) { - if !hasImport(imports, ref) { - return false - } - } - return true + return !isCgoFile(f.file) } // A cross-file move must not cross build constraints or position directives, @@ -319,7 +313,7 @@ func safeLineSuffix(suffix []byte) bool { return rest == "" || strings.HasPrefix(rest, "//") } -func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types.Object, files map[string]*fixFile, typesByObject map[types.Object]fixType) ([]fixMethod, map[types.Object]bool) { +func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types.Object, files map[string]*fixFile, typesByObject map[types.Object]fixType) ([]fixMethod, []analysis.TextEdit, map[types.Object]bool) { var candidates []fixMethod unsafe := make(map[types.Object]bool) moved := make(map[*fixFile][]*ast.FuncDecl) @@ -344,7 +338,302 @@ func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types unsafe[receiver] = true } } - return candidates, unsafe + imports, ok := collectImportEdits(pass, candidates) + if !ok { + unsafe[receiver] = true + } + return candidates, imports, unsafe +} + +// collectImportEdits adds the imports that the moved declarations need and the +// target files lack. It reports false when an import cannot be added safely. +func collectImportEdits(pass *analysis.Pass, candidates []fixMethod) ([]analysis.TextEdit, bool) { + needed := make(map[*fixFile][]importRef) + for _, m := range candidates { + if m.file == m.target.file { + continue + } + needed[m.target.file] = append(needed[m.target.file], declImports(pass, m.method.decl)...) + } + var edits []analysis.TextEdit + for file, refs := range needed { + fileEdits, ok := fileImportEdits(pass, file, refs) + if !ok { + return nil, false + } + edits = append(edits, fileEdits...) + } + return edits, true +} + +func fileImportEdits(pass *analysis.Pass, file *fixFile, refs []importRef) ([]analysis.TextEdit, bool) { + missing := missingImports(pass, file, refs) + if len(missing) == 0 { + return nil, true + } + if !canAddImports(pass, file, missing) { + return nil, false + } + return addImportEdits(pass, file, missing), true +} + +func missingImports(pass *analysis.Pass, file *fixFile, refs []importRef) []importRef { + have := fileImports(pass, file.file) + missing := make(map[importRef]bool) + for _, ref := range refs { + if hasImport(have, ref) { + continue + } + missing[ref] = true + } + return slices.SortedFunc(maps.Keys(missing), compareImportRefs) +} + +func compareImportRefs(a, b importRef) int { + return cmp.Or(strings.Compare(a.path, b.path), strings.Compare(a.name, b.name)) +} + +// canAddImports reports whether the imports can join the file without changing +// what the names in that file mean. A name that the file already binds cannot +// name a second package, and a comment inside the import block would lose the +// import that it describes. +func canAddImports(pass *analysis.Pass, file *fixFile, missing []importRef) bool { + have := fileImports(pass, file.file) + for _, ref := range missing { + if isNameTaken(have, ref.name) { + return false + } + } + block := importBlock(file.file) + if block == nil { + return true + } + return !hasCommentsIn(file, block) +} + +// isNameTaken reports whether the file already binds the name to another +// package. Go forbids one name from naming both an import and a package-level +// declaration, so a name that the moved code already uses cannot clash with a +// package declaration, and only the names in the file need the check. +func isNameTaken(have map[string]string, name string) bool { + _, ok := have[name] + return ok +} + +func importBlock(file *ast.File) *ast.GenDecl { + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + if gen.Tok != token.IMPORT { + continue + } + if gen.Lparen.IsValid() { + return gen + } + } + return nil +} + +func hasCommentsIn(file *fixFile, node ast.Node) bool { + for _, comment := range file.file.Comments { + if comment.Pos() < node.Pos() { + continue + } + if comment.End() > node.End() { + continue + } + return true + } + return false +} + +func addImportEdits(pass *analysis.Pass, file *fixFile, missing []importRef) []analysis.TextEdit { + block := importBlock(file.file) + if block != nil { + return blockImportEdits(pass, file, block, missing) + } + lines := standaloneImportLines(file) + if len(lines) != 0 { + return standaloneImportEdits(pass, file, lines, missing) + } + anchor := endOfLine(file.source, file.token.Offset(file.file.Name.End())) + pos := file.token.Pos(anchor) + return []analysis.TextEdit{{Pos: pos, End: pos, NewText: newImportDecl(pass, missing)}} +} + +// newImportDecl writes imports for a file that has none, with the blank lines +// that separate a standalone declaration from the code around it. +func newImportDecl(pass *analysis.Pass, missing []importRef) []byte { + if len(missing) == 1 { + return []byte("\n\nimport " + importSpecText(pass, missing[0])) + } + var decl bytes.Buffer + decl.WriteString("\n\nimport (\n") + for _, ref := range missing { + decl.WriteString("\t" + importSpecText(pass, ref) + "\n") + } + decl.WriteString(")") + return decl.Bytes() +} + +func standaloneImportLines(file *fixFile) []importLine { + var lines []importLine + for _, decl := range file.file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + if gen.Tok != token.IMPORT { + continue + } + if gen.Lparen.IsValid() { + continue + } + for _, spec := range gen.Specs { + importSpec, ok := spec.(*ast.ImportSpec) + if !ok { + continue + } + lines = append(lines, importLine{spec: importSpec, path: importPath(importSpec)}) + } + } + return lines +} + +// standaloneImportEdits keeps the imports of a file that spells each one as its +// own declaration together, instead of starting a second group of them. +func standaloneImportEdits(pass *analysis.Pass, file *fixFile, lines []importLine, missing []importRef) []analysis.TextEdit { + after := make(map[int][]string) + first := make(map[int][]string) + for _, ref := range missing { + line, ok := importNeighbour(lines, ref.path) + if !ok { + anchor := lineStart(file.source, file.token.Offset(lines[0].spec.Pos())) + first[anchor] = append(first[anchor], "import "+importSpecText(pass, ref)) + continue + } + anchor := endOfLine(file.source, file.token.Offset(line.spec.End())) + after[anchor] = append(after[anchor], "\nimport "+importSpecText(pass, ref)) + } + return joinImportEdits(file, after, first) +} + +// importSpecText writes one import spec the way the moved code names it. +func importSpecText(pass *analysis.Pass, ref importRef) string { + quoted := strconv.Quote(ref.path) + if ref.name == "." { + return ". " + quoted + } + if ref.name == importedName(pass, ref.path) { + return quoted + } + return ref.name + " " + quoted +} + +func importedName(pass *analysis.Pass, path string) string { + for _, pkg := range pass.Pkg.Imports() { + if pkg.Path() == path { + return pkg.Name() + } + } + return "" +} + +type importLine struct { + spec *ast.ImportSpec + path string +} + +// blockImportEdits places each missing import right after the import that +// sorts before it, which is where gofmt leaves it. An import that sorts first +// joins the line of the import that follows it. +func blockImportEdits(pass *analysis.Pass, file *fixFile, block *ast.GenDecl, missing []importRef) []analysis.TextEdit { + lines := blockImportLines(file, block) + indent := specIndent(file, lines) + after := make(map[int][]string) + first := make(map[int][]string) + for _, ref := range missing { + line, ok := importNeighbour(lines, ref.path) + if !ok { + anchor := firstLineStart(file, block) + first[anchor] = append(first[anchor], indent+importSpecText(pass, ref)) + continue + } + anchor := endOfLine(file.source, file.token.Offset(line.spec.End())) + after[anchor] = append(after[anchor], "\n"+indent+importSpecText(pass, ref)) + } + return joinImportEdits(file, after, first) +} + +func joinImportEdits(file *fixFile, after, first map[int][]string) []analysis.TextEdit { + var edits []analysis.TextEdit + for anchor, specs := range after { + pos := file.token.Pos(anchor) + edits = append(edits, analysis.TextEdit{Pos: pos, End: pos, NewText: []byte(strings.Join(specs, ""))}) + } + for anchor, specs := range first { + pos := file.token.Pos(anchor) + edits = append(edits, analysis.TextEdit{Pos: pos, End: pos, NewText: []byte(strings.Join(specs, "\n") + "\n")}) + } + return edits +} + +func blockImportLines(file *fixFile, block *ast.GenDecl) []importLine { + lines := make([]importLine, 0, len(block.Specs)) + for _, spec := range block.Specs { + importSpec, ok := spec.(*ast.ImportSpec) + if !ok { + continue + } + lines = append(lines, importLine{spec: importSpec, path: importPath(importSpec)}) + } + return lines +} + +func importPath(spec *ast.ImportSpec) string { + path, err := strconv.Unquote(spec.Path.Value) + if err != nil { + return spec.Path.Value + } + return path +} + +// importNeighbour finds the import that a new path follows in sort order. +func importNeighbour(lines []importLine, path string) (importLine, bool) { + var neighbour importLine + found := false + for _, line := range lines { + if !isBefore(line.path, path) { + continue + } + if found && !isBefore(neighbour.path, line.path) { + continue + } + neighbour, found = line, true + } + return neighbour, found +} + +func firstLineStart(file *fixFile, block *ast.GenDecl) int { + if len(block.Specs) == 0 { + return endOfLine(file.source, file.token.Offset(block.Lparen)) + } + return lineStart(file.source, file.token.Offset(block.Specs[0].Pos())) +} + +func lineStart(src []byte, offset int) int { + start := bytes.LastIndexByte(src[:offset], '\n') + return start + 1 +} + +func specIndent(file *fixFile, lines []importLine) string { + if len(lines) == 0 { + return "\t" + } + pos := file.token.Offset(lines[0].spec.Pos()) + return string(file.source[lineStart(file.source, pos):pos]) } func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fixType) (fixMethod, bool) { @@ -355,7 +644,7 @@ func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fi return fixMethod{}, false } if srcFile != target.file { - if !target.file.moveTargetSafe(pass, m.decl) { + if !target.file.moveTargetSafe() { return fixMethod{}, false } } @@ -397,7 +686,7 @@ func afterBlankLines(src []byte, lineEnd int) int { return end } -func makeSuggestedFix(candidates []fixMethod, unsafe map[types.Object]bool) *analysis.SuggestedFix { +func makeSuggestedFix(candidates []fixMethod, imports []analysis.TextEdit, unsafe map[types.Object]bool) *analysis.SuggestedFix { groups := make(map[token.Pos][]fixMethod) for _, m := range candidates { if !unsafe[m.method.typeObj] { @@ -421,6 +710,7 @@ func makeSuggestedFix(candidates []fixMethod, unsafe map[types.Object]bool) *ana if len(edits) == 0 { return nil } + edits = append(edits, imports...) return &analysis.SuggestedFix{Message: "Move and order methods after their types", TextEdits: edits} } diff --git a/internal/analysis/methodical/methodical_test.go b/internal/analysis/methodical/methodical_test.go index 1f16045..57c02f0 100644 --- a/internal/analysis/methodical/methodical_test.go +++ b/internal/analysis/methodical/methodical_test.go @@ -13,7 +13,7 @@ func TestAnalyzer(t *testing.T) { } func TestSuggestedFixes(t *testing.T) { - analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "imports", "keeps") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "added", "commented", "imports", "keeps") } func TestAnalyzerSkipsGeneratedFiles(t *testing.T) { diff --git a/internal/analysis/methodical/testdata/src/added/other.go b/internal/analysis/methodical/testdata/src/added/other.go new file mode 100644 index 0000000..cc356b4 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/added/other.go @@ -0,0 +1,10 @@ +package added + +import "fmt" + +type Gadget struct{} + +func (Gadget) Show(s string) { fmt.Print(s) } + +// Alpha needs the fmt import in its new file. +func (Widget) Alpha(s string) { fmt.Print(s) } // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/added/other.go.golden b/internal/analysis/methodical/testdata/src/added/other.go.golden new file mode 100644 index 0000000..bdee28f --- /dev/null +++ b/internal/analysis/methodical/testdata/src/added/other.go.golden @@ -0,0 +1,7 @@ +package added + +import "fmt" + +type Gadget struct{} + +func (Gadget) Show(s string) { fmt.Print(s) } diff --git a/internal/analysis/methodical/testdata/src/added/types.go b/internal/analysis/methodical/testdata/src/added/types.go new file mode 100644 index 0000000..7a44213 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/added/types.go @@ -0,0 +1,3 @@ +package added + +type Widget struct{} diff --git a/internal/analysis/methodical/testdata/src/added/types.go.golden b/internal/analysis/methodical/testdata/src/added/types.go.golden new file mode 100644 index 0000000..f286f7e --- /dev/null +++ b/internal/analysis/methodical/testdata/src/added/types.go.golden @@ -0,0 +1,8 @@ +package added + +import "fmt" + +type Widget struct{} + +// Alpha needs the fmt import in its new file. +func (Widget) Alpha(s string) { fmt.Print(s) } // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/commented/other.go b/internal/analysis/methodical/testdata/src/commented/other.go new file mode 100644 index 0000000..739dec1 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/commented/other.go @@ -0,0 +1,9 @@ +package commented + +import "fmt" + +type Gadget struct{} + +func (Gadget) Show(s string) { fmt.Print(s) } + +func (Widget) Alpha(s string) { fmt.Print(s) } // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/commented/types.go b/internal/analysis/methodical/testdata/src/commented/types.go new file mode 100644 index 0000000..c529d4c --- /dev/null +++ b/internal/analysis/methodical/testdata/src/commented/types.go @@ -0,0 +1,10 @@ +package commented + +import ( + // Keep this note beside its import. + "strings" +) + +type Widget struct{} + +func (Widget) Trim(s string) string { return strings.TrimSpace(s) } diff --git a/internal/driver/iterate_test.go b/internal/driver/iterate_test.go index b6d4857..fc43267 100644 --- a/internal/driver/iterate_test.go +++ b/internal/driver/iterate_test.go @@ -19,10 +19,17 @@ import ( ) func fixRepo(t *testing.T, source string) string { + t.Helper() + return fixRepoFiles(t, map[string]string{"fix.go": source}) +} + +func fixRepoFiles(t *testing.T, files map[string]string) string { t.Helper() dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "go.mod"), []byte("module example.com/fix\n\ngo 1.25\n"), 0o644)) - require.NoError(t, os.WriteFile(filepath.Join(dir, "fix.go"), []byte(source), 0o644)) + for name, source := range files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(source), 0o644)) + } gitCommand(t, dir, "", "init", "-q") gitCommand(t, dir, "", "add", ".") gitCommand(t, dir, "", "-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "-qm", "initial") @@ -110,6 +117,37 @@ func TestMethodicalCumulativeFix(t *testing.T) { } } +func TestMethodicalCrossFileFix(t *testing.T) { + types := "package fix\n\nimport (\n\t\"strings\"\n)\n\ntype Widget struct{}\n\nfunc (Widget) Trim(s string) string { return strings.TrimSpace(s) }\n\nvar Widgets = 0\n" + methods := "package fix\n\nimport \"fmt\"\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nfunc (Widget) Alpha(s string) { fmt.Print(s) }\n\nvar Gadgets = 0\n" + fixRepoFiles(t, map[string]string{"types.go": types, "methods.go": methods}) + code, output, diagnostics := runFix(t, methodical.Analyzer, false) + require.Equal(t, 0, code) + require.Empty(t, output) + require.Empty(t, diagnostics) + + wantTypes := "package fix\n\nimport (\n\t\"fmt\"\n\t\"strings\"\n)\n\ntype Widget struct{}\n\nfunc (Widget) Alpha(s string) { fmt.Print(s) }\n\nfunc (Widget) Trim(s string) string { return strings.TrimSpace(s) }\n\nvar Widgets = 0\n" + wantMethods := "package fix\n\nimport \"fmt\"\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nvar Gadgets = 0\n" + for name, want := range map[string]string{"types.go": wantTypes, "methods.go": wantMethods} { + data, err := os.ReadFile(name) + require.NoError(t, err) + formatted, err := format.Source(data) + require.NoError(t, err) + require.Equal(t, string(formatted), string(data), "%s is not formatted", name) + require.Equal(t, want, string(data), "%s", name) + } + + code, output, diagnostics = runFix(t, methodical.Analyzer, false) + require.Equal(t, 0, code) + require.Empty(t, output) + require.Empty(t, diagnostics) + for name, want := range map[string]string{"types.go": wantTypes, "methods.go": wantMethods} { + data, err := os.ReadFile(name) + require.NoError(t, err) + require.Equal(t, want, string(data), "%s changed on a second run", name) + } +} + func TestFixMoreThan100Passes(t *testing.T) { source := "package fix\nvar Count = 0\n" fixRepo(t, source) From 8d48687b4437dd110c0bf75dbbcc278111ef6fb6 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:16:04 -0600 Subject: [PATCH 5/6] Remove imports that a move leaves unused A method that was the only use of an import could not move, which left most real methods in place. Give the source file the edit that removes the import instead, so the move keeps both files compiling. Remove a whole import declaration when it loses every import, and one line otherwise, with the blank line that set the import apart from its group so that gofmt leaves the file alone. Skip an import block that holds comments, because a comment can describe the import that goes. A removal can still end a file, so the rule that keeps a blank line from ending a file now looks at every removal rather than only at removed methods. Change-Id: Ie2c72e33393da32335aa2ac46700c77482e40e9e --- internal/analysis/methodical/fix.go | 274 +++++++++++++++--- internal/analysis/methodical/fix_test.go | 7 +- .../analysis/methodical/methodical_test.go | 2 +- .../methodical/testdata/src/fix/cross.go | 2 - .../testdata/src/fix/cross.go.golden | 2 - .../methodical/testdata/src/fix/restricted.go | 6 - .../methodical/testdata/src/orphan/other.go | 6 + .../testdata/src/orphan/other.go.golden | 1 + .../methodical/testdata/src/orphan/types.go | 3 + .../testdata/src/orphan/types.go.golden | 8 + internal/driver/iterate_test.go | 6 +- 11 files changed, 255 insertions(+), 62 deletions(-) delete mode 100644 internal/analysis/methodical/testdata/src/fix/restricted.go create mode 100644 internal/analysis/methodical/testdata/src/orphan/other.go create mode 100644 internal/analysis/methodical/testdata/src/orphan/other.go.golden create mode 100644 internal/analysis/methodical/testdata/src/orphan/types.go create mode 100644 internal/analysis/methodical/testdata/src/orphan/types.go.golden diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index ce7e2f2..9c22f56 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -57,7 +57,7 @@ type fixMethod struct { func suggestedFix(pass *analysis.Pass, methods []methodInfo, receiver types.Object) *analysis.SuggestedFix { files, typesByObject := collectFixFiles(pass) candidates, imports, unsafe := collectFixMethods(pass, methods, receiver, files, typesByObject) - return makeSuggestedFix(candidates, imports, unsafe) + return makeSuggestedFix(pass, files, candidates, imports, unsafe) } func collectFixFiles(pass *analysis.Pass) (map[string]*fixFile, map[types.Object]fixType) { @@ -91,22 +91,73 @@ func readFixFile(pass *analysis.Pass, file *ast.File) *fixFile { return &fixFile{file: file, token: tf, source: src} } -// Moving a declaration out of a file can orphan an import, and moving one in -// can need an import, so each direction gets its own rule. -func (f *fixFile) moveSourceSafe(pass *analysis.Pass, moved []*ast.FuncDecl) bool { +func (f *fixFile) importDeclRemoval(gen *ast.GenDecl) (analysis.TextEdit, bool) { + start := gen.Pos() + if gen.Doc != nil { + start = gen.Doc.Pos() + } + end := f.token.Offset(gen.End()) + lineEnd := endOfLine(f.source, end) + if !safeLineSuffix(f.source[end:lineEnd]) { + return analysis.TextEdit{}, false + } + return f.removal(f.token.Offset(start), afterBlankLines(f.source, lineEnd)), true +} + +// importRemovals removes whole declarations when they lose every import, so a +// block that no longer holds anything does not stay behind. +func (f *fixFile) importRemovals(gen *ast.GenDecl, unused []*ast.ImportSpec) ([]analysis.TextEdit, bool) { + if len(unused) == len(gen.Specs) { + edit, ok := f.importDeclRemoval(gen) + if !ok { + return nil, false + } + return []analysis.TextEdit{edit}, true + } + var edits []analysis.TextEdit + for _, spec := range unused { + edit, ok := f.importSpecRemoval(spec) + if !ok { + return nil, false + } + edits = append(edits, edit) + } + return edits, true +} + +// importSpecRemoval takes the blank line next to the import as well, so the +// remaining imports keep the grouping that gofmt leaves in place. +func (f *fixFile) importSpecRemoval(spec *ast.ImportSpec) (analysis.TextEdit, bool) { + start := lineStart(f.source, f.token.Offset(spec.Pos())) + end := f.token.Offset(spec.End()) + lineEnd := endOfLine(f.source, end) + if !safeLineSuffix(f.source[end:lineEnd]) { + return analysis.TextEdit{}, false + } + if lineEnd == len(f.source) { + return analysis.TextEdit{}, false + } + next := lineEnd + 1 + if isBlankLineAt(f.source, next) { + return f.removal(start, endOfLineInclusive(f.source, next)), true + } + if isBlankLineBefore(f.source, start) { + start = lineStart(f.source, start-1) + } + return f.removal(start, next), true +} + +// A source file loses the declarations that move out of it, along with any +// import that only they used, so the move edits those imports too. +func (f *fixFile) moveSourceEdits(pass *analysis.Pass, moved []*ast.FuncDecl) ([]analysis.TextEdit, bool) { if !f.plain() { - return false + return nil, false } if isCgoFile(f.file) { - return false + return nil, false } used := usedImports(pass, f.file, moved) - for _, spec := range f.file.Imports { - if !isImportUsed(pass, used, spec) { - return false - } - } - return true + return f.removeImports(pass, used) } // A target file only gains declarations, so it must be one that the move may @@ -129,6 +180,99 @@ func (f *fixFile) plain() bool { return !containsDirective(f.source) && !bytes.Contains(f.source, []byte("// +build")) } +func (f *fixFile) removal(start, end int) analysis.TextEdit { + return analysis.TextEdit{Pos: f.token.Pos(start), End: f.token.Pos(end)} +} + +// removeImports deletes the imports that no declaration in the file uses any +// more. It reports false when an import cannot go without taking a comment with +// it or leaving a line that gofmt would rewrite. +func (f *fixFile) removeImports(pass *analysis.Pass, used map[importRef]bool) ([]analysis.TextEdit, bool) { + var edits []analysis.TextEdit + for _, decl := range f.file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + if gen.Tok != token.IMPORT { + continue + } + unused := unusedImports(pass, used, gen) + if len(unused) == 0 { + continue + } + if hasCommentsIn(f, gen) { + return nil, false + } + removals, ok := f.importRemovals(gen, unused) + if !ok { + return nil, false + } + edits = append(edits, removals...) + } + return edits, true +} + +// tailRemoval takes the blank line that precedes removals that reach the end of +// the file, and leaves alone a blank line that has to separate what stays. +func (f *fixFile) tailRemoval(merged []fixRange) (analysis.TextEdit, bool) { + if len(merged) == 0 { + return analysis.TextEdit{}, false + } + last := merged[len(merged)-1] + if last.end != len(f.source) { + return analysis.TextEdit{}, false + } + limit := 0 + if len(merged) > 1 { + limit = merged[len(merged)-2].end + } + start := trimBlankTail(f.source, last.start) + start = max(start, limit) + if start == last.start { + return analysis.TextEdit{}, false + } + return analysis.TextEdit{Pos: f.token.Pos(start), End: f.token.Pos(last.start)}, true +} + +func unusedImports(pass *analysis.Pass, used map[importRef]bool, gen *ast.GenDecl) []*ast.ImportSpec { + var unused []*ast.ImportSpec + for _, spec := range gen.Specs { + importSpec, ok := spec.(*ast.ImportSpec) + if !ok { + continue + } + if isImportUsed(pass, used, importSpec) { + continue + } + unused = append(unused, importSpec) + } + return unused +} + +func isBlankLineAt(src []byte, offset int) bool { + if offset >= len(src) { + return false + } + lineEnd := endOfLine(src, offset) + return len(bytes.Trim(src[offset:lineEnd], " \t\r")) == 0 +} + +func isBlankLineBefore(src []byte, offset int) bool { + if offset == 0 { + return false + } + return isBlankLineAt(src, lineStart(src, offset-1)) +} + +func endOfLineInclusive(src []byte, offset int) int { + end := endOfLine(src, offset) + if end < len(src) { + return end + 1 + } + return end +} + // importRef names an import the way the code it serves names it. type importRef struct { name string @@ -315,6 +459,7 @@ func safeLineSuffix(suffix []byte) bool { func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types.Object, files map[string]*fixFile, typesByObject map[types.Object]fixType) ([]fixMethod, []analysis.TextEdit, map[types.Object]bool) { var candidates []fixMethod + var importEdits []analysis.TextEdit unsafe := make(map[types.Object]bool) moved := make(map[*fixFile][]*ast.FuncDecl) for _, m := range methods { @@ -334,15 +479,19 @@ func collectFixMethods(pass *analysis.Pass, methods []methodInfo, receiver types // A file can lose an import when several declarations leave it at once, so // judge each source file by everything that leaves it. for file, decls := range moved { - if !file.moveSourceSafe(pass, decls) { + removals, ok := file.moveSourceEdits(pass, decls) + if !ok { unsafe[receiver] = true + continue } + importEdits = append(importEdits, removals...) } - imports, ok := collectImportEdits(pass, candidates) + added, ok := collectImportEdits(pass, candidates) if !ok { unsafe[receiver] = true } - return candidates, imports, unsafe + importEdits = append(importEdits, added...) + return candidates, importEdits, unsafe } // collectImportEdits adds the imports that the moved declarations need and the @@ -686,7 +835,7 @@ func afterBlankLines(src []byte, lineEnd int) int { return end } -func makeSuggestedFix(candidates []fixMethod, imports []analysis.TextEdit, unsafe map[types.Object]bool) *analysis.SuggestedFix { +func makeSuggestedFix(pass *analysis.Pass, files map[string]*fixFile, candidates []fixMethod, imports []analysis.TextEdit, unsafe map[types.Object]bool) *analysis.SuggestedFix { groups := make(map[token.Pos][]fixMethod) for _, m := range candidates { if !unsafe[m.method.typeObj] { @@ -695,7 +844,6 @@ func makeSuggestedFix(candidates []fixMethod, imports []analysis.TextEdit, unsaf } var edits []analysis.TextEdit for anchor, group := range groups { - trimMovedTail(group) slices.SortFunc(group, func(a, b fixMethod) int { return cmp.Or(cmp.Compare(a.target.order, b.target.order), cmp.Compare(genericRank(a.method.decl), genericRank(b.method.decl)), strings.Compare(a.method.methodName, b.method.methodName)) }) @@ -711,33 +859,74 @@ func makeSuggestedFix(candidates []fixMethod, imports []analysis.TextEdit, unsaf return nil } edits = append(edits, imports...) - return &analysis.SuggestedFix{Message: "Move and order methods after their types", TextEdits: edits} + return &analysis.SuggestedFix{Message: "Move and order methods after their types", TextEdits: trimRemovedTails(pass.Fset, files, edits)} } -// trimMovedTail takes the blank line before the declarations that a move -// removes from the end of a file. They kept the blank line that separated them -// from the declaration above, which would end the file as a blank line. -func trimMovedTail(group []fixMethod) { - slices.SortFunc(group, func(a, b fixMethod) int { - return cmp.Compare(a.start, b.start) - }) - last := len(group) - 1 - fix := group[last].file - if fix.token.Offset(group[last].end) != len(fix.source) { - return +// fixRange is a span of a source file that a fix removes. +type fixRange struct { + start, end int +} + +// trimRemovedTails keeps a fix from ending a file with a blank line. Text that +// a fix removes keeps the blank line that separated it from the declaration +// above, so when the removals reach the end of a file, that blank line goes +// too. +func trimRemovedTails(fset *token.FileSet, files map[string]*fixFile, edits []analysis.TextEdit) []analysis.TextEdit { + removals := make(map[*fixFile][]fixRange) + kept := make([]analysis.TextEdit, 0, len(edits)) + for _, edit := range edits { + fix := editFile(fset, files, edit) + if edit.End == edit.Pos { + kept = append(kept, edit) + continue + } + if fix == nil { + kept = append(kept, edit) + continue + } + removals[fix] = append(removals[fix], fixRange{fix.token.Offset(edit.Pos), fix.token.Offset(edit.End)}) } - first := last - for isAdjacentToPrevious(group, first) { - first-- + for fix, ranges := range removals { + merged := mergeRanges(ranges) + tail, ok := fix.tailRemoval(merged) + if ok { + kept = append(kept, tail) + } + for _, r := range merged { + kept = append(kept, analysis.TextEdit{Pos: fix.token.Pos(r.start), End: fix.token.Pos(r.end)}) + } } - limit := 0 - if first > 0 { - limit = fix.token.Offset(group[first-1].end) + return kept +} + +func editFile(fset *token.FileSet, files map[string]*fixFile, edit analysis.TextEdit) *fixFile { + file := fset.File(edit.Pos) + if file == nil { + return nil } - start := trimBlankTail(fix.source, fix.token.Offset(group[first].start)) - if start > limit { - group[first].start = fix.token.Pos(start) + if fset.File(edit.End) != file { + return nil } + return files[file.Name()] +} + +func mergeRanges(ranges []fixRange) []fixRange { + if len(ranges) == 0 { + return nil + } + slices.SortFunc(ranges, func(a, b fixRange) int { + return cmp.Compare(a.start, b.start) + }) + merged := ranges[:1] + for _, r := range ranges[1:] { + last := &merged[len(merged)-1] + if r.start <= last.end { + last.end = max(last.end, r.end) + continue + } + merged = append(merged, r) + } + return merged } // trimBlankTail moves an offset back over the blank lines before it and leaves @@ -762,15 +951,6 @@ func isBlankByte(b byte) bool { } } -// isAdjacentToPrevious reports whether a removal starts where the one before it -// ends, so both reach back to the same blank line. -func isAdjacentToPrevious(group []fixMethod, i int) bool { - if i == 0 { - return false - } - return group[i-1].end == group[i].start -} - func genericRank(decl *ast.FuncDecl) int { if decl.Type.TypeParams != nil { return 0 diff --git a/internal/analysis/methodical/fix_test.go b/internal/analysis/methodical/fix_test.go index 8f1c62f..1b3d112 100644 --- a/internal/analysis/methodical/fix_test.go +++ b/internal/analysis/methodical/fix_test.go @@ -93,9 +93,14 @@ type Unrelated struct{} // Keep this method in place. func (Unrelated) AlreadyCorrect() {} `) + // The comment in the import block keeps a fix from taking the fmt import + // that Show needs and from deleting it after the move. writeFixture(t, dir, "other.go", `package p -import "fmt" +import ( + // Show prints. + "fmt" +) func (Unsafe) Show() { fmt.Print("x") } `) diff --git a/internal/analysis/methodical/methodical_test.go b/internal/analysis/methodical/methodical_test.go index 57c02f0..42d0c49 100644 --- a/internal/analysis/methodical/methodical_test.go +++ b/internal/analysis/methodical/methodical_test.go @@ -13,7 +13,7 @@ func TestAnalyzer(t *testing.T) { } func TestSuggestedFixes(t *testing.T) { - analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "added", "commented", "imports", "keeps") + analysistest.RunWithSuggestedFixes(t, analysistest.TestData(), methodical.Analyzer, "fix", "generic", "added", "commented", "imports", "keeps", "orphan") } func TestAnalyzerSkipsGeneratedFiles(t *testing.T) { diff --git a/internal/analysis/methodical/testdata/src/fix/cross.go b/internal/analysis/methodical/testdata/src/fix/cross.go index 00fde44..486ea85 100644 --- a/internal/analysis/methodical/testdata/src/fix/cross.go +++ b/internal/analysis/methodical/testdata/src/fix/cross.go @@ -1,5 +1,3 @@ package fix type Cross struct{} - -type Restricted struct{} diff --git a/internal/analysis/methodical/testdata/src/fix/cross.go.golden b/internal/analysis/methodical/testdata/src/fix/cross.go.golden index c0d7ce5..11397eb 100644 --- a/internal/analysis/methodical/testdata/src/fix/cross.go.golden +++ b/internal/analysis/methodical/testdata/src/fix/cross.go.golden @@ -4,5 +4,3 @@ type Cross struct{} // CrossMethod stays documented after the move. func (Cross) CrossMethod() {} // want "method CrossMethod of type Cross should be in same file" - -type Restricted struct{} diff --git a/internal/analysis/methodical/testdata/src/fix/restricted.go b/internal/analysis/methodical/testdata/src/fix/restricted.go deleted file mode 100644 index a9227e6..0000000 --- a/internal/analysis/methodical/testdata/src/fix/restricted.go +++ /dev/null @@ -1,6 +0,0 @@ -package fix - -import "fmt" - -// Show stays here because the move would leave the fmt import unused. -func (Restricted) Show() { fmt.Print("x") } // want "method Show of type Restricted should be in same file" diff --git a/internal/analysis/methodical/testdata/src/orphan/other.go b/internal/analysis/methodical/testdata/src/orphan/other.go new file mode 100644 index 0000000..1bc91bb --- /dev/null +++ b/internal/analysis/methodical/testdata/src/orphan/other.go @@ -0,0 +1,6 @@ +package orphan + +import "fmt" + +// Alpha leaves this file without the fmt import that only it uses. +func (Widget) Alpha(s string) { fmt.Print(s) } // want "method Alpha of type Widget should be in same file" diff --git a/internal/analysis/methodical/testdata/src/orphan/other.go.golden b/internal/analysis/methodical/testdata/src/orphan/other.go.golden new file mode 100644 index 0000000..5cff17d --- /dev/null +++ b/internal/analysis/methodical/testdata/src/orphan/other.go.golden @@ -0,0 +1 @@ +package orphan diff --git a/internal/analysis/methodical/testdata/src/orphan/types.go b/internal/analysis/methodical/testdata/src/orphan/types.go new file mode 100644 index 0000000..335a694 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/orphan/types.go @@ -0,0 +1,3 @@ +package orphan + +type Widget struct{} diff --git a/internal/analysis/methodical/testdata/src/orphan/types.go.golden b/internal/analysis/methodical/testdata/src/orphan/types.go.golden new file mode 100644 index 0000000..db86693 --- /dev/null +++ b/internal/analysis/methodical/testdata/src/orphan/types.go.golden @@ -0,0 +1,8 @@ +package orphan + +import "fmt" + +type Widget struct{} + +// Alpha leaves this file without the fmt import that only it uses. +func (Widget) Alpha(s string) { fmt.Print(s) } // want "method Alpha of type Widget should be in same file" diff --git a/internal/driver/iterate_test.go b/internal/driver/iterate_test.go index fc43267..0dd8cce 100644 --- a/internal/driver/iterate_test.go +++ b/internal/driver/iterate_test.go @@ -119,15 +119,15 @@ func TestMethodicalCumulativeFix(t *testing.T) { func TestMethodicalCrossFileFix(t *testing.T) { types := "package fix\n\nimport (\n\t\"strings\"\n)\n\ntype Widget struct{}\n\nfunc (Widget) Trim(s string) string { return strings.TrimSpace(s) }\n\nvar Widgets = 0\n" - methods := "package fix\n\nimport \"fmt\"\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nfunc (Widget) Alpha(s string) { fmt.Print(s) }\n\nvar Gadgets = 0\n" + methods := "package fix\n\nimport (\n\t\"fmt\"\n\t\"os\"\n)\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nfunc (Widget) Alpha() { fmt.Print(os.Args) }\n\nvar Gadgets = 0\n" fixRepoFiles(t, map[string]string{"types.go": types, "methods.go": methods}) code, output, diagnostics := runFix(t, methodical.Analyzer, false) require.Equal(t, 0, code) require.Empty(t, output) require.Empty(t, diagnostics) - wantTypes := "package fix\n\nimport (\n\t\"fmt\"\n\t\"strings\"\n)\n\ntype Widget struct{}\n\nfunc (Widget) Alpha(s string) { fmt.Print(s) }\n\nfunc (Widget) Trim(s string) string { return strings.TrimSpace(s) }\n\nvar Widgets = 0\n" - wantMethods := "package fix\n\nimport \"fmt\"\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nvar Gadgets = 0\n" + wantTypes := "package fix\n\nimport (\n\t\"fmt\"\n\t\"os\"\n\t\"strings\"\n)\n\ntype Widget struct{}\n\nfunc (Widget) Alpha() { fmt.Print(os.Args) }\n\nfunc (Widget) Trim(s string) string { return strings.TrimSpace(s) }\n\nvar Widgets = 0\n" + wantMethods := "package fix\n\nimport (\n\t\"fmt\"\n)\n\ntype Gadget struct{}\n\nfunc (Gadget) Show(s string) { fmt.Print(s) }\n\nvar Gadgets = 0\n" for name, want := range map[string]string{"types.go": wantTypes, "methods.go": wantMethods} { data, err := os.ReadFile(name) require.NoError(t, err) From 41d1bf10bbd936782c9691b693a5be43f2fa368d Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:16:26 -0600 Subject: [PATCH 6/6] Document the fix preconditions in the README The README showed the checks but never the -fix option, so the rules that decide whether a method can move were only in the code. Record what a fix does, which imports it adds and removes, which cases it skips, and that it only writes files that Git can restore. Change-Id: I64853d59311a3b1a7512ac1702e2da6bc8057836 --- README.md | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/README.md b/README.md index ccfc7c5..1df58bc 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,31 @@ if a && isB() { // allowed The check also covers `for` conditions and bool assignments such as `c := a && b`, including negated variables, field and map access, and comparisons. +## Fixes + +`-fix` applies the fixes that the analyzers suggest, and `-fix -diff` prints the change instead of writing it. + +```sh +constable -fix ./... +constable -fix -diff ./... +``` + +Only the methodical analyzer suggests fixes today. +It places every method of a reported type directly after the type declaration, in order, and a method that lives in another file moves into the file that declares its type. + +A move fixes the import blocks of both files: + +- The new file gains the imports that the moved method needs. +- The old file loses the imports that no declaration uses any more. + +A move is conservative, because the analyzer must be sure that the result compiles and reads well: + +- The fix skips the move when the new file already binds a needed name to another package, or when the import block that it would edit holds comments. +- The fix skips generated files, files with build constraints, Go directives, or a build suffix in the file name, files that use cgo, files with comments that no declaration owns, and declarations that do not start their own line. + +A fix writes only files that Git tracks and that have no changes, so `git checkout` recovers a fix that went wrong. +Diagnostics that remain after the fixes are printed, and the command exits unsuccessfully. + ## Development Run all fixes,