apidiff: add module comparison support
Change-Id: I11fa1aac30c8e2131bd0dd948fdcaf15332f8deb
Reviewed-on: https://go-review.googlesource.com/c/exp/+/495635
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/apidiff/README.md b/apidiff/README.md
index 3d9576c..4f3b22f 100644
--- a/apidiff/README.md
+++ b/apidiff/README.md
@@ -1,8 +1,9 @@
-# Checking Go Package API Compatibility
+# Checking Go API Compatibility
-The `apidiff` tool in this directory determines whether two versions of the same
-package are compatible. The goal is to help the developer make an informed
-choice of semantic version after they have changed the code of their module.
+The `apidiff` tool in this directory determines whether two examples of a
+package or module are compatible. The goal is to help the developer make an
+informed choice of semantic version after they have changed the code of their
+module.
`apidiff` reports two kinds of changes: incompatible ones, which require
incrementing the major part of the semantic version, and compatible ones, which
@@ -10,12 +11,12 @@
code changes that could affect client code, then the patch version should
be incremented.
-Because `apidiff` ignores package import paths, it may be used to display API
-differences between any two packages, not just different versions of the same
-package.
-
-The current version of `apidiff` compares only packages, not modules.
-
+`apidiff` may be used to display API differences between any two packages or
+modules, not just different versions of the same thing. It does this by ignoring
+the package import paths when directly comparing two packages, and
+by ignoring module paths when comparing two modules. That is to say, when
+comparing two modules, the package import paths **do** matter, but are compared
+_relative_ to their respective module root.
## Compatibility Desiderata
@@ -222,6 +223,111 @@
We can now present the definition of compatibility used by `apidiff`.
+### Module Compatibility
+
+> A new module is compatible with an old one if:
+>1. Each package present in the old module also appears in the new module,
+> with matching import paths relative to their respective module root, and
+>2. Each package present in both modules fulfills Package Compatibility as
+> defined below.
+>
+>Otherwise the modules are incompatible.
+
+If a package is converted into a nested module of the original module then
+comparing two versions of the module, before and after nested module creation,
+will produce an incompatible package removal message. This removal message does
+not necessarily mean that client code will need to change. If the package API
+retains Package Compatibility after nested module creation, then only the
+`go.mod` of the client code will need to change. Take the following example:
+
+```
+./
+ go.mod
+ go.sum
+ foo.go
+ bar/bar.go
+```
+
+Where `go.mod` is:
+
+```
+module example.com/foo
+
+go 1.20
+```
+
+Where `bar/bar.go` is:
+
+```
+package bar
+
+var V int
+```
+
+And `foo.go` is:
+
+```
+package foo
+
+import "example.com/foo/bar"
+
+_ = bar.V
+```
+
+Creating a nested module with the package `bar` while retaining Package
+Compatibility is _code_ compatible, because the import path of the package does
+not change:
+
+```
+./
+ go.mod
+ go.sum
+ foo.go
+ bar/
+ bar.go
+ go.mod
+ go.sum
+```
+
+Where `bar/go.mod` is:
+```
+module example.com/foo/bar
+
+go 1.20
+```
+
+And the top-level `go.mod` becomes:
+```
+module example.com/foo
+
+go 1.20
+
+// New dependency on nested module.
+require example.com/foo/bar v1.0.0
+```
+
+If during nested module creation either Package Compatibility is broken, like so
+in `bar/bar.go`:
+
+```
+package bar
+
+// Changed from V to T.
+var T int
+```
+
+Or the nested module uses a name other than the original package's import path,
+like so in `bar/go.mod`:
+
+```
+// Completely different module name
+module example.com/qux
+
+go 1.20
+```
+
+Then the move is backwards incompatible for client code.
+
### Package Compatibility
> A new package is compatible with an old one if:
diff --git a/apidiff/apidiff.go b/apidiff/apidiff.go
index 76669d8..5132a58 100644
--- a/apidiff/apidiff.go
+++ b/apidiff/apidiff.go
@@ -15,12 +15,12 @@
"go/constant"
"go/token"
"go/types"
+ "strings"
)
// Changes reports on the differences between the APIs of the old and new packages.
// It classifies each difference as either compatible or incompatible (breaking.) For
-// a detailed discussion of what constitutes an incompatible change, see the package
-// documentation.
+// a detailed discussion of what constitutes an incompatible change, see the README.
func Changes(old, new *types.Package) Report {
d := newDiffer(old, new)
d.checkPackage()
@@ -34,6 +34,63 @@
return r
}
+// ModuleChanges reports on the differences between the APIs of the old and new
+// modules. It classifies each difference as either compatible or incompatible
+// (breaking). This includes the addition and removal of entire packages. For a
+// detailed discussion of what constitutes an incompatible change, see the README.
+func ModuleChanges(old, new *Module) Report {
+ var r Report
+
+ oldPkgs := make(map[string]*types.Package)
+ for _, p := range old.Packages {
+ oldPkgs[old.relativePath(p)] = p
+ }
+
+ newPkgs := make(map[string]*types.Package)
+ for _, p := range new.Packages {
+ newPkgs[new.relativePath(p)] = p
+ }
+
+ for n, op := range oldPkgs {
+ if np, ok := newPkgs[n]; ok {
+ // shared package, compare surfaces
+ rr := Changes(op, np)
+ r.Changes = append(r.Changes, rr.Changes...)
+ } else {
+ // old package was removed
+ r.Changes = append(r.Changes, packageChange(op, "removed", false))
+ }
+ }
+
+ for n, np := range newPkgs {
+ if _, ok := oldPkgs[n]; !ok {
+ // new package was added
+ r.Changes = append(r.Changes, packageChange(np, "added", true))
+ }
+ }
+
+ return r
+}
+
+func packageChange(p *types.Package, change string, compatible bool) Change {
+ return Change{
+ Message: fmt.Sprintf("package %s: %s", p.Path(), change),
+ Compatible: compatible,
+ }
+}
+
+// Module is a convenience type for representing a Go module with a path and a
+// slice of Packages contained within.
+type Module struct {
+ Path string
+ Packages []*types.Package
+}
+
+// relativePath computes the module-relative package path of the given Package.
+func (m *Module) relativePath(p *types.Package) string {
+ return strings.TrimPrefix(p.Path(), m.Path)
+}
+
type differ struct {
old, new *types.Package
// Correspondences between named types.
diff --git a/apidiff/apidiff_test.go b/apidiff/apidiff_test.go
index 40cd463..86e7955 100644
--- a/apidiff/apidiff_test.go
+++ b/apidiff/apidiff_test.go
@@ -14,8 +14,71 @@
"github.com/google/go-cmp/cmp"
"golang.org/x/tools/go/packages"
+ "golang.org/x/tools/go/packages/packagestest"
)
+func TestModuleChanges(t *testing.T) {
+ packagestest.TestAll(t, testModuleChanges)
+}
+
+func testModuleChanges(t *testing.T, x packagestest.Exporter) {
+ e := packagestest.Export(t, x, []packagestest.Module{
+ {
+ Name: "example.com/moda",
+ Files: map[string]any{
+ "foo/foo.go": "package foo\n\nconst Version = 1",
+ "foo/baz/baz.go": "package baz",
+ },
+ },
+ {
+ Name: "example.com/modb",
+ Files: map[string]any{
+ "foo/foo.go": "package foo\n\nconst Version = 2\nconst Other = 1",
+ "bar/bar.go": "package bar",
+ },
+ },
+ })
+ defer e.Cleanup()
+
+ a, err := loadModule(t, e.Config, "example.com/moda")
+ if err != nil {
+ t.Fatal(err)
+ }
+ b, err := loadModule(t, e.Config, "example.com/modb")
+ if err != nil {
+ t.Fatal(err)
+ }
+ report := ModuleChanges(a, b)
+ if len(report.Changes) == 0 {
+ t.Fatal("expected some changes, but got none")
+ }
+ wanti := []string{
+ "Version: value changed from 1 to 2",
+ "package example.com/moda/foo/baz: removed",
+ }
+ sort.Strings(wanti)
+
+ got := report.messages(false)
+ sort.Strings(got)
+
+ if diff := cmp.Diff(wanti, got); diff != "" {
+ t.Errorf("incompatibles: mismatch (-want, +got)\n%s", diff)
+ }
+
+ wantc := []string{
+ "Other: added",
+ "package example.com/modb/bar: added",
+ }
+ sort.Strings(wantc)
+
+ got = report.messages(true)
+ sort.Strings(got)
+
+ if diff := cmp.Diff(wantc, got); diff != "" {
+ t.Errorf("compatibles: mismatch (-want, +got)\n%s", diff)
+ }
+}
+
func TestChanges(t *testing.T) {
dir, err := os.MkdirTemp("", "apidiff_test")
if err != nil {
@@ -27,11 +90,11 @@
sort.Strings(wanti)
sort.Strings(wantc)
- oldpkg, err := load(t, "apidiff/old", dir)
+ oldpkg, err := loadPackage(t, "apidiff/old", dir)
if err != nil {
t.Fatal(err)
}
- newpkg, err := load(t, "apidiff/new", dir)
+ newpkg, err := loadPackage(t, "apidiff/new", dir)
if err != nil {
t.Fatal(err)
}
@@ -116,7 +179,31 @@
return
}
-func load(t *testing.T, importPath, goPath string) (*packages.Package, error) {
+// Copied from cmd/apidiff/main.go.
+func loadModule(t *testing.T, cfg *packages.Config, modulePath string) (*Module, error) {
+ needsGoPackages(t)
+
+ cfg.Mode = cfg.Mode | packages.LoadTypes
+ loaded, err := packages.Load(cfg, fmt.Sprintf("%s/...", modulePath))
+ if err != nil {
+ return nil, err
+ }
+ if len(loaded) == 0 {
+ return nil, fmt.Errorf("found no packages for module %s", modulePath)
+ }
+ var tpkgs []*types.Package
+ for _, p := range loaded {
+ if len(p.Errors) > 0 {
+ // TODO: use errors.Join once Go 1.21 is released.
+ return nil, p.Errors[0]
+ }
+ tpkgs = append(tpkgs, p.Types)
+ }
+
+ return &Module{Path: modulePath, Packages: tpkgs}, nil
+}
+
+func loadPackage(t *testing.T, importPath, goPath string) (*packages.Package, error) {
needsGoPackages(t)
cfg := &packages.Config{
@@ -137,7 +224,7 @@
}
func TestExportedFields(t *testing.T) {
- pkg, err := load(t, "golang.org/x/exp/apidiff/testdata/exported_fields", "")
+ pkg, err := loadPackage(t, "golang.org/x/exp/apidiff/testdata/exported_fields", "")
if err != nil {
t.Fatal(err)
}
diff --git a/cmd/apidiff/main.go b/cmd/apidiff/main.go
index 92b763c..67261a1 100644
--- a/cmd/apidiff/main.go
+++ b/cmd/apidiff/main.go
@@ -19,6 +19,7 @@
exportDataOutfile = flag.String("w", "", "file for export data")
incompatibleOnly = flag.Bool("incompatible", false, "display only incompatible changes")
allowInternal = flag.Bool("allow-internal", false, "allow apidiff to compare internal packages")
+ moduleMode = flag.Bool("m", false, "compare modules instead of packages")
)
func main() {
@@ -28,6 +29,9 @@
fmt.Fprintf(w, "apidiff OLD NEW\n")
fmt.Fprintf(w, " compares OLD and NEW package APIs\n")
fmt.Fprintf(w, " where OLD and NEW are either import paths or files of export data\n")
+ fmt.Fprintf(w, "apidiff -m OLD NEW\n")
+ fmt.Fprintf(w, " compares OLD and NEW module APIs\n")
+ fmt.Fprintf(w, " where OLD and NEW are module paths\n")
fmt.Fprintf(w, "apidiff -w FILE IMPORT_PATH\n")
fmt.Fprintf(w, " writes export data of the package at IMPORT_PATH to FILE\n")
fmt.Fprintf(w, " NOTE: In a GOPATH-less environment, this option consults the\n")
@@ -36,6 +40,9 @@
fmt.Fprintf(w, " to. In most cases users want the latter behavior, so be sure\n")
fmt.Fprintf(w, " to cd to the exact directory which contains the module\n")
fmt.Fprintf(w, " definition of IMPORT_PATH.\n")
+ fmt.Fprintf(w, "apidiff -m -w FILE MODULE_PATH\n")
+ fmt.Fprintf(w, " writes export data of the module at MODULE_PATH to FILE\n")
+ fmt.Fprintf(w, " Same NOTE for packages applies to modules.\n")
flag.PrintDefaults()
}
@@ -45,40 +52,61 @@
flag.Usage()
os.Exit(2)
}
- pkg := mustLoadPackage(flag.Arg(0))
- if err := writeExportData(pkg, *exportDataOutfile); err != nil {
+ if err := loadAndWrite(flag.Arg(0)); err != nil {
die("writing export data: %v", err)
}
+ os.Exit(0)
+ }
+
+ if len(flag.Args()) != 2 {
+ flag.Usage()
+ os.Exit(2)
+ }
+
+ var report apidiff.Report
+ if *moduleMode {
+ oldmod := mustLoadOrReadModule(flag.Arg(0))
+ newmod := mustLoadOrReadModule(flag.Arg(1))
+
+ report = apidiff.ModuleChanges(oldmod, newmod)
} else {
- if len(flag.Args()) != 2 {
- flag.Usage()
- os.Exit(2)
- }
- oldpkg := mustLoadOrRead(flag.Arg(0))
- newpkg := mustLoadOrRead(flag.Arg(1))
+ oldpkg := mustLoadOrReadPackage(flag.Arg(0))
+ newpkg := mustLoadOrReadPackage(flag.Arg(1))
if !*allowInternal {
- if isInternalPackage(oldpkg.Path()) && isInternalPackage(newpkg.Path()) {
+ if isInternalPackage(oldpkg.Path(), "") && isInternalPackage(newpkg.Path(), "") {
fmt.Fprintf(os.Stderr, "Ignoring internal package %s\n", oldpkg.Path())
os.Exit(0)
}
}
- report := apidiff.Changes(oldpkg, newpkg)
- var err error
- if *incompatibleOnly {
- err = report.TextIncompatible(os.Stdout, false)
- } else {
- err = report.Text(os.Stdout)
- }
- if err != nil {
- die("writing report: %v", err)
- }
+ report = apidiff.Changes(oldpkg, newpkg)
+ }
+
+ var err error
+ if *incompatibleOnly {
+ err = report.TextIncompatible(os.Stdout, false)
+ } else {
+ err = report.Text(os.Stdout)
+ }
+ if err != nil {
+ die("writing report: %v", err)
}
}
-func mustLoadOrRead(importPathOrFile string) *types.Package {
+func loadAndWrite(path string) error {
+ if *moduleMode {
+ module := mustLoadModule(path)
+ return writeModuleExportData(module, *exportDataOutfile)
+ }
+
+ // Loading and writing data for only a single package.
+ pkg := mustLoadPackage(path)
+ return writePackageExportData(pkg, *exportDataOutfile)
+}
+
+func mustLoadOrReadPackage(importPathOrFile string) *types.Package {
fileInfo, err := os.Stat(importPathOrFile)
if err == nil && fileInfo.Mode().IsRegular() {
- pkg, err := readExportData(importPathOrFile)
+ pkg, err := readPackageExportData(importPathOrFile)
if err != nil {
die("reading export data from %s: %v", importPathOrFile, err)
}
@@ -108,12 +136,95 @@
return nil, fmt.Errorf("found no packages for import %s", importPath)
}
if len(pkgs[0].Errors) > 0 {
+ // TODO: use errors.Join once Go 1.21 is released.
return nil, pkgs[0].Errors[0]
}
return pkgs[0], nil
}
-func readExportData(filename string) (*types.Package, error) {
+func mustLoadOrReadModule(modulePathOrFile string) *apidiff.Module {
+ var module *apidiff.Module
+ fileInfo, err := os.Stat(modulePathOrFile)
+ if err == nil && fileInfo.Mode().IsRegular() {
+ module, err = readModuleExportData(modulePathOrFile)
+ if err != nil {
+ die("reading export data from %s: %v", modulePathOrFile, err)
+ }
+ } else {
+ module = mustLoadModule(modulePathOrFile)
+ }
+
+ filterInternal(module, *allowInternal)
+
+ return module
+}
+
+func mustLoadModule(modulepath string) *apidiff.Module {
+ module, err := loadModule(modulepath)
+ if err != nil {
+ die("loading %s: %v", modulepath, err)
+ }
+ return module
+}
+
+func loadModule(modulepath string) (*apidiff.Module, error) {
+ cfg := &packages.Config{Mode: packages.LoadTypes |
+ packages.NeedName | packages.NeedTypes | packages.NeedImports | packages.NeedDeps | packages.NeedModule,
+ }
+ loaded, err := packages.Load(cfg, fmt.Sprintf("%s/...", modulepath))
+ if err != nil {
+ return nil, err
+ }
+ if len(loaded) == 0 {
+ return nil, fmt.Errorf("found no packages for module %s", modulepath)
+ }
+ var tpkgs []*types.Package
+ for _, p := range loaded {
+ if len(p.Errors) > 0 {
+ // TODO: use errors.Join once Go 1.21 is released.
+ return nil, p.Errors[0]
+ }
+ tpkgs = append(tpkgs, p.Types)
+ }
+
+ return &apidiff.Module{Path: loaded[0].Module.Path, Packages: tpkgs}, nil
+}
+
+func readModuleExportData(filename string) (*apidiff.Module, error) {
+ f, err := os.Open(filename)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+ r := bufio.NewReader(f)
+ modPath, err := r.ReadString('\n')
+ if err != nil {
+ return nil, err
+ }
+ modPath = modPath[:len(modPath)-1] // remove delimiter
+ m := map[string]*types.Package{}
+ pkgs, err := gcexportdata.ReadBundle(r, token.NewFileSet(), m)
+ if err != nil {
+ return nil, err
+ }
+
+ return &apidiff.Module{Path: modPath, Packages: pkgs}, nil
+}
+
+func writeModuleExportData(module *apidiff.Module, filename string) error {
+ f, err := os.Create(filename)
+ if err != nil {
+ return err
+ }
+ fmt.Fprintln(f, module.Path)
+ // TODO: Determine if token.NewFileSet is appropriate here.
+ if err := gcexportdata.WriteBundle(f, token.NewFileSet(), module.Packages); err != nil {
+ return err
+ }
+ return f.Close()
+}
+
+func readPackageExportData(filename string) (*types.Package, error) {
f, err := os.Open(filename)
if err != nil {
return nil, err
@@ -129,7 +240,7 @@
return gcexportdata.Read(r, token.NewFileSet(), m, pkgPath)
}
-func writeExportData(pkg *packages.Package, filename string) error {
+func writePackageExportData(pkg *packages.Package, filename string) error {
f, err := os.Create(filename)
if err != nil {
return err
@@ -150,13 +261,32 @@
os.Exit(1)
}
-func isInternalPackage(pkgPath string) bool {
+func filterInternal(m *apidiff.Module, allow bool) {
+ if allow {
+ return
+ }
+
+ var nonInternal []*types.Package
+ for _, p := range m.Packages {
+ if !isInternalPackage(p.Path(), m.Path) {
+ nonInternal = append(nonInternal, p)
+ } else {
+ fmt.Fprintf(os.Stderr, "Ignoring internal package %s\n", p.Path())
+ }
+ }
+ m.Packages = nonInternal
+}
+
+func isInternalPackage(pkgPath, modulePath string) bool {
+ pkgPath = strings.TrimPrefix(pkgPath, modulePath)
switch {
case strings.HasSuffix(pkgPath, "/internal"):
return true
case strings.Contains(pkgPath, "/internal/"):
return true
- case pkgPath == "internal", strings.HasPrefix(pkgPath, "internal/"):
+ case pkgPath == "internal":
+ return true
+ case strings.HasPrefix(pkgPath, "internal/"):
return true
}
return false
diff --git a/cmd/apidiff/main_test.go b/cmd/apidiff/main_test.go
new file mode 100644
index 0000000..7cf8bf3
--- /dev/null
+++ b/cmd/apidiff/main_test.go
@@ -0,0 +1,144 @@
+package main
+
+import (
+ "go/types"
+ "testing"
+
+ "github.com/google/go-cmp/cmp"
+ "golang.org/x/exp/apidiff"
+)
+
+func TestIsInternalPackage(t *testing.T) {
+ for _, tst := range []struct {
+ name, pkg, mod string
+ want bool
+ }{
+ {
+ name: "not internal no module",
+ pkg: "foo",
+ want: false,
+ },
+ {
+ name: "not internal with module",
+ pkg: "example.com/bar/foo",
+ mod: "example.com/bar",
+ want: false,
+ },
+ {
+ name: "internal no module",
+ pkg: "internal",
+ want: true,
+ },
+ {
+ name: "leading internal no module",
+ pkg: "internal/foo",
+ want: true,
+ },
+ {
+ name: "middle internal no module",
+ pkg: "foo/internal/bar",
+ want: true,
+ },
+ {
+ name: "ending internal no module",
+ pkg: "foo/internal",
+ want: true,
+ },
+
+ {
+ name: "leading internal with module",
+ pkg: "example.com/baz/internal/foo",
+ mod: "example.com/baz",
+ want: true,
+ },
+ {
+ name: "middle internal with module",
+ pkg: "example.com/baz/foo/internal/bar",
+ mod: "example.com/baz",
+ want: true,
+ },
+ {
+ name: "ending internal with module",
+ pkg: "example.com/baz/foo/internal",
+ mod: "example.com/baz",
+ want: true,
+ },
+ {
+ name: "not package internal with internal module",
+ pkg: "example.com/internal/foo",
+ mod: "example.com/internal",
+ want: false,
+ },
+ } {
+ t.Run(tst.name, func(t *testing.T) {
+ if got := isInternalPackage(tst.pkg, tst.mod); got != tst.want {
+ t.Errorf("expected %v, got %v for %s/%s", tst.want, got, tst.mod, tst.pkg)
+ }
+ })
+ }
+}
+
+func TestFilterInternal(t *testing.T) {
+ for _, tst := range []struct {
+ name string
+ mod *apidiff.Module
+ allow bool
+ want []*types.Package
+ }{
+ {
+ name: "allow internal",
+ mod: &apidiff.Module{
+ Path: "example.com/foo",
+ Packages: []*types.Package{
+ types.NewPackage("example.com/foo/bar", "bar"),
+ types.NewPackage("example.com/foo/internal", "internal"),
+ types.NewPackage("example.com/foo/internal/buz", "buz"),
+ types.NewPackage("example.com/foo/bar/internal", "internal"),
+ },
+ },
+ allow: true,
+ want: []*types.Package{
+ types.NewPackage("example.com/foo/bar", "bar"),
+ types.NewPackage("example.com/foo/internal", "internal"),
+ types.NewPackage("example.com/foo/internal/buz", "buz"),
+ types.NewPackage("example.com/foo/bar/internal", "internal"),
+ },
+ },
+ {
+ name: "filter internal",
+ mod: &apidiff.Module{
+ Path: "example.com/foo",
+ Packages: []*types.Package{
+ types.NewPackage("example.com/foo/bar", "bar"),
+ types.NewPackage("example.com/foo/internal", "internal"),
+ types.NewPackage("example.com/foo/internal/buz", "buz"),
+ types.NewPackage("example.com/foo/bar/internal", "internal"),
+ },
+ },
+ want: []*types.Package{
+ types.NewPackage("example.com/foo/bar", "bar"),
+ },
+ },
+ {
+ name: "filter internal nothing left",
+ mod: &apidiff.Module{
+ Path: "example.com/foo",
+ Packages: []*types.Package{
+ types.NewPackage("example.com/foo/internal", "internal"),
+ },
+ },
+ want: nil,
+ },
+ } {
+ t.Run(tst.name, func(t *testing.T) {
+ filterInternal(tst.mod, tst.allow)
+ if diff := cmp.Diff(tst.mod.Packages, tst.want, cmp.Comparer(comparePath)); diff != "" {
+ t.Errorf("got(-),want(+):\n%s", diff)
+ }
+ })
+ }
+}
+
+func comparePath(x, y *types.Package) bool {
+ return x.Path() == y.Path()
+}
