apidiff: fix comparison of named types in other packages

Compare named types from packages other than old and new using their
package-qualified names.

When determining whether two such types corresponded, we were using
the IDs of the types. This was wrong, because the ID for an exported
type is just its name, unqualified.

The full story is a bit more subtle, because when comparing modules,
we want to allow for the module paths to be different.  That is left
as a TODO.

gorelease actually tests for modules with different paths, but its
test had a mistaken assumption (see the README.txt in this CL) so we
had to change it.

Change-Id: Id06eff0b43292088a016d8556e0c7c9c86616289
Reviewed-on: https://go-review.googlesource.com/c/exp/+/513635
Run-TryBot: Jonathan Amsterdam <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

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

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

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

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:
 

cmd/gorelease/testdata/internalcompat/internalcompat.test

@@ -3,3 +3,6 @@ version=v1.0.0
 release=v1.0.1
 base=example.com/internalcompat/[email protected]
 -- want --
+# example.com/internalcompat/a/p
+## incompatible changes
+V: changed from example.com/internalcompat/a/q.Q to example.com/internalcompat/b/q.Q