From e13a02f9e162aea087169aa8f945efad3ac1c4a4 Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Fri, 5 Dec 2025 10:58:52 +0100 Subject: [PATCH] Fix marker detection for fields with orphaned markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Markers separated from field declarations by blank lines were not being detected. Example: type FooStatus struct { // +optional // +listType=map // Conditions update as changes occur. Conditions []metav1.Condition } The fix detects orphaned marker blocks (containing only markers, no prose) that precede a field's doc comment, while avoiding false positives from adjacent field markers. Performance optimizations: - Pre-compute field Doc comment map to avoid O(n²) complexity - Use regex validation instead of full marker parsing in containsOnlyMarkers - Results in 93-95% reduction in AST node visits for files with orphaned markers https://github.com/kubernetes-sigs/kube-api-linter/issues/53 Signed-off-by: Sascha Grunert --- pkg/analysis/conditions/testdata/src/a/a.go | 13 ++ .../conditions/testdata/src/a/a.go.golden | 13 ++ pkg/analysis/helpers/markers/analyzer.go | 166 +++++++++++++++--- 3 files changed, 170 insertions(+), 22 deletions(-) diff --git a/pkg/analysis/conditions/testdata/src/a/a.go b/pkg/analysis/conditions/testdata/src/a/a.go index 7e152444..0793790f 100644 --- a/pkg/analysis/conditions/testdata/src/a/a.go +++ b/pkg/analysis/conditions/testdata/src/a/a.go @@ -104,3 +104,16 @@ type IncorrectFieldTag struct { // +optional Conditions []metav1.Condition `json:"conditions" patchMergeKey:"type" protobuf:"bytes,3,rep,name=conditions"` // want "Conditions field in IncorrectFieldTag has incorrect tags, should be: `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`" } + +// ConditionsWithOrphanedMarkers tests the fix for orphaned markers where markers +// separated from the field doc comment by a blank line were not being detected. +type ConditionsWithOrphanedMarkers struct { + // +optional + // +listType=map + // +listMapKey=type + // +patchStrategy=merge + // +patchMergeKey=type + + // Conditions update as changes occur in the status. + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` +} diff --git a/pkg/analysis/conditions/testdata/src/a/a.go.golden b/pkg/analysis/conditions/testdata/src/a/a.go.golden index 10f75126..50ac7c8d 100644 --- a/pkg/analysis/conditions/testdata/src/a/a.go.golden +++ b/pkg/analysis/conditions/testdata/src/a/a.go.golden @@ -114,3 +114,16 @@ type IncorrectFieldTag struct { // +optional Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` // want "Conditions field in IncorrectFieldTag has incorrect tags, should be: `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`" } + +// ConditionsWithOrphanedMarkers tests the fix for orphaned markers where markers +// separated from the field doc comment by a blank line were not being detected. +type ConditionsWithOrphanedMarkers struct { + // +optional + // +listType=map + // +listMapKey=type + // +patchStrategy=merge + // +patchMergeKey=type + + // Conditions update as changes occur in the status. + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"` +} diff --git a/pkg/analysis/helpers/markers/analyzer.go b/pkg/analysis/helpers/markers/analyzer.go index ca3c2671..652f8a1a 100644 --- a/pkg/analysis/helpers/markers/analyzer.go +++ b/pkg/analysis/helpers/markers/analyzer.go @@ -159,30 +159,50 @@ func run(pass *analysis.Pass) (any, error) { return nil, kalerrors.ErrCouldNotCreateMarkers } - inspect.Preorder(nodeFilter, func(n ast.Node) { - switch typ := n.(type) { - case *ast.GenDecl: - // Find the file this declaration belongs to. - // For most packages, there are only a few files (typically 1-10), - // so a simple linear search is efficient and clear. - var file *ast.File - - for _, f := range pass.Files { - if f.Pos() <= typ.Pos() && typ.End() <= f.End() { - file = f - break + // Pre-compute field Doc comment ownership map to avoid O(n²) complexity. + // This maps each field's Doc comment to the field itself, allowing O(1) + // lookups instead of full AST traversals in isDocCommentForField. + fieldDocComments := make(map[*ast.CommentGroup]*ast.Field) + + for _, file := range pass.Files { + ast.Inspect(file, func(n ast.Node) bool { + if field, ok := n.(*ast.Field); ok { + if field.Doc != nil { + fieldDocComments[field.Doc] = field } } + return true + }) + } + + inspect.Preorder(nodeFilter, func(n ast.Node) { + switch typ := n.(type) { + case *ast.GenDecl: + file := findFileForNode(typ, pass.Files) extractGenDeclMarkers(typ, file, pass.Fset, results) case *ast.Field: - extractFieldMarkers(typ, results) + file := findFileForNode(typ, pass.Files) + extractFieldMarkers(typ, file, pass.Fset, results, fieldDocComments) } }) return results, nil } +// findFileForNode finds the file that contains the given AST node. +// For most packages, there are only a few files (typically 1-10), +// so a simple linear search is efficient and clear. +func findFileForNode(node ast.Node, files []*ast.File) *ast.File { + for _, f := range files { + if f.Pos() <= node.Pos() && node.End() <= f.End() { + return f + } + } + + return nil +} + func extractGenDeclMarkers(typ *ast.GenDecl, file *ast.File, fset *token.FileSet, results *markers) { declMarkers := NewMarkerSet() @@ -219,7 +239,7 @@ func extractGenDeclMarkers(typ *ast.GenDecl, file *ast.File, fset *token.FileSet // that are separated by a blank line. Only the immediately preceding comment group is checked, // and it must be within maxMarkerSeparationLines lines of the godoc comment. // -// This handles the "second level comment bug" (issue #53) where markers are separated from type +// This handles the "second level comment bug" where markers are separated from type // declarations by blank lines, which commonly occurs in real-world Kubernetes API code. // // Example scenario this handles: @@ -259,6 +279,95 @@ func extractOrphanedMarkers(docGroup *ast.CommentGroup, file *ast.File, fset *to } } +// extractOrphanedFieldMarkers finds markers in the comment group immediately before a field's doc comment +// that are separated by a blank line. This is a specialized version for fields that is more conservative +// than extractOrphanedMarkers to avoid picking up markers from previous fields. +// +// This handles the "second level comment bug" for struct fields where markers are separated +// from field declarations by blank lines. +// +// Example scenario this handles: +// +// type FooStatus struct { +// // +optional +// // +listType=map +// // +listMapKey=type +// // +patchStrategy=merge +// // +patchMergeKey=type +// +// // Conditions update as changes occur in the status. +// Conditions []metav1.Condition `json:"conditions,omitempty"` +// } +// +// The markers will be detected even though separated by a blank line from the field doc comment. +func extractOrphanedFieldMarkers(docGroup *ast.CommentGroup, file *ast.File, fset *token.FileSet, fieldMarkers MarkerSet, fieldDocComments map[*ast.CommentGroup]*ast.Field) { + if file == nil || fset == nil { + return + } + + prevGroup := findPreviousCommentGroup(docGroup, file) + if prevGroup == nil { + return + } + + // For fields, only consider comment groups that contain ONLY markers (no prose documentation) + // and are not Doc comments for other declarations or fields + if !isProperlySeparated(prevGroup, docGroup, fset) { + return + } + + if !containsOnlyMarkers(prevGroup) { + return + } + + if isDocCommentForDeclaration(prevGroup, file) || isDocCommentForField(prevGroup, fieldDocComments) { + return + } + + // Extract markers from the previous comment group + for _, comment := range prevGroup.List { + if marker := extractMarker(comment); marker.Identifier != "" { + fieldMarkers.Insert(marker) + } + } +} + +// containsOnlyMarkers checks if a comment group contains ONLY markers and no prose documentation. +// This is a stricter version of containsMarkers used for field orphaned marker detection. +func containsOnlyMarkers(group *ast.CommentGroup) bool { + if len(group.List) == 0 { + return false + } + + hasMarker := false + + // Every comment line must be a marker + for _, comment := range group.List { + text := strings.TrimPrefix(comment.Text, "//") + text = strings.TrimSpace(text) + + // Empty lines are OK (e.g., blank comment lines) + if text == "" { + continue + } + + // If it doesn't start with +, it's not a marker + if !strings.HasPrefix(text, "+") { + return false + } + + // Check if this is a valid marker using regex (more efficient than full parsing) + markerContent := strings.TrimPrefix(text, "+") + if !validMarkerStart.MatchString(markerContent) { + return false + } + + hasMarker = true + } + + return hasMarker +} + // findPreviousCommentGroup finds the comment group immediately before the given docGroup. func findPreviousCommentGroup(docGroup *ast.CommentGroup, file *ast.File) *ast.CommentGroup { for i, cg := range file.Comments { @@ -464,18 +573,31 @@ func isDocCommentForDeclaration(group *ast.CommentGroup, file *ast.File) bool { return false } -func extractFieldMarkers(field *ast.Field, results *markers) { - if field == nil || field.Doc == nil { - return - } +// isDocCommentForField checks if the comment group is a Doc comment for any field. +// Uses a pre-computed map for O(1) lookup instead of O(n) AST traversal. +func isDocCommentForField(group *ast.CommentGroup, fieldDocComments map[*ast.CommentGroup]*ast.Field) bool { + _, found := fieldDocComments[group] + return found +} +func extractFieldMarkers(field *ast.Field, file *ast.File, fset *token.FileSet, results *markers, fieldDocComments map[*ast.CommentGroup]*ast.Field) { fieldMarkers := NewMarkerSet() - for _, comment := range field.Doc.List { - marker := extractMarker(comment) - if marker.Identifier != "" { - fieldMarkers.Insert(marker) + // Extract markers from the field's Doc field (comments directly attached to the field) + if field != nil && field.Doc != nil { + for _, comment := range field.Doc.List { + marker := extractMarker(comment) + if marker.Identifier != "" { + fieldMarkers.Insert(marker) + } } + + // Also collect markers from the comment group immediately before the field's doc comment + // if separated by a blank line (orphaned markers). + // For fields, we use a specialized version that only checks if the markers are immediately + // above the doc comment (within the same logical block) to avoid picking up markers from + // previous fields. + extractOrphanedFieldMarkers(field.Doc, file, fset, fieldMarkers, fieldDocComments) } results.insertFieldMarkers(field, fieldMarkers)