Check resolved types at the public boundary
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
231
public_boundary_test.go
Normal file
231
public_boundary_test.go
Normal file
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user