diff --git a/apidiff/correspondence.go b/apidiff/correspondence.go index f92b2891c..3d1178c7e 100644 --- a/apidiff/correspondence.go +++ b/apidiff/correspondence.go @@ -192,8 +192,22 @@ func (d *differ) establishCorrespondence(old *types.Named, new types.Type) bool // That is a bit hard, because there is no easy way to find a new package // matching an old one. if newn, ok := new.(*types.Named); ok { - if old.Obj().Pkg() != d.old || newn.Obj().Pkg() != d.new { - return old.Obj().Id() == newn.Obj().Id() + oobj := old.Obj() + nobj := newn.Obj() + if oobj.Pkg() != d.old || nobj.Pkg() != d.new { + // Compare the fully qualified names of the types. + // + // TODO(jba): when comparing modules, we should only look at the + // paths relative to the module path, because the module paths may differ. + // See cmd/gorelease/testdata/internalcompat. + var opath, npath string + if oobj.Pkg() != nil { + opath = oobj.Pkg().Path() + } + if nobj.Pkg() != nil { + npath = nobj.Pkg().Path() + } + return oobj.Name() == nobj.Name() && opath == npath } // Two generic named types correspond if their type parameter lists correspond. // Since one or the other of those lists will be empty, it doesn't hurt diff --git a/apidiff/testdata/other_packages.go b/apidiff/testdata/other_packages.go new file mode 100644 index 000000000..30c8daec9 --- /dev/null +++ b/apidiff/testdata/other_packages.go @@ -0,0 +1,24 @@ +package p + +// This test demonstrates correct handling of symbols +// in packages other than two being compared. +// See the lines in establishCorrespondence after +// if newn, ok := new.(*types.Named); ok + +// both + +// gofmt insists on grouping imports, so old and new +// must both have both imports. +import ( + "io" + "text/tabwriter" +) + +// old +var V io.Writer +var _ tabwriter.Writer + +// new +// i V: changed from io.Writer to text/tabwriter.Writer +var V tabwriter.Writer +var _ io.Writer diff --git a/cmd/gorelease/gorelease.go b/cmd/gorelease/gorelease.go index 81ace5609..e15532352 100644 --- a/cmd/gorelease/gorelease.go +++ b/cmd/gorelease/gorelease.go @@ -554,6 +554,7 @@ func loadDownloadedModule(ctx context.Context, modPath, version, max string) (m // version control tag to use (with an appropriate prefix, for modules not // in the repository root directory). func makeReleaseReport(ctx context.Context, base, release moduleInfo) (report, error) { + // TODO: use apidiff.ModuleChanges. // Compare each pair of packages. // Ignore internal packages. // If we don't have a base version to compare against just check the new diff --git a/cmd/gorelease/testdata/internalcompat/README.txt b/cmd/gorelease/testdata/internalcompat/README.txt index e3de7e553..89d079e43 100644 --- a/cmd/gorelease/testdata/internalcompat/README.txt +++ b/cmd/gorelease/testdata/internalcompat/README.txt @@ -2,9 +2,20 @@ Modules example.com/internalcompat/{a,b} are copies. One could be a fork of the other. An external package p exposes a type from a package q within the same module. -gorelease should not report differences between these packages. The types +If gorelease ran apidiff on the two modules instead of on the individual +packages, then it should not report differences between these packages. The types are distinct, but they correspond (in apidiff terminology), which is the -important property when considering differences between modules. +important property when considering differences between modules. More +specifically, the fully qualified type names are identical modulo the change +to the module path. + +But at the time gorelease was written, apidiff did not support module +comparison. If considered at the package level, the two packages +example.com/internalcompat/a/p and example.com/internalcompat/b/p +are incompatible, because the packages they refer to are different. + +So case 2 below would apply if whole modules were being diffed, but +it doesn't here because individual packages are being diffed. There are three use cases to consider: diff --git a/cmd/gorelease/testdata/internalcompat/internalcompat.test b/cmd/gorelease/testdata/internalcompat/internalcompat.test index 9bf59a37a..8aee2f509 100644 --- a/cmd/gorelease/testdata/internalcompat/internalcompat.test +++ b/cmd/gorelease/testdata/internalcompat/internalcompat.test @@ -3,3 +3,6 @@ version=v1.0.0 release=v1.0.1 base=example.com/internalcompat/a@v1.0.0 -- want -- +# example.com/internalcompat/a/p +## incompatible changes +V: changed from example.com/internalcompat/a/q.Q to example.com/internalcompat/b/q.Q