From 55866682ae634c26751532902a3c4d7a9ff377c3 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Fri, 4 Sep 2026 21:07:30 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8(checksilent):=20require=20a=20con?= =?UTF-8?q?tract=20shim=20to=20be=20a=20type=20alias?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A shim of a core contract type written as `type X pkg.Y` instead of `type X = pkg.Y` keeps the fields and drops the method set, so anything embedding it stops satisfying the interfaces it satisfied before. The compiler catches that only where the method set is actually exercised. This repository exercises some of the contract types through an interface and some not at all, so the ones it does not exercise compile here and break in a fork or a third-party application - which is the half nobody is watching. The trigger is the right-hand side of the declaration rather than a list of package names, so it covers whatever the lowering ends up shaping without a list to keep in step. Until the contract packages land in core there is nothing here to guard, and the summary says so rather than letting the silence read as a pass. Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx --- tools/checksilent/checks.go | 127 +++++++++++++++++++++++++++++++ tools/checksilent/checks_test.go | 126 ++++++++++++++++++++++++++++++ tools/checksilent/main.go | 14 +++- 3 files changed, 265 insertions(+), 2 deletions(-) diff --git a/tools/checksilent/checks.go b/tools/checksilent/checks.go index 9b591082..f6b6cf72 100644 --- a/tools/checksilent/checks.go +++ b/tools/checksilent/checks.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "go/ast" "go/token" "path" "sort" @@ -18,6 +19,7 @@ const ( checkConfigValue = "config-value-truncation" checkMenuIDConflict = "menu-id-collision" checkImportBoundary = "contract-import-boundary" + checkShimAlias = "contract-shim-alias" ) // Package paths, relative to the module. Spelled once so a module rename @@ -44,6 +46,7 @@ func runChecks(s *snapshot, opt options) ([]Finding, error) { out = append(out, checkConfigValueLength(s)...) out = append(out, checkMenuIDCollisions(s)...) out = append(out, checkContractImportBoundary(s)...) + out = append(out, checkContractShimAlias(s)...) if opt.UIDir != "" { fs, err := checkMenuNames(s, opt.UIDir) @@ -379,6 +382,130 @@ func checkContractImportBoundary(s *snapshot) []Finding { return out } +// --------------------------------------------------------------------------- +// check 7: a shim of a core contract type must be an alias +// --------------------------------------------------------------------------- + +// coreModulePrefix and coreContractSegment together identify a package under +// core's contract namespace. Matched as prefix plus segment rather than as one +// literal path so that a major-version bump of core - which rewrites the +// /v2 in every import - does not quietly turn this check off. +const ( + coreModulePrefix = "github.com/go-admin-team/go-admin-core/" + coreContractSegment = "/sdk/contract/" +) + +// isCoreContractPkg reports whether an import path names one of core's +// contract packages. +func isCoreContractPkg(path string) bool { + return strings.HasPrefix(path, coreModulePrefix) && strings.Contains(path, coreContractSegment) +} + +// checkContractShimAlias reports a shim of a core contract type that was +// written as a defined type instead of an alias. +// +// type ControlBy = models.ControlBy // alias: same type, same method set +// type ControlBy models.ControlBy // defined type: methods are gone +// +// The two lines differ by one character and by everything else. A defined type +// takes the underlying struct and none of the methods declared on it, so a +// model embedding the second one no longer has SetCreateBy or SetUpdateBy and +// no longer satisfies ActiveRecord - which is not a warning, it is a compile +// error, but only in code that actually uses the method set. +// +// That is why the compiler is not enough on its own. This repository exercises +// some of the contract types through interfaces and some not at all; the ones +// it does not exercise compile perfectly well as defined types here and break +// in a third-party application, or in a fork's own module, which is where +// nobody is looking. The check costs one field of the AST - a type alias +// records the position of its '=' - and covers the surface uniformly rather +// than covering whatever app/demo happens to touch this month. +// +// The trigger is the right-hand side, not a list of names: any type declared +// from a core contract package is one of these, whoever wrote it and whenever +// it was added. A type declared from a local struct literal is not caught by +// this - see ScannedShimAliases, which is what stops a run over a tree with no +// shims in it from reading as a clean bill of health. +func checkContractShimAlias(s *snapshot) []Finding { + var out []Finding + for _, sf := range s.Files { + forEachTypeSpec(sf, func(ts *ast.TypeSpec) { + pkg, name, ok := qualifiedType(sf, ts.Type) + if !ok || !isCoreContractPkg(pkg) { + return + } + if ts.Assign.IsValid() { + return // "type X = pkg.Y", which is what it must be + } + out = append(out, s.finding(Error, checkShimAlias, sf, ts, + "%s is declared from %s.%s as a defined type, not an alias;\n"+ + " a defined type keeps the fields and drops the method set, so anything embedding it stops satisfying\n"+ + " the interfaces it satisfied before - here it may still compile, in a fork or a third-party app it does not.\n"+ + " Write it as: type %s = %s.%s", + ts.Name.Name, path.Base(pkg), name, ts.Name.Name, path.Base(pkg), name)) + }) + } + return out +} + +// ScannedShimAliases counts the type aliases into core's contract packages the +// snapshot holds, so the summary can say whether checkContractShimAlias found +// anything to guard at all. +// +// Reported for the same reason ScannedContractRoots is: before the contract +// packages are lowered into core there are no shims here, the check has +// nothing to look at, and a run that printed nothing would look exactly like a +// run over a tree that passed. +func ScannedShimAliases(s *snapshot) int { + n := 0 + for _, sf := range s.Files { + forEachTypeSpec(sf, func(ts *ast.TypeSpec) { + pkg, _, ok := qualifiedType(sf, ts.Type) + if ok && isCoreContractPkg(pkg) && ts.Assign.IsValid() { + n++ + } + }) + } + return n +} + +// forEachTypeSpec visits every type declaration in the file, including the +// ones inside a parenthesised type block. +func forEachTypeSpec(sf *sourceFile, fn func(*ast.TypeSpec)) { + for _, decl := range sf.Syntax.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.TYPE { + continue + } + for _, spec := range gen.Specs { + if ts, ok := spec.(*ast.TypeSpec); ok { + fn(ts) + } + } + } +} + +// qualifiedType resolves a type expression that names a type in another +// package, returning that package's import path and the type name. A bare +// identifier, a struct literal or anything else reports false: this asks +// specifically "is the right-hand side pkg.Name", which is the shape both a +// correct shim and the mistake it guards against are written in. +func qualifiedType(sf *sourceFile, typ ast.Expr) (string, string, bool) { + sel, ok := typ.(*ast.SelectorExpr) + if !ok { + return "", "", false + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return "", "", false + } + path, ok := sf.imports[ident.Name] + if !ok { + return "", "", false + } + return path, sel.Sel.Name, true +} + // migrationVersion reads the 13-digit timestamp a migration file name starts // with. Files outside the two migration directories are not migrations, however // they are named. diff --git a/tools/checksilent/checks_test.go b/tools/checksilent/checks_test.go index 1f1383ef..5707ca81 100644 --- a/tools/checksilent/checks_test.go +++ b/tools/checksilent/checks_test.go @@ -452,3 +452,129 @@ func TestComponentNameParsesBothVueStyles(t *testing.T) { t.Error("a component with no declared name must not be compared") } } + +// --------------------------------------------------------------------------- + +const coreContractModels = "github.com/go-admin-team/go-admin-core/v2/sdk/contract/models" + +// shimFixture writes one shim file declaring ControlBy from core's contract +// package, in whichever of the two forms the caller asks for. +func shimFixture(t *testing.T, decl string) string { + t.Helper() + return fixture(t, map[string]string{ + "common/models/by.go": "package models\n\nimport \"" + coreContractModels + "\"\n\n" + decl + "\n", + }) +} + +func TestShimAliasDetectsADefinedType(t *testing.T) { + root := shimFixture(t, "type ControlBy models.ControlBy") + + f := requireOne(t, check(t, root, options{}), checkShimAlias) + if f.Severity != "ERROR" { + t.Errorf("severity = %s", f.Severity) + } + if !strings.Contains(f.Message, "type ControlBy = models.ControlBy") { + t.Errorf("the message must spell out the fix; got %s", f.Message) + } + if f.File != "common/models/by.go" || f.Line != 5 { + t.Errorf("position = %s:%d", f.File, f.Line) + } +} + +// The counterproof for the check above: the same fixture with the one +// character that makes it correct must produce nothing. Without this the check +// could be reporting every type declaration it sees and the test above would +// still pass. +func TestShimAliasAcceptsAnAlias(t *testing.T) { + root := shimFixture(t, "type ControlBy = models.ControlBy") + if got := only(t, check(t, root, options{}), checkShimAlias); len(got) != 0 { + t.Errorf("reported %v", got) + } +} + +// A parenthesised type block is how a shim package with more than one type +// tends to get written, and a walker that only looked at single-spec +// declarations would skip all but the first. +func TestShimAliasReadsAParenthesisedBlock(t *testing.T) { + root := shimFixture(t, `type ( + Model = models.Model + ControlBy models.ControlBy + ModelTime = models.ModelTime +)`) + f := requireOne(t, check(t, root, options{}), checkShimAlias) + if !strings.Contains(f.Message, "ControlBy") { + t.Errorf("message = %s", f.Message) + } +} + +// A defined type over a package that is not core's contract namespace is +// somebody's ordinary code. The check exists for the surface core promises to +// keep stable, and reporting anything else would make it a style rule. +func TestShimAliasIgnoresOtherPackages(t *testing.T) { + root := fixture(t, map[string]string{ + "app/demo/models/product.go": `package models + +import "go-admin/common/models" + +type Product models.Model +`, + }) + if got := only(t, check(t, root, options{}), checkShimAlias); len(got) != 0 { + t.Errorf("reported %v", got) + } +} + +// The version is part of core's import path and changes on every major bump. +// Matching the whole path literally would turn the check off on that day and +// say nothing about it. +func TestShimAliasSurvivesACoreMajorVersionBump(t *testing.T) { + root := fixture(t, map[string]string{ + "common/models/by.go": `package models + +import "github.com/go-admin-team/go-admin-core/v9/sdk/contract/models" + +type ControlBy models.ControlBy +`, + }) + if got := only(t, check(t, root, options{}), checkShimAlias); len(got) != 1 { + t.Errorf("findings = %v", got) + } +} + +// A tree with no shims in it is the state of this repository until the +// contract packages are lowered, and the check saying nothing there must not +// be reported as a boundary being guarded. +func TestShimAliasCoverageIsReportedAsZeroWhenThereAreNoShims(t *testing.T) { + root := fixture(t, map[string]string{ + "common/models/by.go": "package models\n\ntype ControlBy struct{}\n", + }) + s, err := load(root) + if err != nil { + t.Fatalf("load: %v", err) + } + if n := ScannedShimAliases(s); n != 0 { + t.Errorf("ScannedShimAliases = %d, want 0", n) + } + + var buf strings.Builder + if _, err := run(&buf, root, options{}, false); err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(buf.String(), "guarded nothing") { + t.Errorf("the summary must say the check covered nothing; got:\n%s", buf.String()) + } +} + +func TestShimAliasCoverageCountsTheAliasesItGuards(t *testing.T) { + root := shimFixture(t, `type ( + Model = models.Model + ControlBy = models.ControlBy +)`) + s, err := load(root) + if err != nil { + t.Fatalf("load: %v", err) + } + if n := ScannedShimAliases(s); n != 2 { + t.Errorf("ScannedShimAliases = %d, want 2", n) + } +} diff --git a/tools/checksilent/main.go b/tools/checksilent/main.go index 7132ea2d..ae2b32e7 100644 --- a/tools/checksilent/main.go +++ b/tools/checksilent/main.go @@ -1,9 +1,9 @@ // Command checksilent reports the failures in this repository that do not // announce themselves: no error, no log line, behaviour quietly wrong. // -// Six checks, five of them ERROR and one WARN. An ERROR fails the run; a WARN +// Seven checks, six of them ERROR and one WARN. An ERROR fails the run; a WARN // prints and does not. The split is not about how bad the consequence is - all -// six are bad - but about how certain the detection is. Everything reported as +// seven are bad - but about how certain the detection is. Everything reported as // an ERROR is decided from this repository's own syntax. The one WARN compares // against a second repository through a regular expression, and a check that // can be wrong must not be able to stop a build, or the first response to it @@ -102,4 +102,14 @@ func printSummary(w io.Writer, findings []Finding, opt options, s *snapshot) { fmt.Fprintf(w, "The %s check covered %s; %s does not exist here and was not scanned.\n", checkImportBoundary, strings.Join(scanned, ", "), strings.Join(absent, ", ")) } + // Same reason: until the contract packages are lowered into core there is + // no shim in this tree, so the check has nothing to look at and its + // silence must not be read as a pass. + if n := ScannedShimAliases(s); n == 0 { + fmt.Fprintf(w, "The %s check found no type alias into core's contract packages and guarded nothing.\n", + checkShimAlias) + } else { + fmt.Fprintf(w, "The %s check covered %d type alias(es) into core's contract packages.\n", + checkShimAlias, n) + } }