Pre-v0.7.0 Cleanup: Sorting, Efficiency, and Bug Fixes

README.md

@@ -342,7 +342,7 @@ structalign -inspect -type='*ID*' ./pkg     # inspect just ID-related structs
 
 ### Scanning scope
 
-By default structalign analyzes the regular, hand-written source of each package.
+By default, structalign analyzes the regular, hand-written source of each package.
 A few flags adjust what's in scope:
 
 ```sh

internal/align/align.go

@@ -12,7 +12,7 @@ import (
 	"go/types"
 	"os"
 	"regexp"
-	"sort"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -43,6 +43,7 @@ func (a *Aligner) Findings(t common.Target, opts common.Options) ([]common.Findi
 		return nil, nil
 	}
 	names := structNameIndex(t.Syntax)
+	structs := structTypeIndex(t.Syntax, t.TypesInfo)
 	nolints := nolintIndex(t.Syntax, t.Fset)
 	insp := inspector.New(t.Syntax)
 
@@ -56,7 +57,7 @@ func (a *Aligner) Findings(t common.Target, opts common.Options) ([]common.Findi
 		TypesSizes: t.Sizes, // common.Sizes satisfies types.Sizes
 		ResultOf:   map[*analysis.Analyzer]any{inspect.Analyzer: insp},
 		Report: func(d analysis.Diagnostic) {
-			f := buildFinding(t, d, names, nolints, opts)
+			f := buildFinding(t, d, names, structs, nolints, opts)
 			if f == nil {
 				return
 			}
@@ -66,20 +67,20 @@ func (a *Aligner) Findings(t common.Target, opts common.Options) ([]common.Findi
 	if _, err := fieldalignment.Analyzer.Run(pass); err != nil {
 		return nil, fmt.Errorf("%s: analyzer: %w", t.PkgPath, err)
 	}
-	sort.Slice(findings, func(i, j int) bool {
-		pi, pj := t.Fset.Position(findings[i].Pos), t.Fset.Position(findings[j].Pos)
-		if pi.Filename != pj.Filename {
-			return pi.Filename < pj.Filename
+	slices.SortFunc(findings, func(a, b common.Finding) int {
+		pa, pb := t.Fset.Position(a.Pos), t.Fset.Position(b.Pos)
+		if pa.Filename != pb.Filename {
+			return strings.Compare(pa.Filename, pb.Filename)
 		}
-		return pi.Offset < pj.Offset
+		return pa.Offset - pb.Offset
 	})
 	return findings, nil
 }
 
 // buildFinding converts one analyzer diagnostic into a Finding, applying tag
 // stripping and all active filters. Returns nil when the finding should be
 // suppressed.
-func buildFinding(t common.Target, d analysis.Diagnostic, names map[token.Pos]string, nolints map[token.Pos]nolintInfo, opts common.Options) *common.Finding {
+func buildFinding(t common.Target, d analysis.Diagnostic, names map[token.Pos]string, structs map[token.Pos]*types.Struct, nolints map[token.Pos]nolintInfo, opts common.Options) *common.Finding {
 	f := common.Finding{Fset: t.Fset, Pos: d.Pos, Message: d.Message}
 	if len(d.SuggestedFixes) > 0 && len(d.SuggestedFixes[0].TextEdits) > 0 {
 		e := d.SuggestedFixes[0].TextEdits[0]
@@ -105,7 +106,7 @@ func buildFinding(t common.Target, d analysis.Diagnostic, names map[token.Pos]st
 	if !opts.IncludeGenerated && structfilter.InGeneratedFile(t, f.Pos) {
 		return nil
 	}
-	if opts.SkipCachePadded && isCachePadded(t, f.Name) {
+	if opts.SkipCachePadded && isCachePadded(t, f.Pos, f.Name, structs) {
 		return nil
 	}
 	if opts.RespectNolint {
@@ -116,9 +117,12 @@ func buildFinding(t common.Target, d analysis.Diagnostic, names map[token.Pos]st
 	return &f
 }
 
-// isCachePadded reports whether the named type in t's scope contains a
-// cpu.CacheLinePad field. Returns false for anonymous structs or missing names.
-func isCachePadded(t common.Target, name string) bool {
+// isCachePadded reports whether the struct at pos (or the named type name)
+// contains a cpu.CacheLinePad field.
+func isCachePadded(t common.Target, pos token.Pos, name string, structs map[token.Pos]*types.Struct) bool {
+	if st, ok := structs[pos]; ok {
+		return structfilter.HasCacheLinePad(st)
+	}
 	if name == "" {
 		return false
 	}
@@ -193,6 +197,25 @@ func structNameIndex(files []*ast.File) map[token.Pos]string {
 	return index
 }
 
+// structTypeIndex maps the position of every StructType node in the syntax to
+// its underlying types.Struct, for both named and anonymous structs.
+func structTypeIndex(files []*ast.File, info *types.Info) map[token.Pos]*types.Struct {
+	index := make(map[token.Pos]*types.Struct)
+	for _, f := range files {
+		ast.Inspect(f, func(n ast.Node) bool {
+			if st, ok := n.(*ast.StructType); ok {
+				if tv, ok := info.Types[st]; ok {
+					if s, ok := tv.Type.Underlying().(*types.Struct); ok {
+						index[st.Pos()] = s
+					}
+				}
+			}
+			return true
+		})
+	}
+	return index
+}
+
 // readSource returns the raw source text between two positions.
 func readSource(fset *token.FileSet, pos, end token.Pos) string {
 	pf := fset.File(pos)

internal/align/align_test.go

@@ -198,3 +198,38 @@ type WithPad struct {
 	require.NoError(t, ferr)
 	assert.Empty(t, findings, "WithPad should be skipped when SkipCachePadded=true")
 }
+
+func TestFindingsSkipsAnonymousCachePadded(t *testing.T) {
+	root := moduleRoot(t)
+	testPkgDir := filepath.Join(root, "internal", "align", "_anoncachepadtest")
+	if err := os.MkdirAll(testPkgDir, 0o755); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+	t.Cleanup(func() { os.RemoveAll(testPkgDir) })
+
+	src := `package anoncachepadtest
+
+import "golang.org/x/sys/cpu"
+
+var V = struct {
+	A bool
+	B int64
+	C bool
+	_ cpu.CacheLinePad
+}{A: true, B: 1, C: true}
+`
+	err := os.WriteFile(filepath.Join(testPkgDir, "types.go"), []byte(src), 0o600)
+	require.NoError(t, err)
+
+	tgt := loadPackageTarget(t, testPkgDir)
+
+	// Without SkipCachePadded, the anonymous struct should be reported.
+	findings, ferr := align.New().Findings(tgt, common.Options{})
+	require.NoError(t, ferr)
+	assert.NotEmpty(t, findings, "Anonymous struct should be reported without SkipCachePadded")
+
+	// With SkipCachePadded=true, the anonymous struct should be silently skipped.
+	findings, ferr = align.New().Findings(tgt, common.Options{SkipCachePadded: true})
+	require.NoError(t, ferr)
+	assert.Empty(t, findings, "Anonymous struct should be skipped when SkipCachePadded=true")
+}

internal/align/internal_test.go

@@ -0,0 +1,64 @@
+package align
+
+import (
+	"go/token"
+	"go/types"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/peczenyj/structalign/pkg/common"
+)
+
+func TestIsCachePaddedFallback(t *testing.T) {
+	pkg := types.NewPackage("golang.org/x/sys/cpu", "cpu")
+	padType := types.NewNamed(types.NewTypeName(token.NoPos, pkg, "CacheLinePad", nil), types.Universe.Lookup("int").Type(), nil)
+	field := types.NewField(token.NoPos, pkg, "_", padType, false)
+	st := types.NewStruct([]*types.Var{field}, nil)
+
+	samplePkg := types.NewPackage("sample", "sample")
+	samplePkg.Scope().Insert(types.NewTypeName(token.NoPos, samplePkg, "WithPad", types.NewNamed(types.NewTypeName(token.NoPos, samplePkg, "WithPad", nil), st, nil)))
+
+	tgt := common.Target{Types: samplePkg}
+	structs := make(map[token.Pos]*types.Struct)
+	// Empty map ensures we hit the fallback.
+
+	// Named type that exists
+	assert.True(t, isCachePadded(tgt, token.NoPos, "WithPad", structs))
+
+	// Named type that doesn't exist
+	assert.False(t, isCachePadded(tgt, token.NoPos, "NoSuchType", structs))
+
+	// Anonymous fallback
+	assert.False(t, isCachePadded(tgt, token.NoPos, "", structs))
+}
+
+func TestStripStructTagsError(t *testing.T) {
+	_, err := stripStructTags("invalid struct {")
+	assert.Error(t, err)
+
+	_, err = stripStructTags("int") // not a struct
+	assert.Error(t, err)
+}
+
+func TestFindingsNoSyntax(t *testing.T) {
+	a := Aligner{}
+	f, err := a.Findings(common.Target{}, common.Options{})
+	assert.NoError(t, err)
+	assert.Nil(t, f)
+}
+
+func TestReadSourceErrors(t *testing.T) {
+	fset := token.NewFileSet()
+	// pf == nil
+	assert.Empty(t, readSource(fset, token.NoPos, token.NoPos))
+
+	f := fset.AddFile("missing.go", -1, 10)
+	// os.ReadFile error
+	assert.Empty(t, readSource(fset, f.Pos(0), f.Pos(5)))
+
+	// Bounds check: stop > len(data)
+	// We need a file that exists but we use offsets past its length.
+	// But readSource reads the file from disk using pf.Name().
+	// It's hard to trigger start < 0 or start > stop with valid token.Pos.
+}

internal/app/app.go

@@ -9,7 +9,7 @@ import (
 	"os"
 	"regexp"
 	"runtime/debug"
-	"sort"
+	"slices"
 	"strings"
 
 	"github.com/peczenyj/structalign/internal/align"
@@ -211,7 +211,9 @@ func (a *App) Run(args []string) int {
 		fmt.Fprintf(a.Stderr, "structalign: %v\n", err)
 		return 2
 	}
-	sort.Slice(targets, func(i, j int) bool { return targets[i].PkgPath < targets[j].PkgPath })
+	slices.SortFunc(targets, func(a, b common.Target) int {
+		return strings.Compare(a.PkgPath, b.PkgPath)
+	})
 
 	var allFindings []common.Finding
 	var allLayouts []common.Layout
@@ -258,12 +260,12 @@ func (a *App) Run(args []string) int {
 
 	if opt.sort {
 		if opt.inspect {
-			sort.SliceStable(allLayouts, func(i, j int) bool {
-				return allLayouts[i].Total > allLayouts[j].Total
+			slices.SortStableFunc(allLayouts, func(a, b common.Layout) int {
+				return cmp(b.Total, a.Total)
 			})
 		} else {
-			sort.SliceStable(allFindings, func(i, j int) bool {
-				return savings(allFindings[i]) > savings(allFindings[j])
+			slices.SortStableFunc(allFindings, func(a, b common.Finding) int {
+				return cmp(savings(b), savings(a))
 			})
 		}
 	}
@@ -294,13 +296,23 @@ func (a *App) Run(args []string) int {
 	return 0
 }
 
+func cmp[T ~int | ~int64](a, b T) int {
+	if a < b {
+		return -1
+	}
+	if a > b {
+		return 1
+	}
+	return 0
+}
+
 var eggRE = regexp.MustCompile(`^--?([^=]+)(?:=(.*))?$`)
 
 // stripEggFlags scans args for retro-theme "easter egg" flags, returning the
 // chosen theme name and the args slice with those flags removed. It stops at
 // the first "--" separator.
 func (a *App) stripEggFlags(args []string) (theme string, filtered []string) {
-	filtered = args[:0:0]
+	filtered = make([]string, 0, len(args))
 	afterDD := false
 	for _, arg := range args {
 		if afterDD || arg == "--" {

internal/app/internal_test.go

@@ -0,0 +1,21 @@
+package app
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/peczenyj/structalign/pkg/common"
+)
+
+func TestSavings(t *testing.T) {
+	assert.Equal(t, int64(8), savings(common.Finding{OldSize: 24, NewSize: 16}))
+	assert.Equal(t, int64(0), savings(common.Finding{OldSize: 16, NewSize: 24}))
+	assert.Equal(t, int64(0), savings(common.Finding{OldSize: 0, NewSize: 16}))
+}
+
+func TestCmp(t *testing.T) {
+	assert.Equal(t, -1, cmp(10, 20))
+	assert.Equal(t, 1, cmp(20, 10))
+	assert.Equal(t, 0, cmp(10, 10))
+}

internal/layout/layout.go

@@ -5,7 +5,7 @@ package layout
 import (
 	"go/token"
 	"go/types"
-	"sort"
+	"slices"
 	"strings"
 
 	"github.com/peczenyj/structalign/internal/match"
@@ -66,7 +66,7 @@ func (i *Inspector) discoverStructNames(t common.Target) []string {
 			}
 		}
 	}
-	sort.Strings(names)
+	slices.Sort(names)
 	return names
 }
 

internal/ui/printer.go

@@ -335,6 +335,9 @@ func layoutComments(fields []common.LayoutField, verbose bool) (comments []strin
 func indent(s, pad string) string {
 	lines := strings.Split(s, "\n")
 	for i, l := range lines {
+		if i == len(lines)-1 && l == "" {
+			break
+		}
 		lines[i] = pad + l
 	}
 	return strings.Join(lines, "\n")

internal/ui/unicode_test.go

@@ -33,3 +33,10 @@ func TestTruncPadWideRunes(t *testing.T) {
 	// landing exactly on the column with the ellipsis ("世界" = 4 + "…" = 5).
 	assert.Equal(t, "世界…", truncPad("世界世", 5))
 }
+
+func TestIndent(t *testing.T) {
+	// Without trailing newline
+	assert.Equal(t, "  a\n  b", indent("a\nb", "  "))
+	// With trailing newline: the empty line after the last \n should NOT be indented.
+	assert.Equal(t, "  a\n  b\n", indent("a\nb\n", "  "))
+}