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>
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 {
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
