Skip to content

Commit 896e8b6

Browse files
JoelSpeedJoelSpeed
committed
Refactor optionalfields linter to tidy up
1 parent da94be5 commit 896e8b6

File tree

6 files changed

+463
-287
lines changed

6 files changed

+463
-287
lines changed

‎pkg/analysis/optionalfields/analyzer.go

Lines changed: 141 additions & 286 deletions
Large diffs are not rendered by default.

‎pkg/analysis/optionalfields/analyzer_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
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+
*/
116
package optionalfields_test
217

318
import (

‎pkg/analysis/optionalfields/doc.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,19 @@
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+
117
/*
218
*/
319
package optionalfields

‎pkg/analysis/optionalfields/initializer.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
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+
*/
116
package optionalfields
217

318
import (

‎pkg/analysis/optionalfields/util.go

Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
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 optionalfields
17+
18+
import (
19+
"fmt"
20+
"go/ast"
21+
"go/token"
22+
"strconv"
23+
24+
"golang.org/x/tools/go/analysis"
25+
"k8s.io/utils/ptr"
26+
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
27+
"sigs.k8s.io/kube-api-linter/pkg/config"
28+
)
29+
30+
// isStarExpr checks if the expression is a pointer type.
31+
// If it is, it returns the expression inside the pointer.
32+
func isStarExpr(expr ast.Expr) (bool, ast.Expr) {
33+
if ptrType, ok := expr.(*ast.StarExpr); ok {
34+
return true, ptrType.X
35+
}
36+
37+
return false, expr
38+
}
39+
40+
// isPointerType checks if the expression is a pointer type.
41+
// This is for types that are always implemented as pointers and therefore should
42+
// not be the underlying type of a star expr.
43+
func isPointerType(expr ast.Expr) bool {
44+
switch expr.(type) {
45+
case *ast.StarExpr, *ast.MapType, *ast.ArrayType:
46+
return true
47+
default:
48+
return false
49+
}
50+
}
51+
52+
// structContainsRequiredFields checks whether the struct has any required fields.
53+
// Having a required field means that `{}` is not a valid entity for the struct,
54+
// and therefore the struct should be a pointer when optional.
55+
func structContainsRequiredFields(structType *ast.StructType, markersAccess markers.Markers) bool {
56+
if structType == nil {
57+
return false
58+
}
59+
60+
for _, field := range structType.Fields.List {
61+
fieldMarkers := markersAccess.FieldMarkers(field)
62+
63+
if isFieldRequired(fieldMarkers) {
64+
return true
65+
}
66+
}
67+
68+
return false
69+
}
70+
71+
// isFieldRequired checks if a field has a required marker.
72+
func isFieldRequired(fieldMarkers markers.MarkerSet) bool {
73+
return fieldMarkers.Has("required") || fieldMarkers.Has("kubebuilder:validation:Required")
74+
}
75+
76+
// isFieldOptional checks if a field has an optional marker.
77+
func isFieldOptional(fieldMarkers markers.MarkerSet) bool {
78+
return fieldMarkers.Has("optional") || fieldMarkers.Has("kubebuilder:validation:Optional")
79+
}
80+
81+
// reportShouldAddPointer adds an analysis diagnostic that explains that a pointer should be added.
82+
// Where the pointer policy is suggest fix, it also adds a suggested fix to add the pointer.
83+
func reportShouldAddPointer(pass *analysis.Pass, field *ast.Field, pointerPolicy config.OptionalFieldsPointerPolicy, fieldName, messageFmt string) {
84+
switch pointerPolicy {
85+
case config.OptionalFieldsPointerPolicySuggestFix:
86+
pass.Report(analysis.Diagnostic{
87+
Pos: field.Pos(),
88+
Message: fmt.Sprintf(messageFmt, fieldName),
89+
SuggestedFixes: []analysis.SuggestedFix{
90+
{
91+
Message: "should make the field a pointer",
92+
TextEdits: []analysis.TextEdit{
93+
{
94+
Pos: field.Pos() + token.Pos(len(fieldName)+1),
95+
NewText: []byte("*"),
96+
},
97+
},
98+
},
99+
},
100+
})
101+
case config.OptionalFieldsPointerPolicyWarn:
102+
pass.Reportf(field.Pos(), messageFmt, fieldName)
103+
default:
104+
panic(fmt.Sprintf("unknown pointer policy: %s", pointerPolicy))
105+
}
106+
}
107+
108+
// reportShouldRemovePointer adds an analysis diagnostic that explains that a pointer should be removed.
109+
// Where the pointer policy is suggest fix, it also adds a suggested fix to remove the pointer.
110+
func reportShouldRemovePointer(pass *analysis.Pass, field *ast.Field, pointerPolicy config.OptionalFieldsPointerPolicy, fieldName, messageFmt string) {
111+
switch pointerPolicy {
112+
case config.OptionalFieldsPointerPolicySuggestFix:
113+
pass.Report(analysis.Diagnostic{
114+
Pos: field.Pos(),
115+
Message: fmt.Sprintf(messageFmt, fieldName),
116+
SuggestedFixes: []analysis.SuggestedFix{
117+
{
118+
Message: "should remove the pointer",
119+
TextEdits: []analysis.TextEdit{
120+
{
121+
Pos: field.Pos() + token.Pos(len(fieldName)+1),
122+
End: field.Pos() + token.Pos(len(fieldName)+2),
123+
},
124+
},
125+
},
126+
},
127+
})
128+
case config.OptionalFieldsPointerPolicyWarn:
129+
pass.Reportf(field.Pos(), messageFmt, fieldName)
130+
default:
131+
panic(fmt.Sprintf("unknown pointer policy: %s", pointerPolicy))
132+
}
133+
}
134+
135+
// reportShouldRemoveAllInstancesOfIntegerMarker adds an analysis diagnostic that explains that a marker should be removed.
136+
// This function is used to find non-zero valued markers, and suggest that they are removed when the field is not a pointer.
137+
// This is used for markers like MinLength and MinProperties.
138+
func reportShouldRemoveAllInstancesOfIntegerMarker(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, markerName, fieldName, messageFmt string) {
139+
fieldMarkers := markersAccess.FieldMarkers(field)
140+
141+
for _, marker := range fieldMarkers.Get(markerName) {
142+
markerValue, err := getMarkerIntegerValue(marker)
143+
if err != nil {
144+
pass.Reportf(marker.Pos, "invalid value for %s marker: %v", markerName, err)
145+
return
146+
}
147+
148+
if markerValue > 0 {
149+
reportShouldRemoveMarker(pass, field, marker, fieldName, messageFmt)
150+
}
151+
}
152+
}
153+
154+
// reportShouldAddMarker adds an analysis diagnostic that explains that a marker should be removed.
155+
// This is used where we see a marker that would conflict with a field that lacks omitempty.
156+
func reportShouldRemoveMarker(pass *analysis.Pass, field *ast.Field, marker markers.Marker, fieldName, messageFmt string) {
157+
pass.Report(analysis.Diagnostic{
158+
Pos: field.Pos(),
159+
Message: fmt.Sprintf(messageFmt, fieldName),
160+
SuggestedFixes: []analysis.SuggestedFix{
161+
{
162+
Message: fmt.Sprintf("should remove the marker: %s", marker.RawComment),
163+
TextEdits: []analysis.TextEdit{
164+
{
165+
Pos: marker.Pos,
166+
End: marker.End + 1,
167+
},
168+
},
169+
},
170+
},
171+
})
172+
}
173+
174+
// getMarkerIntegerValueByName extracts the numeric value from the first
175+
// instace of the marker with the given name.
176+
// Works for markers like MaxLength, MinLength, etc.
177+
func getMarkerIntegerValueByName(marker markers.MarkerSet, markerName string) (*int, error) {
178+
markerList := marker.Get(markerName)
179+
if len(markerList) == 0 {
180+
return nil, errMarkerMissingValue
181+
}
182+
183+
markerValue, err := getMarkerIntegerValue(markerList[0])
184+
if err != nil {
185+
return nil, fmt.Errorf("error getting marker value: %w", err)
186+
}
187+
188+
return &markerValue, nil
189+
}
190+
191+
// getMarkerIntegerValue extracts a numeric value from the default
192+
// value of a marker.
193+
// Works for markers like MaxLength, MinLength, etc.
194+
func getMarkerIntegerValue(marker markers.Marker) (int, error) {
195+
rawValue, ok := marker.Expressions[""]
196+
if !ok {
197+
return 0, errMarkerMissingValue
198+
}
199+
200+
value, err := strconv.Atoi(rawValue)
201+
if err != nil {
202+
return 0, fmt.Errorf("error converting value to integer: %w", err)
203+
}
204+
205+
return value, nil
206+
}
207+
208+
// getMarkerFloatValueByName extracts the numeric value from the first
209+
// instace of the marker with the given name.
210+
// Works for markers like MaxLength, MinLength, etc.
211+
func getMarkerFloatValueByName(marker markers.MarkerSet, markerName string) (*float64, error) {
212+
markerList := marker.Get(markerName)
213+
if len(markerList) == 0 {
214+
return nil, errMarkerMissingValue
215+
}
216+
217+
markerValue, err := getMarkerFloatValue(markerList[0])
218+
if err != nil {
219+
return nil, fmt.Errorf("error getting marker value: %w", err)
220+
}
221+
222+
return &markerValue, nil
223+
}
224+
225+
// getMarkerFloatValue extracts a numeric value from the default
226+
// value of a marker.
227+
// Works for markers like MaxLength, MinLength, etc.
228+
func getMarkerFloatValue(marker markers.Marker) (float64, error) {
229+
rawValue, ok := marker.Expressions[""]
230+
if !ok {
231+
return 0, errMarkerMissingValue
232+
}
233+
234+
value, err := strconv.ParseFloat(rawValue, 64)
235+
if err != nil {
236+
return 0, fmt.Errorf("error converting value to float: %w", err)
237+
}
238+
239+
return value, nil
240+
}
241+
242+
// structHasGreaterThanZeroMaxProperties checks if the struct has a minProperties marker.
243+
// It inspects the minProperties marker on the struct type itself.
244+
func structHasGreaterThanZeroMinProperties(structType *ast.StructType, structMarkers markers.MarkerSet) (bool, error) {
245+
if structType == nil {
246+
return false, nil
247+
}
248+
249+
for _, marker := range structMarkers.Get(minPropertiesMarker) {
250+
markerValue, err := getMarkerIntegerValue(marker)
251+
if err != nil {
252+
return false, fmt.Errorf("error getting marker value: %w", err)
253+
}
254+
255+
if markerValue > 0 {
256+
return true, nil
257+
}
258+
}
259+
260+
return false, nil
261+
}
262+
263+
func integerRangeIncludesZero(minimum, maximum *int) bool {
264+
return ptr.Deref(minimum, -1) == 0 ||
265+
ptr.Deref(maximum, -1) == 0 ||
266+
ptr.Deref(minimum, 0) < 0 && ptr.Deref(maximum, 0) > 0
267+
}
268+
269+
func floatRangeIncludesZero(minimum, maximum *float64) bool {
270+
return ptr.Deref(minimum, -1) == 0 ||
271+
ptr.Deref(maximum, -1) == 0 ||
272+
ptr.Deref(minimum, 0) < 0 && ptr.Deref(maximum, 0) > 0
273+
}

‎pkg/config/linters_config.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ type NoMapsConfig struct {
142142
Policy NoMapsPolicy `json:"policy"`
143143
}
144144

145-
// OptionalFields is the policy for the optionalfields linter.
145+
// OptionalFieldsConfig is the configuration for the optionalfields linter.
146146
type OptionalFieldsConfig struct {
147147
// pointers is the policy for pointers in optional fields.
148148
// This defines how the linter should handle optional fields, and whether they should be pointers or not.
@@ -155,6 +155,7 @@ type OptionalFieldsConfig struct {
155155
OmitEmpty OptionalFieldsOmitEmpty `json:"omitempty"`
156156
}
157157

158+
// OptionalFieldsPointers is the configuration for pointers in optional fields.
158159
type OptionalFieldsPointers struct {
159160
// preference determines whether the linter should prefer pointers for all optional fields,
160161
// or only for optional fields where validation or serialization requires a pointer.
@@ -172,6 +173,7 @@ type OptionalFieldsPointers struct {
172173
Policy OptionalFieldsPointerPolicy `json:"policy"`
173174
}
174175

176+
// OptionalFieldsOmitEmpty is the configuration for the `omitempty` tag on optional fields.
175177
type OptionalFieldsOmitEmpty struct {
176178
// policy determines whether the linter should require omitempty for all optional fields.
177179
// Valid values are "SuggestFix" and "Ignore".

0 commit comments

Comments
 (0)