Skip to content

Preserve orphan comments (e.g. kubebuilder scaffold marker) inside import blocks - #248

Open
devthedevil wants to merge 3 commits into
daixiang0:masterfrom
devthedevil:fix/kubebuilder-scaffold-comment
Open

Preserve orphan comments (e.g. kubebuilder scaffold marker) inside import blocks#248
devthedevil wants to merge 3 commits into
daixiang0:masterfrom
devthedevil:fix/kubebuilder-scaffold-comment

Conversation

@devthedevil

Copy link
Copy Markdown

Summary

Fixes #135. Since PR #120 ("optimize detect import blocks"), gci silently drops any comment inside an import block that isn't attached (as Doc or trailing Comment) to a specific *ast.ImportSpec. That PR's own description acknowledged this as a known breaking change ("introduce breaking change: remove isolated comments").

The most common way to hit this is a standalone marker comment such as:

import (
	"os"
	"fmt"
	// +kubebuilder:scaffold:imports
)

kubebuilder/controller-gen relies on that comment being present at that exact position to know where to insert future imports. Running gci on such a file removes the marker entirely, which breaks the codegen workflow.

Root cause

pkg/parse/parse.go's ParseFile only records byte ranges for import specs (Start/End derived from each spec's Doc/Name/Path/Comment). pkg/gci/gci.go's LoadFormat rebuilds the import block purely by concatenating those recorded byte ranges, so any comment that isn't formally attached to a spec never makes it into the rewritten output.

Change

  • pkg/parse/parse.go: added findTailComment, which scans f.Comments for a comment group that sits after the last recognized import spec but before the import block's closing paren (i.e. not attached to any spec). ParseFile now additionally returns that comment's byte range (-1, -1 if none exists).
  • pkg/gci/gci.go: LoadFormat takes the two new return values and, right before appending the closing ), re-inserts the orphan comment's original bytes so it survives the rewrite.
  • pkg/gci/testdata.go: added a trailing-marker-comment test case reproducing the exact example from the issue.

The change is intentionally scoped to the trailing-comment case described in the issue (a comment between the last import and the closing paren). A related but distinct case mentioned in the issue discussion — a leading orphan comment ending up merged onto the import ( line — is not addressed here to keep this change minimal and focused.

Testing

Added trailing-marker-comment to pkg/gci/testdata.go's testCases, which is exercised by the existing TestRun in pkg/gci/gci_test.go:

in: `package main

import (
	"os"
	"fmt"
	// +kubebuilder:scaffold:imports
)
`,
out: `package main

import (
	"fmt"
	"os"
	// +kubebuilder:scaffold:imports
)
`,

I don't have a Go toolchain available in the environment I drafted this in, so I hand-traced the AST byte offsets against this reproduction case and against the existing fixtures (multi-block imports, comments outside the import block, cgo blocks, etc.) to check for regressions, but please run CI/go test ./... before merging.

Closes #135

A comment that sits on its own line inside an import block but isn't
attached (as Doc or trailing Comment) to any *ast.ImportSpec was being
silently dropped when the block was rewritten. This breaks tools like
kubebuilder/controller-gen that rely on a standalone marker comment
(e.g. `// +kubebuilder:scaffold:imports`) to know where to insert
future imports.

ParseFile now also returns the byte range of any such orphan comment
found between the last recognized import spec and the closing paren.
LoadFormat now receives the orphan-comment byte range from
parse.ParseFile and re-inserts it right before the closing paren of
the rewritten import block, so standalone comments (e.g. a trailing
`// +kubebuilder:scaffold:imports` marker) are no longer dropped.
Reproduces the kubebuilder scaffold marker case from daixiang0#135: a comment
on its own line after the last import but before the closing paren
must survive formatting instead of being dropped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix removal of kubebuilder imports marker

1 participant