Merge pull request #2568 from dgageot/board/custom-go-linter-ideas-for-code-robustne-bdb66fc1

lint: add config-versioning robustness cops + fix v6/v7 package names

Author: dgageot

lint/config_package_name.go

@@ -0,0 +1,51 @@
+package main
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strings"
+
+	"github.com/dgageot/rubocop-go/cop"
+)
+
+// ConfigPackageName enforces that files under pkg/config/<dir>/ declare a
+// package name that matches their directory:
+//
+//   - pkg/config/vN/      → package vN
+//   - pkg/config/latest/  → package latest
+//   - pkg/config/types/   → package types
+//
+// This catches a class of copy-paste bugs that occur when a "latest" version
+// is frozen into a numbered vN directory: the package clause is easy to
+// forget, and the broken state remains compilable as long as importers use
+// an explicit alias.
+type ConfigPackageName struct{}
+
+func (*ConfigPackageName) Name() string { return "Lint/ConfigPackageName" }
+func (*ConfigPackageName) Description() string {
+	return "Files under pkg/config/<dir>/ must declare package <dir>"
+}
+func (*ConfigPackageName) Severity() cop.Severity { return cop.Error }
+
+func (c *ConfigPackageName) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
+	filename := fset.Position(file.Package).Filename
+	dir := configDir(filename)
+	if dir == "" {
+		return nil
+	}
+
+	got := file.Name.Name
+	switch got {
+	case dir:
+		return nil
+	case dir + "_test":
+		// Black-box test packages are a legitimate Go convention.
+		if strings.HasSuffix(filename, "_test.go") {
+			return nil
+		}
+	}
+
+	return []cop.Offense{offense(c, fset, file.Name,
+		fmt.Sprintf("file in pkg/config/%s/ must declare package %s, got %s", dir, dir, got))}
+}

lint/config_version_constant.go

