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) + } }