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, diff --git a/internal/analysis/methodical/fix.go b/internal/analysis/methodical/fix.go index 5bdff9f..9c22f56 100644 --- a/internal/analysis/methodical/fix.go +++ b/internal/analysis/methodical/fix.go @@ -6,8 +6,10 @@ import ( "go/ast" "go/token" "go/types" + "maps" "path/filepath" "slices" + "strconv" "strings" "golang.org/x/tools/go/analysis" @@ -32,10 +34,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 { @@ -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(pass, files, candidates, imports, unsafe) } func collectFixFiles(pass *analysis.Pass) (map[string]*fixFile, map[types.Object]fixType) { @@ -87,18 +88,334 @@ 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 { +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 nil, false + } + if isCgoFile(f.file) { + return nil, false + } + used := usedImports(pass, f.file, moved) + return f.removeImports(pass, used) +} + +// 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 + } + return !isCgoFile(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 !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 !containsDirective(src) && !bytes.Contains(src, []byte("// +build")) + 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 + 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.Node) []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" +} + +// 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 { @@ -140,9 +457,11 @@ 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 + var importEdits []analysis.TextEdit unsafe := make(map[types.Object]bool) + moved := make(map[*fixFile][]*ast.FuncDecl) for _, m := range methods { if m.typeObj != receiver { continue @@ -153,8 +472,317 @@ 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 { + removals, ok := file.moveSourceEdits(pass, decls) + if !ok { + unsafe[receiver] = true + continue + } + importEdits = append(importEdits, removals...) + } + added, ok := collectImportEdits(pass, candidates) + if !ok { + unsafe[receiver] = true + } + importEdits = append(importEdits, added...) + return candidates, importEdits, 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)...) } - return candidates, unsafe + 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) { @@ -165,10 +793,7 @@ func fixMethodFor(pass *analysis.Pass, m methodInfo, srcFile *fixFile, target fi return fixMethod{}, false } if srcFile != target.file { - if !srcFile.crossSafe { - return fixMethod{}, false - } - if !target.file.crossSafe { + if !target.file.moveTargetSafe() { return fixMethod{}, false } } @@ -210,7 +835,7 @@ func afterBlankLines(src []byte, lineEnd int) int { return end } -func makeSuggestedFix(candidates []fixMethod, 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] { @@ -219,7 +844,6 @@ func makeSuggestedFix(candidates []fixMethod, unsafe map[types.Object]bool) *ana } 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)) }) @@ -234,33 +858,75 @@ func makeSuggestedFix(candidates []fixMethod, unsafe map[types.Object]bool) *ana if len(edits) == 0 { return nil } - return &analysis.SuggestedFix{Message: "Move and order methods after their types", TextEdits: edits} + edits = append(edits, imports...) + 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 @@ -285,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 0801107..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") + 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/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/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 684235c..0000000 --- a/internal/analysis/methodical/testdata/src/fix/restricted.go +++ /dev/null @@ -1,5 +0,0 @@ -package fix - -import "fmt" - -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/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) } 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) } 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 b6d4857..0dd8cce 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 (\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\"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) + 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)