@@ -0,0 +1,77 @@
+package main
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strconv"
+
+	"github.com/dgageot/rubocop-go/cop"
+)
+
+// ConfigVersionConstant enforces that a file under pkg/config/vN/ declaring a
+// `const Version = "<value>"` uses "<N>" as its value.
+//
+// This guards against the common mistake of bumping the directory name
+// without bumping the constant (or vice versa) when freezing the
+// work-in-progress config and creating a new "latest". A mismatch would
+// silently break the parser dispatch in pkg/config/versions.go, which
+// registers parsers keyed by `Version`.
+//
+// Files under pkg/config/latest/ are intentionally exempt: their `Version`
+// is the next, work-in-progress value (one greater than the highest vN).
+type ConfigVersionConstant struct{}
+
+func (*ConfigVersionConstant) Name() string { return "Lint/ConfigVersionConstant" }
+func (*ConfigVersionConstant) Description() string {
+	return "Version constant in pkg/config/vN/ must equal \"N\""
+}
+func (*ConfigVersionConstant) Severity() cop.Severity { return cop.Error }
+
+func (c *ConfigVersionConstant) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
+	dirVersion, ok := versionFromDir(configDir(fset.Position(file.Package).Filename))
+	if !ok {
+		return nil
+	}
+	expected := strconv.Itoa(dirVersion)
+
+	var offenses []cop.Offense
+	for _, lit := range versionStringLiterals(file) {
+		got, err := strconv.Unquote(lit.Value)
+		if err != nil || got == expected {
+			continue
+		}
+		offenses = append(offenses, offense(c, fset, lit,
+			fmt.Sprintf("Version in pkg/config/v%s/ must be %q, got %q", expected, expected, got)))
+	}
+	return offenses
+}
+
+// versionStringLiterals returns the value literal of every top-level
+// `const Version = "<string>"` declaration in file.
+func versionStringLiterals(file *ast.File) []*ast.BasicLit {
+	var lits []*ast.BasicLit
+	for _, decl := range file.Decls {
+		gen, ok := decl.(*ast.GenDecl)
+		if !ok || gen.Tok != token.CONST {
+			continue
+		}
+		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
+}

lint/config_version_import.go

@@ -4,17 +4,14 @@ import (
 	"fmt"
 	"go/ast"
 	"go/token"
-	"path/filepath"
-	"regexp"
-	"strconv"
 	"strings"
 
 	"github.com/dgageot/rubocop-go/cop"
 )
 
-// ConfigVersionImport enforces that config version packages (pkg/config/vN)
-// only import their immediate predecessor (pkg/config/v{N-1}) and the shared
-// types package (pkg/config/types). This preserves the strict migration chain:
+// ConfigVersionImport enforces that config version packages (pkg/config/vN
+// and pkg/config/latest) only import their immediate predecessor and the
+// shared types package, preserving the strict migration chain:
 // v0 → v1 → v2 → … → latest.
 type ConfigVersionImport struct{}
 
@@ -24,100 +21,69 @@ func (*ConfigVersionImport) Description() string {
 }
 func (*ConfigVersionImport) Severity() cop.Severity { return cop.Error }
 
-// configVersionRe matches "pkg/config/vN" at the end of an import path.
-var configVersionRe = regexp.MustCompile(`pkg/config/v(\d+)$`)
-
-// Check inspects import declarations in config version packages.
 func (c *ConfigVersionImport) Check(fset *token.FileSet, file *ast.File) []cop.Offense {
 	if len(file.Imports) == 0 {
 		return nil
 	}
 
-	// Determine which config version package this file belongs to.
-	filename := fset.Position(file.Package).Filename
-	dirVersion, isVersioned := extractDirVersion(filename)
-	dirIsLatest := isLatestDir(filename)
+	dir := configDir(fset.Position(file.Package).Filename)
+	if dir == "" {
+		return nil
+	}
+	// Black-box test files (package <dir>_test) are external to the package
+	// and may import what they please.
+	if strings.HasSuffix(file.Name.Name, "_test") {
+		return nil
+	}
 
-	if !isVersioned && !dirIsLatest {
+	dirVersion, isVersioned := versionFromDir(dir)
+	isLatest := dir == "latest"
+	if !isVersioned && !isLatest {
 		return nil
 	}
 
 	var offenses []cop.Offense
-
 	for _, imp := range file.Imports {
-		importPath := strings.Trim(imp.Path.Value, `"`)
-
-		if !strings.Contains(importPath, "pkg/config/") {
-			continue
-		}
+		path := importPath(imp)
 
-		if strings.HasSuffix(importPath, "pkg/config/types") {
+		if !strings.Contains(path, "pkg/config/") || strings.HasSuffix(path, "pkg/config/types") {
 			continue
 		}
-
-		if isVersioned {
-			offenses = append(offenses, c.checkVersionedImport(fset, imp, importPath, dirVersion)...)
-		} else if dirIsLatest {
-			offenses = append(offenses, c.checkLatestImport(fset, imp, importPath)...)
+		if msg := importViolation(path, dirVersion, isLatest); msg != "" {
+			offenses = append(offenses, offense(c, fset, imp.Path, msg))
 		}
 	}
-
 	return offenses
 }
 
-func (c *ConfigVersionImport) checkVersionedImport(fset *token.FileSet, imp *ast.ImportSpec, importPath string, dirVersion int) []cop.Offense {
-	if strings.HasSuffix(importPath, "pkg/config/latest") {
-		return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
-			fmt.Sprintf("config v%d must not import pkg/config/latest", dirVersion))}
+// importViolation returns a non-empty error message if the given import path
+// is forbidden inside a config-version package, or "" if the import is fine.
+// dirVersion is the importing package's N (only meaningful when !isLatest).
+func importViolation(path string, dirVersion int, isLatest bool) string {
+	if isLatest {
+		// pkg/config/latest may only import other config-version packages.
+		// (The "must be the immediate predecessor" rule lives in the
+		// LatestImportsPredecessor cop.)
+		if _, ok := versionFromImport(path); ok {
+			return ""
+		}
+		return "pkg/config/latest must only import config version or types packages, not " + path
 	}
 
-	m := configVersionRe.FindStringSubmatch(importPath)
-	if m == nil {
-		return nil
+	// Versioned package (vN).
+	if strings.HasSuffix(path, "pkg/config/latest") {
+		return fmt.Sprintf("config v%d must not import pkg/config/latest", dirVersion)
+	}
+	imported, ok := versionFromImport(path)
+	if !ok {
+		return ""
 	}
-
-	importedVersion, _ := strconv.Atoi(m[1])
 	expected := dirVersion - 1
-
 	if expected < 0 {
-		return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
-			"config v0 must not import other config version packages")}
+		return "config v0 must not import other config version packages"
 	}
-
-	if importedVersion != expected {
-		return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
-			fmt.Sprintf("config v%d must import v%d (its predecessor), not v%d", dirVersion, expected, importedVersion))}
-	}
-
-	return nil
-}
-
-func (c *ConfigVersionImport) checkLatestImport(fset *token.FileSet, imp *ast.ImportSpec, importPath string) []cop.Offense {
-	if configVersionRe.MatchString(importPath) {
-		return nil
+	if imported != expected {
+		return fmt.Sprintf("config v%d must import v%d (its predecessor), not v%d", dirVersion, expected, imported)
 	}
-
-	return []cop.Offense{cop.NewOffense(c, fset, imp.Path.Pos(), imp.Path.End(),
-		"pkg/config/latest should only import config version or types packages, not "+importPath)}
-}
-
-func extractDirVersion(filename string) (int, bool) {
-	normalized := filepath.ToSlash(filename)
-
-	re := regexp.MustCompile(`/pkg/config/v(\d+)/`)
-	m := re.FindStringSubmatch(normalized)
-	if m == nil {
-		return 0, false
-	}
-
-	v, err := strconv.Atoi(m[1])
-	if err != nil {
-		return 0, false
-	}
-	return v, true
-}
-
-func isLatestDir(filename string) bool {
-	normalized := filepath.ToSlash(filename)
-	return strings.Contains(normalized, "/pkg/config/latest/")
+	return ""
 }