From 294fd7787d0223b9b8f0509a1832c3d3af655933 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 10 Nov 2025 11:52:47 -0500 Subject: [PATCH 1/3] Add linter for nonpointerstruct optionality --- docs/linters.md | 15 +++ pkg/analysis/nonpointerstructs/analyzer.go | 115 ++++++++++++++++++ .../nonpointerstructs/analyzer_test.go | 34 ++++++ pkg/analysis/nonpointerstructs/doc.go | 30 +++++ pkg/analysis/nonpointerstructs/initializer.go | 35 ++++++ .../nonpointerstructs/testdata/src/a/a.go | 71 +++++++++++ pkg/analysis/optionalfields/analyzer.go | 9 +- pkg/analysis/utils/zero_value.go | 10 ++ pkg/registration/doc.go | 1 + 9 files changed, 313 insertions(+), 7 deletions(-) create mode 100644 pkg/analysis/nonpointerstructs/analyzer.go create mode 100644 pkg/analysis/nonpointerstructs/analyzer_test.go create mode 100644 pkg/analysis/nonpointerstructs/doc.go create mode 100644 pkg/analysis/nonpointerstructs/initializer.go create mode 100644 pkg/analysis/nonpointerstructs/testdata/src/a/a.go diff --git a/docs/linters.md b/docs/linters.md index 594f8407..4e96bca9 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -16,6 +16,7 @@ - [NoDurations](#nodurations) - Prevents usage of duration types - [NoFloats](#nofloats) - Prevents usage of floating-point types - [Nomaps](#nomaps) - Restricts usage of map types +- [NonPointerStructs](#nonpointerstructs) - Ensures non-pointer structs are marked correctly with required/optional markers - [NoNullable](#nonullable) - Prevents usage of the nullable marker - [Nophase](#nophase) - Prevents usage of 'Phase' fields - [Notimestamp](#notimestamp) - Prevents usage of 'TimeStamp' fields @@ -514,6 +515,20 @@ lintersConfig: policy: Enforce | AllowStringToStringMaps | Ignore # Determines how the linter should handle maps of simple types. Defaults to AllowStringToStringMaps. ``` +## NonPointerStructs + +The `nonpointerstructs` linter checks that non-pointer structs that contain required fields are marked as required. +Non-pointer structs that contain no required fields are marked as optional. + +This linter is important for types validated in Go as there is no way to validate the optionality of the fields at runtime, +aside from checking the fields within them. + +If a struct is marked required, this can only be validated by having a required field within it. +If there are no required fields, the struct is implicitly optional and must be marked as so. + +To have an optional struct field that includes required fields, the struct must be a pointer. +To have a required struct field that includes no required fields, the struct must be a pointer. + ## NoNullable The `nonullable` linter ensures that types and fields do not have the `nullable` marker. diff --git a/pkg/analysis/nonpointerstructs/analyzer.go b/pkg/analysis/nonpointerstructs/analyzer.go new file mode 100644 index 00000000..751b183e --- /dev/null +++ b/pkg/analysis/nonpointerstructs/analyzer.go @@ -0,0 +1,115 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package nonpointerstructs + +import ( + "fmt" + "go/ast" + + "golang.org/x/tools/go/analysis" + kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors" + "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" + "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector" + markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils" + "sigs.k8s.io/kube-api-linter/pkg/markers" +) + +const name = "nonpointerstructs" + +func newAnalyzer() *analysis.Analyzer { + return &analysis.Analyzer{ + Name: name, + Doc: "Checks that non-pointer structs that contain required fields are marked as required. Non-pointer structs that contain no required fields are marked as optional.", + Run: run, + Requires: []*analysis.Analyzer{inspector.Analyzer}, + } +} + +func run(pass *analysis.Pass) (any, error) { + inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) + if !ok { + return nil, kalerrors.ErrCouldNotGetInspector + } + + inspect.InspectFields(func(field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, qualifiedFieldName string) { + checkField(pass, field, markersAccess, jsonTagInfo, qualifiedFieldName) + }) + + return nil, nil //nolint:nilnil +} + +func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string) { + if field.Type == nil { + return + } + + if jsonTagInfo.Inline { + return + } + + structType, ok := asNonPointerStruct(pass, field.Type) + if !ok { + return + } + + hasRequiredField := hasRequiredField(structType, markersAccess) + isOptional := utils.IsFieldOptional(field, markersAccess) + isRequired := utils.IsFieldRequired(field, markersAccess) + + switch { + case hasRequiredField && isRequired, !hasRequiredField && isOptional: + // This is the desired case. + case hasRequiredField: + pass.Reportf(field.Pos(), "field %s is a non-pointer struct with required fields. It must be marked as required.", qualifiedFieldName) + case !hasRequiredField: + pass.Reportf(field.Pos(), "field %s is a non-pointer struct with no required fields. It must be marked as optional.", qualifiedFieldName) + } +} + +func asNonPointerStruct(pass *analysis.Pass, field ast.Expr) (*ast.StructType, bool) { + switch typ := field.(type) { + case *ast.StructType: + return typ, true + case *ast.Ident: + typeSpec, ok := utils.LookupTypeSpec(pass, typ) + if !ok { + return nil, false + } + + return asNonPointerStruct(pass, typeSpec.Type) + default: + return nil, false + } +} + +func hasRequiredField(structType *ast.StructType, markersAccess markershelper.Markers) bool { + for _, field := range structType.Fields.List { + if utils.IsFieldRequired(field, markersAccess) { + return true + } + } + + structMarkers := markersAccess.StructMarkers(structType) + + if structMarkers.Has(markers.KubebuilderMinPropertiesMarker) && !structMarkers.HasWithValue(fmt.Sprintf("%s=0", markers.KubebuilderMinPropertiesMarker)) { + // A non-zero min properties marker means that the struct is validated to have at least one field. + // This means it can be treated the same as having a required field. + return true + } + + return false +} diff --git a/pkg/analysis/nonpointerstructs/analyzer_test.go b/pkg/analysis/nonpointerstructs/analyzer_test.go new file mode 100644 index 00000000..a1568b8e --- /dev/null +++ b/pkg/analysis/nonpointerstructs/analyzer_test.go @@ -0,0 +1,34 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package nonpointerstructs_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "sigs.k8s.io/kube-api-linter/pkg/analysis/nonpointerstructs" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + + analyzer, err := nonpointerstructs.Initializer().Init(nil) + if err != nil { + t.Fatalf("initializing nonpointerstructs linter: %v", err) + } + + analysistest.Run(t, testdata, analyzer, "a") +} diff --git a/pkg/analysis/nonpointerstructs/doc.go b/pkg/analysis/nonpointerstructs/doc.go new file mode 100644 index 00000000..be529b4a --- /dev/null +++ b/pkg/analysis/nonpointerstructs/doc.go @@ -0,0 +1,30 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/* +nonpointerstructs is a linter that checks that non-pointer structs that contain required fields are marked as required. +Non-pointer structs that contain no required fields are marked as optional. + +This linter is important for types validated in Go as there is no way to validate the optionality of the fields at runtime, +aside from checking the fields within them. + +If a struct is marked required, this can only be validated by having a required field within it. +If there are no required fields, the struct is implicitly optional and must be marked as so. + +To have an optional struct field that includes required fields, the struct must be a pointer. +To have a required struct field that includes no required fields, the struct must be a pointer. +*/ +package nonpointerstructs diff --git a/pkg/analysis/nonpointerstructs/initializer.go b/pkg/analysis/nonpointerstructs/initializer.go new file mode 100644 index 00000000..a46122a9 --- /dev/null +++ b/pkg/analysis/nonpointerstructs/initializer.go @@ -0,0 +1,35 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package nonpointerstructs + +import ( + "sigs.k8s.io/kube-api-linter/pkg/analysis/initializer" + "sigs.k8s.io/kube-api-linter/pkg/analysis/registry" +) + +func init() { + registry.DefaultRegistry().RegisterLinter(Initializer()) +} + +// Initializer returns the AnalyzerInitializer for this +// Analyzer so that it can be added to the registry. +func Initializer() initializer.AnalyzerInitializer { + return initializer.NewInitializer( + name, + newAnalyzer(), + true, + ) +} diff --git a/pkg/analysis/nonpointerstructs/testdata/src/a/a.go b/pkg/analysis/nonpointerstructs/testdata/src/a/a.go new file mode 100644 index 00000000..5282c2bd --- /dev/null +++ b/pkg/analysis/nonpointerstructs/testdata/src/a/a.go @@ -0,0 +1,71 @@ +package a + +type A struct { + WithRequired WithRequiredField `json:"withRequired"` // want "field A.WithRequired is a non-pointer struct with required fields. It must be marked as required." + WithOptional WithOptionalField `json:"withOptional"` // want "field A.WithOptional is a non-pointer struct with no required fields. It must be marked as optional." + WithRequiredAndOptional WithRequiredAndOptionalField `json:"withRequiredAndOptional"` // want "field A.WithRequiredAndOptional is a non-pointer struct with required fields. It must be marked as required." + WithOptionalAndMinProperties WithOptionalFieldsAndMinProperties `json:"withOptionalAndMinProperties"` // want "field A.WithOptionalAndMinProperties is a non-pointer struct with required fields. It must be marked as required." + + // No diagnostic with correct markers + + // +required + WithRequiredFieldCorrect WithRequiredField `json:"withRequiredFieldCorrect"` + + // +optional + WithOptionalFieldCorrect WithOptionalField `json:"withOptionalFieldCorrect"` + + // +kubebuilder:validation:Required + WithRequiredAndOptionalFieldCorrect WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldCorrect"` + + // +k8s:required + WithOptionalFieldsAndMinPropertiesCorrect WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesCorrect"` + + // Diagnostic with incorrect markers + + // +kubebuilder:validation:Optional + WithRequiredFieldIncorrect WithRequiredField `json:"withRequiredFieldIncorrect"` // want "field A.WithRequiredFieldIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // +kubebuilder:validation:Required + WithOptionalFieldIncorrect WithOptionalField `json:"withOptionalFieldIncorrect"` // want "field A.WithOptionalFieldIncorrect is a non-pointer struct with no required fields. It must be marked as optional." + + // +kubebuilder:validation:Optional + WithRequiredAndOptionalFieldIncorrect WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldIncorrect"` // want "field A.WithRequiredAndOptionalFieldIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // +kubebuilder:validation:Optional + WithOptionalFieldsAndMinPropertiesIncorrect WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesIncorrect"` // want "field A.WithOptionalFieldsAndMinPropertiesIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // Pointers are ignored in this linter. + WithRequiredFieldAndPointer *WithRequiredField `json:"withRequiredFieldAndPointer"` + WithOptionalFieldAndPointer *WithOptionalField `json:"withOptionalFieldAndPointer"` + WithRequiredAndOptionalFieldAndPointer *WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldAndPointer"` + WithOptionalFieldsAndMinPropertiesAndPointer *WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesAndPointer"` + + // Inline structs are ignored in this linter. + WithRequiredField `json:",inline"` + WithOptionalField `json:",inline"` + WithRequiredAndOptionalField `json:",inline"` + WithOptionalFieldsAndMinProperties `json:",inline"` +} + +type WithRequiredField struct { + // +required + RequiredField string `json:"requiredField"` +} + +type WithOptionalField struct { + // +optional + OptionalField string `json:"optionalField"` +} + +type WithRequiredAndOptionalField struct { + // +k8s:required + RequiredField string `json:"requiredField"` + // +k8s:optional + OptionalField string `json:"optionalField"` +} + +// +kubebuilder:validation:MinProperties=1 +type WithOptionalFieldsAndMinProperties struct { + // +k8s:optional + OptionalField string `json:"optionalField"` +} diff --git a/pkg/analysis/optionalfields/analyzer.go b/pkg/analysis/optionalfields/analyzer.go index b053a57e..4bc66d8f 100644 --- a/pkg/analysis/optionalfields/analyzer.go +++ b/pkg/analysis/optionalfields/analyzer.go @@ -23,6 +23,7 @@ import ( "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags" "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/inspector" markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers" + "sigs.k8s.io/kube-api-linter/pkg/analysis/utils" "sigs.k8s.io/kube-api-linter/pkg/analysis/utils/serialization" "sigs.k8s.io/kube-api-linter/pkg/markers" ) @@ -105,8 +106,7 @@ func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAcce return } - fieldMarkers := markersAccess.FieldMarkers(field) - if !isFieldOptional(fieldMarkers) { + if !utils.IsFieldOptional(field, markersAccess) { // The field is not marked optional, so we don't need to check it. return } @@ -136,8 +136,3 @@ func defaultConfig(cfg *OptionalFieldsConfig) { cfg.OmitZero.Policy = OptionalFieldsOmitZeroPolicySuggestFix } } - -// isFieldOptional checks if a field has an optional marker. -func isFieldOptional(fieldMarkers markershelper.MarkerSet) bool { - return fieldMarkers.Has(markers.OptionalMarker) || fieldMarkers.Has(markers.KubebuilderOptionalMarker) -} diff --git a/pkg/analysis/utils/zero_value.go b/pkg/analysis/utils/zero_value.go index 94701664..16705758 100644 --- a/pkg/analysis/utils/zero_value.go +++ b/pkg/analysis/utils/zero_value.go @@ -449,3 +449,13 @@ func IsFieldRequired(field *ast.Field, markersAccess markershelper.Markers) bool fieldMarkers.Has(markers.KubebuilderRequiredMarker) || fieldMarkers.Has(markers.K8sRequiredMarker) } + +// IsFieldOptional checks if the field is optional. +// It checks for the presence of the optional marker, the kubebuilder optional marker, or the k8s optional marker. +func IsFieldOptional(field *ast.Field, markersAccess markershelper.Markers) bool { + fieldMarkers := markersAccess.FieldMarkers(field) + + return fieldMarkers.Has(markers.OptionalMarker) || + fieldMarkers.Has(markers.KubebuilderOptionalMarker) || + fieldMarkers.Has(markers.K8sOptionalMarker) +} diff --git a/pkg/registration/doc.go b/pkg/registration/doc.go index 439a7513..c9070c7c 100644 --- a/pkg/registration/doc.go +++ b/pkg/registration/doc.go @@ -38,6 +38,7 @@ import ( _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nodurations" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nofloats" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nomaps" + _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nonpointerstructs" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nonullable" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/nophase" _ "sigs.k8s.io/kube-api-linter/pkg/analysis/noreferences" From dbbbcbf2fd2c7b614606e0469503dbf0bcadc433 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 11 Nov 2025 10:14:46 -0500 Subject: [PATCH 2/3] Update integration tests to account for nonpointerstructs --- .../testdata/default_configurations/affinity.go | 4 ++-- .../testdata/default_configurations/object_references.go | 7 +++---- .../integration/testdata/default_configurations/volume.go | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/integration/testdata/default_configurations/affinity.go b/tests/integration/testdata/default_configurations/affinity.go index 64eb5417..8c03e9d4 100644 --- a/tests/integration/testdata/default_configurations/affinity.go +++ b/tests/integration/testdata/default_configurations/affinity.go @@ -93,7 +93,7 @@ type WeightedPodAffinityTerm struct { // in the range 1-100. Weight int32 `json:"weight" protobuf:"varint,1,opt,name=weight"` // want "optionalorrequired: field WeightedPodAffinityTerm.Weight must be marked as optional or required" // Required. A pod affinity term, associated with the corresponding weight. // want "commentstart: godoc for field WeightedPodAffinityTerm.PodAffinityTerm should start with 'podAffinityTerm ...'" - PodAffinityTerm PodAffinityTerm `json:"podAffinityTerm" protobuf:"bytes,2,opt,name=podAffinityTerm"` // want "optionalorrequired: field WeightedPodAffinityTerm.PodAffinityTerm must be marked as optional or required" + PodAffinityTerm PodAffinityTerm `json:"podAffinityTerm" protobuf:"bytes,2,opt,name=podAffinityTerm"` // want "optionalorrequired: field WeightedPodAffinityTerm.PodAffinityTerm must be marked as optional or required" "nonpointerstructs: field WeightedPodAffinityTerm.PodAffinityTerm is a non-pointer struct with no required fields. It must be marked as optional." } // Defines a set of pods (namely those matching the labelSelector @@ -193,7 +193,7 @@ type PreferredSchedulingTerm struct { // Weight associated with matching the corresponding nodeSelectorTerm, in the range 1-100. // want "commentstart: godoc for field PreferredSchedulingTerm.Weight should start with 'weight ...'" Weight int32 `json:"weight" protobuf:"varint,1,opt,name=weight"` // want "optionalorrequired: field PreferredSchedulingTerm.Weight must be marked as optional or required" // A node selector term, associated with the corresponding weight. // want "commentstart: godoc for field PreferredSchedulingTerm.Preference should start with 'preference ...'" - Preference NodeSelectorTerm `json:"preference" protobuf:"bytes,2,opt,name=preference"` // want "optionalorrequired: field PreferredSchedulingTerm.Preference must be marked as optional or required" + Preference NodeSelectorTerm `json:"preference" protobuf:"bytes,2,opt,name=preference"` // want "optionalorrequired: field PreferredSchedulingTerm.Preference must be marked as optional or required" "nonpointerstructs: field PreferredSchedulingTerm.Preference is a non-pointer struct with no required fields. It must be marked as optional." } // The node this Taint is attached to has the "effect" on diff --git a/tests/integration/testdata/default_configurations/object_references.go b/tests/integration/testdata/default_configurations/object_references.go index 09091ef2..a4e3c707 100644 --- a/tests/integration/testdata/default_configurations/object_references.go +++ b/tests/integration/testdata/default_configurations/object_references.go @@ -1,7 +1,5 @@ package defaultconfigurations -import "k8s.io/apimachinery/pkg/types" - // ObjectReference contains enough information to let you inspect or modify the referred object. // --- // New uses of this type are discouraged because of difficulty describing its usage when embedded in APIs. @@ -33,10 +31,11 @@ type ObjectReference struct { // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names // +optional Name string `json:"name,omitempty" protobuf:"bytes,3,opt,name=name"` // want "optionalfields: field Name should be a pointer." - // UID of the referent. // want "commentstart: godoc for field ObjectReference.UID should start with 'uid ...'" + // UID of the referent. // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids // +optional - UID types.UID `json:"uid,omitempty" protobuf:"bytes,4,opt,name=uid,casttype=k8s.io/apimachinery/pkg/types.UID"` // want "optionalfields: field UID should be a pointer." + // UID types.UID `json:"uid,omitempty" protobuf:"bytes,4,opt,name=uid,casttype=k8s.io/apimachinery/pkg/types.UID"` + // API version of the referent. // want "commentstart: godoc for field ObjectReference.APIVersion should start with 'apiVersion ...'" // +optional APIVersion string `json:"apiVersion,omitempty" protobuf:"bytes,5,opt,name=apiVersion"` // want "optionalfields: field APIVersion should be a pointer." diff --git a/tests/integration/testdata/default_configurations/volume.go b/tests/integration/testdata/default_configurations/volume.go index 4e63238e..684c8fed 100644 --- a/tests/integration/testdata/default_configurations/volume.go +++ b/tests/integration/testdata/default_configurations/volume.go @@ -1634,7 +1634,7 @@ type PersistentVolumeClaimTemplate struct { // copied unchanged into the PVC that gets created from this // template. The same fields as in a PersistentVolumeClaim // are also valid here. - Spec PersistentVolumeClaimSpec `json:"spec" protobuf:"bytes,2,name=spec"` // want "optionalorrequired: field PersistentVolumeClaimTemplate.Spec must be marked as optional or required" + Spec PersistentVolumeClaimSpec `json:"spec" protobuf:"bytes,2,name=spec"` // want "optionalorrequired: field PersistentVolumeClaimTemplate.Spec must be marked as optional or required" "nonpointerstructs: field PersistentVolumeClaimTemplate.Spec is a non-pointer struct with no required fields. It must be marked as optional." } // Adapts a ConfigMap into a projected volume. From 0159f57d7e70a21267ea50315e96e94b3c7bf0d5 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 11 Nov 2025 10:30:54 -0500 Subject: [PATCH 3/3] Add automatic fixes to nonpointerstructs --- docs/linters.md | 15 +++ pkg/analysis/nonpointerstructs/analyzer.go | 107 ++++++++++++++++-- .../nonpointerstructs/analyzer_test.go | 4 +- pkg/analysis/nonpointerstructs/config.go | 29 +++++ pkg/analysis/nonpointerstructs/initializer.go | 36 +++++- .../testdata/src/a/a.go.golden | 75 ++++++++++++ 6 files changed, 255 insertions(+), 11 deletions(-) create mode 100644 pkg/analysis/nonpointerstructs/config.go create mode 100644 pkg/analysis/nonpointerstructs/testdata/src/a/a.go.golden diff --git a/docs/linters.md b/docs/linters.md index 4e96bca9..e3437979 100644 --- a/docs/linters.md +++ b/docs/linters.md @@ -529,6 +529,21 @@ If there are no required fields, the struct is implicitly optional and must be m To have an optional struct field that includes required fields, the struct must be a pointer. To have a required struct field that includes no required fields, the struct must be a pointer. +### Configuration + +```yaml +lintersConfig: + nonpointerstructs: + preferredRequiredMarker: required | kubebuilder:validation:Required | k8s:required # The preferred required marker to use for required fields when providing fixes. Defaults to `required`. + preferredOptionalMarker: optional | kubebuilder:validation:Optional | k8s:optional # The preferred optional marker to use for optional fields when providing fixes. Defaults to `optional`. +``` + +### Fixes + +The `nonpointerstructs` linter can automatically fix non-pointer struct fields that are not marked as required or optional. + +It will suggest to mark the field as required or optional, depending on the fields within the non-pointer struct. + ## NoNullable The `nonullable` linter ensures that types and fields do not have the `nullable` marker. diff --git a/pkg/analysis/nonpointerstructs/analyzer.go b/pkg/analysis/nonpointerstructs/analyzer.go index 751b183e..08448b1a 100644 --- a/pkg/analysis/nonpointerstructs/analyzer.go +++ b/pkg/analysis/nonpointerstructs/analyzer.go @@ -30,29 +30,45 @@ import ( const name = "nonpointerstructs" -func newAnalyzer() *analysis.Analyzer { +func newAnalyzer(cfg *Config) *analysis.Analyzer { + if cfg == nil { + cfg = &Config{} + } + + defaultConfig(cfg) + + a := &analyzer{ + preferredRequiredMarker: cfg.PreferredRequiredMarker, + preferredOptionalMarker: cfg.PreferredOptionalMarker, + } + return &analysis.Analyzer{ Name: name, Doc: "Checks that non-pointer structs that contain required fields are marked as required. Non-pointer structs that contain no required fields are marked as optional.", - Run: run, + Run: a.run, Requires: []*analysis.Analyzer{inspector.Analyzer}, } } -func run(pass *analysis.Pass) (any, error) { +type analyzer struct { + preferredRequiredMarker string + preferredOptionalMarker string +} + +func (a *analyzer) run(pass *analysis.Pass) (any, error) { inspect, ok := pass.ResultOf[inspector.Analyzer].(inspector.Inspector) if !ok { return nil, kalerrors.ErrCouldNotGetInspector } inspect.InspectFields(func(field *ast.Field, jsonTagInfo extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, qualifiedFieldName string) { - checkField(pass, field, markersAccess, jsonTagInfo, qualifiedFieldName) + a.checkField(pass, field, markersAccess, jsonTagInfo, qualifiedFieldName) }) return nil, nil //nolint:nilnil } -func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string) { +func (a *analyzer) checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, jsonTagInfo extractjsontags.FieldTagInfo, qualifiedFieldName string) { if field.Type == nil { return } @@ -74,9 +90,9 @@ func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markershelp case hasRequiredField && isRequired, !hasRequiredField && isOptional: // This is the desired case. case hasRequiredField: - pass.Reportf(field.Pos(), "field %s is a non-pointer struct with required fields. It must be marked as required.", qualifiedFieldName) + a.handleShouldBeRequired(pass, field, markersAccess, qualifiedFieldName) case !hasRequiredField: - pass.Reportf(field.Pos(), "field %s is a non-pointer struct with no required fields. It must be marked as optional.", qualifiedFieldName) + a.handleShouldBeOptional(pass, field, markersAccess, qualifiedFieldName) } } @@ -113,3 +129,80 @@ func hasRequiredField(structType *ast.StructType, markersAccess markershelper.Ma return false } + +func defaultConfig(cfg *Config) { + if cfg.PreferredRequiredMarker == "" { + cfg.PreferredRequiredMarker = markers.RequiredMarker + } + + if cfg.PreferredOptionalMarker == "" { + cfg.PreferredOptionalMarker = markers.OptionalMarker + } +} + +func (a *analyzer) handleShouldBeRequired(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { + fieldMarkers := markersAccess.FieldMarkers(field) + + textEdits := []analysis.TextEdit{} + + for _, marker := range []string{markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker} { + for _, m := range fieldMarkers.Get(marker) { + textEdits = append(textEdits, analysis.TextEdit{ + Pos: m.Pos, + End: m.End + 1, // Add 1 to include the newline character + NewText: nil, + }) + } + } + + textEdits = append(textEdits, analysis.TextEdit{ + Pos: field.Pos(), + End: field.Pos(), + NewText: fmt.Appendf(nil, "// +%s\n", a.preferredRequiredMarker), + }) + + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + End: field.Pos(), + Message: fmt.Sprintf("field %s is a non-pointer struct with required fields. It must be marked as required.", qualifiedFieldName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "should mark the field as required", + TextEdits: textEdits, + }, + }, + }) +} + +func (a *analyzer) handleShouldBeOptional(pass *analysis.Pass, field *ast.Field, markersAccess markershelper.Markers, qualifiedFieldName string) { + fieldMarkers := markersAccess.FieldMarkers(field) + + textEdits := []analysis.TextEdit{} + + for _, marker := range []string{markers.RequiredMarker, markers.KubebuilderRequiredMarker, markers.K8sRequiredMarker} { + for _, m := range fieldMarkers.Get(marker) { + textEdits = append(textEdits, analysis.TextEdit{ + Pos: m.Pos, + End: m.End + 1, // Add 1 to include the newline character + NewText: nil, + }) + } + } + + textEdits = append(textEdits, analysis.TextEdit{ + Pos: field.Pos(), + End: field.Pos(), + NewText: fmt.Appendf(nil, "// +%s\n", a.preferredOptionalMarker), + }) + + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s is a non-pointer struct with no required fields. It must be marked as optional.", qualifiedFieldName), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: "should mark the field as optional", + TextEdits: textEdits, + }, + }, + }) +} diff --git a/pkg/analysis/nonpointerstructs/analyzer_test.go b/pkg/analysis/nonpointerstructs/analyzer_test.go index a1568b8e..36d52fe3 100644 --- a/pkg/analysis/nonpointerstructs/analyzer_test.go +++ b/pkg/analysis/nonpointerstructs/analyzer_test.go @@ -25,10 +25,10 @@ import ( func Test(t *testing.T) { testdata := analysistest.TestData() - analyzer, err := nonpointerstructs.Initializer().Init(nil) + analyzer, err := nonpointerstructs.Initializer().Init(&nonpointerstructs.Config{}) if err != nil { t.Fatalf("initializing nonpointerstructs linter: %v", err) } - analysistest.Run(t, testdata, analyzer, "a") + analysistest.RunWithSuggestedFixes(t, testdata, analyzer, "a") } diff --git a/pkg/analysis/nonpointerstructs/config.go b/pkg/analysis/nonpointerstructs/config.go new file mode 100644 index 00000000..c4956590 --- /dev/null +++ b/pkg/analysis/nonpointerstructs/config.go @@ -0,0 +1,29 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package nonpointerstructs + +// Config is the configuration for the nonpointerstructs linter. +type Config struct { + // preferredRequiredMarker is the preferred marker to use for required fields when providing fixes. + // If this field is not set, the default value is "required". + // Valid values are "required" and "kubebuilder:validation:Required" and "k8s:required". + PreferredRequiredMarker string `json:"preferredRequiredMarker"` + + // preferredOptionalMarker is the preferred marker to use for optional fields when providing fixes. + // If this field is not set, the default value is "optional". + // Valid values are "optional" and "kubebuilder:validation:Optional" and "k8s:optional". + PreferredOptionalMarker string `json:"preferredOptionalMarker"` +} diff --git a/pkg/analysis/nonpointerstructs/initializer.go b/pkg/analysis/nonpointerstructs/initializer.go index a46122a9..c615e7d1 100644 --- a/pkg/analysis/nonpointerstructs/initializer.go +++ b/pkg/analysis/nonpointerstructs/initializer.go @@ -16,8 +16,13 @@ limitations under the License. package nonpointerstructs import ( + "fmt" + + "golang.org/x/tools/go/analysis" + "k8s.io/apimachinery/pkg/util/validation/field" "sigs.k8s.io/kube-api-linter/pkg/analysis/initializer" "sigs.k8s.io/kube-api-linter/pkg/analysis/registry" + "sigs.k8s.io/kube-api-linter/pkg/markers" ) func init() { @@ -27,9 +32,36 @@ func init() { // Initializer returns the AnalyzerInitializer for this // Analyzer so that it can be added to the registry. func Initializer() initializer.AnalyzerInitializer { - return initializer.NewInitializer( + return initializer.NewConfigurableInitializer( name, - newAnalyzer(), + initAnalyzer, true, + validateConfig, ) } + +func initAnalyzer(cfg *Config) (*analysis.Analyzer, error) { + return newAnalyzer(cfg), nil +} + +func validateConfig(cfg *Config, fldPath *field.Path) field.ErrorList { + if cfg == nil { + return field.ErrorList{} + } + + fieldErrors := field.ErrorList{} + + switch cfg.PreferredRequiredMarker { + case "", markers.RequiredMarker, markers.KubebuilderRequiredMarker, markers.K8sRequiredMarker: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("preferredRequiredMarker"), cfg.PreferredRequiredMarker, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", markers.RequiredMarker, markers.KubebuilderRequiredMarker, markers.K8sRequiredMarker))) + } + + switch cfg.PreferredOptionalMarker { + case "", markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker: + default: + fieldErrors = append(fieldErrors, field.Invalid(fldPath.Child("preferredOptionalMarker"), cfg.PreferredOptionalMarker, fmt.Sprintf("invalid value, must be one of %q, %q, %q or omitted", markers.OptionalMarker, markers.KubebuilderOptionalMarker, markers.K8sOptionalMarker))) + } + + return fieldErrors +} diff --git a/pkg/analysis/nonpointerstructs/testdata/src/a/a.go.golden b/pkg/analysis/nonpointerstructs/testdata/src/a/a.go.golden new file mode 100644 index 00000000..3683b600 --- /dev/null +++ b/pkg/analysis/nonpointerstructs/testdata/src/a/a.go.golden @@ -0,0 +1,75 @@ +package a + +type A struct { + // +required + WithRequired WithRequiredField `json:"withRequired"` // want "field A.WithRequired is a non-pointer struct with required fields. It must be marked as required." + // +optional + WithOptional WithOptionalField `json:"withOptional"` // want "field A.WithOptional is a non-pointer struct with no required fields. It must be marked as optional." + // +required + WithRequiredAndOptional WithRequiredAndOptionalField `json:"withRequiredAndOptional"` // want "field A.WithRequiredAndOptional is a non-pointer struct with required fields. It must be marked as required." + // +required + WithOptionalAndMinProperties WithOptionalFieldsAndMinProperties `json:"withOptionalAndMinProperties"` // want "field A.WithOptionalAndMinProperties is a non-pointer struct with required fields. It must be marked as required." + + // No diagnostic with correct markers + + // +required + WithRequiredFieldCorrect WithRequiredField `json:"withRequiredFieldCorrect"` + + // +optional + WithOptionalFieldCorrect WithOptionalField `json:"withOptionalFieldCorrect"` + + // +kubebuilder:validation:Required + WithRequiredAndOptionalFieldCorrect WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldCorrect"` + + // +k8s:required + WithOptionalFieldsAndMinPropertiesCorrect WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesCorrect"` + + // Diagnostic with incorrect markers + + // +required + WithRequiredFieldIncorrect WithRequiredField `json:"withRequiredFieldIncorrect"` // want "field A.WithRequiredFieldIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // +optional + WithOptionalFieldIncorrect WithOptionalField `json:"withOptionalFieldIncorrect"` // want "field A.WithOptionalFieldIncorrect is a non-pointer struct with no required fields. It must be marked as optional." + + // +required + WithRequiredAndOptionalFieldIncorrect WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldIncorrect"` // want "field A.WithRequiredAndOptionalFieldIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // +required + WithOptionalFieldsAndMinPropertiesIncorrect WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesIncorrect"` // want "field A.WithOptionalFieldsAndMinPropertiesIncorrect is a non-pointer struct with required fields. It must be marked as required." + + // Pointers are ignored in this linter. + WithRequiredFieldAndPointer *WithRequiredField `json:"withRequiredFieldAndPointer"` + WithOptionalFieldAndPointer *WithOptionalField `json:"withOptionalFieldAndPointer"` + WithRequiredAndOptionalFieldAndPointer *WithRequiredAndOptionalField `json:"withRequiredAndOptionalFieldAndPointer"` + WithOptionalFieldsAndMinPropertiesAndPointer *WithOptionalFieldsAndMinProperties `json:"withOptionalFieldsAndMinPropertiesAndPointer"` + + // Inline structs are ignored in this linter. + WithRequiredField `json:",inline"` + WithOptionalField `json:",inline"` + WithRequiredAndOptionalField `json:",inline"` + WithOptionalFieldsAndMinProperties `json:",inline"` +} + +type WithRequiredField struct { + // +required + RequiredField string `json:"requiredField"` +} + +type WithOptionalField struct { + // +optional + OptionalField string `json:"optionalField"` +} + +type WithRequiredAndOptionalField struct { + // +k8s:required + RequiredField string `json:"requiredField"` + // +k8s:optional + OptionalField string `json:"optionalField"` +} + +// +kubebuilder:validation:MinProperties=1 +type WithOptionalFieldsAndMinProperties struct { + // +k8s:optional + OptionalField string `json:"optionalField"` +}