From 3c18d7e978aaff340aafcd752a98c4dd07f6868e Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Thu, 26 Jun 2025 09:45:58 -0400 Subject: [PATCH 1/3] linters: add forbiddenmarkers and nonullable linters Signed-off-by: Bryce Palmer --- pkg/analysis/forbiddenmarkers/analyzer.go | 121 ++++++++++++++++++ .../forbiddenmarkers/analyzer_test.go | 18 +++ pkg/analysis/forbiddenmarkers/doc.go | 35 +++++ pkg/analysis/forbiddenmarkers/initializer.go | 45 +++++++ .../forbiddenmarkers/testdata/src/a/a.go | 20 +++ .../testdata/src/a/a.go.golden | 17 +++ pkg/analysis/nonullable/analyzer.go | 37 ++++++ pkg/analysis/nonullable/analyzer_test.go | 12 ++ pkg/analysis/nonullable/doc.go | 23 ++++ pkg/analysis/nonullable/initializer.go | 45 +++++++ pkg/analysis/nonullable/testdata/src/a/a.go | 20 +++ .../nonullable/testdata/src/a/a.go.golden | 18 +++ pkg/analysis/registry.go | 4 + pkg/analysis/registry_test.go | 3 + pkg/config/linters_config.go | 9 ++ 15 files changed, 427 insertions(+) create mode 100644 pkg/analysis/forbiddenmarkers/analyzer.go create mode 100644 pkg/analysis/forbiddenmarkers/analyzer_test.go create mode 100644 pkg/analysis/forbiddenmarkers/doc.go create mode 100644 pkg/analysis/forbiddenmarkers/initializer.go create mode 100644 pkg/analysis/forbiddenmarkers/testdata/src/a/a.go create mode 100644 pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden create mode 100644 pkg/analysis/nonullable/analyzer.go create mode 100644 pkg/analysis/nonullable/analyzer_test.go create mode 100644 pkg/analysis/nonullable/doc.go create mode 100644 pkg/analysis/nonullable/initializer.go create mode 100644 pkg/analysis/nonullable/testdata/src/a/a.go create mode 100644 pkg/analysis/nonullable/testdata/src/a/a.go.golden diff --git a/pkg/analysis/forbiddenmarkers/analyzer.go b/pkg/analysis/forbiddenmarkers/analyzer.go new file mode 100644 index 00000000..68ae2b91 --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/analyzer.go @@ -0,0 +1,121 @@ +/* +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 forbiddenmarkers + +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" + "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/config" +) + +const name = "forbiddenmarkers" + +type analyzer struct { + forbiddenMarkers []string +} + +func NewAnalyzer(cfg config.ForbiddenMarkersConfig) *analysis.Analyzer { + a := &analyzer{ + forbiddenMarkers: cfg.Markers, + } + + return &analysis.Analyzer{ + Name: name, + Doc: "Check that no forbidden markers are present on types and fields.", + Run: a.run, + Requires: []*analysis.Analyzer{inspector.Analyzer}, + } +} + +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, stack []ast.Node, _ extractjsontags.FieldTagInfo, markersAccess markers.Markers) { + checkField(pass, field, markersAccess, a.forbiddenMarkers) + }) + + inspect.InspectTypeSpec(func(typeSpec *ast.TypeSpec, markersAccess markers.Markers) { + checkType(pass, typeSpec, markersAccess, a.forbiddenMarkers) + }) + + return nil, nil //nolint:nilnil +} + +func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, forbiddenMarkers []string) { + if field == nil || len(field.Names) == 0 { + return + } + + markers := utils.TypeAwareMarkerCollectionForField(pass, markersAccess, field) + check(markers, forbiddenMarkers, reportField(pass, field)) +} + +func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markers.Markers, forbiddenMarkers []string) { + if typeSpec == nil { + return + } + + markers := markersAccess.TypeMarkers(typeSpec) + check(markers, forbiddenMarkers, reportType(pass, typeSpec)) +} + +func check(markerSet markers.MarkerSet, forbiddenMarkers []string, reportFunc func(marker markers.Marker)) { + for _, marker := range forbiddenMarkers { + marks := markerSet.Get(marker) + for _, mark := range marks { + reportFunc(mark) + } + } +} + +func reportField(pass *analysis.Pass, field *ast.Field) func(marker markers.Marker) { + return func(marker markers.Marker) { + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field %s has forbidden marker %q", field.Names[0].Name, marker.Identifier), + SuggestedFixes: []analysis.SuggestedFix{ + { + Message: fmt.Sprintf("remove forbidden marker %q", marker.Identifier), + TextEdits: []analysis.TextEdit{ + { + Pos: marker.Pos, + End: marker.End + 1, + }, + }, + }, + }, + }) + } +} + +func reportType(pass *analysis.Pass, typeSpec *ast.TypeSpec) func(marker markers.Marker) { + return func(marker markers.Marker) { + pass.Report(analysis.Diagnostic{ + Pos: typeSpec.Pos(), + Message: fmt.Sprintf("type %s has forbidden marker %q", typeSpec.Name, marker.Identifier), + }) + } +} diff --git a/pkg/analysis/forbiddenmarkers/analyzer_test.go b/pkg/analysis/forbiddenmarkers/analyzer_test.go new file mode 100644 index 00000000..470bf482 --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/analyzer_test.go @@ -0,0 +1,18 @@ +package forbiddenmarkers_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "sigs.k8s.io/kube-api-linter/pkg/analysis/forbiddenmarkers" + "sigs.k8s.io/kube-api-linter/pkg/config" +) + +func TestWithConfiguration(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, forbiddenmarkers.NewAnalyzer(config.ForbiddenMarkersConfig{ + Markers: []string{ + "forbidden", + }, + }), "a/...") +} diff --git a/pkg/analysis/forbiddenmarkers/doc.go b/pkg/analysis/forbiddenmarkers/doc.go new file mode 100644 index 00000000..f7343bcf --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/doc.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. +*/ + +/* +* The `forbiddenmarkers` linter ensures that types and fields do not contain any markers +* that are forbidden. +* +* By default, `forbiddenmarkers` is not enabled. +* +* It can be configured with a list of marker identifiers that are forbidden +* ```yaml +* lintersConfig: +* forbiddenMarkers: +* markers: +* - some:forbidden:marker +* - anotherforbidden +* ``` +* +* Fixes are suggested to remove all markers that are forbidden. +* + */ +package forbiddenmarkers diff --git a/pkg/analysis/forbiddenmarkers/initializer.go b/pkg/analysis/forbiddenmarkers/initializer.go new file mode 100644 index 00000000..1ab4e1a0 --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/initializer.go @@ -0,0 +1,45 @@ +/* +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 forbiddenmarkers + +import ( + "golang.org/x/tools/go/analysis" + "sigs.k8s.io/kube-api-linter/pkg/config" +) + +// Initializer returns the AnalyzerInitializer for this +// Analyzer so that it can be added to the registry. +func Initializer() initializer { + return initializer{} +} + +// intializer implements the AnalyzerInitializer interface. +type initializer struct{} + +// Name returns the name of the Analyzer. +func (initializer) Name() string { + return name +} + +// Init returns the intialized Analyzer. +func (initializer) Init(cfg config.LintersConfig) (*analysis.Analyzer, error) { + return NewAnalyzer(cfg.ForbiddenMarkers), nil +} + +// Default determines whether this Analyzer is on by default, or not. +func (initializer) Default() bool { + return false +} diff --git a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go new file mode 100644 index 00000000..16336038 --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go @@ -0,0 +1,20 @@ +package a + +// +forbidden +type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "forbidden"` + +// +allowed +type AllowedMarkerType string + +type Test struct { + // +forbidden + ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "forbidden"` + + ForbiddenMarkerFieldTypeAlias ForbiddenMarkerType `json:"forbiddenMarkerFieldTypeAlias"` // want `field ForbiddenMarkerFieldTypeAlias has forbidden marker "forbidden"` + + // +allowed + AllowedMarkerField string `json:"allowedMarkerField"` + + AllowedMarkerFieldTypeAlias AllowedMarkerType `json:"AllowedMarkerFieldTypeAlias"` +} + diff --git a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden new file mode 100644 index 00000000..f91ef71e --- /dev/null +++ b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden @@ -0,0 +1,17 @@ +package a + +type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "forbidden"` + +// +allowed +type AllowedMarkerType string + +type Test struct { + ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "forbidden"` + + ForbiddenMarkerFieldTypeAlias ForbiddenMarkerType `json:"forbiddenMarkerFieldTypeAlias"` // want `field ForbiddenMarkerFieldTypeAlias has forbidden marker "forbidden"` + + // +allowed + AllowedMarkerField string `json:"allowedMarkerField"` + + AllowedMarkerFieldTypeAlias AllowedMarkerType `json:"AllowedMarkerFieldTypeAlias"` +} diff --git a/pkg/analysis/nonullable/analyzer.go b/pkg/analysis/nonullable/analyzer.go new file mode 100644 index 00000000..79413036 --- /dev/null +++ b/pkg/analysis/nonullable/analyzer.go @@ -0,0 +1,37 @@ +/* +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 nonullable + +import ( + "golang.org/x/tools/go/analysis" + "sigs.k8s.io/kube-api-linter/pkg/analysis/forbiddenmarkers" + "sigs.k8s.io/kube-api-linter/pkg/config" +) + +const name = "nonullable" + +func newAnalyzer() *analysis.Analyzer { + analyzer := forbiddenmarkers.NewAnalyzer(config.ForbiddenMarkersConfig{ + Markers: []string{ + "nullable", + }, + }) + + analyzer.Name = name + analyzer.Doc = "Check that nullable marker is not present on any types or fields." + + return analyzer +} diff --git a/pkg/analysis/nonullable/analyzer_test.go b/pkg/analysis/nonullable/analyzer_test.go new file mode 100644 index 00000000..9d5cce41 --- /dev/null +++ b/pkg/analysis/nonullable/analyzer_test.go @@ -0,0 +1,12 @@ +package nonullable + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, newAnalyzer(), "a/...") +} diff --git a/pkg/analysis/nonullable/doc.go b/pkg/analysis/nonullable/doc.go new file mode 100644 index 00000000..2b244c22 --- /dev/null +++ b/pkg/analysis/nonullable/doc.go @@ -0,0 +1,23 @@ +/* +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. +*/ + +/* +* The `nonullable` linter ensures that types and fields do not have the `nullable` marker. +* +* Fixes are suggested to remove the `nullable` marker. +* + */ +package nonullable diff --git a/pkg/analysis/nonullable/initializer.go b/pkg/analysis/nonullable/initializer.go new file mode 100644 index 00000000..f4b4aa10 --- /dev/null +++ b/pkg/analysis/nonullable/initializer.go @@ -0,0 +1,45 @@ +/* +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 nonullable + +import ( + "golang.org/x/tools/go/analysis" + "sigs.k8s.io/kube-api-linter/pkg/config" +) + +// Initializer returns the AnalyzerInitializer for this +// Analyzer so that it can be added to the registry. +func Initializer() initializer { + return initializer{} +} + +// intializer implements the AnalyzerInitializer interface. +type initializer struct{} + +// Name returns the name of the Analyzer. +func (initializer) Name() string { + return name +} + +// Init returns the intialized Analyzer. +func (initializer) Init(_ config.LintersConfig) (*analysis.Analyzer, error) { + return newAnalyzer(), nil +} + +// Default determines whether this Analyzer is on by default, or not. +func (initializer) Default() bool { + return true +} diff --git a/pkg/analysis/nonullable/testdata/src/a/a.go b/pkg/analysis/nonullable/testdata/src/a/a.go new file mode 100644 index 00000000..409dc5ed --- /dev/null +++ b/pkg/analysis/nonullable/testdata/src/a/a.go @@ -0,0 +1,20 @@ +package a + +// +nullable +type NullableMarkerType string // want `type NullableMarkerType has forbidden marker "nullable"` + +// +allowed +type AllowedMarkerType string + +type Test struct { + // +nullable + NullableMarkerField string `json:"nullableMarkerField"`// want `field NullableMarkerField has forbidden marker "nullable"` + + NullableMarkerFieldTypeAlias NullableMarkerType `json:"nullableMarkerFieldTypeAlias"` // want `field NullableMarkerFieldTypeAlias has forbidden marker "nullable"` + + // +allowed + AllowedMarkerField string `json:"allowedMarkerField"` + + AllowedMarkerFieldTypeAlias AllowedMarkerType `json:"AllowedMarkerFieldTypeAlias"` +} + diff --git a/pkg/analysis/nonullable/testdata/src/a/a.go.golden b/pkg/analysis/nonullable/testdata/src/a/a.go.golden new file mode 100644 index 00000000..24beb282 --- /dev/null +++ b/pkg/analysis/nonullable/testdata/src/a/a.go.golden @@ -0,0 +1,18 @@ +package a + +type NullableMarkerType string // want `type NullableMarkerType has forbidden marker "nullable"` + +// +allowed +type AllowedMarkerType string + +type Test struct { + NullableMarkerField string `json:"nullableMarkerField"`// want `field NullableMarkerField has forbidden marker "nullable"` + + NullableMarkerFieldTypeAlias NullableMarkerType `json:"nullableMarkerFieldTypeAlias"` // want `field NullableMarkerFieldTypeAlias has forbidden marker "nullable"` + + // +allowed + AllowedMarkerField string `json:"allowedMarkerField"` + + AllowedMarkerFieldTypeAlias AllowedMarkerType `json:"AllowedMarkerFieldTypeAlias"` +} + diff --git a/pkg/analysis/registry.go b/pkg/analysis/registry.go index 6e8f8fc5..3e649c08 100644 --- a/pkg/analysis/registry.go +++ b/pkg/analysis/registry.go @@ -22,12 +22,14 @@ import ( "sigs.k8s.io/kube-api-linter/pkg/analysis/commentstart" "sigs.k8s.io/kube-api-linter/pkg/analysis/conditions" "sigs.k8s.io/kube-api-linter/pkg/analysis/duplicatemarkers" + "sigs.k8s.io/kube-api-linter/pkg/analysis/forbiddenmarkers" "sigs.k8s.io/kube-api-linter/pkg/analysis/integers" "sigs.k8s.io/kube-api-linter/pkg/analysis/jsontags" "sigs.k8s.io/kube-api-linter/pkg/analysis/maxlength" "sigs.k8s.io/kube-api-linter/pkg/analysis/nobools" "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/nonullable" "sigs.k8s.io/kube-api-linter/pkg/analysis/nophase" "sigs.k8s.io/kube-api-linter/pkg/analysis/optionalfields" "sigs.k8s.io/kube-api-linter/pkg/analysis/optionalorrequired" @@ -79,12 +81,14 @@ func NewRegistry() Registry { conditions.Initializer(), commentstart.Initializer(), duplicatemarkers.Initializer(), + forbiddenmarkers.Initializer(), integers.Initializer(), jsontags.Initializer(), maxlength.Initializer(), nobools.Initializer(), nofloats.Initializer(), nomaps.Initializer(), + nonullable.Initializer(), nophase.Initializer(), optionalfields.Initializer(), optionalorrequired.Initializer(), diff --git a/pkg/analysis/registry_test.go b/pkg/analysis/registry_test.go index 42c6fdfe..ec1bc712 100644 --- a/pkg/analysis/registry_test.go +++ b/pkg/analysis/registry_test.go @@ -44,6 +44,7 @@ var _ = Describe("Registry", func() { "requiredfields", "statusoptional", "uniquemarkers", + "nonullable", )) }) }) @@ -68,6 +69,8 @@ var _ = Describe("Registry", func() { "statusoptional", "statussubresource", "uniquemarkers", + "nonullable", + "forbiddenmarkers", )) }) }) diff --git a/pkg/config/linters_config.go b/pkg/config/linters_config.go index 85e8235d..a36e05dc 100644 --- a/pkg/config/linters_config.go +++ b/pkg/config/linters_config.go @@ -40,6 +40,9 @@ type LintersConfig struct { // uniqueMarkers contains configuration for the uniquemarkers linter. UniqueMarkers UniqueMarkersConfig `json:"uniqueMarkers"` + + // forbiddenMarkers contains configuration for the forbiddenmarkers linter. + ForbiddenMarkers ForbiddenMarkersConfig `json:"forbiddenMarkers"` } // ConditionsFirstField is the policy for the conditions linter. @@ -293,3 +296,9 @@ type UniqueMarker struct { // Entries must be unique. Attributes []string `json:"attributes"` } + +// ForbiddenMarkersConfig contains the configuration for the forbiddenmarkers linter. +type ForbiddenMarkersConfig struct { + // markers configures the marker identifiers that are forbidden. + Markers []string `json:"markers"` +} From de8da93b91fc320d667c930e0650c8f87a69f6c2 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Thu, 26 Jun 2025 09:53:40 -0400 Subject: [PATCH 2/3] fixup! linter issues Signed-off-by: Bryce Palmer --- pkg/analysis/forbiddenmarkers/analyzer.go | 2 ++ pkg/analysis/forbiddenmarkers/analyzer_test.go | 15 +++++++++++++++ pkg/analysis/nonullable/analyzer_test.go | 15 +++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/pkg/analysis/forbiddenmarkers/analyzer.go b/pkg/analysis/forbiddenmarkers/analyzer.go index 68ae2b91..392c7fe8 100644 --- a/pkg/analysis/forbiddenmarkers/analyzer.go +++ b/pkg/analysis/forbiddenmarkers/analyzer.go @@ -34,6 +34,8 @@ type analyzer struct { forbiddenMarkers []string } +// NewAnalyzer creates a new analysis.Analyzer for the forbiddenmarkers +// linter based on the provided config.ForbiddenMarkersConfig. func NewAnalyzer(cfg config.ForbiddenMarkersConfig) *analysis.Analyzer { a := &analyzer{ forbiddenMarkers: cfg.Markers, diff --git a/pkg/analysis/forbiddenmarkers/analyzer_test.go b/pkg/analysis/forbiddenmarkers/analyzer_test.go index 470bf482..00fb8c59 100644 --- a/pkg/analysis/forbiddenmarkers/analyzer_test.go +++ b/pkg/analysis/forbiddenmarkers/analyzer_test.go @@ -1,3 +1,18 @@ +/* +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 forbiddenmarkers_test import ( diff --git a/pkg/analysis/nonullable/analyzer_test.go b/pkg/analysis/nonullable/analyzer_test.go index 9d5cce41..7f1b78a8 100644 --- a/pkg/analysis/nonullable/analyzer_test.go +++ b/pkg/analysis/nonullable/analyzer_test.go @@ -1,3 +1,18 @@ +/* +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 nonullable import ( From 36bf9d5270aa9b1e4b56d71af433402283bd33e9 Mon Sep 17 00:00:00 2001 From: Bryce Palmer Date: Fri, 27 Jun 2025 15:45:04 -0400 Subject: [PATCH 3/3] wip: support for attribute and value constraints Signed-off-by: Bryce Palmer --- pkg/analysis/forbiddenmarkers/analyzer.go | 77 +++++++++++++++++-- .../forbiddenmarkers/analyzer_test.go | 59 +++++++++++++- .../forbiddenmarkers/testdata/src/a/a.go | 15 ++-- .../testdata/src/a/a.go.golden | 9 ++- pkg/analysis/nonullable/analyzer.go | 12 +-- pkg/config/linters_config.go | 38 ++++++++- 6 files changed, 185 insertions(+), 25 deletions(-) diff --git a/pkg/analysis/forbiddenmarkers/analyzer.go b/pkg/analysis/forbiddenmarkers/analyzer.go index 392c7fe8..ba124f56 100644 --- a/pkg/analysis/forbiddenmarkers/analyzer.go +++ b/pkg/analysis/forbiddenmarkers/analyzer.go @@ -31,22 +31,46 @@ import ( const name = "forbiddenmarkers" type analyzer struct { - forbiddenMarkers []string + forbiddenMarkers []config.ForbiddenMarker +} + +// ForbiddenMarkersOptions is a function that configures the +// forbiddenmarkers analysis.Analyzer +type ForbiddenMarkersOption func(a *analysis.Analyzer) + +// WithName sets the name of the forbiddenmarkers analysis.Analyzer +func WithName(name string) ForbiddenMarkersOption { + return func(a *analysis.Analyzer) { + a.Name = name + } +} + +// WithDoc sets the doc string of the forbiddenmarkers analysis.Analyzer +func WithDoc(doc string) ForbiddenMarkersOption { + return func(a *analysis.Analyzer) { + a.Doc = doc + } } // NewAnalyzer creates a new analysis.Analyzer for the forbiddenmarkers // linter based on the provided config.ForbiddenMarkersConfig. -func NewAnalyzer(cfg config.ForbiddenMarkersConfig) *analysis.Analyzer { +func NewAnalyzer(cfg config.ForbiddenMarkersConfig, opts ...ForbiddenMarkersOption) *analysis.Analyzer { a := &analyzer{ forbiddenMarkers: cfg.Markers, } - return &analysis.Analyzer{ + analyzer := &analysis.Analyzer{ Name: name, Doc: "Check that no forbidden markers are present on types and fields.", Run: a.run, Requires: []*analysis.Analyzer{inspector.Analyzer}, } + + for _, opt := range opts { + opt(analyzer) + } + + return analyzer } func (a *analyzer) run(pass *analysis.Pass) (any, error) { @@ -66,7 +90,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { return nil, nil //nolint:nilnil } -func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, forbiddenMarkers []string) { +func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Markers, forbiddenMarkers []config.ForbiddenMarker) { if field == nil || len(field.Names) == 0 { return } @@ -75,7 +99,7 @@ func checkField(pass *analysis.Pass, field *ast.Field, markersAccess markers.Mar check(markers, forbiddenMarkers, reportField(pass, field)) } -func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markers.Markers, forbiddenMarkers []string) { +func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess markers.Markers, forbiddenMarkers []config.ForbiddenMarker) { if typeSpec == nil { return } @@ -84,15 +108,52 @@ func checkType(pass *analysis.Pass, typeSpec *ast.TypeSpec, markersAccess marker check(markers, forbiddenMarkers, reportType(pass, typeSpec)) } -func check(markerSet markers.MarkerSet, forbiddenMarkers []string, reportFunc func(marker markers.Marker)) { +func check(markerSet markers.MarkerSet, forbiddenMarkers []config.ForbiddenMarker, reportFunc func(marker markers.Marker)) { for _, marker := range forbiddenMarkers { - marks := markerSet.Get(marker) + marks := markerSet.Get(marker.Identifier) for _, mark := range marks { - reportFunc(mark) + if markerMatchesAttributeRules(mark, marker.Attributes...) { + reportFunc(mark) + } } } } +// TODO: this should probably return some representation of the marker that is failing the +// attribute rules so that it can be returned to users helpfully. +func markerMatchesAttributeRules(marker markers.Marker, attrRules ...config.ForbiddenMarkerAttribute) bool { + matchesAll := true + for _, attrRule := range attrRules { + if val, ok := marker.Expressions[attrRule.Attribute]; ok { + // if no values are specified, that means the existence match is enough + // and we can continue to the next rule + if len(attrRule.Values) == 0 { + continue + } + + // if the value doesn't match one of the forbidden ones, this marker is not forbidden + matchesOneValue := false + for _, value := range attrRule.Values { + if val == value { + matchesOneValue = true + break + } + } + + if !matchesOneValue { + matchesAll = false + break + } + } + // if the marker doesn't contain the attribute for a specified rule it fails the AND + // operation. + matchesAll = false + break + } + + return matchesAll +} + func reportField(pass *analysis.Pass, field *ast.Field) func(marker markers.Marker) { return func(marker markers.Marker) { pass.Report(analysis.Diagnostic{ diff --git a/pkg/analysis/forbiddenmarkers/analyzer_test.go b/pkg/analysis/forbiddenmarkers/analyzer_test.go index 00fb8c59..96dae602 100644 --- a/pkg/analysis/forbiddenmarkers/analyzer_test.go +++ b/pkg/analysis/forbiddenmarkers/analyzer_test.go @@ -26,8 +26,63 @@ import ( func TestWithConfiguration(t *testing.T) { testdata := analysistest.TestData() analysistest.RunWithSuggestedFixes(t, testdata, forbiddenmarkers.NewAnalyzer(config.ForbiddenMarkersConfig{ - Markers: []string{ - "forbidden", + Markers: []config.ForbiddenMarker{ + { + Identifier: "custom:forbidden", + }, + { + Identifier: "custom:AttrNoValues", + Attributes: []config.ForbiddenMarkerAttribute{ + { + Attribute: "fruit", + }, + }, + }, + { + Identifier: "custom:AttrValues", + Attributes: []config.ForbiddenMarkerAttribute{ + { + Attribute: "fruit", + Values: []string{ + "apple", + "orange", + "banana", + }, + }, + }, + }, + { + Identifier: "custom:AttrsNoValues", + Attributes: []config.ForbiddenMarkerAttribute{ + { + Attribute: "fruit", + }, + { + Attribute: "color", + }, + }, + }, + { + Identifier: "custom:AttrsValues", + Attributes: []config.ForbiddenMarkerAttribute{ + { + Attribute: "fruit", + Values: []string{ + "apple", + "orange", + "banana", + }, + }, + { + Attribute: "color", + Values: []string{ + "red", + "blue", + "green", + }, + }, + }, + }, }, }), "a/...") } diff --git a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go index 16336038..2a7f3b0a 100644 --- a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go +++ b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go @@ -1,14 +1,20 @@ package a -// +forbidden -type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "forbidden"` +// +custom:forbidden +type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "custom:forbidden"` + +// +custom:AttrNoValues:fruit=apple +type ForbiddenMarkerWithAttrType string // want `type ForbiddenMarkerWithAttrType has forbidden marker "forbidden:AttrNoValues"` // +allowed type AllowedMarkerType string +// +custom:AttrNoValues:color=blue +type AllowedMarkerWithAttrType string + type Test struct { - // +forbidden - ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "forbidden"` + // +custom:forbidden + ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "custom:forbidden"` ForbiddenMarkerFieldTypeAlias ForbiddenMarkerType `json:"forbiddenMarkerFieldTypeAlias"` // want `field ForbiddenMarkerFieldTypeAlias has forbidden marker "forbidden"` @@ -17,4 +23,3 @@ type Test struct { AllowedMarkerFieldTypeAlias AllowedMarkerType `json:"AllowedMarkerFieldTypeAlias"` } - diff --git a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden index f91ef71e..e7071b94 100644 --- a/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden +++ b/pkg/analysis/forbiddenmarkers/testdata/src/a/a.go.golden @@ -1,12 +1,17 @@ package a -type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "forbidden"` +type ForbiddenMarkerType string // want `type ForbiddenMarkerType has forbidden marker "custom:forbidden"` + +type ForbiddenMarkerWithAttrType string // want `type ForbiddenMarkerWithAttrType has forbidden marker "forbidden:AttrNoValues"` // +allowed type AllowedMarkerType string +// +custom:AttrNoValues:color=blue +type AllowedMarkerWithAttrType string + type Test struct { - ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "forbidden"` + ForbiddenMarkerField string `json:"forbiddenMarkerField"`// want `field ForbiddenMarkerField has forbidden marker "custom:forbidden"` ForbiddenMarkerFieldTypeAlias ForbiddenMarkerType `json:"forbiddenMarkerFieldTypeAlias"` // want `field ForbiddenMarkerFieldTypeAlias has forbidden marker "forbidden"` diff --git a/pkg/analysis/nonullable/analyzer.go b/pkg/analysis/nonullable/analyzer.go index 79413036..14b5730b 100644 --- a/pkg/analysis/nonullable/analyzer.go +++ b/pkg/analysis/nonullable/analyzer.go @@ -22,16 +22,16 @@ import ( ) const name = "nonullable" +const doc = "Check that nullable marker is not present on any types or fields." func newAnalyzer() *analysis.Analyzer { analyzer := forbiddenmarkers.NewAnalyzer(config.ForbiddenMarkersConfig{ - Markers: []string{ - "nullable", + Markers: []config.ForbiddenMarker{ + { + Identifier: "nullable", + }, }, - }) - - analyzer.Name = name - analyzer.Doc = "Check that nullable marker is not present on any types or fields." + }, forbiddenmarkers.WithName(name), forbiddenmarkers.WithDoc(doc)) return analyzer } diff --git a/pkg/config/linters_config.go b/pkg/config/linters_config.go index a36e05dc..1f489124 100644 --- a/pkg/config/linters_config.go +++ b/pkg/config/linters_config.go @@ -299,6 +299,40 @@ type UniqueMarker struct { // ForbiddenMarkersConfig contains the configuration for the forbiddenmarkers linter. type ForbiddenMarkersConfig struct { - // markers configures the marker identifiers that are forbidden. - Markers []string `json:"markers"` + // markers configures the markers that are forbidden. + // Entries must have unique identifiers. + Markers []ForbiddenMarker `json:"markers"` +} + +type ForbiddenMarker struct { + // identifier configures the marker identifier that is forbidden + Identifier string `json:"identifier"` + + // attributes configures the attributes that are forbidden for this marker. + // when set, only marker declarations that match the identifier + attributes + // are considered forbidden. + // + // When multiple attributes are specified, it is evaluated as an AND operation. + // For example, assuming: + // - identifier is some:Marker + // - attributes has entries for "fruit" and "color" attributes + // - marker declaration of `some:Marker:fruit=apple` would be allowed because it does not + // specify the color attribute. + // + // Entries must have unique attribute values. + Attributes []ForbiddenMarkerAttribute `json:"attributes"` +} + +type ForbiddenMarkerAttribute struct { + // attribute configures an attribute that is forbidden for the + // associated marker identifier. + Attribute string `json:"attribute"` + + // values configures the particular values for this attribute that + // is forbidden. + // If values is specified, only marker declarations with this attribute + // set to one of the values specified will be considered forbidden. + // + // Entries must be unique. + Values []string `json:"values"` }