Skip to content

Commit 96c80ca

Browse files
nayuta723nayuta723
committed
refactor(markerscope): fix linters and add test golden files
Signed-off-by: nayuta-ai <nayuta723@gmail.com>
1 parent 55e7885 commit 96c80ca

18 files changed

+1143
-185
lines changed

‎pkg/analysis/markerscope/analyzer.go‎

Lines changed: 85 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,12 @@ limitations under the License.
1616
package markerscope
1717

1818
import (
19+
"errors"
1920
"fmt"
2021
"go/ast"
2122
"go/types"
2223
"maps"
2324
"slices"
24-
"strings"
2525

2626
"golang.org/x/tools/go/analysis"
2727
"golang.org/x/tools/go/analysis/passes/inspect"
@@ -143,48 +143,12 @@ func (a *analyzer) checkFieldMarkers(pass *analysis.Pass, field *ast.Field, mark
143143

144144
// Check if FieldScope is allowed
145145
if !rule.Scope.Allows(FieldScope) {
146-
var message string
147-
148-
var fixes []analysis.SuggestedFix
149-
150-
if rule.Scope == TypeScope {
151-
message = fmt.Sprintf("marker %q can only be applied to types", marker.Identifier)
152-
153-
if a.policy == MarkerScopePolicySuggestFix {
154-
fixes = a.suggestMoveToFieldsIfCompatible(pass, field, marker, rule)
155-
}
156-
} else {
157-
// This shouldn't happen in practice, but handle it gracefully
158-
message = fmt.Sprintf("marker %q cannot be applied to fields", marker.Identifier)
159-
}
160-
161-
pass.Report(analysis.Diagnostic{
162-
Pos: marker.Pos,
163-
End: marker.End,
164-
Message: message,
165-
SuggestedFixes: fixes,
166-
})
167-
146+
a.reportFieldScopeViolation(pass, field, marker, rule)
168147
continue
169148
}
170149

171150
// Check type constraints if present
172-
if err := a.validateFieldTypeConstraint(pass, field, rule, a.allowDangerousTypes); err != nil {
173-
if a.policy == MarkerScopePolicySuggestFix {
174-
pass.Report(analysis.Diagnostic{
175-
Pos: marker.Pos,
176-
End: marker.End,
177-
Message: fmt.Sprintf("marker %q: %s", marker.Identifier, err),
178-
SuggestedFixes: a.suggestMoveToFieldsIfCompatible(pass, field, marker, rule),
179-
})
180-
} else {
181-
pass.Report(analysis.Diagnostic{
182-
Pos: marker.Pos,
183-
End: marker.End,
184-
Message: fmt.Sprintf("marker %q: %s", marker.Identifier, err),
185-
})
186-
}
187-
}
151+
a.checkFieldTypeConstraintViolation(pass, field, marker, rule)
188152
}
189153
}
190154

@@ -226,6 +190,67 @@ func (a *analyzer) checkSingleTypeMarkers(pass *analysis.Pass, typeSpec *ast.Typ
226190
}
227191
}
228192

