From 6c821b850fe6061a472e0915e188aab9679e3767 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:43:34 +0300 Subject: [PATCH 1/9] docs: add TODO.md with issues found by formatting nelm codebase Signed-off-by: Ilya Lesikov --- TODO.md | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..8d6d664 --- /dev/null +++ b/TODO.md @@ -0,0 +1,70 @@ +# 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 (`pkg/featgate/feat.go`) +**Issue**: `FeatGates = []*FeatGate{}` moved to end of `var` block, overwriting populated slice (filled by `NewFeatGate` calls) with empty one. +**Fix**: Don't sort var specs by name at all. Only group by exportability (`_` first, exported, unexported) and preserve original relative order within each group. Dependency analysis via AST is unreliable (can't detect indirect dependencies through function calls), and alphabetical sorting can break initialization order. + +### 2. Comment loss (`ChartTSEntryPoints`) (`pkg/common/common.go`) +**Issue**: Comment `// ChartTSEntryPoints defines supported TypeScript/JavaScript entry points (in priority order).` lost during var merge. Variable `ChartTSEntryPoints` was moved to a merged block, but the comment was detached. +**Fix**: When extracting specs from `GenDecl` in `collectGenDecl` (both `token.CONST` and `token.VAR` cases), transfer `GenDecl.Decs.Start` (doc comments) to the first `ValueSpec.Decs.Start` before discarding the `GenDecl`. This ensures comments travel with the spec during merge/sort. For multi-spec blocks, only transfer to the first spec (inner comments are already on the spec). + +### 2a. Verify inline comment attachment during const reordering +**Issue**: Wormatter reorders `const` specs. We need tests ensuring inline comments remain attached to the correct spec after any reordering. +**Evidence**: `internal/plan/release_info.go` shows const spec reordering; comments still look correct there, but this should be covered by tests to prevent regressions. +**Fix**: No code change needed — DST decorations on `ValueSpec` (including inline `End` comments) travel with the node during `sort.SliceStable`. Add regression tests to verify inline comments remain attached to the correct spec after reordering. + +### 2b. Verify free-floating comment handling +**Issue**: Need to verify that free-floating comments (not clearly attached to any decl/spec/block) are preserved and remain correctly positioned when wormatter merges/reorders declarations. +**Fix**: Detect free-floating comments (section headers separated from code by a blank line) before reordering. In DST, these are identifiable by a trailing `"\n"` entry in `Decs.Start`. If any top-level declaration has such a detached comment, return an error for that file ("file has free-floating comments, cannot safely reorder declarations"). Add tests for this detection and for normal doc comments (no trailing `"\n"`) being preserved correctly. + +--- + +## Major + +### 3. Test table struct field reordering buries `name` field +**Issue**: `name` field moved to end of test structs (alphabetical sort), making table tests unreadable. +**Fix**: Only reorder struct fields in named type declarations (`type Foo struct{...}`). Change `reorderStructFields` to look for `*dst.TypeSpec` nodes and reorder the `*dst.StructType` inside them, instead of targeting all `*dst.StructType` nodes indiscriminately. This skips anonymous structs (table tests, inline struct fields, composite literal types). + +### 4. JSON-serialized struct field reordering changes wire format +**Issue**: Struct field order changes JSON output order in Go; wormatter reorders json-tagged fields in exported types (e.g. `internal/plan/operation.go`, `internal/plan/plan.go`). +**Fix**: If any field in a struct has an encoding-related struct tag (`json`, `yaml`, `xml`, `toml`, `protobuf`), skip field reordering for that entire struct (preserve original field order). Don't mix sorted and unsorted fields — it creates confusing output. Non-encoding tags (`validate`, `db`, etc.) don't trigger the skip. + +--- + +## Minor + +### 5. Spurious blank lines in structs +**Issue**: Blank lines inserted between fields in both public and private structs (e.g., `NoActivityTimeout` / `Ownership` in public structs; `logStore` / `maxLogEventTableWidth` in private `tablesBuilder` struct in `internal/track/progress_tables.go`; `discoveryClient` / `dynamicClient` in private `KubeClient` struct in `internal/kube/client_kube.go`). +**Fix**: The grouping logic in `assembleFieldList` is correct (only sets `EmptyLine` at group boundaries), but the loops preserve stale `EmptyLine` decorations from the original source via a `if f.Decs.Before != dst.EmptyLine` guard. Fix: unconditionally set `Decs.Before = dst.NewLine` for all fields within a group, then set `EmptyLine` only on the first field of each new group. + +### 6. Ordered typed string constants reordered +**Issue**: Typed string constant blocks that are intentionally ordered get reordered alphabetically (e.g. stage ordering in `pkg/common/common.go`, and previously observed `ReleaseType` constants). +**Fix**: Create a const-specific sort function (separate from vars). Sort const specs by: exportability → type name → name **only for untyped consts** (empty type). Typed consts (`Stage`, `ReleaseType`, etc.) preserve their original relative order within the same type group — `sort.SliceStable` handles this naturally by returning `false` for same-type comparisons. This keeps intentional orderings (pipeline stages, priorities) intact while still alphabetizing untyped consts. + +### 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`). From e0b83f42707949f9071caab8c702713569552117 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:46:01 +0300 Subject: [PATCH 2/9] fix: preserve var initialization order instead of sorting alphabetically Var specs are now grouped by exportability only (blank _, exported, unexported) and preserve their original relative order within each group. This prevents breaking initialization dependencies where later vars depend on earlier ones being initialized first. Signed-off-by: Ilya Lesikov --- README.md | 4 ++- go.mod | 3 ++ go.sum | 4 +-- pkg/formatter/collector.go | 2 +- pkg/formatter/sorting.go | 6 ++++ pkg/formatter/testdata/expected.go | 4 +-- pkg/formatter/testdata/var_order_expected.go | 28 +++++++++++++++ pkg/formatter/testdata/var_order_input.go | 27 +++++++++++++++ pkg/formatter/var_order_ai_test.go | 36 ++++++++++++++++++++ 9 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 pkg/formatter/testdata/var_order_expected.go create mode 100644 pkg/formatter/testdata/var_order_input.go create mode 100644 pkg/formatter/var_order_ai_test.go diff --git a/README.md b/README.md index 4794699..c1fd964 100644 --- a/README.md +++ b/README.md @@ -117,7 +117,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:** sorted alphabetically, no empty lines. +- **Variables:** original order preserved (to avoid breaking initialization dependencies), no empty lines.
Example diff --git a/go.mod b/go.mod index b5e79e9..05ac14f 100644 --- a/go.mod +++ b/go.mod @@ -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 diff --git a/go.sum b/go.sum index 023201c..0e497b1 100644 --- a/go.sum +++ b/go.sum @@ -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= diff --git a/pkg/formatter/collector.go b/pkg/formatter/collector.go index 4c78543..8ac71a6 100644 --- a/pkg/formatter/collector.go +++ b/pkg/formatter/collector.go @@ -107,7 +107,7 @@ func (c *declCollector) collectTypeNames(f *dst.File) { func (c *declCollector) sort() { sortSpecsByExportabilityThenName(c.constSpecs) - sortSpecsByExportabilityThenName(c.varSpecs) + sortVarSpecsByExportability(c.varSpecs) for typeName := range c.constructors { sortFuncDeclsByName(c.constructors[typeName]) diff --git a/pkg/formatter/sorting.go b/pkg/formatter/sorting.go index 978f5e6..19d494d 100644 --- a/pkg/formatter/sorting.go +++ b/pkg/formatter/sorting.go @@ -49,6 +49,12 @@ func sortSpecsByExportabilityThenName(specs []dst.Spec) { }) } +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 diff --git a/pkg/formatter/testdata/expected.go b/pkg/formatter/testdata/expected.go index 4690a0e..7b303cf 100644 --- a/pkg/formatter/testdata/expected.go +++ b/pkg/formatter/testdata/expected.go @@ -48,10 +48,10 @@ var ( DefaultStatus StatusCode = "default" ErrorStatus StatusCode = "error" + globalZ = 10 globalA = 5 - globalB = 3 globalMiddle = 7 - globalZ = 10 + globalB = 3 singleConst = 1 sliceOfStructs = []struct { content string diff --git a/pkg/formatter/testdata/var_order_expected.go b/pkg/formatter/testdata/var_order_expected.go new file mode 100644 index 0000000..a4a3aa2 --- /dev/null +++ b/pkg/formatter/testdata/var_order_expected.go @@ -0,0 +1,28 @@ +package main + +import "fmt" + +var ( + FeatGates = []*FeatGate{} + Alpha = NewFeatGate("alpha") + Beta = NewFeatGate("beta") + Gamma = NewFeatGate("gamma") + + privateFirst = 1 + privateSecond = 2 +) + +type FeatGate struct { + Name string +} + +func NewFeatGate(name string) *FeatGate { + fg := &FeatGate{Name: name} + FeatGates = append(FeatGates, fg) + + return fg +} + +func main() { + fmt.Println(FeatGates) +} diff --git a/pkg/formatter/testdata/var_order_input.go b/pkg/formatter/testdata/var_order_input.go new file mode 100644 index 0000000..cfe69cb --- /dev/null +++ b/pkg/formatter/testdata/var_order_input.go @@ -0,0 +1,27 @@ +package main + +import "fmt" + +var FeatGates = []*FeatGate{} + +func NewFeatGate(name string) *FeatGate { + fg := &FeatGate{Name: name} + FeatGates = append(FeatGates, fg) + + return fg +} + +var Alpha = NewFeatGate("alpha") +var Beta = NewFeatGate("beta") +var Gamma = NewFeatGate("gamma") + +var privateFirst = 1 +var privateSecond = 2 + +type FeatGate struct { + Name string +} + +func main() { + fmt.Println(FeatGates) +} diff --git a/pkg/formatter/var_order_ai_test.go b/pkg/formatter/var_order_ai_test.go new file mode 100644 index 0000000..c231058 --- /dev/null +++ b/pkg/formatter/var_order_ai_test.go @@ -0,0 +1,36 @@ +//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_VarOrderPreservesInitDependencies(t *testing.T) { + inputPath := filepath.Join("testdata", "var_order_input.go") + expectedPath := filepath.Join("testdata", "var_order_expected.go") + actualPath := filepath.Join("testdata", "var_order_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)) +} From 2060b7f7ef48177ca0bcf16f7f3f78e5dcc5f084 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:47:33 +0300 Subject: [PATCH 3/9] fix: preserve doc comments when merging var/const declarations When extracting specs from GenDecl to merge into a single block, doc comments (Decs.Start) and inline comments (Decs.End) on the GenDecl were lost. transferGenDeclDecsToSpecs now transfers them to the first/last spec before discarding the GenDecl. Signed-off-by: Ilya Lesikov --- pkg/formatter/collector.go | 14 +++++++ pkg/formatter/comment_loss_ai_test.go | 42 +++++++++++++++++++ .../testdata/comment_loss_expected.go | 24 +++++++++++ pkg/formatter/testdata/comment_loss_input.go | 21 ++++++++++ pkg/formatter/testdata/expected.go | 17 +++++--- 5 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 pkg/formatter/comment_loss_ai_test.go create mode 100644 pkg/formatter/testdata/comment_loss_expected.go create mode 100644 pkg/formatter/testdata/comment_loss_input.go diff --git a/pkg/formatter/collector.go b/pkg/formatter/collector.go index 8ac71a6..e7dd269 100644 --- a/pkg/formatter/collector.go +++ b/pkg/formatter/collector.go @@ -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) @@ -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) diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go new file mode 100644 index 0000000..602929c --- /dev/null +++ b/pkg/formatter/comment_loss_ai_test.go @@ -0,0 +1,42 @@ +//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 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)) +} diff --git a/pkg/formatter/testdata/comment_loss_expected.go b/pkg/formatter/testdata/comment_loss_expected.go new file mode 100644 index 0000000..9f10e76 --- /dev/null +++ b/pkg/formatter/testdata/comment_loss_expected.go @@ -0,0 +1,24 @@ +package main + +const ( + // SomeConst is a documented constant. + SomeConst = "hello" + + // anotherConst is private. + anotherConst = "world" +) + +var ( + // ChartTSEntryPoints defines supported TypeScript/JavaScript entry points (in priority order). + ChartTSEntryPoints = []string{ + "index.ts", + "index.js", + } + // MaxRetries is the maximum number of retries. + MaxRetries = 3 + + // defaultTimeout is the default timeout duration. + defaultTimeout = 30 +) + +func main() {} diff --git a/pkg/formatter/testdata/comment_loss_input.go b/pkg/formatter/testdata/comment_loss_input.go new file mode 100644 index 0000000..98885c0 --- /dev/null +++ b/pkg/formatter/testdata/comment_loss_input.go @@ -0,0 +1,21 @@ +package main + +// ChartTSEntryPoints defines supported TypeScript/JavaScript entry points (in priority order). +var ChartTSEntryPoints = []string{ + "index.ts", + "index.js", +} + +// MaxRetries is the maximum number of retries. +var MaxRetries = 3 + +// defaultTimeout is the default timeout duration. +var defaultTimeout = 30 + +// SomeConst is a documented constant. +const SomeConst = "hello" + +// anotherConst is private. +const anotherConst = "world" + +func main() {} diff --git a/pkg/formatter/testdata/expected.go b/pkg/formatter/testdata/expected.go index 7b303cf..a44359d 100644 --- a/pkg/formatter/testdata/expected.go +++ b/pkg/formatter/testdata/expected.go @@ -22,7 +22,8 @@ const ( ConstA = "a" ConstB = "b" ConstMiddle = "m" - ConstZ = "z" + // Test: consts should be merged and sorted + ConstZ = "z" StatusError StatusCode = "error" StatusOK StatusCode = "ok" @@ -39,20 +40,24 @@ const ( ) var ( + // Test: blank var interface check _ fmt.Stringer = (*Server)(nil) _ Reader = (*Server)(nil) _ Writer = (*Client)(nil) GlobalPublic = "public" + // Test: custom type grouping in var block DefaultStatus StatusCode = "default" ErrorStatus StatusCode = "error" - globalZ = 10 - globalA = 5 - globalMiddle = 7 - globalB = 3 - singleConst = 1 + // Test: vars should be merged and sorted + globalZ = 10 + globalA = 5 + globalMiddle = 7 + globalB = 3 + singleConst = 1 + // Test: slice of anonymous structs with positional literals sliceOfStructs = []struct { content string path string From 74b648a6c2ca3ea623835ef89d486806bc31cfd3 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:48:32 +0300 Subject: [PATCH 4/9] fix: preserve inline comments on single-spec declarations during merge Transfer GenDecl.Decs.End (inline comments) to the last spec's Decs.End. Work around DST limitation where the last spec's End comment in a parenthesized block gets rendered after the closing paren by moving it to Start via fixLastSpecEndComment. Signed-off-by: Ilya Lesikov --- pkg/formatter/comment_loss_ai_test.go | 4 +++ pkg/formatter/declarations.go | 17 +++++++++++- .../testdata/inline_comments_expected.go | 24 +++++++++++++++++ .../testdata/inline_comments_input.go | 26 +++++++++++++++++++ 4 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 pkg/formatter/testdata/inline_comments_expected.go create mode 100644 pkg/formatter/testdata/inline_comments_input.go diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index 602929c..15bf20f 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -17,6 +17,10 @@ func TestAI_CommentPreservationDuringMerge(t *testing.T) { runFormatterTest(t, "comment_loss") } +func TestAI_InlineCommentAttachmentDuringReorder(t *testing.T) { + runFormatterTest(t, "inline_comments") +} + func runFormatterTest(t *testing.T, name string) { t.Helper() diff --git a/pkg/formatter/declarations.go b/pkg/formatter/declarations.go index d7c7368..97bbcd2 100644 --- a/pkg/formatter/declarations.go +++ b/pkg/formatter/declarations.go @@ -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 @@ -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 } diff --git a/pkg/formatter/testdata/inline_comments_expected.go b/pkg/formatter/testdata/inline_comments_expected.go new file mode 100644 index 0000000..8be8f6d --- /dev/null +++ b/pkg/formatter/testdata/inline_comments_expected.go @@ -0,0 +1,24 @@ +package main + +const ( + AlphaConst = "alpha" // alpha inline comment + AnotherSingle = "another" // another inline comment + BetaConst = "beta" // beta inline comment + SingleWithComment = "single" // single inline comment + ZetaConst = "zeta" // zeta inline comment + + // doc comment for TypedA + TypedA MyType = "a" // typed-a inline + // doc comment for TypedZ + TypedZ MyType = "z" +) + +var ( + varZ = 1 // var-z inline + // var-a inline + varA = 2 +) + +type MyType string + +func main() {} diff --git a/pkg/formatter/testdata/inline_comments_input.go b/pkg/formatter/testdata/inline_comments_input.go new file mode 100644 index 0000000..867bc2f --- /dev/null +++ b/pkg/formatter/testdata/inline_comments_input.go @@ -0,0 +1,26 @@ +package main + +const ( + ZetaConst = "zeta" // zeta inline comment + AlphaConst = "alpha" // alpha inline comment + BetaConst = "beta" // beta inline comment +) + +const ( + // doc comment for TypedZ + TypedZ MyType = "z" + // doc comment for TypedA + TypedA MyType = "a" // typed-a inline +) + +const SingleWithComment = "single" // single inline comment +const AnotherSingle = "another" // another inline comment + +var ( + varZ = 1 // var-z inline + varA = 2 // var-a inline +) + +type MyType string + +func main() {} From 92df72cbdcca55716803a088f914392bc51c38ce Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:49:35 +0300 Subject: [PATCH 5/9] fix: detect free-floating comments and return error before reordering Free-floating comments (section headers separated from code by a blank line) on var/const declarations now cause FormatFile to return an error instead of silently corrupting the comment position during merge. Signed-off-by: Ilya Lesikov --- README.md | 4 ++++ pkg/formatter/comment_loss_ai_test.go | 22 ++++++++++++++++++++++ pkg/formatter/file.go | 19 +++++++++++++++++++ pkg/formatter/helpers.go | 4 ++++ 4 files changed, 49 insertions(+) diff --git a/README.md b/README.md index c1fd964..abd80b0 100644 --- a/README.md +++ b/README.md @@ -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: diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index 15bf20f..a5cb8f8 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -17,6 +17,28 @@ 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_InlineCommentAttachmentDuringReorder(t *testing.T) { runFormatterTest(t, "inline_comments") } diff --git a/pkg/formatter/file.go b/pkg/formatter/file.go index 4baeff5..1733386 100644 --- a/pkg/formatter/file.go +++ b/pkg/formatter/file.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" + "github.com/dave/dst" "github.com/dave/dst/decorator" "mvdan.cc/gofumpt/format" ) @@ -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) @@ -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 { diff --git a/pkg/formatter/helpers.go b/pkg/formatter/helpers.go index 919d6b2..08639e3 100644 --- a/pkg/formatter/helpers.go +++ b/pkg/formatter/helpers.go @@ -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])) } From b1c26bd9369c99550b7ce0fe6c35319e59a2dcc7 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:50:18 +0300 Subject: [PATCH 6/9] fix: only reorder struct fields in named type declarations Anonymous structs (table tests, inline struct fields, composite literal types) are no longer reordered. reorderStructFields now targets *dst.TypeSpec nodes containing *dst.StructType, skipping all anonymous struct types to preserve table test readability. Signed-off-by: Ilya Lesikov --- README.md | 4 ++- pkg/formatter/comment_loss_ai_test.go | 4 +++ pkg/formatter/structs.go | 6 +++- .../testdata/anon_struct_expected.go | 29 +++++++++++++++++++ pkg/formatter/testdata/anon_struct_input.go | 28 ++++++++++++++++++ pkg/formatter/testdata/expected.go | 10 +++---- 6 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 pkg/formatter/testdata/anon_struct_expected.go create mode 100644 pkg/formatter/testdata/anon_struct_input.go diff --git a/README.md b/README.md index abd80b0..77c1f79 100644 --- a/README.md +++ b/README.md @@ -235,7 +235,7 @@ 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 | |-------|-------|---------| @@ -243,6 +243,8 @@ Fields are grouped (separated by empty lines): | 2 | Public | Alphabetically | | 3 | Private | Alphabetically | +Anonymous structs (table tests, inline struct fields, composite literal types) are **not** reordered. + **Struct literals** with named fields are reordered to match the struct definition.
diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index a5cb8f8..7c9c85c 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -39,6 +39,10 @@ func TestAI_NormalDocCommentNoError(t *testing.T) { require.NoError(t, formatter.FormatFile(actualPath, formatter.Options{})) } +func TestAI_AnonymousStructFieldsNotReordered(t *testing.T) { + runFormatterTest(t, "anon_struct") +} + func TestAI_InlineCommentAttachmentDuringReorder(t *testing.T) { runFormatterTest(t, "inline_comments") } diff --git a/pkg/formatter/structs.go b/pkg/formatter/structs.go index 8534fa4..c64c436 100644 --- a/pkg/formatter/structs.go +++ b/pkg/formatter/structs.go @@ -8,7 +8,11 @@ import ( func reorderStructFields(f *dst.File) { dst.Inspect(f, func(n dst.Node) bool { - if st, ok := n.(*dst.StructType); ok { + ts, ok := n.(*dst.TypeSpec) + if !ok { + return true + } + if st, ok := ts.Type.(*dst.StructType); ok { reorderFields(st) } diff --git a/pkg/formatter/testdata/anon_struct_expected.go b/pkg/formatter/testdata/anon_struct_expected.go new file mode 100644 index 0000000..d65f627 --- /dev/null +++ b/pkg/formatter/testdata/anon_struct_expected.go @@ -0,0 +1,29 @@ +package main + +import "fmt" + +type Named struct { + Alpha string + Zulu string + + bravo int + yankee int +} + +func main() { + tests := []struct { + name string + input int + expected string + }{ + {name: "first", input: 1, expected: "one"}, + {name: "second", input: 2, expected: "two"}, + } + + anon := struct { + Zebra string + Apple string + }{Zebra: "z", Apple: "a"} + + fmt.Println(tests, anon) +} diff --git a/pkg/formatter/testdata/anon_struct_input.go b/pkg/formatter/testdata/anon_struct_input.go new file mode 100644 index 0000000..e381e6f --- /dev/null +++ b/pkg/formatter/testdata/anon_struct_input.go @@ -0,0 +1,28 @@ +package main + +import "fmt" + +type Named struct { + Zulu string + Alpha string + bravo int + yankee int +} + +func main() { + tests := []struct { + name string + input int + expected string + }{ + {name: "first", input: 1, expected: "one"}, + {name: "second", input: 2, expected: "two"}, + } + + anon := struct { + Zebra string + Apple string + }{Zebra: "z", Apple: "a"} + + fmt.Println(tests, anon) +} diff --git a/pkg/formatter/testdata/expected.go b/pkg/formatter/testdata/expected.go index a44359d..7df451d 100644 --- a/pkg/formatter/testdata/expected.go +++ b/pkg/formatter/testdata/expected.go @@ -59,11 +59,11 @@ var ( singleConst = 1 // Test: slice of anonymous structs with positional literals sliceOfStructs = []struct { - content string path string + content string }{ - {content: "content1", path: filepath.Join("a", "b")}, - {content: "content2", path: filepath.Join("c", "d")}, + {path: filepath.Join("a", "b"), content: "content1"}, + {path: filepath.Join("c", "d"), content: "content2"}, } ) @@ -263,9 +263,9 @@ func ProcessDataPublic(data string) string { // Test: anonymous struct with positional literal func createAnonymous() interface{} { return struct { - A string B int - }{A: "hello", B: 42} + A string + }{B: 42, A: "hello"} } // Test: empty literal - no change From b34cd8e198611b849f7015a3a20f772dee11f029 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:51:13 +0300 Subject: [PATCH 7/9] fix: skip struct field reordering for structs with encoding tags Structs with json, yaml, xml, toml, or protobuf struct tags now preserve their original field order to prevent changing wire format. Signed-off-by: Ilya Lesikov --- README.md | 2 +- pkg/formatter/comment_loss_ai_test.go | 4 +++ pkg/formatter/structs.go | 29 +++++++++++++++++-- .../testdata/encoding_tags_expected.go | 29 +++++++++++++++++++ pkg/formatter/testdata/encoding_tags_input.go | 27 +++++++++++++++++ 5 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 pkg/formatter/testdata/encoding_tags_expected.go create mode 100644 pkg/formatter/testdata/encoding_tags_input.go diff --git a/README.md b/README.md index 77c1f79..8ba9969 100644 --- a/README.md +++ b/README.md @@ -243,7 +243,7 @@ Fields in **named type declarations** (`type Foo struct{...}`) are grouped (sepa | 2 | Public | Alphabetically | | 3 | Private | Alphabetically | -Anonymous structs (table tests, inline struct fields, composite literal types) are **not** reordered. +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. diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index 7c9c85c..8700503 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -39,6 +39,10 @@ func TestAI_NormalDocCommentNoError(t *testing.T) { require.NoError(t, formatter.FormatFile(actualPath, formatter.Options{})) } +func TestAI_EncodingTaggedStructFieldsNotReordered(t *testing.T) { + runFormatterTest(t, "encoding_tags") +} + func TestAI_AnonymousStructFieldsNotReordered(t *testing.T) { runFormatterTest(t, "anon_struct") } diff --git a/pkg/formatter/structs.go b/pkg/formatter/structs.go index c64c436..50f3a6f 100644 --- a/pkg/formatter/structs.go +++ b/pkg/formatter/structs.go @@ -1,7 +1,9 @@ package formatter import ( + "reflect" "sort" + "strings" "github.com/dave/dst" ) @@ -12,14 +14,37 @@ func reorderStructFields(f *dst.File) { if !ok { return true } - if st, ok := ts.Type.(*dst.StructType); ok { - reorderFields(st) + st, ok := ts.Type.(*dst.StructType) + if !ok || hasEncodingTags(st) { + return true } + reorderFields(st) return true }) } +var encodingTagKeys = []string{"json", "yaml", "xml", "toml", "protobuf"} + +func hasEncodingTags(st *dst.StructType) bool { + if st.Fields == nil { + return false + } + for _, field := range st.Fields.List { + if field.Tag == nil { + continue + } + tag := reflect.StructTag(strings.Trim(field.Tag.Value, "`")) + for _, key := range encodingTagKeys { + if _, ok := tag.Lookup(key); ok { + return true + } + } + } + + return false +} + func collectStructDefinitions(f *dst.File) map[string][]string { structDefs := make(map[string][]string) diff --git a/pkg/formatter/testdata/encoding_tags_expected.go b/pkg/formatter/testdata/encoding_tags_expected.go new file mode 100644 index 0000000..87f79a1 --- /dev/null +++ b/pkg/formatter/testdata/encoding_tags_expected.go @@ -0,0 +1,29 @@ +package main + +type Operation struct { + Type string `json:"type"` + Name string `json:"name"` + Action string `json:"action"` +} + +type Plan struct { + Version int `yaml:"version"` + Name string `yaml:"name"` + Steps []*Operation `yaml:"steps"` +} + +type NoTags struct { + Alpha string + Zulu string + + bravo int +} + +type WithValidate struct { + Alpha string `validate:"required"` + Zulu string `validate:"required"` + + bravo int +} + +func main() {} diff --git a/pkg/formatter/testdata/encoding_tags_input.go b/pkg/formatter/testdata/encoding_tags_input.go new file mode 100644 index 0000000..2c514a4 --- /dev/null +++ b/pkg/formatter/testdata/encoding_tags_input.go @@ -0,0 +1,27 @@ +package main + +type Operation struct { + Type string `json:"type"` + Name string `json:"name"` + Action string `json:"action"` +} + +type Plan struct { + Version int `yaml:"version"` + Name string `yaml:"name"` + Steps []*Operation `yaml:"steps"` +} + +type NoTags struct { + Zulu string + Alpha string + bravo int +} + +type WithValidate struct { + Zulu string `validate:"required"` + Alpha string `validate:"required"` + bravo int +} + +func main() {} From fe791af5dc4cc405a92398d1ab0a5ca08b5bc68e Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:51:52 +0300 Subject: [PATCH 8/9] fix: remove spurious blank lines between struct fields assembleFieldList now unconditionally resets both Decs.Before and Decs.After for all fields, preventing stale EmptyLine decorations from the original source from leaking through. Signed-off-by: Ilya Lesikov --- pkg/formatter/comment_loss_ai_test.go | 4 ++++ pkg/formatter/structs.go | 23 +++++++++---------- .../testdata/struct_spacing_expected.go | 18 +++++++++++++++ .../testdata/struct_spacing_input.go | 23 +++++++++++++++++++ 4 files changed, 56 insertions(+), 12 deletions(-) create mode 100644 pkg/formatter/testdata/struct_spacing_expected.go create mode 100644 pkg/formatter/testdata/struct_spacing_input.go diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index 8700503..8e69742 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -39,6 +39,10 @@ func TestAI_NormalDocCommentNoError(t *testing.T) { require.NoError(t, formatter.FormatFile(actualPath, formatter.Options{})) } +func TestAI_NoSpuriousBlankLinesInStructs(t *testing.T) { + runFormatterTest(t, "struct_spacing") +} + func TestAI_EncodingTaggedStructFieldsNotReordered(t *testing.T) { runFormatterTest(t, "encoding_tags") } diff --git a/pkg/formatter/structs.go b/pkg/formatter/structs.go index 50f3a6f..dca1d35 100644 --- a/pkg/formatter/structs.go +++ b/pkg/formatter/structs.go @@ -201,25 +201,24 @@ func assembleFieldList(embedded, public, private []*dst.Field) []*dst.Field { for _, f := range embedded { f.Decs.Before = dst.NewLine + f.Decs.After = dst.None result = append(result, f) } - if len(public) > 0 && len(embedded) > 0 { - public[0].Decs.Before = dst.EmptyLine - } - for _, f := range public { - if f.Decs.Before != dst.EmptyLine { - f.Decs.Before = dst.NewLine + for i, f := range public { + f.Decs.Before = dst.NewLine + f.Decs.After = dst.None + if i == 0 && len(embedded) > 0 { + f.Decs.Before = dst.EmptyLine } result = append(result, f) } - if len(private) > 0 && (len(embedded) > 0 || len(public) > 0) { - private[0].Decs.Before = dst.EmptyLine - } - for _, f := range private { - if f.Decs.Before != dst.EmptyLine { - f.Decs.Before = dst.NewLine + for i, f := range private { + f.Decs.Before = dst.NewLine + f.Decs.After = dst.None + if i == 0 && (len(embedded) > 0 || len(public) > 0) { + f.Decs.Before = dst.EmptyLine } result = append(result, f) } diff --git a/pkg/formatter/testdata/struct_spacing_expected.go b/pkg/formatter/testdata/struct_spacing_expected.go new file mode 100644 index 0000000..4cb95c3 --- /dev/null +++ b/pkg/formatter/testdata/struct_spacing_expected.go @@ -0,0 +1,18 @@ +package main + +type tablesBuilder struct { + discoveryClient string + dynamicClient string + logStore string + maxLogEventTableWidth int +} + +type KubeClient struct { + NoActivityTimeout int + Ownership string + + name string + port int +} + +func main() {} diff --git a/pkg/formatter/testdata/struct_spacing_input.go b/pkg/formatter/testdata/struct_spacing_input.go new file mode 100644 index 0000000..e22b4f2 --- /dev/null +++ b/pkg/formatter/testdata/struct_spacing_input.go @@ -0,0 +1,23 @@ +package main + +type tablesBuilder struct { + logStore string + + maxLogEventTableWidth int + + discoveryClient string + + dynamicClient string +} + +type KubeClient struct { + NoActivityTimeout int + + Ownership string + + name string + + port int +} + +func main() {} From 553dfc75748bd444e28bae9c5ecd01e9a35226e5 Mon Sep 17 00:00:00 2001 From: Ilya Lesikov Date: Fri, 20 Feb 2026 15:53:05 +0300 Subject: [PATCH 9/9] fix: preserve original order of typed string constants Typed const specs (Stage, ReleaseType, etc.) now preserve their original relative order within the same type group. Only untyped consts are alphabetized, keeping intentional orderings intact. Signed-off-by: Ilya Lesikov --- README.md | 4 +- TODO.md | 41 ++++++++----------- pkg/formatter/collector.go | 2 +- pkg/formatter/comment_loss_ai_test.go | 4 ++ pkg/formatter/sorting.go | 22 ++++++++++ pkg/formatter/testdata/expected.go | 2 +- .../testdata/inline_comments_expected.go | 5 ++- .../testdata/typed_consts_expected.go | 24 +++++++++++ pkg/formatter/testdata/typed_consts_input.go | 26 ++++++++++++ 9 files changed, 99 insertions(+), 31 deletions(-) create mode 100644 pkg/formatter/testdata/typed_consts_expected.go create mode 100644 pkg/formatter/testdata/typed_consts_input.go diff --git a/README.md b/README.md index 8ba9969..c7ea8e5 100644 --- a/README.md +++ b/README.md @@ -122,8 +122,8 @@ func main() {} | 3 | Private (lowercase) | By custom type | **Within each group:** -- **Constants:** sorted alphabetically, no empty lines. -- **Variables:** original order preserved (to avoid breaking initialization dependencies), no empty lines. +- **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).
Example diff --git a/TODO.md b/TODO.md index 8d6d664..2e39c64 100644 --- a/TODO.md +++ b/TODO.md @@ -6,46 +6,37 @@ Issues discovered by formatting the [nelm](https://github.com/werf/nelm) codebas ## Critical -### 1. Variable reordering breaks initialization dependencies (`pkg/featgate/feat.go`) -**Issue**: `FeatGates = []*FeatGate{}` moved to end of `var` block, overwriting populated slice (filled by `NewFeatGate` calls) with empty one. -**Fix**: Don't sort var specs by name at all. Only group by exportability (`_` first, exported, unexported) and preserve original relative order within each group. Dependency analysis via AST is unreliable (can't detect indirect dependencies through function calls), and alphabetical sorting can break initialization order. +### ~~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`) (`pkg/common/common.go`) -**Issue**: Comment `// ChartTSEntryPoints defines supported TypeScript/JavaScript entry points (in priority order).` lost during var merge. Variable `ChartTSEntryPoints` was moved to a merged block, but the comment was detached. -**Fix**: When extracting specs from `GenDecl` in `collectGenDecl` (both `token.CONST` and `token.VAR` cases), transfer `GenDecl.Decs.Start` (doc comments) to the first `ValueSpec.Decs.Start` before discarding the `GenDecl`. This ensures comments travel with the spec during merge/sort. For multi-spec blocks, only transfer to the first spec (inner comments are already on the spec). +### ~~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 -**Issue**: Wormatter reorders `const` specs. We need tests ensuring inline comments remain attached to the correct spec after any reordering. -**Evidence**: `internal/plan/release_info.go` shows const spec reordering; comments still look correct there, but this should be covered by tests to prevent regressions. -**Fix**: No code change needed — DST decorations on `ValueSpec` (including inline `End` comments) travel with the node during `sort.SliceStable`. Add regression tests to verify inline comments remain attached to the correct spec after reordering. +### ~~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 -**Issue**: Need to verify that free-floating comments (not clearly attached to any decl/spec/block) are preserved and remain correctly positioned when wormatter merges/reorders declarations. -**Fix**: Detect free-floating comments (section headers separated from code by a blank line) before reordering. In DST, these are identifiable by a trailing `"\n"` entry in `Decs.Start`. If any top-level declaration has such a detached comment, return an error for that file ("file has free-floating comments, cannot safely reorder declarations"). Add tests for this detection and for normal doc comments (no trailing `"\n"`) being preserved correctly. +### ~~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 -**Issue**: `name` field moved to end of test structs (alphabetical sort), making table tests unreadable. -**Fix**: Only reorder struct fields in named type declarations (`type Foo struct{...}`). Change `reorderStructFields` to look for `*dst.TypeSpec` nodes and reorder the `*dst.StructType` inside them, instead of targeting all `*dst.StructType` nodes indiscriminately. This skips anonymous structs (table tests, inline struct fields, composite literal types). +### ~~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 -**Issue**: Struct field order changes JSON output order in Go; wormatter reorders json-tagged fields in exported types (e.g. `internal/plan/operation.go`, `internal/plan/plan.go`). -**Fix**: If any field in a struct has an encoding-related struct tag (`json`, `yaml`, `xml`, `toml`, `protobuf`), skip field reordering for that entire struct (preserve original field order). Don't mix sorted and unsorted fields — it creates confusing output. Non-encoding tags (`validate`, `db`, etc.) don't trigger the skip. +### ~~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 -**Issue**: Blank lines inserted between fields in both public and private structs (e.g., `NoActivityTimeout` / `Ownership` in public structs; `logStore` / `maxLogEventTableWidth` in private `tablesBuilder` struct in `internal/track/progress_tables.go`; `discoveryClient` / `dynamicClient` in private `KubeClient` struct in `internal/kube/client_kube.go`). -**Fix**: The grouping logic in `assembleFieldList` is correct (only sets `EmptyLine` at group boundaries), but the loops preserve stale `EmptyLine` decorations from the original source via a `if f.Decs.Before != dst.EmptyLine` guard. Fix: unconditionally set `Decs.Before = dst.NewLine` for all fields within a group, then set `EmptyLine` only on the first field of each new group. +### ~~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 -**Issue**: Typed string constant blocks that are intentionally ordered get reordered alphabetically (e.g. stage ordering in `pkg/common/common.go`, and previously observed `ReleaseType` constants). -**Fix**: Create a const-specific sort function (separate from vars). Sort const specs by: exportability → type name → name **only for untyped consts** (empty type). Typed consts (`Stage`, `ReleaseType`, etc.) preserve their original relative order within the same type group — `sort.SliceStable` handles this naturally by returning `false` for same-type comparisons. This keeps intentional orderings (pipeline stages, priorities) intact while still alphabetizing untyped consts. +### ~~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. diff --git a/pkg/formatter/collector.go b/pkg/formatter/collector.go index e7dd269..740c3da 100644 --- a/pkg/formatter/collector.go +++ b/pkg/formatter/collector.go @@ -120,7 +120,7 @@ func (c *declCollector) collectTypeNames(f *dst.File) { } func (c *declCollector) sort() { - sortSpecsByExportabilityThenName(c.constSpecs) + sortConstSpecsByExportabilityThenName(c.constSpecs) sortVarSpecsByExportability(c.varSpecs) for typeName := range c.constructors { diff --git a/pkg/formatter/comment_loss_ai_test.go b/pkg/formatter/comment_loss_ai_test.go index 8e69742..bf2f17e 100644 --- a/pkg/formatter/comment_loss_ai_test.go +++ b/pkg/formatter/comment_loss_ai_test.go @@ -39,6 +39,10 @@ func TestAI_NormalDocCommentNoError(t *testing.T) { 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") } diff --git a/pkg/formatter/sorting.go b/pkg/formatter/sorting.go index 19d494d..b793d2b 100644 --- a/pkg/formatter/sorting.go +++ b/pkg/formatter/sorting.go @@ -49,6 +49,28 @@ 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])) diff --git a/pkg/formatter/testdata/expected.go b/pkg/formatter/testdata/expected.go index 7df451d..92f918e 100644 --- a/pkg/formatter/testdata/expected.go +++ b/pkg/formatter/testdata/expected.go @@ -25,8 +25,8 @@ const ( // Test: consts should be merged and sorted ConstZ = "z" - StatusError StatusCode = "error" StatusOK StatusCode = "ok" + StatusError StatusCode = "error" StatusPending StatusCode = "pending" constPrivate = "private" diff --git a/pkg/formatter/testdata/inline_comments_expected.go b/pkg/formatter/testdata/inline_comments_expected.go index 8be8f6d..a76fd88 100644 --- a/pkg/formatter/testdata/inline_comments_expected.go +++ b/pkg/formatter/testdata/inline_comments_expected.go @@ -7,10 +7,11 @@ const ( SingleWithComment = "single" // single inline comment ZetaConst = "zeta" // zeta inline comment - // doc comment for TypedA - TypedA MyType = "a" // typed-a inline // doc comment for TypedZ TypedZ MyType = "z" + // doc comment for TypedA + // typed-a inline + TypedA MyType = "a" ) var ( diff --git a/pkg/formatter/testdata/typed_consts_expected.go b/pkg/formatter/testdata/typed_consts_expected.go new file mode 100644 index 0000000..992c606 --- /dev/null +++ b/pkg/formatter/testdata/typed_consts_expected.go @@ -0,0 +1,24 @@ +package main + +const ( + UntypedA = "a" + UntypedM = "m" + UntypedZ = "z" + + ReleaseTypeMajor ReleaseType = "major" + ReleaseTypeMinor ReleaseType = "minor" + ReleaseTypePatch ReleaseType = "patch" + + StagePreInstall Stage = "pre-install" + StageInstall Stage = "install" + StagePostInstall Stage = "post-install" + + untypedPrivateA = "a" + untypedPrivateZ = "z" +) + +type Stage string + +type ReleaseType string + +func main() {} diff --git a/pkg/formatter/testdata/typed_consts_input.go b/pkg/formatter/testdata/typed_consts_input.go new file mode 100644 index 0000000..a50f85b --- /dev/null +++ b/pkg/formatter/testdata/typed_consts_input.go @@ -0,0 +1,26 @@ +package main + +type Stage string + +const ( + StagePreInstall Stage = "pre-install" + StageInstall Stage = "install" + StagePostInstall Stage = "post-install" +) + +type ReleaseType string + +const ( + ReleaseTypeMajor ReleaseType = "major" + ReleaseTypeMinor ReleaseType = "minor" + ReleaseTypePatch ReleaseType = "patch" +) + +const UntypedZ = "z" +const UntypedA = "a" +const UntypedM = "m" + +const untypedPrivateZ = "z" +const untypedPrivateA = "a" + +func main() {}