apidiff: fix confusing changed messages

Depending on the names of types, apidiff could produce confusing
messages about types changing to their own names.

See testdata/order.go for more detail.

Change-Id: I41af43abc5fbda74a54cbe07899ae7fa834f2733
Reviewed-on: https://go-review.googlesource.com/c/exp/+/513676
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

apidiff/apidiff.go

diff --git a/apidiff/apidiff.go b/apidiff/apidiff.go
index 92c24fe..06d2e92 100644
--- a/apidiff/apidiff.go
+++ b/apidiff/apidiff.go
@@ -139,7 +139,33 @@
 }
 
 func (d *differ) checkPackage(oldRootPackagePath string) {
-	// Old changes.
+	// Determine what has changed between old and new.
+
+	// First, establish correspondences between types with the same name, before
+	// looking at aliases. This will avoid confusing messages like "T: changed
+	// from T to T", which can happen if a correspondence between an alias
+	// and a named type is established first.
+	// See testdata/order.go.
+	for _, name := range d.old.Scope().Names() {
+		oldobj := d.old.Scope().Lookup(name)
+		if tn, ok := oldobj.(*types.TypeName); ok {
+			if oldn, ok := tn.Type().(*types.Named); ok {
+				if !oldn.Obj().Exported() {
+					continue
+				}
+				// Does new have a named type of the same name? Look up using
+				// the old named type's name, oldn.Obj().Name(), not the
+				// TypeName tn, which may be an alias.
+				newobj := d.new.Scope().Lookup(oldn.Obj().Name())
+				if newobj != nil {
+					d.checkObjects(oldobj, newobj)
+				}
+			}
+		}
+	}
+
+	// Next, look at all exported symbols in the old world and compare them
+	// with the same-named symbols in the new world.
 	for _, name := range d.old.Scope().Names() {
 		oldobj := d.old.Scope().Lookup(name)
 		if !oldobj.Exported() {
@@ -152,7 +178,8 @@
 		}
 		d.checkObjects(oldobj, newobj)
 	}
-	// New additions.
+
+	// Now look at what has been added in the new package.
 	for _, name := range d.new.Scope().Names() {
 		newobj := d.new.Scope().Lookup(name)
 		if newobj.Exported() && d.old.Scope().Lookup(name) == nil {

apidiff/testdata/order.go

diff --git a/apidiff/testdata/order.go b/apidiff/testdata/order.go
new file mode 100644
index 0000000..f5b2adf
--- /dev/null
+++ b/apidiff/testdata/order.go
@@ -0,0 +1,37 @@
+package p
+
+// apidiff's output (but not its correctness) could depend on the order
+// in which declarations were visited.
+
+// For example, this always correctly reported that U changed from T to U:
+
+// old
+type U = T
+
+// new
+// i U: changed from T to U
+type U T
+
+// both
+type T int
+
+// But the test below, which is just a renaming of the one above, would report
+// that B was changed from B to B! apidiff was not wrong--there is an
+// incompatibility here--but it expressed itself poorly.
+//
+// This happened because old.A was processed first (Scope.Names returns names
+// sorted), resulting in old.B corresponding with new.A. Later, when old.B
+// was processed, it was matched with new.B. But since there was already a
+// correspondence for old.B, the blame for the incompatibility was put on new.B.
+
+// The fix is to establish the correspondence between same-named types first.
+
+// old
+type A = B
+
+// new
+// i A: changed from B to A
+type A B
+
+// both
+type B int