From bc8542abab26d45cc57ab1f2fbfb1c1111984f9c Mon Sep 17 00:00:00 2001 From: David Gageot Date: Wed, 29 Apr 2026 14:58:02 +0200 Subject: [PATCH] Use the latest rubocop-go Signed-off-by: David Gageot --- go.mod | 2 +- go.sum | 4 +- lint/config_latest_tag_consistency.go | 22 ++-- lint/config_package_name.go | 4 +- lint/config_version_constant.go | 44 ++------ lint/config_version_import.go | 4 +- lint/config_versions_registered.go | 33 +----- lint/configpath.go | 15 --- lint/hook_builtins_registered.go | 125 +++++---------------- lint/hook_config_sync.go | 122 ++++----------------- lint/latest_imports_predecessor.go | 2 +- lint/runtime_event_registry.go | 38 +++---- lint/runtime_session_scoped.go | 52 +++------ lint/tui_view_purity.go | 151 +++----------------------- 14 files changed, 123 insertions(+), 495 deletions(-) diff --git a/go.mod b/go.mod index 76a175bf8..cd7ed603d 100644 --- a/go.mod +++ b/go.mod @@ -145,7 +145,7 @@ require ( github.com/containerd/stargz-snapshotter/estargz v0.18.2 // indirect github.com/cyphar/filepath-securejoin v0.4.1 // indirect github.com/davecgh/go-spew v1.1.1 // indirect - github.com/dgageot/rubocop-go v0.0.0-20260429095109-a9cea3bf3e72 + github.com/dgageot/rubocop-go v0.0.0-20260429125723-198995cc80c9 github.com/distribution/reference v0.6.0 // indirect github.com/dlclark/regexp2 v1.11.5 // indirect github.com/docker/distribution v2.8.3+incompatible // indirect diff --git a/go.sum b/go.sum index 3a28aff30..5672bb5ed 100644 --- a/go.sum +++ b/go.sum @@ -188,8 +188,8 @@ github.com/danieljoos/wincred v1.2.2/go.mod h1:w7w4Utbrz8lqeMbDAK0lkNJUv5sAOkFi7 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/dgageot/rubocop-go v0.0.0-20260429095109-a9cea3bf3e72 h1:2BdfP/9nnmcCHUBZhOueTb6rQojfZdlh2hI3E65cY2I= -github.com/dgageot/rubocop-go v0.0.0-20260429095109-a9cea3bf3e72/go.mod h1:r8YOJV5+/30NZ8HW/2NbWUObBGDXGvfHrjgury5YlFI= +github.com/dgageot/rubocop-go v0.0.0-20260429125723-198995cc80c9 h1:LwBQXSuGbtHC3rzVOBQIsSofzTc7k9QWRqoQdfPbG+s= +github.com/dgageot/rubocop-go v0.0.0-20260429125723-198995cc80c9/go.mod h1:r8YOJV5+/30NZ8HW/2NbWUObBGDXGvfHrjgury5YlFI= github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6 h1:88fWkkjwzuI4tRTqadbJIbA9O+gO67oyu+2OpHHuuT8= github.com/dgageot/ultraviolet v0.0.0-20260313154905-9451997d56b6/go.mod h1:SQpCTRNBtzJkwku5ye4S3HEuthAlGy2n9VXZnWkEW98= github.com/distribution/reference v0.6.0 h1:0IXCQ5g4/QMHHkarYzh5l+u8T3t73zM5QvfrDyIgxBk= diff --git a/lint/config_latest_tag_consistency.go b/lint/config_latest_tag_consistency.go index 2b6a6000c..88dce7e9d 100644 --- a/lint/config_latest_tag_consistency.go +++ b/lint/config_latest_tag_consistency.go @@ -3,7 +3,6 @@ package main import ( "go/ast" "reflect" - "strconv" "strings" "github.com/dgageot/rubocop-go/cop" @@ -47,7 +46,8 @@ func NewConfigLatestTagConsistency() *ConfigLatestTagConsistency { } func (c *ConfigLatestTagConsistency) Check(p *cop.Pass) { - if configDir(p.Filename()) != "latest" { + dir, _ := p.PathSegment("pkg/config") + if dir != "latest" { return } // Black-box test files don't ship struct definitions for the wire format. @@ -55,25 +55,18 @@ func (c *ConfigLatestTagConsistency) Check(p *cop.Pass) { return } - ast.Inspect(p.File, func(n ast.Node) bool { - field, ok := n.(*ast.Field) - if !ok || field.Tag == nil { - return true - } - raw, err := strconv.Unquote(field.Tag.Value) - if err != nil { - return true + p.ForEachStructField(func(_ *ast.TypeSpec, field *ast.Field, tag reflect.StructTag) { + if field.Tag == nil { + return } - tag := reflect.StructTag(raw) - jsonTag, hasJSON := tag.Lookup("json") yamlTag, hasYAML := tag.Lookup("yaml") if !hasJSON || !hasYAML { - return true + return } // Embedded fields use ",inline" on both sides; nothing to compare. if isInline(jsonTag) || isInline(yamlTag) { - return true + return } jsonOmit, jsonMod := jsonOmitModifier(jsonTag) @@ -85,7 +78,6 @@ func (c *ConfigLatestTagConsistency) Check(p *cop.Pass) { modifierLabel(jsonOmit, jsonMod), modifierLabel(yamlOmit, "omitempty"), fieldNames(field)) } - return true }) } diff --git a/lint/config_package_name.go b/lint/config_package_name.go index 6aab086d6..9f6fe07fe 100644 --- a/lint/config_package_name.go +++ b/lint/config_package_name.go @@ -29,8 +29,8 @@ func NewConfigPackageName() *ConfigPackageName { } func (c *ConfigPackageName) Check(p *cop.Pass) { - dir := configDir(p.Filename()) - if dir == "" { + dir, ok := p.PathSegment("pkg/config") + if !ok { return } diff --git a/lint/config_version_constant.go b/lint/config_version_constant.go index 3d25f9a8c..ed1554c30 100644 --- a/lint/config_version_constant.go +++ b/lint/config_version_constant.go @@ -1,8 +1,6 @@ package main import ( - "go/ast" - "go/token" "strconv" "github.com/dgageot/rubocop-go/cop" @@ -33,42 +31,20 @@ func NewConfigVersionConstant() *ConfigVersionConstant { } func (c *ConfigVersionConstant) Check(p *cop.Pass) { - dirVersion, ok := versionFromDir(configDir(p.Filename())) + dir, _ := p.PathSegment("pkg/config") + dirVersion, ok := versionFromDir(dir) if !ok { return } expected := strconv.Itoa(dirVersion) - for _, lit := range versionStringLiterals(p) { - got, err := strconv.Unquote(lit.Value) - if err != nil || got == expected { - continue - } - p.Report(lit, "Version in pkg/config/v%s/ must be %q, got %q", expected, expected, got) + lit, ok := p.StringConstNodes()["Version"] + if !ok { + return } -} - -// versionStringLiterals returns the value literal of every top-level -// `const Version = ""` declaration in the file under inspection. -func versionStringLiterals(p *cop.Pass) []*ast.BasicLit { - var lits []*ast.BasicLit - p.ForEachConst(func(gen *ast.GenDecl) { - for _, spec := range gen.Specs { - vs, ok := spec.(*ast.ValueSpec) - if !ok { - continue - } - for i, name := range vs.Names { - if name.Name != "Version" || i >= len(vs.Values) { - continue - } - lit, ok := vs.Values[i].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - continue - } - lits = append(lits, lit) - } - } - }) - return lits + got, err := strconv.Unquote(lit.Value) + if err != nil || got == expected { + return + } + p.Report(lit, "Version in pkg/config/v%s/ must be %q, got %q", expected, expected, got) } diff --git a/lint/config_version_import.go b/lint/config_version_import.go index 124ef2b31..7cc3464b2 100644 --- a/lint/config_version_import.go +++ b/lint/config_version_import.go @@ -34,8 +34,8 @@ func (c *ConfigVersionImport) Check(p *cop.Pass) { return } - dir := configDir(p.Filename()) - if dir == "" { + dir, ok := p.PathSegment("pkg/config") + if !ok { return } dirVersion, isVersioned := versionFromDir(dir) diff --git a/lint/config_versions_registered.go b/lint/config_versions_registered.go index fe53623e3..2c9d9ffd4 100644 --- a/lint/config_versions_registered.go +++ b/lint/config_versions_registered.go @@ -42,16 +42,15 @@ func NewConfigVersionsRegistered() *ConfigVersionsRegistered { } func (c *ConfigVersionsRegistered) Check(p *cop.Pass) { - filename := p.Filename() - if !isVersionsGo(filename) { + if !p.FileMatches("pkg/config/versions.go") { return } - want, err := versionPackagesOnDisk(filepath.Dir(filename)) + want, err := versionPackagesOnDisk(filepath.Dir(p.Filename())) if err != nil || len(want) == 0 { return } - got := registeredPackages(p) + got := p.SelectorReceivers("Register") var missing []string for _, name := range want { @@ -70,13 +69,6 @@ func (c *ConfigVersionsRegistered) Check(p *cop.Pass) { "pkg/config/versions.go is missing Register call(s) for: %s", strings.Join(missing, ", ")) } -// isVersionsGo reports whether filename is the canonical -// pkg/config/versions.go used by the dispatch table. -func isVersionsGo(filename string) bool { - slash := filepath.ToSlash(filename) - return strings.HasSuffix(slash, "/pkg/config/versions.go") || slash == "pkg/config/versions.go" -} - // versionPackagesOnDisk lists the package directories under pkg/config/ that // the dispatch table is expected to register: every vN/ and latest/. func versionPackagesOnDisk(dir string) ([]string, error) { @@ -102,25 +94,6 @@ func versionPackagesOnDisk(dir string) ([]string, error) { return names, nil } -// registeredPackages returns the set of package selectors that appear as -// `.Register(...)` call expressions anywhere in the file. The cop only -// cares about whether a name is mentioned, not how many times. -func registeredPackages(p *cop.Pass) map[string]bool { - got := map[string]bool{} - p.ForEachCall(func(call *ast.CallExpr) { - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Register" { - return - } - ident, ok := sel.X.(*ast.Ident) - if !ok { - return - } - got[ident.Name] = true - }) - return got -} - // registryAnchor picks the AST node used to position the offense. Preferring // the `versions` function declaration keeps the diagnostic close to the // dispatch table; if that function is absent (unexpected), the file's diff --git a/lint/configpath.go b/lint/configpath.go index 350946b53..aa6a7d715 100644 --- a/lint/configpath.go +++ b/lint/configpath.go @@ -12,21 +12,6 @@ import ( // pkg/config// paths and pkg/config/vN import paths, so each cop can // focus on its rule rather than on regular expressions. -// configDirRe matches files under pkg/config//. The captured group is -// the directory name immediately under pkg/config/. It accepts both -// absolute and relative paths. -var configDirRe = regexp.MustCompile(`(?:^|/)pkg/config/([^/]+)/[^/]+\.go$`) - -// configDir returns the directory name immediately under pkg/config/ for -// filename, or "" if filename is not under pkg/config//. -func configDir(filename string) string { - m := configDirRe.FindStringSubmatch(filepath.ToSlash(filename)) - if m == nil { - return "" - } - return m[1] -} - // versionFromDir parses a "vN" directory name and returns N. Returns false // for any other name (latest, types, vendor, ...). func versionFromDir(dir string) (int, bool) { diff --git a/lint/hook_builtins_registered.go b/lint/hook_builtins_registered.go index ceb73c248..2794d12e8 100644 --- a/lint/hook_builtins_registered.go +++ b/lint/hook_builtins_registered.go @@ -2,10 +2,6 @@ package main import ( "go/ast" - "go/parser" - "go/token" - "os" - "path/filepath" "slices" "strings" @@ -58,19 +54,25 @@ func NewHookBuiltinsRegistered() *HookBuiltinsRegistered { } func (c *HookBuiltinsRegistered) Check(p *cop.Pass) { - if !isHookBuiltinsRegisterFile(p.Filename()) { + if !p.FileMatches("pkg/hooks/builtins/builtins.go") { return } - declared, err := builtinNameConstants(filepath.Dir(p.Filename())) + declared, err := exportedBuiltinNames(p) if err != nil || len(declared) == 0 { return } - registered, anchor := registerBuiltinIdents(p) - if anchor == nil { - return - } + registered := p.IdentSetFromCalls("RegisterBuiltin", 0) + + // Anchor on the first RegisterBuiltin call, falling back to the package + // clause if the function was reshaped beyond recognition. + var anchor ast.Node = p.File.Name + p.ForEachMethodCall("RegisterBuiltin", func(call *ast.CallExpr) { + if anchor == p.File.Name { + anchor = call + } + }) var missing []string for _, name := range declared { @@ -87,98 +89,27 @@ func (c *HookBuiltinsRegistered) Check(p *cop.Pass) { strings.Join(missing, ", ")) } -// isHookBuiltinsRegisterFile reports whether filename is the canonical -// pkg/hooks/builtins/builtins.go that owns the Register() entry point. -func isHookBuiltinsRegisterFile(filename string) bool { - slash := filepath.ToSlash(filename) - return strings.HasSuffix(slash, "/pkg/hooks/builtins/builtins.go") || - slash == "pkg/hooks/builtins/builtins.go" -} - -// builtinNameConstants returns the identifier of every `const Name = "..."` -// declaration in dir whose value is a string literal — but only from -// per-builtin files. builtins.go itself and any _test.go files are -// excluded so the cop doesn't flag them or pick up unrelated test -// constants. -func builtinNameConstants(dir string) ([]string, error) { - entries, err := os.ReadDir(dir) +// exportedBuiltinNames returns the identifiers of every exported `const Name = "..."` +// declaration in pkg/hooks/builtins/, excluding builtins.go itself and any +// test files (which is not where new builtins land). +func exportedBuiltinNames(p *cop.Pass) ([]string, error) { + files, err := p.ParseDir(".", cop.ParseDirOptions{ + SkipTests: true, + SkipFiles: []string{"builtins.go"}, + }) if err != nil { return nil, err } - fset := token.NewFileSet() - var names []string - for _, e := range entries { - if e.IsDir() { - continue - } - fname := e.Name() - if !strings.HasSuffix(fname, ".go") || strings.HasSuffix(fname, "_test.go") { - continue - } - if fname == "builtins.go" { - continue - } - f, err := parser.ParseFile(fset, filepath.Join(dir, fname), nil, 0) - if err != nil { - continue - } - for _, decl := range f.Decls { - gd, ok := decl.(*ast.GenDecl) - if !ok || gd.Tok != token.CONST { - continue - } - for _, spec := range gd.Specs { - vs, ok := spec.(*ast.ValueSpec) - if !ok { - continue - } - for i, n := range vs.Names { - if i >= len(vs.Values) { - continue - } - lit, ok := vs.Values[i].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - continue - } - if !ast.IsExported(n.Name) { - continue - } - names = append(names, n.Name) - } - } + seen := map[string]struct{}{} + for _, f := range files { + for name := range cop.StringConstsIn(f, ast.IsExported) { + seen[name] = struct{}{} } } + names := make([]string, 0, len(seen)) + for n := range seen { + names = append(names, n) + } slices.Sort(names) - names = slices.Compact(names) return names, nil } - -// registerBuiltinIdents collects the set of identifiers that appear as the -// first positional argument of a `RegisterBuiltin(, …)` call anywhere -// in the inspected file. It also returns the call expression that the cop -// uses to anchor diagnostics; nil means no Register() body was found and -// the cop should bail out. -func registerBuiltinIdents(p *cop.Pass) (map[string]bool, ast.Node) { - registered := map[string]bool{} - var anchor ast.Node - p.ForEachCall(func(call *ast.CallExpr) { - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "RegisterBuiltin" || len(call.Args) == 0 { - return - } - if anchor == nil { - anchor = call - } - id, ok := call.Args[0].(*ast.Ident) - if !ok { - return - } - registered[id.Name] = true - }) - if anchor == nil { - // Fall back to the file's package clause so the diagnostic still - // surfaces if Register() was reshaped beyond recognition. - anchor = p.File.Name - } - return registered, anchor -} diff --git a/lint/hook_config_sync.go b/lint/hook_config_sync.go index e98df525a..fb135e4de 100644 --- a/lint/hook_config_sync.go +++ b/lint/hook_config_sync.go @@ -2,9 +2,6 @@ package main import ( "go/ast" - "go/parser" - "go/token" - "path/filepath" "reflect" "slices" "strconv" @@ -51,12 +48,19 @@ func NewHookConfigSync() *HookConfigSync { } func (c *HookConfigSync) Check(p *cop.Pass) { - if !isLatestConfigTypesGo(p.Filename()) { + if !p.FileMatches("pkg/config/latest/types.go") { return } - hookEvents, err := readHookEventConstants(hooksTypesGoPath(p.Filename())) - if err != nil || len(hookEvents) == 0 { + // pkg/config/latest/types.go ↔ ../../hooks/types.go + hookFile, err := p.ParseSibling("../../hooks/types.go") + if err != nil { + return + } + hookEvents := cop.StringConstsIn(hookFile, func(name string) bool { + return strings.HasPrefix(name, "Event") + }) + if len(hookEvents) == 0 { return } @@ -105,106 +109,28 @@ func (c *HookConfigSync) Check(p *cop.Pass) { } } -// isLatestConfigTypesGo reports whether filename is the canonical -// pkg/config/latest/types.go that owns HooksConfig. -func isLatestConfigTypesGo(filename string) bool { - slash := filepath.ToSlash(filename) - return strings.HasSuffix(slash, "/pkg/config/latest/types.go") || - slash == "pkg/config/latest/types.go" -} - -// hooksTypesGoPath returns the location of pkg/hooks/types.go relative to -// the repository root inferred from the latest config file's path. The -// cop walks up from `.../pkg/config/latest/types.go` to find the repo root. -func hooksTypesGoPath(latestTypesGo string) string { - // .../pkg/config/latest/types.go -> .../pkg/hooks/types.go - return filepath.Join(filepath.Dir(filepath.Dir(filepath.Dir(latestTypesGo))), "hooks", "types.go") -} - -// readHookEventConstants parses the file at path and returns a map of -// EventXxx constant name -> string value for every const whose name -// starts with "Event" and whose value is a string literal. -func readHookEventConstants(path string) (map[string]string, error) { - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, path, nil, 0) - if err != nil { - return nil, err - } - out := map[string]string{} - for _, decl := range f.Decls { - gd, ok := decl.(*ast.GenDecl) - if !ok || gd.Tok != token.CONST { - continue - } - for _, spec := range gd.Specs { - vs, ok := spec.(*ast.ValueSpec) - if !ok { - continue - } - for i, name := range vs.Names { - if !strings.HasPrefix(name.Name, "Event") { - continue - } - if i >= len(vs.Values) { - continue - } - lit, ok := vs.Values[i].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - continue - } - val, err := strconv.Unquote(lit.Value) - if err != nil { - continue - } - out[name.Name] = val - } - } - } - return out, nil -} - // readHooksConfigFields scans the file in p for the HooksConfig type and // returns the field-name -> json-tag-name map together with the type spec // itself (so the caller can anchor diagnostics on it). func readHooksConfigFields(p *cop.Pass) (map[string]string, *ast.TypeSpec) { fields := map[string]string{} var spec *ast.TypeSpec - for _, decl := range p.File.Decls { - gd, ok := decl.(*ast.GenDecl) + p.ForEachStructField(func(ts *ast.TypeSpec, fld *ast.Field, tag reflect.StructTag) { + if ts.Name.Name != "HooksConfig" { + return + } + spec = ts + jsonTag, ok := tag.Lookup("json") if !ok { - continue + return } - for _, s := range gd.Specs { - ts, ok := s.(*ast.TypeSpec) - if !ok || ts.Name.Name != "HooksConfig" { - continue - } - st, ok := ts.Type.(*ast.StructType) - if !ok { - continue - } - spec = ts - for _, fld := range st.Fields.List { - if fld.Tag == nil { - continue - } - raw, err := strconv.Unquote(fld.Tag.Value) - if err != nil { - continue - } - jsonTag, ok := reflect.StructTag(raw).Lookup("json") - if !ok { - continue - } - name, _, _ := strings.Cut(jsonTag, ",") - if name == "" || name == "-" { - continue - } - for _, n := range fld.Names { - fields[n.Name] = name - } - } + name, _, _ := strings.Cut(jsonTag, ",") + if name == "" || name == "-" { + return } - } + for _, n := range fld.Names { + fields[n.Name] = name + } + }) return fields, spec } diff --git a/lint/latest_imports_predecessor.go b/lint/latest_imports_predecessor.go index 79ea634bf..2f45038cd 100644 --- a/lint/latest_imports_predecessor.go +++ b/lint/latest_imports_predecessor.go @@ -35,7 +35,7 @@ func (c *LatestImportsPredecessor) Check(p *cop.Pass) { if p.IsBlackBoxTest() { return } - if configDir(p.Filename()) != "latest" { + if dir, _ := p.PathSegment("pkg/config"); dir != "latest" { return } highest, ok := highestSiblingVersion(p.Filename()) diff --git a/lint/runtime_event_registry.go b/lint/runtime_event_registry.go index 947e38ad6..b98d5222c 100644 --- a/lint/runtime_event_registry.go +++ b/lint/runtime_event_registry.go @@ -2,9 +2,7 @@ package main import ( "go/ast" - "go/parser" "go/token" - "path/filepath" "slices" "strconv" "strings" @@ -50,14 +48,17 @@ func NewRuntimeEventRegistry() *RuntimeEventRegistry { } func (c *RuntimeEventRegistry) Check(p *cop.Pass) { - if !isRuntimeClientGo(p.Filename()) { + if !p.FileMatches("pkg/runtime/client.go") { return } // Sibling event.go is the source of truth for the set of emitted events. - eventGo := filepath.Join(filepath.Dir(p.Filename()), "event.go") - emitted, err := emittedEventTypes(eventGo) - if err != nil || len(emitted) == 0 { + eventFile, err := p.ParseSibling("event.go") + if err != nil { + return + } + emitted := emittedEventTypes(eventFile) + if len(emitted) == 0 { return } @@ -88,26 +89,13 @@ func (c *RuntimeEventRegistry) Check(p *cop.Pass) { "pkg/runtime/client.go is missing registry entries for: %s", strings.Join(missing, ", ")) } -// isRuntimeClientGo reports whether filename is the canonical -// pkg/runtime/client.go that owns the event-decoder registry. -func isRuntimeClientGo(filename string) bool { - slash := filepath.ToSlash(filename) - return strings.HasSuffix(slash, "/pkg/runtime/client.go") || slash == "pkg/runtime/client.go" -} - // emittedEventTypes returns a map of EventTypeName -> wire-format Type -// string for every &XxxEvent{Type: "yyy"} composite literal in the file -// at path (typically pkg/runtime/event.go). Constructors that don't -// initialise the Type field are skipped — a separate cop could enforce -// that they always do. -func emittedEventTypes(path string) (map[string]string, error) { - fset := token.NewFileSet() - f, err := parser.ParseFile(fset, path, nil, 0) - if err != nil { - return nil, err - } +// string for every &XxxEvent{Type: "yyy"} composite literal in the given +// file (typically pkg/runtime/event.go). Constructors that don't initialise +// the Type field are skipped. +func emittedEventTypes(file *ast.File) map[string]string { emitted := map[string]string{} - ast.Inspect(f, func(n ast.Node) bool { + ast.Inspect(file, func(n ast.Node) bool { cl, ok := n.(*ast.CompositeLit) if !ok { return true @@ -137,7 +125,7 @@ func emittedEventTypes(path string) (map[string]string, error) { } return true }) - return emitted, nil + return emitted } // registryEntries finds the registry map literal in pkg/runtime/client.go and diff --git a/lint/runtime_session_scoped.go b/lint/runtime_session_scoped.go index 7810ab56a..88eebd4db 100644 --- a/lint/runtime_session_scoped.go +++ b/lint/runtime_session_scoped.go @@ -2,7 +2,6 @@ package main import ( "go/ast" - "path/filepath" "strings" "github.com/dgageot/rubocop-go/cop" @@ -42,7 +41,7 @@ func NewRuntimeSessionScoped() *RuntimeSessionScoped { } func (c *RuntimeSessionScoped) Check(p *cop.Pass) { - if !isRuntimeEventGo(p.Filename()) { + if !p.FileMatches("pkg/runtime/event.go") { return } @@ -62,42 +61,23 @@ func (c *RuntimeSessionScoped) Check(p *cop.Pass) { } } -// isRuntimeEventGo reports whether filename is the canonical -// pkg/runtime/event.go that defines all runtime event types. -func isRuntimeEventGo(filename string) bool { - slash := filepath.ToSlash(filename) - return strings.HasSuffix(slash, "/pkg/runtime/event.go") || slash == "pkg/runtime/event.go" -} - // eventStructsWithSessionID maps EventTypeName -> its declaring *ast.TypeSpec // for every top-level struct type whose name ends in "Event" and that -// declares (or transitively re-declares via its own field — embedded fields -// are out of scope here) a SessionID field. +// declares a SessionID field. Embedded fields are out of scope. func eventStructsWithSessionID(p *cop.Pass) map[string]*ast.TypeSpec { out := map[string]*ast.TypeSpec{} - for _, decl := range p.File.Decls { - gd, ok := decl.(*ast.GenDecl) - if !ok { - continue + p.ForEachStruct(func(ts *ast.TypeSpec, st *ast.StructType) { + if !strings.HasSuffix(ts.Name.Name, "Event") || st.Fields == nil { + return } - for _, spec := range gd.Specs { - ts, ok := spec.(*ast.TypeSpec) - if !ok || !strings.HasSuffix(ts.Name.Name, "Event") { - continue - } - st, ok := ts.Type.(*ast.StructType) - if !ok || st.Fields == nil { - continue - } - for _, fld := range st.Fields.List { - for _, name := range fld.Names { - if name.Name == "SessionID" { - out[ts.Name.Name] = ts - } + for _, fld := range st.Fields.List { + for _, name := range fld.Names { + if name.Name == "SessionID" { + out[ts.Name.Name] = ts } } } - } + }) return out } @@ -107,18 +87,12 @@ func eventStructsWithSessionID(p *cop.Pass) map[string]*ast.TypeSpec { func pointerReceiversWithMethod(p *cop.Pass, method string) map[string]bool { with := map[string]bool{} p.ForEachFunc(func(fn *ast.FuncDecl) { - if fn.Name.Name != method || fn.Recv == nil || len(fn.Recv.List) != 1 { + if fn.Name.Name != method { return } - star, ok := fn.Recv.List[0].Type.(*ast.StarExpr) - if !ok { - return - } - id, ok := star.X.(*ast.Ident) - if !ok { - return + if r, ok := cop.Receiver(fn); ok && r.IsPointer { + with[r.TypeName] = true } - with[id.Name] = true }) return with } diff --git a/lint/tui_view_purity.go b/lint/tui_view_purity.go index 234683eab..f49a8e0e6 100644 --- a/lint/tui_view_purity.go +++ b/lint/tui_view_purity.go @@ -3,7 +3,6 @@ package main import ( "go/ast" "go/token" - "strings" "github.com/dgageot/rubocop-go/cop" ) @@ -16,27 +15,20 @@ import ( // // The cop runs on every Go file under pkg/tui/ and inspects each method // named View whose signature is `View() string`. Any assignment whose -// left-hand side is `recv.field` is flagged, with two pragmatic exemptions: +// left-hand side is `recv.field` is flagged, with a pragmatic exemption +// for slice-cache patterns commonly used by click-zone caches: // -// - Slice cache patterns are allowed: -// recv.field = nil -// recv.field = recv.field[:0] -// recv.field = append(recv.field, …) -// These are common for click-zone caches that View populates while -// rendering and Update consumes when handling mouse events. They are a -// known compromise; the field is not used elsewhere in View to influence -// control flow. -// -// - Lines carrying a //rubocop:disable Lint/TUIViewPurity comment (the -// rubocop-go suppression form, distinct from golangci-lint's //nolint -// so the two tools do not validate each other's rule names) are -// skipped, allowing deliberate caches to be marked locally with a -// short justification. +// recv.field = nil +// recv.field = recv.field[:0] +// recv.field = append(recv.field, …) // // Anything else — assigning a literal, a method call result, or a value // that is also read elsewhere in View — is reported. Such mutations make // View() effectively part of the state machine, which is exactly what // Update() exists for. +// +// Per-line suppression is provided centrally by the runner: annotate the +// line with `//rubocop:disable Lint/TUIViewPurity` to opt out. type TUIViewPurity struct { cop.Meta } @@ -51,24 +43,22 @@ func NewTUIViewPurity() *TUIViewPurity { } func (c *TUIViewPurity) Check(p *cop.Pass) { - if !isTUIFile(p.Filename()) { + if !p.FileUnder("pkg/tui") { return } - suppress := suppressedLines(p, c.Name()) - p.ForEachFunc(func(fn *ast.FuncDecl) { - recv, ok := pointerReceiver(fn) - if !ok || !isViewMethod(fn) { + recv, ok := cop.Receiver(fn) + if !ok || !recv.IsPointer || recv.Name == "" || !isViewMethod(fn) { return } - c.checkBody(p, fn.Body, recv, suppress) + c.checkBody(p, fn.Body, recv.Name) }) } // checkBody walks fn body and reports an offense for every assignment to a // receiver field that is not part of the slice-cache exemption set. -func (c *TUIViewPurity) checkBody(p *cop.Pass, body *ast.BlockStmt, recv string, suppress map[int]bool) { +func (c *TUIViewPurity) checkBody(p *cop.Pass, body *ast.BlockStmt, recv string) { if body == nil { return } @@ -78,17 +68,13 @@ func (c *TUIViewPurity) checkBody(p *cop.Pass, body *ast.BlockStmt, recv string, return true } for i, lhs := range assign.Lhs { - field, ok := receiverField(lhs, recv) - if !ok { + x, field, ok := cop.MatchSelector(lhs) + if !ok || x != recv { continue } if i < len(assign.Rhs) && isSliceCachePattern(assign.Rhs[i], recv, field) { continue } - line := p.FileSet.Position(assign.Pos()).Line - if suppress[line] { - continue - } p.Report(assign, "View() must not mutate %s.%s; move the side effect to Update or compute it in a local variable"+ " (or annotate the line with //rubocop:disable Lint/TUIViewPurity if it is an intentional click-zone cache)", @@ -98,22 +84,6 @@ func (c *TUIViewPurity) checkBody(p *cop.Pass, body *ast.BlockStmt, recv string, }) } -// pointerReceiver returns the receiver name when fn has exactly one -// pointer receiver, e.g. (d *fooDialog). -func pointerReceiver(fn *ast.FuncDecl) (string, bool) { - if fn.Recv == nil || len(fn.Recv.List) != 1 { - return "", false - } - r := fn.Recv.List[0] - if _, ok := r.Type.(*ast.StarExpr); !ok { - return "", false - } - if len(r.Names) == 0 { - return "", false - } - return r.Names[0].Name, true -} - // isViewMethod reports whether fn is exactly `func (...) View() string`. // The cop intentionally ignores helpers that happen to be called View* // because they are not part of the Bubble Tea contract. @@ -131,22 +101,6 @@ func isViewMethod(fn *ast.FuncDecl) bool { return ok && id.Name == "string" } -// receiverField returns the field name when expr is `recv.` (a direct -// selector on the receiver) and reports whether the match succeeded. -// Nested selectors like recv.sub.field are intentionally not flagged because -// the cop cannot tell whether `recv.sub` aliases the receiver state. -func receiverField(expr ast.Expr, recv string) (string, bool) { - sel, ok := expr.(*ast.SelectorExpr) - if !ok { - return "", false - } - id, ok := sel.X.(*ast.Ident) - if !ok || id.Name != recv { - return "", false - } - return sel.Sel.Name, true -} - // isSliceCachePattern reports whether rhs is one of the recognised // "slice cache" idioms for the field recv.field: // @@ -160,86 +114,15 @@ func isSliceCachePattern(rhs ast.Expr, recv, field string) bool { if id, ok := rhs.(*ast.Ident); ok && id.Name == "nil" { return true } - if slc, ok := rhs.(*ast.SliceExpr); ok && isReceiverField(slc.X, recv, field) { + if slc, ok := rhs.(*ast.SliceExpr); ok && cop.IsSelector(slc.X, recv, field) { return true } if call, ok := rhs.(*ast.CallExpr); ok { if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "append" && len(call.Args) > 0 { - if isReceiverField(call.Args[0], recv, field) { + if cop.IsSelector(call.Args[0], recv, field) { return true } } } return false } - -// isReceiverField reports whether expr is exactly `recv.field`. -func isReceiverField(expr ast.Expr, recv, field string) bool { - sel, ok := expr.(*ast.SelectorExpr) - if !ok { - return false - } - id, ok := sel.X.(*ast.Ident) - if !ok { - return false - } - return id.Name == recv && sel.Sel.Name == field -} - -// isTUIFile reports whether filename lives under pkg/tui/, which is where -// Bubble Tea models are defined. The cop is scoped to this directory so -// that ad-hoc View() helpers elsewhere in the repository (e.g. test fakes) -// are not subject to the rule. -func isTUIFile(filename string) bool { - slash := strings.ReplaceAll(filename, "\\", "/") - return strings.Contains(slash, "/pkg/tui/") || strings.HasPrefix(slash, "pkg/tui/") -} - -// suppressedLines builds the set of source lines that carry a -// //rubocop:disable comment. The directive intentionally uses a -// different prefix from golangci-lint's //nolint so that the two tools do -// not validate each other's rule names — golangci-lint's `nolintlint` -// rejects custom cops it has never heard of. -// Both inline trailing comments and full-line comments above the -// offending statement are honoured (covering the line on which the -// comment ends and the next line, respectively). -func suppressedLines(p *cop.Pass, copName string) map[int]bool { - suppressed := map[int]bool{} - for _, group := range p.File.Comments { - for _, c := range group.List { - if !mentionsCop(c.Text, copName) { - continue - } - pos := p.FileSet.Position(c.Slash) - end := p.FileSet.Position(c.End()) - // Inline comment: applies to the line where the comment ends - // (Go scanner positions inline //... on the same line as code). - suppressed[end.Line] = true - // Full-line comment: applies to the next non-blank line. - suppressed[pos.Line+1] = true - } - } - return suppressed -} - -// mentionsCop reports whether comment is a //rubocop:disable directive that -// names copName (case-sensitive, matched as a comma-separated token to -// avoid substring false positives such as "Lint/TUIViewPurityExtra"). -func mentionsCop(comment, copName string) bool { - const prefix = "//rubocop:disable" - rest, ok := strings.CutPrefix(comment, prefix) - if !ok { - return false - } - rest = strings.TrimLeft(rest, " \t") - // Trim any trailing " // explanation" that follows the directive. - if idx := strings.Index(rest, " "); idx >= 0 { - rest = rest[:idx] - } - for name := range strings.SplitSeq(rest, ",") { - if strings.TrimSpace(name) == copName { - return true - } - } - return false -}