diff --git a/.custom-gcl.yml b/.custom-gcl.yml index 99dc94dc4e6..8a619c6c308 100644 --- a/.custom-gcl.yml +++ b/.custom-gcl.yml @@ -2,7 +2,7 @@ version: v2.7.1 name: golangci-lint-custom destination: _output plugins: -- module: github.com/kgateway-dev/kgateway/v2/hack/krtequals - path: ./hack/krtequals +- module: github.com/kgateway-dev/krtequals + version: 'v0.0.0-20251210234050-024ef4e99e5c' - module: 'sigs.k8s.io/kube-api-linter' version: 'v0.0.0-20250715075424-4fab82d26a8e' # Pin to a commit while there's no tag diff --git a/.golangci.yaml b/.golangci.yaml index 1908165d330..5944086a43b 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -40,6 +40,7 @@ linters: description: Checks Equals() implementations for KRT-style semantic equality issues settings: deepEqual: false + checkUnexported: false # TODO: enable this. there's several violations that need to be fixed. kubeapilinter: type: module description: Kube API Linter lints Kube like APIs based on API conventions and best practices diff --git a/Makefile b/Makefile index 2f32ceef028..76de6ffc01f 100644 --- a/Makefile +++ b/Makefile @@ -121,7 +121,6 @@ mod-download: ## Download the dependencies .PHONY: mod-tidy-nested mod-tidy-nested: ## Tidy go mod files in nested modules @echo "Tidying hack/utils/applier..." && cd hack/utils/applier && go mod tidy - @echo "Tidying hack/krtequals..." && cd hack/krtequals && go mod tidy .PHONY: mod-tidy mod-tidy: mod-download mod-tidy-nested ## Tidy the go mod file diff --git a/hack/krtequals/analyzer.go b/hack/krtequals/analyzer.go deleted file mode 100644 index 5cacbeabe30..00000000000 --- a/hack/krtequals/analyzer.go +++ /dev/null @@ -1,295 +0,0 @@ -package krtequals - -import ( - "go/ast" - "go/token" - "go/types" - "strings" - - "golang.org/x/tools/go/analysis" - "golang.org/x/tools/go/analysis/passes/inspect" - "golang.org/x/tools/go/ast/inspector" -) - -// TODO(tim): Determine if we need to add a way for devs that define an Equals() method that's -// unrelated to KRT collection to opt out of the analyzer. - -// TODO(tim): [krtEqualsNone, krtEqualsTODO] the right naming for these markers? - -// Config controls optional checks performed by the krtequals analyzer. -type Config struct { - // DeepEqual toggles the rule that flags usage of reflect.DeepEqual inside Equals methods. - // This check is disabled by default for incremental rollout. - DeepEqual bool `json:"deepEqual"` - // TODO(time): time.Equals() for direct time.Time comparisons -} - -type analyzer struct { - cfg Config -} - -func newAnalyzer(cfg *Config) *analysis.Analyzer { - a := &analyzer{cfg: *cfg} - return &analysis.Analyzer{ - Name: "krtequals", - Doc: "Checks Equals() implementations for KRT-style semantic equality issues", - Run: a.Run, - Requires: []*analysis.Analyzer{inspect.Analyzer}, - } -} - -type structInfo struct { - name string - fields map[string]*fieldInfo -} - -type fieldInfo struct { - name string - pos token.Pos - exported bool - ignore bool - todo bool -} - -func (a *analyzer) Run(pass *analysis.Pass) (any, error) { - ins := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) - - structs := collectStructs(ins) - processedEquals := make(map[string]bool) - - ins.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) { - fd, ok := n.(*ast.FuncDecl) - if !ok || fd.Name == nil || fd.Body == nil { - return - } - if fd.Name.Name != "Equals" { - return - } - if a.cfg.DeepEqual { - checkReflectDeepEqual(pass, fd) - } - if fd.Recv == nil || len(fd.Recv.List) == 0 { - return - } - - recvField := fd.Recv.List[0] - recvTypeName := namedTypeFromExpr(recvField.Type) - if recvTypeName == "" { - return - } - if processedEquals[recvTypeName] { - return - } - processedEquals[recvTypeName] = true - - recvIdent := "" - if len(recvField.Names) > 0 && recvField.Names[0] != nil { - recvIdent = recvField.Names[0].Name - } - - paramNames := paramIdentsWithType(fd.Type.Params, recvTypeName) - - usedFields := collectUsedFieldsInEquals(fd.Body, recvIdent, paramNames) - - sinfo, ok := structs[recvTypeName] - if !ok { - return - } - for _, f := range sinfo.fields { - if !f.exported || f.ignore || f.todo { - continue - } - if !usedFields[f.name] { - pass.Reportf(f.pos, "field %q in type %q is not used in Equals; either compare it or add // +noKrtEquals", f.name, sinfo.name) - } - } - }) - - return nil, nil -} - -func collectStructs(ins *inspector.Inspector) map[string]*structInfo { - structs := make(map[string]*structInfo) - - ins.Preorder([]ast.Node{(*ast.TypeSpec)(nil)}, func(n ast.Node) { - ts, ok := n.(*ast.TypeSpec) - if !ok { - return - } - st, ok := ts.Type.(*ast.StructType) - if !ok { - return - } - - si := &structInfo{ - name: ts.Name.Name, - fields: make(map[string]*fieldInfo), - } - - if st.Fields == nil { - structs[si.name] = si - return - } - - for _, field := range st.Fields.List { - if len(field.Names) == 0 { - continue - } - ignore, todo := fieldMarkers(field) - for _, nameIdent := range field.Names { - if nameIdent == nil { - continue - } - fi := &fieldInfo{ - name: nameIdent.Name, - pos: field.Pos(), - exported: nameIdent.IsExported(), - ignore: ignore, - todo: todo, - } - si.fields[fi.name] = fi - } - } - - structs[si.name] = si - }) - - return structs -} - -func checkReflectDeepEqual(pass *analysis.Pass, fd *ast.FuncDecl) { - if pass.TypesInfo == nil { - return - } - - ast.Inspect(fd.Body, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return true - } - - pkgIdent, ok := sel.X.(*ast.Ident) - if !ok { - return true - } - - obj := pass.TypesInfo.Uses[pkgIdent] - pkgName, ok := obj.(*types.PkgName) - if !ok { - return true - } - - if pkgName.Imported().Path() == "reflect" && sel.Sel.Name == "DeepEqual" { - pass.Reportf(call.Pos(), "Equals() method uses reflect.DeepEqual which is slow and ignores //+noKrtEquals markers") - } - - return true - }) -} - -func namedTypeFromExpr(expr ast.Expr) string { - switch t := expr.(type) { - case *ast.Ident: - return t.Name - case *ast.StarExpr: - if id, ok := t.X.(*ast.Ident); ok { - return id.Name - } - } - return "" -} - -func paramIdentsWithType(params *ast.FieldList, typeName string) []string { - if params == nil { - return nil - } - var out []string - for _, field := range params.List { - if !sameNamedType(field.Type, typeName) { - continue - } - for _, nameIdent := range field.Names { - if nameIdent != nil && nameIdent.Name != "" { - out = append(out, nameIdent.Name) - } - } - } - return out -} - -func sameNamedType(expr ast.Expr, typeName string) bool { - switch t := expr.(type) { - case *ast.Ident: - return t.Name == typeName - case *ast.StarExpr: - if id, ok := t.X.(*ast.Ident); ok { - return id.Name == typeName - } - } - return false -} - -func collectUsedFieldsInEquals(body *ast.BlockStmt, recvIdent string, paramIdents []string) map[string]bool { - used := make(map[string]bool) - if body == nil { - return used - } - - paramSet := make(map[string]struct{}, len(paramIdents)) - for _, p := range paramIdents { - paramSet[p] = struct{}{} - } - - ast.Inspect(body, func(n ast.Node) bool { - sel, ok := n.(*ast.SelectorExpr) - if !ok { - return true - } - - id, ok := sel.X.(*ast.Ident) - if !ok { - return true - } - - if id.Name == recvIdent { - used[sel.Sel.Name] = true - return true - } - - if _, ok := paramSet[id.Name]; ok { - used[sel.Sel.Name] = true - } - - return true - }) - - return used -} - -func fieldMarkers(field *ast.Field) (ignore bool, todo bool) { - ignoreDoc, todoDoc := extractSpecialMarkers(field.Doc) - ignoreLine, todoLine := extractSpecialMarkers(field.Comment) - return ignoreDoc || ignoreLine, todoDoc || todoLine -} - -func extractSpecialMarkers(cg *ast.CommentGroup) (ignore bool, todo bool) { - if cg == nil { - return - } - for _, c := range cg.List { - text := strings.ToLower(c.Text) - if strings.Contains(text, "+krtequalstodo") { - ignore = true - todo = true - } - if strings.Contains(text, "+nokrtequals") { - ignore = true - } - } - return -} diff --git a/hack/krtequals/analyzer_test.go b/hack/krtequals/analyzer_test.go deleted file mode 100644 index 593fc646f76..00000000000 --- a/hack/krtequals/analyzer_test.go +++ /dev/null @@ -1,25 +0,0 @@ -package krtequals - -import ( - "testing" - - "golang.org/x/tools/go/analysis/analysistest" -) - -func TestDeepEqualCheckEnabled(t *testing.T) { - testdata := analysistest.TestData() - analyser := newAnalyzer(&Config{DeepEqual: true}) - analysistest.Run(t, testdata, analyser, "deepequalon") -} - -func TestDeepEqualCheckDisabled(t *testing.T) { - testdata := analysistest.TestData() - analyser := newAnalyzer(&Config{DeepEqual: false}) - analysistest.Run(t, testdata, analyser, "deepequaloff") -} - -func TestMarkers(t *testing.T) { - testdata := analysistest.TestData() - analyser := newAnalyzer(&Config{}) - analysistest.Run(t, testdata, analyser, "markers") -} diff --git a/hack/krtequals/go.mod b/hack/krtequals/go.mod deleted file mode 100644 index 34ffbf46dfd..00000000000 --- a/hack/krtequals/go.mod +++ /dev/null @@ -1,13 +0,0 @@ -module github.com/kgateway-dev/kgateway/v2/hack/krtequals - -go 1.25.3 - -require ( - github.com/golangci/plugin-module-register v0.1.2 - golang.org/x/tools v0.32.0 -) - -require ( - golang.org/x/mod v0.24.0 // indirect - golang.org/x/sync v0.13.0 // indirect -) diff --git a/hack/krtequals/go.sum b/hack/krtequals/go.sum deleted file mode 100644 index e2328570de8..00000000000 --- a/hack/krtequals/go.sum +++ /dev/null @@ -1,10 +0,0 @@ -github.com/golangci/plugin-module-register v0.1.2 h1:e5WM6PO6NIAEcij3B053CohVp3HIYbzSuP53UAYgOpg= -github.com/golangci/plugin-module-register v0.1.2/go.mod h1:1+QGTsKBvAIvPvoY/os+G5eoqxWn70HYDm2uvUyGuVw= -github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= -github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -golang.org/x/mod v0.24.0 h1:ZfthKaKaT4NrhGVZHO1/WDTwGES4De8KtWO0SIbNJMU= -golang.org/x/mod v0.24.0/go.mod h1:IXM97Txy2VM4PJ3gI61r1YEk/gAj6zAHN3AdZt6S9Ww= -golang.org/x/sync v0.13.0 h1:AauUjRAJ9OSnvULf/ARrrVywoJDy0YS2AwQ98I37610= -golang.org/x/sync v0.13.0/go.mod h1:1dzgHSNfp02xaA81J2MS99Qcpr2w7fw1gpm99rleRqA= -golang.org/x/tools v0.32.0 h1:Q7N1vhpkQv7ybVzLFtTjvQya2ewbwNDZzUgfXGqtMWU= -golang.org/x/tools v0.32.0/go.mod h1:ZxrU41P/wAbZD8EDa6dDCa6XfpkhJ7HFMjHJXfBDu8s= diff --git a/hack/krtequals/plugin.go b/hack/krtequals/plugin.go deleted file mode 100644 index 45731c0d7d7..00000000000 --- a/hack/krtequals/plugin.go +++ /dev/null @@ -1,33 +0,0 @@ -package krtequals - -import ( - "github.com/golangci/plugin-module-register/register" - "golang.org/x/tools/go/analysis" -) - -func init() { - register.Plugin("krtequals", New) -} - -type plugin struct { - cfg Config -} - -var _ register.LinterPlugin = &plugin{} - -// New constructs the module plugin for golangci-lint. -func New(settings any) (register.LinterPlugin, error) { - s, err := register.DecodeSettings[Config](settings) - if err != nil { - return nil, err - } - return &plugin{cfg: s}, nil -} - -func (p *plugin) BuildAnalyzers() ([]*analysis.Analyzer, error) { - return []*analysis.Analyzer{newAnalyzer(&p.cfg)}, nil -} - -func (p *plugin) GetLoadMode() string { - return register.LoadModeTypesInfo -} diff --git a/hack/krtequals/testdata/src/deepequaloff/deepequal.go b/hack/krtequals/testdata/src/deepequaloff/deepequal.go deleted file mode 100644 index f2f830819e6..00000000000 --- a/hack/krtequals/testdata/src/deepequaloff/deepequal.go +++ /dev/null @@ -1,9 +0,0 @@ -package deepequaloff - -import "reflect" - -type Sample struct{} - -func (Sample) Equals(other Sample) bool { - return reflect.DeepEqual(Sample{}, other) -} diff --git a/hack/krtequals/testdata/src/deepequalon/deepequal.go b/hack/krtequals/testdata/src/deepequalon/deepequal.go deleted file mode 100644 index fcf486fb67b..00000000000 --- a/hack/krtequals/testdata/src/deepequalon/deepequal.go +++ /dev/null @@ -1,9 +0,0 @@ -package deepequalon - -import "reflect" - -type Sample struct{} - -func (Sample) Equals(other Sample) bool { - return reflect.DeepEqual(Sample{}, other) // want `Equals\(\) method uses reflect\.DeepEqual which is slow and ignores //\+noKrtEquals markers` -} diff --git a/hack/krtequals/testdata/src/markers/markers.go b/hack/krtequals/testdata/src/markers/markers.go deleted file mode 100644 index 4986b50f3a0..00000000000 --- a/hack/krtequals/testdata/src/markers/markers.go +++ /dev/null @@ -1,17 +0,0 @@ -package markers - -type Sample struct { - Used int - Missing int // want "field \"Missing\" in type \"Sample\" is not used in Equals" - // +noKrtEquals reason: internal bookkeeping - Ignored int - // +krtEqualsTodo reconcile waypoint usage - Todo int - // TodoWithGodoc has a godoc comment in addition to the marker - // +krtEqualsTodo reason: TODO - TodoWithGodoc int -} - -func (s Sample) Equals(other Sample) bool { - return s.Used == other.Used -}