Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ wormatter --exclude "*_test.go" .
wormatter --exclude "*.pb.go" --exclude "vendor/*" .
```

### Free-Floating Comments

Files with free-floating comments (section headers separated from code by a blank line) on `var` or `const` declarations will produce an error, since these comments cannot be safely preserved during declaration merging and reordering. Normal doc comments (directly attached to the declaration) are preserved correctly.

### Generated Files

Files starting with any of these comments are automatically skipped:
Expand Down Expand Up @@ -117,7 +121,9 @@ func main() {}
| 2 | Public (uppercase) | By custom type |
| 3 | Private (lowercase) | By custom type |

**Within each group:** sorted alphabetically, no empty lines.
**Within each group:**
- **Constants:** untyped consts sorted alphabetically; typed consts (e.g. `Stage`, `StatusCode`) preserve their original order to keep intentional orderings intact.
- **Variables:** original order preserved (to avoid breaking initialization dependencies).

<details>
<summary>Example</summary>
Expand Down Expand Up @@ -229,14 +235,16 @@ func (s *Server) stop() {}

### Struct Fields

Fields are grouped (separated by empty lines):
Fields in **named type declarations** (`type Foo struct{...}`) are grouped (separated by empty lines):

| Order | Group | Sorting |
|-------|-------|---------|
| 1 | Embedded | Alphabetically by type name |
| 2 | Public | Alphabetically |
| 3 | Private | Alphabetically |

Anonymous structs (table tests, inline struct fields, composite literal types) are **not** reordered. Structs with encoding-related struct tags (`json`, `yaml`, `xml`, `toml`, `protobuf`) are also **not** reordered, to preserve wire format compatibility.

**Struct literals** with named fields are reordered to match the struct definition.

<details>
Expand Down
61 changes: 61 additions & 0 deletions TODO.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Wormatter Issues & Fixes (Found in nelm)

Issues discovered by formatting the [nelm](https://github.com/werf/nelm) codebase and analyzing results.

---

## Critical

### ~~1. Variable reordering breaks initialization dependencies~~ → Fixed
**Fix applied**: Var specs are now grouped by exportability only (`_` first, exported, unexported) and preserve original relative order within each group. `sortVarSpecsByExportability` replaces `sortSpecsByExportabilityThenName` for vars.

### ~~2. Comment loss (`ChartTSEntryPoints`)~~ → Fixed
**Fix applied**: `transferGenDeclDocToFirstSpec` transfers `GenDecl.Decs.Start` to the first `ValueSpec.Decs.Start` in `collectGenDecl` for both `token.CONST` and `token.VAR` cases.

### ~~2a. Verify inline comment attachment during const reordering~~ → Fixed
**Fix applied**: Added regression tests. Also fixed end-comment transfer: `GenDecl.Decs.End` (inline comments on single-spec declarations) is now transferred to the spec's `Decs.End`. DST limitation: the last spec's `End` comment in a block is moved to `Start` via `fixLastSpecEndComment` to prevent misplacement.

### ~~2b. Verify free-floating comment handling~~ → Fixed
**Fix applied**: `checkFreeFloatingComments` detects free-floating comments (trailing `"\n"` in `Decs.Start`) on var/const declarations before reordering and returns an error.

---

## Major

### ~~3. Test table struct field reordering buries `name` field~~ → Fixed
**Fix applied**: `reorderStructFields` now inspects `*dst.TypeSpec` nodes and reorders the `*dst.StructType` inside them, skipping anonymous structs (table tests, inline struct fields, composite literal types).

### ~~4. JSON-serialized struct field reordering changes wire format~~ → Fixed
**Fix applied**: `hasEncodingTags` checks if any field has `json`, `yaml`, `xml`, `toml`, or `protobuf` struct tags. If so, `reorderStructFields` skips that struct entirely.

---

## Minor

### ~~5. Spurious blank lines in structs~~ → Fixed
**Fix applied**: `assembleFieldList` now unconditionally sets `Decs.Before = dst.NewLine` and `Decs.After = dst.None` for all fields, then sets `EmptyLine` only on the first field of each new group boundary.

### ~~6. Ordered typed string constants reordered~~ → Fixed
**Fix applied**: `sortConstSpecsByExportabilityThenName` sorts by exportability → type name → name only for untyped consts. Typed consts preserve their original relative order within the same type group via `sort.SliceStable`.

### 7. Table test cases reordered in slice literals
**Issue**: Elements in table-driven test case slices appear to be reordered (e.g. `internal/plan/plan_build_test.go`, where `{ name: `...`, input: ..., expect: ... }` cases moved around). This is noisy at best and can be semantically risky if test cases are order-dependent.
**Fix**: Duplicate of #3 + #8. The code does not reorder slice elements — the perceived reordering is caused by struct field reordering *within* each element (anonymous struct fields sorted by #3, keyed literal fields reordered by #8). Fixing those issues resolves this.

### ~~8. Keyed composite literals reordered (struct literals)~~ → WontFix
**Issue**: Keyed elements inside struct literals are being reordered to match struct definition order. This can theoretically change side-effect order (Go evaluates element expressions in source order).
**Decision**: Keep reordering. Side-effect-dependent composite literal values are rare and a code smell. The reordering is intentional behavior.

### ~~9. Map literal entries reordered~~ → Misdiagnosed / WontFix
**Issue**: Map literal entries appeared to be reordered in `internal/resource/sensitive_test.go`.
**Analysis**: The code does NOT reorder map entries — `resolveSortedFieldOrder` returns `nil` for `*dst.MapType`, so `reorderCompositeLitFields` is never called for maps. The perceived reordering was caused by other changes (struct field/declaration reordering) shifting surrounding code in the diff.

---

## WontFix / Working as Intended

- Function signature collapsing: `runFailurePlan` (507 chars) stays single-line. User preference is "don't touch".
- Test method/function reordering: Test methods remain sorted alphabetically. User preference is "do nothing".
- Embedded struct reordering: `*Options` structs continue sorting embedded fields alphabetically. User preference is "Sort Alphabetically".
- Const block merging: The formatter merges const blocks and groups by type within the merged block. While this changes the source layout (types detached from consts), it's consistent with the "one const block per file" style. The grouping by type inside the block is sufficient.
- Reordering `init()` relative to package-level initializers (was #2c): Safe per the Go spec — all package-level variables are initialized before any `init()` runs, regardless of source position. The current implementation preserves the relative order of multiple `init()` functions within the same file (collected and emitted in source order via `collector.go`).
3 changes: 3 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,18 @@ require (
github.com/dave/dst v0.27.3
github.com/samber/lo v1.52.0
github.com/spf13/cobra v1.10.2
github.com/stretchr/testify v1.11.1
golang.org/x/mod v0.31.0
gonum.org/v1/gonum v0.16.0
mvdan.cc/gofumpt v0.9.2
)

require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/hexops/gotextdiff v1.0.3 // indirect
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/sergi/go-diff v1.4.0 // indirect
github.com/spf13/pflag v1.0.9 // indirect
go.uber.org/atomic v1.7.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI=
Expand Down
18 changes: 16 additions & 2 deletions pkg/formatter/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ func (c *declCollector) collectGenDecl(d *dst.GenDecl) {
if hasIota(d) {
c.iotaConstDecls = append(c.iotaConstDecls, d)
} else {
transferGenDeclDecsToSpecs(d)
c.constSpecs = append(c.constSpecs, d.Specs...)
}
case token.VAR:
transferGenDeclDecsToSpecs(d)
for _, spec := range d.Specs {
if isBlankVarSpec(spec) {
c.blankVarSpecs = append(c.blankVarSpecs, spec)
Expand All @@ -91,6 +93,18 @@ func (c *declCollector) collectGenDecl(d *dst.GenDecl) {
}
}

func transferGenDeclDecsToSpecs(d *dst.GenDecl) {
if len(d.Specs) == 0 {
return
}
if first, ok := d.Specs[0].(*dst.ValueSpec); ok && len(d.Decs.Start) > 0 && len(first.Decs.Start) == 0 {
first.Decs.Start = d.Decs.Start
}
if last, ok := d.Specs[len(d.Specs)-1].(*dst.ValueSpec); ok && len(d.Decs.End) > 0 && len(last.Decs.End) == 0 {
last.Decs.End = d.Decs.End
}
}

func (c *declCollector) collectTypeNames(f *dst.File) {
for _, decl := range f.Decls {
gd, ok := decl.(*dst.GenDecl)
Expand All @@ -106,8 +120,8 @@ func (c *declCollector) collectTypeNames(f *dst.File) {
}

func (c *declCollector) sort() {
sortSpecsByExportabilityThenName(c.constSpecs)
sortSpecsByExportabilityThenName(c.varSpecs)
sortConstSpecsByExportabilityThenName(c.constSpecs)
sortVarSpecsByExportability(c.varSpecs)

for typeName := range c.constructors {
sortFuncDeclsByName(c.constructors[typeName])
Expand Down
84 changes: 84 additions & 0 deletions pkg/formatter/comment_loss_ai_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
//go:build ai_tests

package formatter_test

import (
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/werf/wormatter/pkg/formatter"
)

func TestAI_CommentPreservationDuringMerge(t *testing.T) {
runFormatterTest(t, "comment_loss")
}

func TestAI_FreeFloatingCommentDetection(t *testing.T) {
actualPath := filepath.Join("testdata", "floating_actual.go")
content := "package main\n\n// Section: Database vars\n\nvar dbHost = \"localhost\"\n\nfunc main() {}\n"

require.NoError(t, os.WriteFile(actualPath, []byte(content), 0o644))
defer os.Remove(actualPath)

err := formatter.FormatFile(actualPath, formatter.Options{})
require.Error(t, err)
assert.Contains(t, err.Error(), "free-floating comments")
}

func TestAI_NormalDocCommentNoError(t *testing.T) {
actualPath := filepath.Join("testdata", "doc_comment_actual.go")
content := "package main\n\n// dbHost is the database host.\nvar dbHost = \"localhost\"\n\nfunc main() {}\n"

require.NoError(t, os.WriteFile(actualPath, []byte(content), 0o644))
defer os.Remove(actualPath)

require.NoError(t, formatter.FormatFile(actualPath, formatter.Options{}))
}

func TestAI_TypedConstsPreserveOrder(t *testing.T) {
runFormatterTest(t, "typed_consts")
}

func TestAI_NoSpuriousBlankLinesInStructs(t *testing.T) {
runFormatterTest(t, "struct_spacing")
}

func TestAI_EncodingTaggedStructFieldsNotReordered(t *testing.T) {
runFormatterTest(t, "encoding_tags")
}

func TestAI_AnonymousStructFieldsNotReordered(t *testing.T) {
runFormatterTest(t, "anon_struct")
}

func TestAI_InlineCommentAttachmentDuringReorder(t *testing.T) {
runFormatterTest(t, "inline_comments")
}

func runFormatterTest(t *testing.T, name string) {
t.Helper()

inputPath := filepath.Join("testdata", name+"_input.go")
expectedPath := filepath.Join("testdata", name+"_expected.go")
actualPath := filepath.Join("testdata", name+"_actual.go")

inputBytes, err := os.ReadFile(inputPath)
require.NoError(t, err)

require.NoError(t, os.WriteFile(actualPath, inputBytes, 0o644))
defer os.Remove(actualPath)

require.NoError(t, formatter.FormatFile(actualPath, formatter.Options{}))

actualBytes, err := os.ReadFile(actualPath)
require.NoError(t, err)

expectedBytes, err := os.ReadFile(expectedPath)
require.NoError(t, err)

assert.Equal(t, string(expectedBytes), string(actualBytes))
}
17 changes: 16 additions & 1 deletion pkg/formatter/declarations.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,25 @@ func mergeSpecsIntoBlock(tok token.Token, specs []dst.Spec) *dst.GenDecl {
}
if len(specs) > 1 {
gd.Lparen = true
fixLastSpecEndComment(specs)
addEmptyLinesBetweenSpecGroups(specs)
}

return gd
}

func fixLastSpecEndComment(specs []dst.Spec) {
if len(specs) == 0 {
return
}
vs, ok := specs[len(specs)-1].(*dst.ValueSpec)
if !ok || len(vs.Decs.End) == 0 {
return
}
vs.Decs.Start = append(vs.Decs.Start, vs.Decs.End...)
vs.Decs.End = nil
}

func addEmptyLinesBetweenSpecGroups(specs []dst.Spec) {
var lastGroup int
var lastType string
Expand All @@ -126,7 +139,9 @@ func addEmptyLinesBetweenSpecGroups(specs []dst.Spec) {
} else {
vs.Decs.Before = dst.NewLine
}
vs.Decs.After = dst.None
if len(vs.Decs.End) == 0 {
vs.Decs.After = dst.None
}
lastGroup = currentGroup
lastType = currentType
}
Expand Down
19 changes: 19 additions & 0 deletions pkg/formatter/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"path/filepath"
"strings"

"github.com/dave/dst"
"github.com/dave/dst/decorator"
"mvdan.cc/gofumpt/format"
)
Expand Down Expand Up @@ -55,6 +56,10 @@ func FormatFile(filePath string, opts Options) error {
return nil
}

if err := checkFreeFloatingComments(f, filePath); err != nil {
return err
}

collapseFuncSignatures(f)
originalFieldOrder := collectOriginalFieldOrder(f)
convertPositionalToKeyed(f, originalFieldOrder)
Expand Down Expand Up @@ -101,6 +106,20 @@ func FormatFile(filePath string, opts Options) error {
return os.WriteFile(filePath, formatted, 0o644)
}

func checkFreeFloatingComments(f *dst.File, filePath string) error {
for _, decl := range f.Decls {
gd, ok := decl.(*dst.GenDecl)
if !ok || (gd.Tok != token.CONST && gd.Tok != token.VAR) {
continue
}
if hasFreeFloatingComment(gd.Decs.Start) {
return fmt.Errorf("%s: file has free-floating comments, cannot safely reorder declarations", filePath)
}
}

return nil
}

func matchesAnyPattern(path string, patterns []string) bool {
for _, pattern := range patterns {
if matched, _ := filepath.Match(pattern, path); matched {
Expand Down
4 changes: 4 additions & 0 deletions pkg/formatter/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@ func isBlankVarSpec(spec dst.Spec) bool {
})
}

func hasFreeFloatingComment(decs dst.Decorations) bool {
return len(decs) > 0 && decs[len(decs)-1] == "\n"
}

func isExported(name string) bool {
return len(name) > 0 && unicode.IsUpper(rune(name[0]))
}
Expand Down
28 changes: 28 additions & 0 deletions pkg/formatter/sorting.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,34 @@ func sortSpecsByExportabilityThenName(specs []dst.Spec) {
})
}

func sortConstSpecsByExportabilityThenName(specs []dst.Spec) {
sort.SliceStable(specs, func(i, j int) bool {
nameI := getSpecFirstName(specs[i])
nameJ := getSpecFirstName(specs[j])
groupI := getExportGroup(nameI)
groupJ := getExportGroup(nameJ)
if groupI != groupJ {
return groupI < groupJ
}
typeI := getSpecTypeName(specs[i])
typeJ := getSpecTypeName(specs[j])
if typeI != typeJ {
return typeI < typeJ
}
if typeI != "" {
return false
}

return nameI < nameJ
})
}

func sortVarSpecsByExportability(specs []dst.Spec) {
sort.SliceStable(specs, func(i, j int) bool {
return getExportGroup(getSpecFirstName(specs[i])) < getExportGroup(getSpecFirstName(specs[j]))
})
}

func splitAndGroupTypeDecls(typeDecls []*dst.GenDecl) []dst.Decl {
var simpleTypes, funcInterfaces, nonFuncInterfaces, structs []dst.Decl

Expand Down
Loading