Merge pull request #49 from neongreen/copilot/fix-comments-preservation

Fix comment preservation when extracting functions with `dissect move`

Author: neongreen

dissect/AGENTS.md

@@ -0,0 +1,70 @@
+# Agent Guidelines for Dissect
+
+## Code Manipulation Requirements
+
+### AST-Based Manipulation Only
+
+**All Go code manipulation in the dissect tool MUST use AST (Abstract Syntax Tree) operations.**
+
+### Prohibited Approaches
+
+❌ **String manipulation of Go code is strictly prohibited**
+
+This includes but is not limited to:
+- String slicing to extract code snippets
+- String concatenation to build Go code
+- Text-based search and replace on Go source
+- Regex operations on Go source code
+- Manual text parsing of Go syntax
+
+### Required Approaches
+
+✅ **AST-based operations are required**
+
+All code manipulation must use the Go AST packages:
+- `go/ast` - Abstract syntax tree representation
+- `go/parser` - Parsing Go source to AST
+- `go/token` - Position information and file sets
+- `go/format` - Formatting AST back to source
+- `go/printer` - Printing AST nodes
+
+### Rationale
+
+String-based manipulation of Go code is fragile and error-prone because it:
+
+1. **Cannot handle syntax variations**: Go allows multiple valid formatting styles. String matching fails when code is formatted differently.
+
+2. **Breaks with whitespace changes**: String slicing by position breaks when whitespace, comments, or formatting changes.
+
+3. **Cannot preserve semantic information**: Type information, scope, and other semantic data are lost.
+
+4. **Fails on edge cases**: Build tags, compiler directives, and other Go features require proper parsing.
+
+5. **Cannot properly handle comments**: Comments in Go have complex relationships with declarations that only AST preserves correctly.
+
+### Examples
+
+❌ **Prohibited** - String slicing:
+```go
+content, _ := os.ReadFile(file)
+funcText := string(content[start:end])  // String manipulation!
+```
+
+✅ **Required** - AST manipulation:
+```go
+fset := token.NewFileSet()
+node, _ := parser.ParseFile(fset, file, nil, parser.ParseComments)
+// Work with node.Decls, node.Comments, etc.
+format.Node(output, fset, node)
+```
+
+### When to Use gopls
+
+The `gopls` language server can be used for complex refactoring operations, but:
+- The results must still be processed using AST operations
+- Do not extract string snippets from gopls output
+- Parse gopls results with `go/parser` and manipulate the resulting AST
+
+### Exception
+
+The only exception is when dealing with non-Go files or when the output is for display purposes only (logging, error messages, etc.).

dissect/cmd/move.go

@@ -1,11 +1,13 @@
 package main
 
 import (
+	"bytes"
 	"dissect/pkg/commands"
 	"dissect/pkg/gopls"
 	"dissect/pkg/goutils"
 	"fmt"
 	"go/ast"
+	"go/printer"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -156,18 +158,12 @@ func moveIdentifier(sourceFile string, identifier string, targetFile string, mod
 	// Remove the temp file when done
 	defer os.Remove(tempFile)
 
-	// Parse the temp file to extract the function declaration and imports using AST
-	_, tempNode, err := goutils.ReadGoFile(tempFile)
+	// Parse the temp file to extract the function (with comments preserved by gopls)
+	tempFset, tempNode, err := goutils.ReadGoFile(tempFile)
 	if err != nil {
 		return fmt.Errorf("error parsing temp file: %w", err)
 	}
 
-	// Parse the target file
-	targetFset, targetNode, err := goutils.ReadGoFile(targetFile)
-	if err != nil {
-		return fmt.Errorf("error parsing target file: %w", err)
-	}
-
 	// Find the function in the temp file
 	var funcToMove *ast.FuncDecl
 	for _, decl := range tempNode.Decls {
@@ -180,28 +176,55 @@ func moveIdentifier(sourceFile string, identifier string, targetFile string, mod
 	if funcToMove == nil {
 		return fmt.Errorf("no function found in temp file")
 	}
+	
+	// Extract imports from temp file for later merging
+	var tempImports []*ast.ImportSpec
+	for _, imp := range tempNode.Imports {
+		tempImports = append(tempImports, imp)
+	}
 
-	// Merge imports from temp file to target file
-	// Build a map of existing imports in target
+	// Serialize just the function declaration (not a whole file) using printer
+	// This gives us the function with its Doc comments as source text
+	var funcBuf bytes.Buffer
+	cfg := printer.Config{Mode: printer.UseSpaces | printer.TabIndent, Tabwidth: 8}
+	if err := cfg.Fprint(&funcBuf, tempFset, funcToMove); err != nil {
+		return fmt.Errorf("error serializing function: %w", err)
+	}
+	
+	// Read the current target file content
+	targetContent, err := os.ReadFile(targetFile)
+	if err != nil {
+		return fmt.Errorf("error reading target file: %w", err)
+	}
+	
+	// Append the serialized function to the target file
+	// printer.Fprint on a FuncDecl includes the Doc comments automatically
+	newContent := string(targetContent) + "\n" + funcBuf.String() + "\n"
+	if err := os.WriteFile(targetFile, []byte(newContent), 0644); err != nil {
+		return fmt.Errorf("error writing target file: %w", err)
+	}
+	
+	// Now reparse the entire target file to get a proper AST
+	targetFset, targetNode, err := goutils.ReadGoFile(targetFile)
+	if err != nil {
+		return fmt.Errorf("error reparsing target file: %w", err)
+	}
+	
+	// Merge imports from temp file using AST operations
 	existingImports := make(map[string]bool)
 	for _, imp := range targetNode.Imports {
 		existingImports[imp.Path.Value] = true
 	}
-
-	// Add new imports from temp file
-	for _, imp := range tempNode.Imports {
+	
+	for _, imp := range tempImports {
 		if !existingImports[imp.Path.Value] {
 			targetNode.Imports = append(targetNode.Imports, imp)
 		}
 	}
-
-	// Add the function declaration to the target file
-	targetNode.Decls = append(targetNode.Decls, funcToMove)
-
-	// Write the modified target file back using AST
-	err = goutils.WriteGoFile(targetFile, targetFset, targetNode)
-	if err != nil {
-		return fmt.Errorf("error writing target file: %w", err)
+	
+	// Write back the target file with merged imports
+	if err := goutils.WriteGoFile(targetFile, targetFset, targetNode); err != nil {
+		return fmt.Errorf("error writing target file with imports: %w", err)
 	}
 
 	// Run goimports to organize imports and format properly