From 6102b36adf31206a3eb7440632e532febc4c05a7 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 12 Sep 2026 17:23:47 +0000 Subject: [PATCH] Check resolved types at the public boundary --- docs/roadmap/remediation.md | 12 +- import_boundary_test.go | 91 -------------- public_boundary_test.go | 231 ++++++++++++++++++++++++++++++++++++ 3 files changed, 241 insertions(+), 93 deletions(-) create mode 100644 public_boundary_test.go diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index cfed797..f8454ed 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -2,7 +2,7 @@ Review baseline: `5dc7e18`. Reviewed commits `f81def9` through `5dc7e18` against `eb7de8e`, the agreed [roadmap](public.md), and [implementation plan](implementation.md). -Status: three open findings; F1 resolved. Review evidence below describes the +Status: two open findings; F1 and F2 resolved. Review evidence below describes the original baseline; resolution records describe the subsequent fixes. The previous provenance finding remains resolved; its record is preserved at @@ -58,7 +58,15 @@ everyday check, and the race suite. ## F2 — Close the public-boundary check's type-resolution gaps -Priority: medium. Status: open. +Priority: medium. Status: resolved. + +Resolution: [public_boundary_test.go](../../public_boundary_test.go) loads the +active build's compiler export data and walks resolved Go types, exported method +sets, and generic constraints. Declaration fixtures cover exposure and valid +private-backed wrappers. The original three probe declarations now each fail +the actual root check; the temporary probe was removed afterwards. The everyday +check passed with Go 1.26.7, and boundary tests passed with Go 1.25.5 and Go 1.26.7 +on Linux. No production behavior or public surface was changed. ### Evidence and impact diff --git a/import_boundary_test.go b/import_boundary_test.go index 9a8a2df..20b644b 100644 --- a/import_boundary_test.go +++ b/import_boundary_test.go @@ -1,7 +1,6 @@ package configkit import ( - "go/ast" "go/parser" "go/token" "io/fs" @@ -9,7 +8,6 @@ import ( "strconv" "strings" "testing" - "unicode" ) func TestDependencyImportBoundary(t *testing.T) { @@ -59,92 +57,3 @@ func TestDependencyImportBoundary(t *testing.T) { t.Fatal(err) } } - -func TestPublicFacadeBoundary(t *testing.T) { - t.Parallel() - - entries, err := filepath.Glob("*.go") - if err != nil { - t.Fatal(err) - } - for _, path := range entries { - if strings.HasSuffix(path, "_test.go") { - continue - } - file, err := parser.ParseFile(token.NewFileSet(), path, nil, 0) - if err != nil { - t.Fatal(err) - } - imports := importedPaths(file) - for _, declaration := range file.Decls { - switch declaration := declaration.(type) { - case *ast.FuncDecl: - if exportedIdentifier(declaration.Name.Name) && usesInternalType(declaration.Type, imports) { - t.Errorf("%s exposes an internal type in its signature", declaration.Name) - } - case *ast.GenDecl: - for _, specification := range declaration.Specs { - typeSpec, ok := specification.(*ast.TypeSpec) - if !ok || !exportedIdentifier(typeSpec.Name.Name) { - continue - } - if typeSpec.Assign.IsValid() { - t.Errorf("%s is an exported type alias", typeSpec.Name) - } - structure, ok := typeSpec.Type.(*ast.StructType) - if !ok { - continue - } - for _, field := range structure.Fields.List { - exposed := len(field.Names) == 0 - for _, name := range field.Names { - exposed = exposed || exportedIdentifier(name.Name) - } - if exposed && usesInternalType(field.Type, imports) { - t.Errorf("%s exposes an internal type through a field", typeSpec.Name) - } - } - } - } - } - } -} - -func importedPaths(file *ast.File) map[string]string { - paths := make(map[string]string, len(file.Imports)) - for _, imported := range file.Imports { - path, err := strconv.Unquote(imported.Path.Value) - if err != nil { - continue - } - name := filepath.Base(path) - if imported.Name != nil { - name = imported.Name.Name - } - paths[name] = path - } - return paths -} - -func usesInternalType(expression ast.Expr, imports map[string]string) bool { - internal := false - ast.Inspect(expression, func(node ast.Node) bool { - selector, ok := node.(*ast.SelectorExpr) - if !ok { - return true - } - packageName, ok := selector.X.(*ast.Ident) - if ok && strings.Contains(imports[packageName.Name], "/internal/") { - internal = true - } - return !internal - }) - return internal -} - -func exportedIdentifier(name string) bool { - for _, character := range name { - return unicode.IsUpper(character) - } - return false -} diff --git a/public_boundary_test.go b/public_boundary_test.go new file mode 100644 index 0000000..4e31adf --- /dev/null +++ b/public_boundary_test.go @@ -0,0 +1,231 @@ +package configkit + +import ( + "bytes" + "encoding/json" + "go/ast" + "go/importer" + "go/parser" + "go/token" + "go/types" + "io" + "os" + "os/exec" + "strings" + "testing" +) + +func TestPublicFacadeBoundary(t *testing.T) { + t.Parallel() + + // Use the active toolchain's build selection and export data, not a second + // source-level approximation of aliases, generics, or promoted method sets. + command := exec.Command("go", "list", "-export", "-deps", "-json", ".") + command.Env = append(os.Environ(), "GOPROXY=off") + var stderr bytes.Buffer + command.Stderr = &stderr + output, err := command.Output() + if err != nil { + t.Fatalf("load package exports: %v\n%s", err, &stderr) + } + exports := make(map[string]string) + var root string + decoder := json.NewDecoder(bytes.NewReader(output)) + for { + var entry struct { + ImportPath string + Export string + DepOnly bool + } + if err := decoder.Decode(&entry); err == io.EOF { + break + } else if err != nil { + t.Fatal(err) + } + exports[entry.ImportPath] = entry.Export + if !entry.DepOnly { + root = entry.ImportPath + } + } + loader := importer.ForCompiler(token.NewFileSet(), "gc", func(path string) (io.ReadCloser, error) { + return os.Open(exports[path]) + }) + pkg, err := loader.Import(root) + if err != nil { + t.Fatal(err) + } + for _, name := range publicBoundaryViolations(pkg) { + t.Errorf("%s exposes an internal type or is a public type alias", name) + } +} + +func publicBoundaryViolations(pkg *types.Package) []string { + var violations []string + for _, name := range pkg.Scope().Names() { + object := pkg.Scope().Lookup(name) + if !object.Exported() { + continue + } + if named, ok := object.(*types.TypeName); ok && named.IsAlias() { + violations = append(violations, name) + continue + } + if exposesInternalType(object.Type(), pkg, make(map[types.Type]bool)) { + violations = append(violations, name) + } + } + return violations +} + +func exposesInternalType(value types.Type, root *types.Package, seen map[types.Type]bool) bool { + if value == nil || seen[value] { + return false + } + seen[value] = true + visit := func(child types.Type) bool { return exposesInternalType(child, root, seen) } + parameters := func(list *types.TypeParamList) bool { + for i := 0; i < list.Len(); i++ { + if visit(list.At(i)) { + return true + } + } + return false + } + switch value := value.(type) { + case *types.Alias: + return visit(types.Unalias(value)) + case *types.Named: + owner := value.Obj().Pkg() + if owner != nil && (strings.HasPrefix(owner.Path(), "internal/") || strings.Contains(owner.Path(), "/internal/")) { + return true + } + for i := 0; i < value.TypeArgs().Len(); i++ { + if visit(value.TypeArgs().At(i)) { + return true + } + } + // Other public packages own their implementation boundaries. Only root + // named types need their exported fields, methods, and constraints walked. + if owner != root { + return false + } + if parameters(value.TypeParams()) || visit(value.Underlying()) { + return true + } + for _, receiver := range []types.Type{value, types.NewPointer(value)} { + methods := types.NewMethodSet(receiver) + for i := 0; i < methods.Len(); i++ { + method := methods.At(i).Obj() + if method.Exported() && visit(method.Type()) { + return true + } + } + } + case *types.Pointer: + return visit(value.Elem()) + case *types.Slice: + return visit(value.Elem()) + case *types.Array: + return visit(value.Elem()) + case *types.Map: + return visit(value.Key()) || visit(value.Elem()) + case *types.Chan: + return visit(value.Elem()) + case *types.Struct: + for i := 0; i < value.NumFields(); i++ { + field := value.Field(i) + if (field.Exported() || field.Embedded()) && visit(field.Type()) { + return true + } + } + case *types.Signature: + return parameters(value.TypeParams()) || parameters(value.RecvTypeParams()) || visit(value.Params()) || visit(value.Results()) + case *types.Tuple: + for i := 0; i < value.Len(); i++ { + if visit(value.At(i).Type()) { + return true + } + } + case *types.Interface: + for i := 0; i < value.NumMethods(); i++ { + method := value.Method(i) + if method.Exported() && visit(method.Type()) { + return true + } + } + for i := 0; i < value.NumEmbeddeds(); i++ { + if visit(value.EmbeddedType(i)) { + return true + } + } + case *types.TypeParam: + return visit(value.Constraint()) + case *types.Union: + for i := 0; i < value.Len(); i++ { + if visit(value.Term(i).Type()) { + return true + } + } + } + return false +} + +type boundaryFixtureImporter struct{ engine *types.Package } + +func (i boundaryFixtureImporter) Import(path string) (*types.Package, error) { + return i.engine, nil +} + +func TestPublicBoundaryDeclarations(t *testing.T) { + t.Parallel() + fset := token.NewFileSet() + engineFile, err := parser.ParseFile(fset, "engine.go", "package engine; type Path struct{}", 0) + if err != nil { + t.Fatal(err) + } + engine, err := new(types.Config).Check("fixture/internal/engine", fset, []*ast.File{engineFile}, nil) + if err != nil { + t.Fatal(err) + } + for _, test := range []struct { + name, declaration string + exposed bool + }{ + {"collection", "type API []engine.Path", true}, + {"alias result", "type hidden = engine.Path; func API() hidden { panic(0) }", true}, + {"constraint", "type API[T interface{ ~[]engine.Path }] struct{}", true}, + {"alias", "type API = engine.Path", true}, + {"field", "type API struct { Value engine.Path }", true}, + {"embedding", "type API struct { engine.Path }", true}, + {"function type", "type API func(engine.Path)", true}, + {"interface", "type API interface { Value() engine.Path }", true}, + {"pointer method", "type API struct{}; func (*API) Value() engine.Path { panic(0) }", true}, + {"promoted method", "type hidden struct{}; func (*hidden) Value() engine.Path { panic(0) }; type API struct{ hidden }", true}, + {"nested record", "type hidden struct { Values map[string][2]engine.Path }; type API struct { Value *hidden }", true}, + {"channel", "var API chan engine.Path", true}, + {"function constraint", "func API[T interface{ ~[]engine.Path }]() {}", true}, + {"generic argument", "type hidden[T any] struct{ value T }; func API() hidden[engine.Path] { panic(0) }", true}, + {"wrapper", "type API struct{ value engine.Path }; func (API) String() string { return \"\" }", false}, + {"public record", "type Path struct{ value engine.Path }; type API struct{ Paths []Path }", false}, + {"generic wrapper", "type API[T any] struct{ Config T; value engine.Path }", false}, + {"recursive wrapper", "type API struct{ Next *API; value engine.Path }", false}, + {"private helper", "type hidden = engine.Path; type API struct{ value hidden }", false}, + } { + t.Run(test.name, func(t *testing.T) { + // Separate declarations also ensure resolution is package-wide. + file, err := parser.ParseFile(fset, "fixture.go", "package facade; import \"fixture/internal/engine\"; var _ engine.Path; "+test.declaration, 0) + if err != nil { + t.Fatal(err) + } + config := types.Config{Importer: boundaryFixtureImporter{engine}} + pkg, err := config.Check("fixture", fset, []*ast.File{file}, nil) + if err != nil { + t.Fatal(err) + } + got := publicBoundaryViolations(pkg) + if (len(got) != 0) != test.exposed { + t.Fatalf("violations = %v; want exposed=%v", got, test.exposed) + } + }) + } +}