193+
// reportFieldScopeViolation reports a scope violation for a field marker.
194+
func (a *analyzer) reportFieldScopeViolation(pass *analysis.Pass, field *ast.Field, marker markershelper.Marker, rule MarkerScopeRule) {
195+
var message string
196+
197+
var fixes []analysis.SuggestedFix
198+
199+
if rule.Scope == TypeScope {
200+
message = fmt.Sprintf("marker %q can only be applied to types", marker.Identifier)
201+
202+
if a.policy == MarkerScopePolicySuggestFix {
203+
fixes = a.suggestMoveToFieldsIfCompatible(pass, field, marker, rule)
204+
}
205+
} else {
206+
// This shouldn't happen in practice, but handle it gracefully
207+
message = fmt.Sprintf("marker %q cannot be applied to fields", marker.Identifier)
208+
}
209+
210+
pass.Report(analysis.Diagnostic{
211+
Pos: marker.Pos,
212+
End: marker.End,
213+
Message: message,
214+
SuggestedFixes: fixes,
215+
})
216+
}
217+
218+
// checkFieldTypeConstraintViolation checks and reports type constraint violations for field markers.
219+
func (a *analyzer) checkFieldTypeConstraintViolation(pass *analysis.Pass, field *ast.Field, marker markershelper.Marker, rule MarkerScopeRule) {
220+
if err := a.validateFieldTypeConstraint(pass, field, rule, a.allowDangerousTypes); err != nil {
221+
var fixes []analysis.SuggestedFix
222+
223+
if a.policy == MarkerScopePolicySuggestFix {
224+
// Check if this is a "should be on type definition" error
225+
var moveErr *MarkerShouldBeOnTypeDefinitionError
226+
if errors.As(err, &moveErr) {
227+
// Suggest moving to type definition
228+
fixes = a.suggestMoveToFieldsIfCompatible(pass, field, marker, rule)
229+
} else {
230+
// Type constraint violation - suggest removing the marker
231+
fixes = []analysis.SuggestedFix{
232+
{
233+
Message: "Remove invalid marker",
234+
TextEdits: []analysis.TextEdit{
235+
{
236+
Pos: marker.Pos,
237+
End: marker.End + 1, // Include newline
238+
},
239+
},
240+
},
241+
}
242+
}
243+
}
244+
245+
pass.Report(analysis.Diagnostic{
246+
Pos: marker.Pos,
247+
End: marker.End,
248+
Message: fmt.Sprintf("marker %q: %s", marker.Identifier, err),
249+
SuggestedFixes: fixes,
250+
})
251+
}
252+
}
253+
229254
// reportTypeScopeViolation reports a scope violation for a type marker.
230255
func (a *analyzer) reportTypeScopeViolation(pass *analysis.Pass, typeSpec *ast.TypeSpec, marker markershelper.Marker, rule MarkerScopeRule) {
231256
var message string
@@ -256,7 +281,26 @@ func (a *analyzer) checkTypeConstraintViolation(pass *analysis.Pass, typeSpec *a
256281
var fixes []analysis.SuggestedFix
257282

258283
if a.policy == MarkerScopePolicySuggestFix {
259-
fixes = a.suggestMoveToField(pass, typeSpec, marker, rule)
284+
// Check if this is a "should be on field" error (though validateTypeSpecTypeConstraint doesn't return this)
285+
// For consistency with checkFieldMarkers, we check the error type
286+
var moveErr *MarkerShouldBeOnTypeDefinitionError
287+
if errors.As(err, &moveErr) {
288+
// This shouldn't happen for type specs, but handle it for consistency
289+
fixes = a.suggestMoveToField(pass, typeSpec, marker, rule)
290+
} else {
291+
// Type constraint violation - suggest removing the marker
292+
fixes = []analysis.SuggestedFix{
293+
{
294+
Message: "Remove invalid marker",
295+
TextEdits: []analysis.TextEdit{
296+
{
297+
Pos: marker.Pos,
298+
End: marker.End + 1, // Include newline
299+
},
300+
},
301+
},
302+
}
303+
}
260304
}
261305

262306
message := fmt.Sprintf("marker %q: %s", marker.Identifier, err)
@@ -364,7 +408,6 @@ func (a *analyzer) suggestMoveToField(pass *analysis.Pass, typeSpec *ast.TypeSpe
364408
}
365409

366410
fieldTypeSpecs := utils.LookupFieldsUsingType(pass, typeSpec)
367-
fmt.Println("fieldTypeSpecs", fieldTypeSpecs)
368411

369412
var edits []analysis.TextEdit
370413

@@ -422,7 +465,6 @@ func (a *analyzer) suggestMoveToFieldsIfCompatible(pass *analysis.Pass, field *a
422465
Pos: marker.Pos,
423466
End: marker.End + 1,
424467
})
425-
426468
// Add marker to the line before the type definition
427469
markerText := a.extractMarkerText(marker)
428470

@@ -445,5 +487,5 @@ func (a *analyzer) suggestMoveToFieldsIfCompatible(pass *analysis.Pass, field *a
445487
}
446488

447489
func (a *analyzer) extractMarkerText(marker markershelper.Marker) string {
448-
return strings.Split(marker.RawComment, " //")[0] + "\n"
490+
return marker.RawComment + "\n"
449491
}

‎pkg/analysis/markerscope/analyzer_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@ func TestAnalyzerWarnOnly(t *testing.T) {
3030
analysistest.Run(t, testdata, analyzer, "a")
3131
}
3232

33+
func TestAnalyzerSuggestFixes(t *testing.T) {
34+
testdata := analysistest.TestData()
35+
cfg := &MarkerScopeConfig{
36+
Policy: MarkerScopePolicySuggestFix,
37+
}
38+
analyzer := newAnalyzer(cfg)
39+
analysistest.RunWithSuggestedFixes(t, testdata, analyzer, "a")
40+
}
41+
3342
func TestAnalyzerWithCustomMarkers(t *testing.T) {
3443
testdata := analysistest.TestData()
3544
cfg := &MarkerScopeConfig{
@@ -72,12 +81,3 @@ func TestAnalyzerWithCustomMarkers(t *testing.T) {
7281
analyzer := newAnalyzer(cfg)
7382
analysistest.Run(t, testdata, analyzer, "b")
7483
}
75-
76-
func TestAnalyzerWithSuggestFix(t *testing.T) {
77-
testdata := analysistest.TestData()
78-
cfg := &MarkerScopeConfig{
79-
Policy: MarkerScopePolicySuggestFix,
80-
}
81-
analyzer := newAnalyzer(cfg)
82-
analysistest.RunWithSuggestedFixes(t, testdata, analyzer, "c")
83-
}

‎pkg/analysis/markerscope/testdata/src/a/array.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ type IntegerArray []int32
2626
// +kubebuilder:validation:MaxItems=100
2727
type BooleanArray []bool
2828

29+
type FieldMarkerOnNamedArray []string
30+
2931
// Type definitions with invalid markers
3032
// +kubebuilder:validation:MinItems=1 // want `marker "kubebuilder:validation:MinItems": type string is not allowed \(expected one of: \[array\]\)`
3133
type InvalidArrayMarkerOnStringType string
@@ -82,8 +84,8 @@ type ArrayMarkersFieldTest struct {
8284
ValidBooleanArrayType BooleanArray `json:"validBooleanArrayType"`
8385

8486
// Invalid: Field marker on named array type (should use type definition)
85-
// +kubebuilder:validation:MinItems=5 // want `marker "kubebuilder:validation:MinItems": marker should be declared on the type definition of StringArray instead of the field`
86-
InvalidFieldMarkerOnNamedArray StringArray `json:"invalidFieldMarkerOnNamedArray"`
87+
// +kubebuilder:validation:MinItems=5 // want `marker "kubebuilder:validation:MinItems": marker should be declared on the type definition of FieldMarkerOnNamedArray instead of the field`
88+
InvalidFieldMarkerOnNamedArray FieldMarkerOnNamedArray `json:"invalidFieldMarkerOnNamedArray"`
8789

8890
// Invalid: Using invalid named type with array marker
8991
InvalidNamedStringType InvalidArrayMarkerOnStringType `json:"invalidNamedStringType"`

‎pkg/analysis/markerscope/testdata/src/a/array.go.golden‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
package a
17+
18+
// +kubebuilder:validation:MinItems=1
19+
// +kubebuilder:validation:MaxItems=10
20+
// +kubebuilder:validation:UniqueItems=true
21+
type StringArray []string
22+
23+
// +kubebuilder:validation:MinItems=0
24+
type IntegerArray []int32
25+
26+
// +kubebuilder:validation:MaxItems=100
27+
type BooleanArray []bool
28+
29+
// +kubebuilder:validation:MinItems=5 // want `marker "kubebuilder:validation:MinItems": marker should be declared on the type definition of FieldMarkerOnNamedArray instead of the field`
30+
type FieldMarkerOnNamedArray []string
31+
32+
// Type definitions with invalid markers
33+
type InvalidArrayMarkerOnStringType string
34+
35+
type InvalidArrayMarkerOnMapType map[string]string
36+
37+
type ArrayMarkersFieldTest struct {
38+
// Valid: MinItems marker on array field
39+
// +kubebuilder:validation:MinItems=1
40+
ValidMinItems []string `json:"validMinItems"`
41+
42+
// Valid: MaxItems marker on array field
43+
// +kubebuilder:validation:MaxItems=10
44+
ValidMaxItems []string `json:"validMaxItems"`
45+
46+
// Valid: UniqueItems marker on array field
47+
// +kubebuilder:validation:UniqueItems=true
48+
ValidUniqueItems []string `json:"validUniqueItems"`
49+
50+
// Valid: All array markers combined on array field
51+
// +kubebuilder:validation:MinItems=1
52+
// +kubebuilder:validation:MaxItems=10
53+
// +kubebuilder:validation:UniqueItems=true
54+
ValidAllArrayMarkers []string `json:"validAllArrayMarkers"`
55+
56+
// Invalid: MinItems marker on string field
57+
InvalidMinItemsOnString string `json:"invalidMinItemsOnString"`
58+
59+
// Invalid: MaxItems marker on map field
60+
InvalidMaxItemsOnMap map[string]string `json:"invalidMaxItemsOnMap"`
61+
62+
// Invalid: UniqueItems marker on integer field
63+
InvalidUniqueItemsOnInteger int32 `json:"invalidUniqueItemsOnInteger"`
64+
65+
// Invalid: MinItems marker on boolean field
66+
InvalidMinItemsOnBoolean bool `json:"invalidMinItemsOnBoolean"`
67+
68+
// Invalid: MaxItems marker on struct field
69+
InvalidMaxItemsOnStruct ArrayItem `json:"invalidMaxItemsOnStruct"`
70+
71+
// Valid: Using named array type with markers
72+
ValidNamedArrayType StringArray `json:"validNamedArrayType"`
73+
74+
// Valid: Using named array type (IntegerArray)
75+
ValidIntegerArrayType IntegerArray `json:"validIntegerArrayType"`
76+
77+
// Valid: Using named array type (BooleanArray)
78+
ValidBooleanArrayType BooleanArray `json:"validBooleanArrayType"`
79+
80+
// Invalid: Field marker on named array type (should use type definition)
81+
InvalidFieldMarkerOnNamedArray FieldMarkerOnNamedArray `json:"invalidFieldMarkerOnNamedArray"`
82+
83+
// Invalid: Using invalid named type with array marker
84+
InvalidNamedStringType InvalidArrayMarkerOnStringType `json:"invalidNamedStringType"`
85+
86+
// Invalid: Using invalid named type with array marker
87+
InvalidNamedMapType InvalidArrayMarkerOnMapType `json:"invalidNamedMapType"`
88+
}
89+
90+
type ArrayItem struct {
91+
Name string `json:"name"`
92+
}

0 commit comments

Comments
 (0)